feat(workspace): name tonight's first drop plan on the map - #1040
feat(workspace): name tonight's first drop plan on the map#1040seonghobae wants to merge 12 commits into
Conversation
Name the earliest corroborated full-band arrival after a thin texture so the entering part can land together when the map fills. Heuristic demo topology stays unnamed until real stem activity proves the drop.
|
Warning Review limit reachedNext included review available in 10 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthrough분석 엔진이 검증된 스템 활동에서 첫 드롭 계획을 생성합니다. 데스크톱 워크스페이스는 계획을 현지화해 표시하고, 해당 곡 맵 섹션으로 이동합니다. Changes첫 드롭 계획
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🔵 Low · up to The PR adds first-drop guidance and map navigation across analysis and desktop UI. Current evidence identifies localized correctness and copy follow-ups—drums may be omitted from density corroboration, invalid model guidance may render incorrectly, and Korean endings are inconsistent—without a security, availability, or release-blocking impact; merge is reasonable with owner awareness and follow-up. Sequence Diagram(s)sequenceDiagram
participant StemActivity
participant AnalysisEngine
participant Workspace
participant SongMap
StemActivity->>AnalysisEngine: 이전 및 현재 섹션 활동 제공
AnalysisEngine->>AnalysisEngine: 확인된 density fill에서 첫 드롭 계획 선택
AnalysisEngine-->>Workspace: dropPlan과 dropPlanSource 전달
Workspace->>Workspace: 계획 문구 현지화
Workspace->>SongMap: 일치하는 섹션으로 스크롤
SongMap-->>Workspace: 열린 계획 상태 표시
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 84.85% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 66 functions across 25 files. (6 skipped: 6 unsupported.) ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| drop_plan: Option<String>, | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| drop_plan_source: Option<String>, |
There was a problem hiding this comment.
📝 Info: Rust payload accepts dropPlanSource without dropPlan; TS rejects it
The native RehearsalRolePayload adds drop_plan and drop_plan_source as independent optional fields with no cross-field rule, so a payload carrying dropPlanSource alone parses in Rust. The TS validateRehearsalRole rejects that same shape. Both guard the same persisted contract, so they disagree. Impact is small: the engine always emits both, and the TS side fails closed.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if previous_active - current_active: | ||
| return None |
There was a problem hiding this comment.
📝 Info: "Previous sources stay" is enforced at role level, not source level
resolveSafeFirstDropPlan and the engine's _activity_drop_plan both reject a drop when any previous active role id is missing from the current section, comparing role ids not collapsed sources. A within-accompaniment swap blocks the drop even though the shared other source stays. Consistent across both layers and fail-closed.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (song !== previousSongRef.current) { | ||
| const isLocalWorkspaceUpdate = song === localSongUpdateRef.current; | ||
| if (!isLocalWorkspaceUpdate) { | ||
| workspaceInstanceRef.current = song; | ||
| } | ||
| localSongUpdateRef.current = null; | ||
| previousSongRef.current = song; | ||
| } |
There was a problem hiding this comment.
📝 Info: Opened-drop stability depends on parent re-passing the exact updated object
Workspace mutates refs during render to keep the opened drop stable across immutable practice-progress edits and reset it on real song swaps. The guard relies on the parent re-rendering with the same object handed to onSongUpdate, tracked via localSongUpdateRef. A parent that forwards a new but equal object for an internal edit would reset the opened drop.
Was this helpful? React with 👍 or 👎 to provide feedback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| if ( | ||
| typeof sectionId !== "string" || | ||
| sectionId.trim().length === 0 || | ||
| typeof sectionLabel !== "string" || | ||
| !SECTION_FORM_LABEL_SET.has(sectionLabel) || | ||
| timeRange === null || | ||
| previousTimeRange === null || | ||
| previousTimeRange.end !== timeRange.start | ||
| ) { | ||
| return []; | ||
| } | ||
|
|
||
| const previousActiveIds = rankedGraphRoleIds(previousSection as RehearsalSection, true); | ||
| const currentActiveIds = rankedGraphRoleIds(section as RehearsalSection, true); | ||
| const previousSourceCount = activeSourceCount(previousActiveIds); | ||
| const currentSourceCount = activeSourceCount(currentActiveIds); | ||
| if (previousSourceCount < 1 || previousSourceCount > 2 || currentSourceCount < 3) { | ||
| return []; | ||
| } | ||
| for (const roleId of previousActiveIds) { | ||
| if (!currentActiveIds.has(roleId)) { | ||
| return []; | ||
| } | ||
| } | ||
| let entered = false; | ||
| for (const roleId of currentActiveIds) { | ||
| if (!previousActiveIds.has(roleId)) { | ||
| entered = true; | ||
| break; | ||
| } | ||
| } | ||
| if (!entered) { | ||
| return []; | ||
| } |
There was a problem hiding this comment.
📝 Info: Engine and UI double-gate drop-plan emission
The Python engine emits the drop copy per active role, while resolveFirstDropPlan re-derives the same density gate and adds checks the engine lacks: sections must abut in time, the label must be a known form label, and ranges must be bounded integers. An engine-emitted drop in a non-abutting section shows nothing in the UI. Real boundaries abut, so this holds today, but the two gates can diverge on hand-edited or future data.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (song !== previousSongRef.current) { | ||
| const isLocalWorkspaceUpdate = song === localSongUpdateRef.current; | ||
| if (!isLocalWorkspaceUpdate) { | ||
| workspaceInstanceRef.current = song; | ||
| } | ||
| localSongUpdateRef.current = null; | ||
| previousSongRef.current = song; | ||
| } |
There was a problem hiding this comment.
📝 Info: Workspace ref plumbing preserves armed state across self-edits
The added refs distinguish self-emitted immutable song updates from externally swapped songs, holding workspaceInstanceRef stable so the callout keeps its armed state on a practice-progress edit while still resetting on a real song swap. Consistent with the callout's songIdentity comparison.
Was this helpful? React with 👍 or 👎 to provide feedback.
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| drop_plan_source: Option<String>, |
There was a problem hiding this comment.
📝 Info: Native drop_plan_source accepts arbitrary strings
drop_plan_source is an unconstrained Option<String>, while the shared-types validateRehearsalRole restricts dropPlanSource to model/user. The native loader accepts values the TypeScript contract rejects. App-written payloads only emit model, so impact is limited to hand-edited or corrupted project files.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Buyer-visible next action
This PR names tonight's first drop plan on the rehearsal map when an entering part lands a full-band arrival after a thin texture: previous graph 1–2 distinct sources, current graph ≥3 sources, previous sources stay, and a new named entrance arrives. Open scrolls that rendered map section so the landing part can come in together when the texture fills.
A drop is a corroborated density fill that arrives. Breakdown is a staying sparse hold. Dropout is a leaving part. Cutoff is a stop-time. Pickup is rest-then-enter. Heuristic-only topology stays unnamed.
Summary
Hit this drop with {target}; come in together when the texture fills.(or the solo fill) only from real stem activity that proves the thin-to-full arrival. Sharedotherstems may corroborate density but never own the landing.dropPlancopy only from owned data properties and snapshots it once before ranking. Inherited, accessor-backed, or Proxy-substituted runtime metadata remains guidance-only.{at} {role} 드롭 열기.data-section-index) and fails closed on ambiguous or missing targets. Reduced motion usesbehavior: "auto".Exact current identity
develop@749511c3ad4000090048718f685c6bee6b3d2c25(fix(security): establish canonical npm, PDF.js, Nanoid, and Undici baseline #783 integrated, feat(workspace): name tonight's first playable range on the map #957 playable range on develop).feat/workspace-first-drop-plan.Verification
tests/test_drop_plan.pyplus extractor/roles/activitydrop_plan_contractround-triptsc --noEmitfor desktop and shared-types./scripts/harness/quickcheck.sh(CI)Security Notes
Attack surface
Untrusted rehearsal-song JSON, Proxy/inherited role metadata, and rendered-map DOM selectors used for Open navigation.
Trust boundary
UI resolver reads only own data properties (
Object.getOwnPropertyDescriptor) and fails closed. Engine drop copy is emitted only from corroborated stem activity. Native payload admits optionaldropPlan/dropPlanSourcewithdeny_unknown_fields.Mitigations
Bounded copy (180 code points), reject multiline/blank plans, reject provenance without copy, snapshot guidance before ranking, fail closed on ambiguous
song-structure-gridmounts, preserve user copy verbatim unless model provenance is explicit.Test points
Hostile Proxy get, inherited metadata, malformed song root, Korean particle-safe copy, navigation failure, reduced motion, demo unnamed, 2→3 solo fill, two named partners, other-stem unnamed, density-drop-is-not-drop, live stem extract.
Dependency and Supply Chain
Canonical #783 is protected
developshipped truth. This branch inherits that JavaScript baseline and does not duplicate or suppress it. Inherited npm HIGH must not be suppressed elsewhere.i18n impact
Reviewer checklist
developKeep unmerged until the unchanged then-current head has every applicable repository and central CI/build/release/security/SAST/SBOM/supply-chain/coverage/review gate terminal-success, zero valid unresolved findings, and a qualifying independent non-author last-push approval under live branch protection.
Queued, pending, skipped, cancelled, failed, predecessor-head, protected-base, model-only, self/author, or administrative-bypass evidence is not success.
Summary by CodeRabbit
새로운 기능
개선 사항