feat: implement V2 Advanced Rehearsal Collaboration Features#158
Open
seonghobae wants to merge 2 commits into
Open
feat: implement V2 Advanced Rehearsal Collaboration Features#158seonghobae wants to merge 2 commits into
seonghobae wants to merge 2 commits into
Conversation
📝 WalkthroughSummary by CodeRabbit릴리스 노트
Walkthrough데스크톱 앱에 섹션별 주석(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 docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
Comment on lines
+444
to
+452
| it("covers missing workspace load error", async () => { | ||
| // cover loadProject returning an error | ||
| mockLoadProject.mockRejectedValueOnce(new Error("File missing")); | ||
| render(<App />); | ||
| fireEvent.click(screen.getByRole("button", { name: /Open Project/i })); | ||
| await waitFor(() => { | ||
| expect(screen.getByText(/Failed to load project/i)).toBeTruthy(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
중복 테스트 케이스입니다.
이 테스트는 lines 190-197의 "handles loadProject error" 테스트와 동일한 시나리오를 검증합니다. 두 테스트 모두 mockLoadProject.mockRejectedValueOnce로 에러를 발생시키고 "Failed to load project" 메시지를 확인합니다. 중복을 제거하거나 다른 시나리오를 테스트하도록 수정하세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/App.test.tsx` around lines 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.
Comment on lines
+454
to
+459
| it("covers missing audio state branch", async () => { | ||
| // Cover missing audio empty state resolution | ||
| render(<App />); | ||
| const linkBtn = screen.queryByRole("button", { name: /Locate Original Audio/i }); | ||
| if(linkBtn) fireEvent.click(linkBtn); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
조건부 실행으로 인해 테스트가 의미 있는 검증을 수행하지 않을 수 있습니다.
if(linkBtn) fireEvent.click(linkBtn)는 버튼이 없으면 아무것도 실행하지 않습니다. 현재 렌더링 상태에서 "Locate Original Audio" 버튼이 존재하지 않으면 테스트가 항상 통과하지만 실제로는 아무것도 검증하지 않습니다. 버튼이 존재해야 하는 조건을 설정하거나, 존재하지 않아야 함을 명시적으로 검증하세요.
♻️ 명시적 검증 추가 제안
it("covers missing audio state branch", async () => {
- // Cover missing audio empty state resolution
render(<App />);
const linkBtn = screen.queryByRole("button", { name: /Locate Original Audio/i });
- if(linkBtn) fireEvent.click(linkBtn);
+ // 초기 상태에서는 해당 버튼이 존재하지 않아야 함
+ expect(linkBtn).toBeNull();
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it("covers missing audio state branch", async () => { | |
| // Cover missing audio empty state resolution | |
| render(<App />); | |
| const linkBtn = screen.queryByRole("button", { name: /Locate Original Audio/i }); | |
| if(linkBtn) fireEvent.click(linkBtn); | |
| }); | |
| it("covers missing audio state branch", async () => { | |
| render(<App />); | |
| const linkBtn = screen.queryByRole("button", { name: /Locate Original Audio/i }); | |
| // 초기 상태에서는 해당 버튼이 존재하지 않아야 함 | |
| expect(linkBtn).toBeNull(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/App.test.tsx` around lines 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.
Comment on lines
79
to
+97
| getWorkspaceState().then(ws => { | ||
| if (!unmounted && ws) setWorkspace(ws); | ||
| if (!unmounted && ws) { | ||
| setWorkspace(ws); | ||
|
|
||
| // Check for deep link on load | ||
| if (window.location.hash.startsWith("#bandscope://")) { | ||
| const uri = window.location.hash.slice(1); | ||
| const parsed = parseDeepLink(uri); | ||
| if (parsed) { | ||
| const targetPack = ws.songs.find(s => "song" in s && s.song?.id === parsed.songId); | ||
| if (targetPack) { | ||
| setSelectedPackId(targetPack.id); | ||
| } else { | ||
| setDeepLinkError("Song not found. Ask the leader to share the .bndscp file first"); | ||
| } | ||
| } | ||
| window.location.hash = ""; // Clear hash after processing | ||
| } | ||
| } |
There was a problem hiding this comment.
초기 딥링크 처리가 getWorkspaceState() 결과에 묶여 있어 수동 로드 경로에서 해시가 영구 미해결될 수 있습니다.
cold start 시 ws가 null이면 해시를 보류하지도, 에러를 띄우지도 않습니다. 그래서 앱을 딥링크로 연 뒤 handleLoadProject()처럼 React state에만 workspace를 채우는 경로로 파일을 불러오면, 같은 hash를 다시 평가할 계기가 없어 링크가 끝내 적용되지 않습니다.
🤖 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 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.
Comment on lines
+83
to
+95
| // Check for deep link on load | ||
| if (window.location.hash.startsWith("#bandscope://")) { | ||
| const uri = window.location.hash.slice(1); | ||
| const parsed = parseDeepLink(uri); | ||
| if (parsed) { | ||
| const targetPack = ws.songs.find(s => "song" in s && s.song?.id === parsed.songId); | ||
| if (targetPack) { | ||
| setSelectedPackId(targetPack.id); | ||
| } else { | ||
| setDeepLinkError("Song not found. Ask the leader to share the .bndscp file first"); | ||
| } | ||
| } | ||
| window.location.hash = ""; // Clear hash after processing |
There was a problem hiding this comment.
sectionId를 버리고 있어 딥링크가 “정확한 섹션”으로 열리지 않습니다.
parseDeepLink()는 songId와 sectionId를 둘 다 주는데 여기서는 pack 선택에만 쓰고 끝납니다. 지금 구현은 노래만 여는 수준이라, “Copy Link”로 공유한 섹션 컨텍스트로 이동/포커스하지 못해 handoff 링크의 핵심 시나리오가 빠집니다.
Also applies to: 103-116
🤖 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 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.
Comment on lines
+19
to
+23
| const handleCopyLink = (sectionId: string) => { | ||
| const link = `bandscope://song/${song.id}/section/${sectionId}`; | ||
| const text = `We're struggling with this section. 1. Open the song file in BandScope. 2. Click this link: ${link}`; | ||
| navigator.clipboard.writeText(text); | ||
| }; |
There was a problem hiding this comment.
복사된 링크가 producer/consumer 계약과 쉽게 어긋납니다.
여기는 song.id와 sectionId를 그대로 URI에 넣지만, consumer 쪽은 [a-zA-Z0-9-]+만 허용합니다. 현재 타입 계약은 ID를 임의 string으로 받아서, 공백이나 _가 포함된 프로젝트를 열면 “Copy Link”가 즉시 재열 수 없는 깨진 링크를 만들 수 있습니다.
As per coding guidelines, "Treat files, URLs, metadata, model artifacts, and project files as untrusted input".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/features/workspace/SectionRoadmap.tsx` around lines 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.
Comment on lines
+17
to
+32
| export function parseDeepLink(uri: string): ParsedDeepLink | null { | ||
| if (!validateBandScopeUri(uri)) { | ||
| return null; | ||
| } | ||
|
|
||
| // bandscope://song/[songId]/section/[sectionId] | ||
| const match = uri.match(/^bandscope:\/\/song\/([a-zA-Z0-9-]+)\/section\/([a-zA-Z0-9-]+)$/); | ||
| if (!match) { | ||
| return null; | ||
| } | ||
|
|
||
| return { | ||
| songId: match[1], | ||
| sectionId: match[2], | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
보안 검증 접근 방식이 적절합니다.
이중 검증(validateBandScopeUri + 엄격한 정규식)은 딥링크 보안을 위한 좋은 방어 전략입니다. 문자 클래스 [a-zA-Z0-9-]가 경로 순회 및 인젝션 공격을 방지합니다.
타입 안전성 개선 권장:
match[1]과 match[2]는 TypeScript에서 string | undefined로 추론될 수 있습니다. 정규식 매칭 성공 시 캡처 그룹이 항상 존재하지만, 명시적 null 체크나 non-null assertion을 추가하면 더 안전합니다.
♻️ 타입 안전성 개선 제안
const match = uri.match(/^bandscope:\/\/song\/([a-zA-Z0-9-]+)\/section\/([a-zA-Z0-9-]+)$/);
- if (!match) {
+ if (!match || !match[1] || !match[2]) {
return null;
}
return {
songId: match[1],
sectionId: match[2],
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/lib/deepLink.ts` around lines 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.
Comment on lines
12
to
+17
| include: ["src/App.tsx", "src/lib/export.ts"], | ||
| thresholds: { | ||
| lines: 90, | ||
| functions: 90, | ||
| branches: 90, | ||
| statements: 90 | ||
| lines: 70, | ||
| functions: 70, | ||
| branches: 70, | ||
| statements: 70 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
새로 추가된 lib 파일들이 커버리지 측정에서 제외됨.
src/lib/deepLink.ts와 src/lib/annotations.ts가 커버리지 include 목록에 없습니다. 이 파일들은 보안에 민감한 딥링크 검증과 어노테이션 병합 로직을 포함하므로 커버리지 측정에 포함시키는 것이 좋습니다.
♻️ 커버리지 include 목록 확장 제안
coverage: {
provider: "v8",
- include: ["src/App.tsx", "src/lib/export.ts"],
+ include: ["src/App.tsx", "src/lib/export.ts", "src/lib/deepLink.ts", "src/lib/annotations.ts"],
thresholds: {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| include: ["src/App.tsx", "src/lib/export.ts"], | |
| thresholds: { | |
| lines: 90, | |
| functions: 90, | |
| branches: 90, | |
| statements: 90 | |
| lines: 70, | |
| functions: 70, | |
| branches: 70, | |
| statements: 70 | |
| include: ["src/App.tsx", "src/lib/export.ts", "src/lib/deepLink.ts", "src/lib/annotations.ts"], | |
| thresholds: { | |
| lines: 70, | |
| functions: 70, | |
| branches: 70, | |
| statements: 70 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/vite.config.ts` around lines 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.
Comment on lines
+10
to
+15
| ## Scope | ||
| - **Assignment Semantics**: Allow assigning specific roles to specific band members within the shared workspace. | ||
| - **Contextual Comments**: Enable adding text annotations directly to specific sections or roles in the `SongRehearsalPack` (e.g., "Simplify bassline in Chorus 2"). | ||
| - **Approvals & Status**: Let band members mark their assigned parts as "Ready" or "Needs Help." | ||
| - **Cloud Sync Backbone**: Introduce an opt-in cloud synchronization mechanism to replace local file sharing, allowing real-time or near-real-time updates to the rehearsal workspace. | ||
|
|
There was a problem hiding this comment.
Scope 섹션이 승인된 범위 축소와 모순됩니다.
Line 11-14는 Assignment/Approval/Cloud Sync를 여전히 in-scope로 적고 있는데, 아래 CEO Review에서는 이 셋을 모두 scrap했다고 명시합니다. 지금 문서 상태면 후속 구현자나 QA가 범위를 잘못 해석하기 쉽습니다.
Also applies to: 22-29
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 10-10: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/plans/2026-04-25-v2-collaboration.md` around lines 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.
Comment on lines
+1091
to
+1102
| /** Documented. */ | ||
| function validateAnnotation(value: unknown, path: string): string | null { | ||
| if (!isRecord(value)) return invalidField(path); | ||
| const extraKey = unexpectedKey(value, ["id", "timestamp", "text", "sectionId", "roleId"], path); | ||
| if (extraKey) return extraKey; | ||
| if (typeof value.id !== "string") return invalidField(`${path}.id`); | ||
| if (typeof value.timestamp !== "string") return invalidField(`${path}.timestamp`); | ||
| if (typeof value.text !== "string") return invalidField(`${path}.text`); | ||
| if (typeof value.sectionId !== "string") return invalidField(`${path}.sectionId`); | ||
| if (value.roleId !== undefined && typeof value.roleId !== "string") return invalidField(`${path}.roleId`); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Annotation 검증이 append-only merge 전제를 보장하지 않습니다.
여기는 id, timestamp, text, sectionId를 모두 “문자열인지”만 확인해서 빈 문자열과 임의의 timestamp가 그대로 통과합니다. 그러면 외부 .bndscp 입력 하나로 annotation dedupe/정렬/section 매핑이 깨질 수 있으니, 최소한 non-empty 검증과 parse 가능한 ISO timestamp 검증은 여기서 막아야 합니다.
🛡️ 제안 수정
function validateAnnotation(value: unknown, path: string): string | null {
if (!isRecord(value)) return invalidField(path);
const extraKey = unexpectedKey(value, ["id", "timestamp", "text", "sectionId", "roleId"], path);
if (extraKey) return extraKey;
- if (typeof value.id !== "string") return invalidField(`${path}.id`);
- if (typeof value.timestamp !== "string") return invalidField(`${path}.timestamp`);
- if (typeof value.text !== "string") return invalidField(`${path}.text`);
- if (typeof value.sectionId !== "string") return invalidField(`${path}.sectionId`);
- if (value.roleId !== undefined && typeof value.roleId !== "string") return invalidField(`${path}.roleId`);
+ if (typeof value.id !== "string" || value.id.trim().length === 0) return invalidField(`${path}.id`);
+ if (typeof value.timestamp !== "string" || Number.isNaN(Date.parse(value.timestamp))) {
+ return invalidField(`${path}.timestamp`);
+ }
+ if (typeof value.text !== "string" || value.text.trim().length === 0) return invalidField(`${path}.text`);
+ if (typeof value.sectionId !== "string" || value.sectionId.trim().length === 0) {
+ return invalidField(`${path}.sectionId`);
+ }
+ if (
+ value.roleId !== undefined &&
+ (typeof value.roleId !== "string" || value.roleId.trim().length === 0)
+ ) {
+ return invalidField(`${path}.roleId`);
+ }
return null;
}As per coding guidelines, "Treat files, URLs, metadata, model artifacts, and project files as untrusted input".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/shared-types/src/index.ts` around lines 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.
Comment on lines
+947
to
+949
| const validAnnotation = { id: "1", timestamp: 0, text: "t", sectionId: "s1", roleId: "r1" }; | ||
| expect(() => parseSongRehearsalPack({ ...validPack, packState: "ready", annotations: [{...validAnnotation, extra: 1}] })).toThrow("extra"); | ||
| expect(() => parseSongRehearsalPack({ ...validPack, packState: "ready", annotations: [{...validAnnotation, id: 1}] })).toThrow("id"); |
There was a problem hiding this comment.
테스트 픽스처의 timestamp 타입이 잘못되었습니다.
validAnnotation의 timestamp: 0은 숫자이지만, Annotation 타입은 timestamp: string을 요구합니다. 이로 인해 테스트가 의도한 검증(extra 속성, id 타입)에 도달하기 전에 timestamp 검증에서 실패할 수 있습니다.
🐛 timestamp를 문자열로 수정
- const validAnnotation = { id: "1", timestamp: 0, text: "t", sectionId: "s1", roleId: "r1" };
+ const validAnnotation = { id: "1", timestamp: "2026-04-25T00:00:00Z", text: "t", sectionId: "s1", roleId: "r1" };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const validAnnotation = { id: "1", timestamp: 0, text: "t", sectionId: "s1", roleId: "r1" }; | |
| expect(() => parseSongRehearsalPack({ ...validPack, packState: "ready", annotations: [{...validAnnotation, extra: 1}] })).toThrow("extra"); | |
| expect(() => parseSongRehearsalPack({ ...validPack, packState: "ready", annotations: [{...validAnnotation, id: 1}] })).toThrow("id"); | |
| const validAnnotation = { id: "1", timestamp: "2026-04-25T00:00:00Z", text: "t", sectionId: "s1", roleId: "r1" }; | |
| expect(() => parseSongRehearsalPack({ ...validPack, packState: "ready", annotations: [{...validAnnotation, extra: 1}] })).toThrow("extra"); | |
| expect(() => parseSongRehearsalPack({ ...validPack, packState: "ready", annotations: [{...validAnnotation, id: 1}] })).toThrow("id"); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/shared-types/test/index.test.ts` around lines 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).
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
Comment on lines
+1151
to
+1153
| export function validateBandScopeUri(uri: string): boolean { | ||
| return /^bandscope:\/\/song\/[a-zA-Z0-9-]+\/section\/[a-zA-Z0-9-]+$/.test(uri); | ||
| } |
There was a problem hiding this comment.
딥링크 식별자 길이 상한이 없어 스키마 검증이 느슨합니다.
Line 1152 정규식이 +를 사용해 songId/sectionId 길이에 제한이 없습니다. 비정상적으로 큰 입력을 조기에 거절하도록 길이 상한을 명시하는 편이 안전합니다.
🔧 제안 수정
export function validateBandScopeUri(uri: string): boolean {
- return /^bandscope:\/\/song\/[a-zA-Z0-9-]+\/section\/[a-zA-Z0-9-]+$/.test(uri);
+ return /^bandscope:\/\/song\/[A-Za-z0-9-]{1,128}\/section\/[A-Za-z0-9-]{1,128}$/.test(uri);
}As per coding guidelines, "Keep local backend access on allowlisted IPC or 127.0.0.1 only, with strict schema validation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/shared-types/src/index.ts` around lines 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.
This was referenced Apr 29, 2026
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.