feat: implement V2 Advanced Rehearsal Collaboration Features#158
Closed
seonghobae wants to merge 20 commits into
Closed
feat: implement V2 Advanced Rehearsal Collaboration Features#158seonghobae wants to merge 20 commits into
seonghobae wants to merge 20 commits into
Conversation
Contributor
|
Warning Review limit reached
More reviews will be available in 1 hour, 3 minutes, and 59 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (15)
📝 WalkthroughWalkthrough데스크톱 앱에 섹션별 주석(annotations) 지원과 bandscope:// 딥링크 파싱/처리 흐름이 추가되며, 주석 병합 유틸과 관련 타입·검증 및 테스트/문서가 도입·연동됩니다. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as 사용자
participant App as App.tsx
participant DeepLink as deepLink.ts
participant Packs as PackStore
participant Merge as mergeAnnotations
participant Workspace as Workspace
participant Section as SectionRoadmap
participant Clipboard as 클립보드
User->>App: hash에 bandscope://song/{id}/section/{sid} 설정
App->>DeepLink: parseDeepLink(hash)
DeepLink-->>App: {songId, sectionId} 또는 null
App->>Packs: songId로 팩 조회
alt 팩 발견
App->>Merge: mergeAnnotations(existing, pack.annotations)
Merge-->>App: 병합된 annotations
App->>Workspace: 전달(annotations, onAddAnnotation)
Workspace->>Section: 전달
Section->>Section: 섹션별 주석 렌더/버튼 노출
else 팩 없음
App->>App: deepLinkError 설정
App-->>User: 오류 패널 표시
end
User->>Section: "링크 복사" 클릭
Section->>Clipboard: bandscope://song/{id}/section/{sid} + 안내 복사
Clipboard-->>User: 복사 완료
User->>Section: "주석 추가" 클릭
Section->>User: 주석 입력 프롬프트
User->>Section: 주석 텍스트 제출
Section->>Workspace: onAddAnnotation(newAnnotation)
Workspace->>App: 주석 추가 요청
App->>Merge: mergeAnnotations(current, [newAnnotation])
Merge-->>App: 병합된 annotations
App->>Workspace: 업데이트된 annotations 반영
Estimated code review effort🎯 3 (보통) | ⏱️ ~22분 Possibly related PRs
시
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/App.tsx (1)
183-197:⚠️ Potential issue | 🟠 Major주석이 현재 source of truth에 반영되지 않아 갱신/저장 후 유실됩니다.
onAddAnnotation()은 React local state만 수정하고, 저장/불러오기는 여전히RehearsalSong단위라 pack-levelannotations를 round-trip하지 못합니다. 이 상태로는 다음 workspace push, 앱 재시작, 저장-재열기 중 하나만 일어나도 새 주석이 사라집니다.Also applies to: 209-216, 347-356
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/App.tsx` around lines 183 - 197, handleLoadProject currently constructs a temporary workspace from a RehearsalSong and onAddAnnotation only mutates React local state, causing pack-level annotations to never be persisted; update the flow so loadProject returns a RehearsalWorkspace (or include pack-level annotations) and change onAddAnnotation to write annotations into the workspace/songs[].song.pack or songs[].annotations structure that is used as the source-of-truth saved by the app, then ensure setWorkspace receives the full RehearsalWorkspace (with pack-level annotations) so subsequent save/load cycles preserve annotations; locate and update handleLoadProject, loadProject, onAddAnnotation and the setWorkspace call sites (also apply same fix around the other occurrences noted: the blocks at ~209-216 and ~347-356) to round-trip annotations into the persisted workspace.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/App.test.tsx`:
- Around line 444-452: The test "covers missing workspace load error" duplicates
the earlier "handles loadProject error" scenario by calling
mockLoadProject.mockRejectedValueOnce and asserting "Failed to load project";
remove or change it: either delete this duplicate test entirely, or modify it to
exercise a different scenario (e.g., have mockLoadProject resolve to
null/undefined or reject with a different error and assert the corresponding UI
message). Update the test name accordingly and keep references to
mockLoadProject and the App render/fireEvent code paths (render(<App />),
fireEvent.click(screen.getByRole("button", { name: /Open Project/i }))) so the
altered test still targets loadProject behavior without duplicating the existing
"handles loadProject error" test.
- Around line 454-459: The test currently uses a conditional if(linkBtn)
fireEvent.click(linkBtn) which can silently skip assertions; replace this with
an explicit expectation and action: after render(<App />) get or query the
button by role/name using linkBtn (the existing variable), then either assert it
exists with expect(linkBtn).toBeInTheDocument() and then call
fireEvent.click(linkBtn) to exercise the branch, or assert it is absent with
expect(linkBtn).toBeNull() depending on the intended coverage of the "missing
audio" branch; update the test to use the explicit expect +
fireEvent.click(linkBtn) (or expect(...).toBeNull()) instead of the conditional.
In `@apps/desktop/src/App.tsx`:
- Around line 79-97: The deep-link handling is currently only executed inside
the getWorkspaceState() success path so if ws is null the hash is ignored and
never reprocessed; refactor by extracting the logic that reads
window.location.hash and calls parseDeepLink into a single helper (e.g.,
processDeepLink(uri, workspace)) and ensure it is invoked both after
getWorkspaceState() resolves and again after handleLoadProject()/any code path
that sets workspace state; alternatively, when getWorkspaceState() yields null
detect the deep-link, save a pendingDeepLink (or parsed songId) in component
state and, in the setter that runs when workspace is later populated
(setWorkspace or inside handleLoadProject), call the same logic to find
targetPack via ws.songs and then setSelectedPackId or setDeepLinkError and clear
window.location.hash.
- Around line 83-95: The deep-link handling calls parseDeepLink(uri) but only
uses parsed.songId to setSelectedPackId and discards parsed.sectionId, so shared
links can't open the exact section; update the handler (the block using
parseDeepLink, ws.songs, setSelectedPackId, setDeepLinkError) to also check
parsed.sectionId: after locating targetPack from ws.songs, if parsed.sectionId
is present, find the matching section within that pack (e.g., pack.sections or
pack.song.sections), setSelectedSectionId (or the app’s section-selection state)
and move focus/scroll to that section, falling back to just selecting the pack
if the section is not found and set a clear deep-link error via setDeepLinkError
when neither song nor section can be resolved. Ensure window.location.hash is
still cleared after processing.
In `@apps/desktop/src/features/workspace/SectionRoadmap.tsx`:
- Around line 19-23: The code builds a bandscope:// URI using raw song.id and
sectionId which can contain characters disallowed by the consumer regex
[a-zA-Z0-9-]+, producing broken links; update handleCopyLink to
sanitize/normalize both song.id and sectionId before constructing the link. Add
a small sanitizer (e.g., sanitizeId) that replaces any character not matching
/[A-Za-z0-9-]/ with '-' (and collapses multiple '-' and trims leading/trailing
'-') and use sanitizeId(song.id) and sanitizeId(sectionId) when creating link
and before calling navigator.clipboard.writeText; keep
navigator.clipboard.writeText(text) behavior unchanged but ensure the text
contains the sanitized link so consumers can open it.
- Around line 26-34: handleAddNote currently checks prompt() result for
truthiness before trimming, so all-space input like " " passes and creates an
empty annotation; fix by trimming the prompt result first and only call
onAddAnnotation when the trimmed text is non-empty. Update the handleAddNote
implementation (the prompt/result handling and the conditional that calls
onAddAnnotation) to do const text = window.prompt(...); const trimmed =
text?.trim(); if (trimmed && onAddAnnotation) { onAddAnnotation({ id:
crypto.randomUUID(), timestamp: new Date().toISOString(), text: trimmed,
sectionId }); } so stored annotations never contain whitespace-only text.
In `@apps/desktop/src/lib/annotations.ts`:
- Around line 21-22: The current merged.sort comparator uses new
Date(a.timestamp).getTime() which yields NaN for invalid timestamps; update the
comparator used on merged to defensively parse timestamps, e.g. compute const ta
= Date.parse(a.timestamp); const tb = Date.parse(b.timestamp); const tsa =
Number.isFinite(ta) ? ta : fallbackA; const tsb = Number.isFinite(tb) ? tb :
fallbackB; then return tsa - tsb or, to keep deterministic ordering, fall back
to original index tie-breaker; modify the merged.sort call (the comparator using
a.timestamp and b.timestamp) to implement this parsing, NaN check, a clear
fallback strategy (e.g. treat invalid timestamps as very old or very new, or
sort them after valid ones), and include stable tie-break using original indices
so sort is deterministic.
In `@apps/desktop/src/lib/deepLink.ts`:
- Around line 17-32: The parseDeepLink function currently uses match[1] and
match[2] which TypeScript may infer as string | undefined; after confirming the
regex matched, make the types explicit by adding a non-null assertion or an
explicit null-check before returning (e.g., assert match[1] and match[2] are
defined or use const [ , songId, sectionId ] = match and narrow types) so
parseDeepLink returns songId and sectionId as strings without the undefined
union; update the return to use the asserted/checked variables and keep the
existing validation with validateBandScopeUri.
In `@apps/desktop/vite.config.ts`:
- Around line 12-17: The coverage include list in vite.config.ts is missing the
new library files; update the include array (currently listing "src/App.tsx" and
"src/lib/export.ts") to also contain "src/lib/deepLink.ts" and
"src/lib/annotations.ts" so those modules (deepLink and annotations) are
measured by coverage; ensure the exact filenames match the exported module names
(deepLink and annotations) referenced in your codebase.
In `@docs/plans/2026-04-25-v2-collaboration.md`:
- Around line 10-15: The Scope section currently lists "Assignment Semantics",
"Contextual Comments", "Approvals & Status", and "Cloud Sync Backbone" as
in-scope but the CEO Review later states these are scrapped; update the document
so the Scope and CEO Review are consistent by either removing those four bullet
items from the Scope or explicitly marking them as out-of-scope/deferred, and
mirror the same change in the later block referenced (the CEO Review area
covering the same topics) so both places (the "Scope" bullets and the text that
previously covered lines 22-29) align and cannot be misinterpreted by
implementers or QA.
In `@packages/shared-types/src/index.ts`:
- Around line 1091-1102: The validateAnnotation function currently only checks
types and allows empty strings and invalid timestamps; update validateAnnotation
to reject empty strings for id, text, and sectionId (e.g., check typeof ===
"string" && value.trim().length > 0) and validate timestamp is a parseable ISO
timestamp (e.g., Date.parse(value.timestamp) yields a valid time and the string
matches an ISO-like pattern); also enforce that optional roleId, when present,
is a non-empty string. Keep existing unexpectedKey checks and return
invalidField(...) with the same path identifiers (validateAnnotation, fields
id/timestamp/text/sectionId/roleId) on failure.
In `@packages/shared-types/test/index.test.ts`:
- Around line 947-949: The test fixture validAnnotation uses timestamp: 0
(number) but the Annotation type expects a string, causing validation to fail
early; update validAnnotation in the test to use a string timestamp (e.g., "0")
so parseSongRehearsalPack runs its intended checks (the calls to
parseSongRehearsalPack with extra and id mismatches will then throw the expected
"extra" and "id" errors).
---
Outside diff comments:
In `@apps/desktop/src/App.tsx`:
- Around line 183-197: handleLoadProject currently constructs a temporary
workspace from a RehearsalSong and onAddAnnotation only mutates React local
state, causing pack-level annotations to never be persisted; update the flow so
loadProject returns a RehearsalWorkspace (or include pack-level annotations) and
change onAddAnnotation to write annotations into the workspace/songs[].song.pack
or songs[].annotations structure that is used as the source-of-truth saved by
the app, then ensure setWorkspace receives the full RehearsalWorkspace (with
pack-level annotations) so subsequent save/load cycles preserve annotations;
locate and update handleLoadProject, loadProject, onAddAnnotation and the
setWorkspace call sites (also apply same fix around the other occurrences noted:
the blocks at ~209-216 and ~347-356) to round-trip annotations into the
persisted workspace.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f8106e54-3846-4308-b8e0-89595a42f765
📒 Files selected for processing (12)
apps/desktop/src/App.test.tsxapps/desktop/src/App.tsxapps/desktop/src/features/workspace/SectionRoadmap.tsxapps/desktop/src/features/workspace/Workspace.tsxapps/desktop/src/lib/annotations.tsapps/desktop/src/lib/deepLink.tsapps/desktop/src/lib/job_runner.tsapps/desktop/vite.config.tsdocs/plans/2026-04-25-v2-collaboration.mdpackages/shared-types/src/index.tspackages/shared-types/test/index.test.tspackages/shared-types/vitest.config.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/shared-types/src/index.ts`:
- Around line 1151-1153: The validateBandScopeUri regex in function
validateBandScopeUri currently allows unbounded songId/sectionId
(`[a-zA-Z0-9-]+`); update that regex to enforce a reasonable upper length (e.g.,
replace both `+` quantifiers with a length range like `{1,64}`) so songId and
sectionId cannot be arbitrarily large, then run/adjust any tests that rely on
the new length constraint.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 10051c39-1238-4412-b757-ccd1171f934e
📒 Files selected for processing (2)
apps/desktop/src/features/workspace/Workspace.tsxpackages/shared-types/src/index.ts
This was referenced Apr 29, 2026
Collaborator
Author
|
@coderabbitai resolve |
Contributor
✅ Action performedComments resolved and changes approved. |
…local-handoff # Conflicts: # apps/desktop/package.json # apps/desktop/src/App.test.tsx # apps/desktop/src/App.tsx # package-lock.json # packages/shared-types/test/index.test.ts
…collaboration # Conflicts: # apps/desktop/src/App.test.tsx # apps/desktop/src/App.tsx # apps/desktop/src/features/workspace/SectionRoadmap.tsx # apps/desktop/src/features/workspace/Workspace.tsx # packages/shared-types/test/index.test.ts
Collaborator
Author
|
@coderabbitai resolve |
Contributor
✅ Action performedComments resolved and changes approved. |
# Conflicts: # packages/shared-types/test/index.test.ts
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Collaborator
Author
|
@coderabbitai resolve |
Contributor
✅ Action performedComments resolved and changes approved. |
Collaborator
Author
|
@coderabbitai resolve |
Contributor
✅ Action performedComments resolved and changes approved. |
Collaborator
Author
|
@coderabbitai resolve |
Contributor
Rate Limit Exceeded
|
Collaborator
Author
|
Closing as stale and not mergeable in its current form. This branch contains useful product direction, but the current head would roll back current develop state across CI/action pins, the PR scheduler workflow/docs, section validation utilities, YouTube/security tests, supply-chain parser hardening, dependency versions, and other recently merged fixes. Security/dependency concerns that need a fresh implementation:
A safe replacement should be rebuilt from current develop with a narrow dependency admission record, current lockfiles, explicit archive limits, path/id validation, IPC allowlist coverage, and focused tests. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Resolves #152. This PR implements the V2 Advanced Collaboration features focusing on local-first annotation syncing and deep-linked snippets for WhatsApp/messaging handoffs.
Changes
shared-typeswithAnnotationobjects supporting append-only conflict resolution inSongRehearsalPack.bandscope://song/...) preventing Path Traversal and LFI.lib/annotations.tsfor merging annotations securely without overwriting personal UI preferences.WorkspaceandSectionRoadmapto include hover actions for 'Copy Link' and 'Add Note', along with an Annotation Drawer rendering inline context notes.Verification
npm run test --workspacespasses successfully, correctly handling Edge cases related to missing song loads from deep-links../scripts/harness/quickcheck.shchecks succeed cleanly, establishing secure Trust Boundaries across the local OS-to-App deep link integration.