feat: add shared rehearsal contract baseline#37
Merged
Conversation
|
Caution Review failedThe pull request is closed. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (8)
Cache: Disabled due to Reviews > Disable Cache setting Disabled knowledge base sources:
📝 WalkthroughSummary by CodeRabbit릴리스 노트
Walkthroughpackages/shared-types에 리허설 도메인 타입·상수·런타임 검증자·데모 팩토리(createDemoRehearsalSong) 및 파서가 추가되었고, 데스크톱 앱은 해당 데모 데이터를 useMemo로 생성해 섹션·역할·화음·신뢰도·수동 오버라이드를 렌더하도록 UI·테스트·로컬라이제이션과 테스트 스크립트를 확장되었습니다. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App (컴포넌트)
participant Factory as createDemoRehearsalSong()
participant Validator as parseRehearsalSong / isRehearsalSong
participant UI as React Renderer
App->>Factory: 데모 RehearsalSong 생성 요청
Factory-->>App: RehearsalSong 객체 반환
App->>Validator: 런타임 검증 호출 (is/parse)
Validator-->>App: 검증 결과 / 에러 경로 반환
App->>UI: 제목·exportSummary 렌더
App->>UI: 섹션 반복 → label·groove·section confidence 렌더
App->>UI: 역할 반복 → name·harmony·cue·role confidence·manual overrides 렌더
UI-->>App: 렌더 완료
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.tsx`:
- Around line 42-47: The list rendering uses role.manualOverrides.map and sets
key={override.field}, which can collide because ManualOverride currently allows
only field:"harmony"; update the key to be unique and stable by including either
an existing unique identifier (e.g., override.id) or, if none exists, compose a
deterministic key from multiple override properties or the map index (e.g.,
`${override.field}-${override.source}-${override.value.chord}` or
`${override.field}-${i}`) in the span's key prop to avoid duplicate keys during
rendering.
- Line 14: The call to createDemoRehearsalSong() in App.tsx creates a new
instance on every render; change the assignment of rehearsalSong to use React's
useMemo so the demo song is created once (or only when relevant inputs change),
e.g. import useMemo from React and replace the direct call with a memoized
factory (() => createDemoRehearsalSong()) with an appropriate dependency array
(empty if no dependencies or include props/state that should trigger
recreation).
In `@packages/shared-types/src/index.ts`:
- Around line 87-89: The isRecord type guard currently returns true for arrays
because typeof [] === "object"; update isRecord to explicitly exclude arrays by
adding a check such as !Array.isArray(value) so it only returns true for plain
object records (keep the existing null check and object typeof check). Locate
the function isRecord and modify its predicate to include the Array.isArray
exclusion while preserving the return type value is Record<string, unknown>.
🪄 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: e4ba9864-9292-4969-a6cf-b939ab797235
📒 Files selected for processing (8)
ARCHITECTURE.mdapps/desktop/package.jsonapps/desktop/src/App.test.tsxapps/desktop/src/App.tsxapps/desktop/src/locales/en/common.jsonapps/desktop/src/locales/ko/common.jsonpackages/shared-types/src/index.tspackages/shared-types/test/index.test.ts
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.tsx`:
- Line 33: The UI currently renders internal tokens like
section.confidence.level and section.confidence.source directly (e.g., "medium",
"high", "model", "user"); instead map those internal values to user-facing
labels via the i18n translator instead of rendering raw tokens: create a small
mapping function or lookup (e.g., a getConfidenceLabel(level) and
getConfidenceSourceLabel(source) used where App.tsx renders
{section.confidence.level} and {section.confidence.source}, and use t(...) keys
like "confidence.level.medium"/"confidence.level.high" and
"confidence.source.model"/"confidence.source.user" so the UI shows
rehearsal-friendly, localized phrases; apply the same mapping for the other
occurrences noted (lines rendering section.confidence at 41-42 and 46).
In `@packages/shared-types/src/index.ts`:
- Around line 91-92: The current isStringArray type guard allows sparse arrays
(holes) because Array.prototype.every skips empty slots; update isStringArray to
also verify the array is dense (no missing indices) before confirming all items
are strings—e.g., ensure for every index from 0 to value.length-1 the index
exists (or use a length vs present-count check) and then check typeof each
element === "string"; apply the same dense-array check to the other array guards
referenced (the helpers around lines with the same pattern used by
parseRehearsalSong and the guards at 156-157, 168-169, 318-319) so
parseRehearsalSong cannot receive undefined slots for sections[0], roles[0],
manualOverrides[0], or focusSections[0].
🪄 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: 51ff440d-0699-48c0-adfc-86cbb8efbb10
📒 Files selected for processing (3)
apps/desktop/src/App.tsxpackages/shared-types/src/index.tspackages/shared-types/test/index.test.ts
There was a problem hiding this comment.
♻️ Duplicate comments (2)
apps/desktop/src/App.tsx (1)
32-46:⚠️ Potential issue | 🟡 Minor내부 계약 토큰을 그대로 노출하지 마세요.
medium,model,user를 그대로 출력해서 ko-KR에서도 영어 내부값이 보입니다. 표시용 라벨로 한 번 매핑해서 리허설 문맥의 문구로 바꾸는 편이 맞습니다.As per coding guidelines, "Prefer practical, friendly, rehearsal-first wording over academic or authority-heavy language in UX copy and UI text".♻️ 제안된 수정
export function App() { const t = createTranslator(detectPreferredLocale()); const rehearsalSong = useMemo(() => createDemoRehearsalSong(), []); + const confidenceLabels = { + low: t("confidenceLow"), + medium: t("confidenceMedium"), + high: t("confidenceHigh") + } as const; + const provenanceLabels = { + model: t("provenanceModel"), + user: t("provenanceUser") + } as const; @@ <p> - {t("sectionConfidence")}: {section.confidence.level} ({section.confidence.source}) + {t("sectionConfidence")}: {confidenceLabels[section.confidence.level]} ( + {provenanceLabels[section.confidence.source]}) </p> @@ - <span> - {t("roleConfidence")} {role.confidence.level}</span> - <span> - {t("harmonySource")} {role.harmony.source}</span> + <span> - {t("roleConfidence")} {confidenceLabels[role.confidence.level]}</span> + <span> - {t("harmonySource")} {provenanceLabels[role.harmony.source]}</span> {role.manualOverrides.map((override, index) => ( <span key={`${override.field}-${override.source}-${override.value.chord}-${index}`}> {" "} - - {t("manualOverride")} {override.value.chord} ({override.source}) + - {t("manualOverride")} {override.value.chord} ( + {provenanceLabels[override.source]}) </span> ))}
apps/desktop/src/locales/en/common.json와apps/desktop/src/locales/ko/common.json에도 대응 키를 같이 추가해 주세요.🤖 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 32 - 46, The UI is directly rendering internal tokens like medium, model, and user (seen where section.roles and role.manualOverrides values are output), so replace direct outputs with a localized label lookup (e.g., use t("medium."+medium) or a helper like getDisplayLabel("medium", medium)) wherever section.confidence.source, role.harmony.source, and override.source/value are rendered; add corresponding keys under medium.*, model.*, and user.* to both en/common.json and ko/common.json to provide rehearsal-friendly copy. Ensure the rendering uses the t() lookup instead of raw token strings and keep existing keys for confidence/harmony labels (section.confidence, roleConfidence, harmonySource, manualOverride) unchanged.packages/shared-types/src/index.ts (1)
91-92:⚠️ Potential issue | 🟠 Major희소 배열이 아직 계약 검증을 통과합니다.
여기서는
every()가 빈 슬롯을 건너뛰기 때문에sections: new Array(1)같은 payload도isRehearsalSong()을 통과할 수 있습니다. 그러면parseRehearsalSong()이undefined슬롯을 가진 값을 정상 계약으로 반환하게 됩니다.🔧 제안된 수정
+function isArrayOf<T>( + value: unknown, + predicate: (item: unknown) => item is T +): value is T[] { + return ( + Array.isArray(value) && + Array.from({ length: value.length }, (_, index) => + index in value && predicate(value[index]) + ).every(Boolean) + ); +} + function isStringArray(value: unknown): value is string[] { - return Array.isArray(value) && value.every((item) => typeof item === "string"); + return isArrayOf(value, (item): item is string => typeof item === "string"); } @@ - Array.isArray(value.manualOverrides) && - value.manualOverrides.every((override) => isManualOverride(override)) + isArrayOf(value.manualOverrides, isManualOverride) @@ - Array.isArray(value.roles) && - value.roles.every((role) => isRehearsalRole(role)) + isArrayOf(value.roles, isRehearsalRole) @@ - Array.isArray(value.sections) && - value.sections.every((section) => isRehearsalSection(section)) && + isArrayOf(value.sections, isRehearsalSection) &&Also applies to: 156-157, 168-169, 178-178, 318-319
🤖 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 91 - 92, The array type-guard isStringArray (and other similar guards that use Array.every) wrongly accepts sparse arrays because every() skips holes; modify isStringArray to first confirm Array.isArray(value) and that Object.keys(value).length === value.length (no holes), then run value.every(item => typeof item === "string"); apply the same pattern to the other array-guard functions with the same structure so parseRehearsalSong and related validators no longer accept sparse arrays.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/desktop/src/App.tsx`:
- Around line 32-46: The UI is directly rendering internal tokens like medium,
model, and user (seen where section.roles and role.manualOverrides values are
output), so replace direct outputs with a localized label lookup (e.g., use
t("medium."+medium) or a helper like getDisplayLabel("medium", medium)) wherever
section.confidence.source, role.harmony.source, and override.source/value are
rendered; add corresponding keys under medium.*, model.*, and user.* to both
en/common.json and ko/common.json to provide rehearsal-friendly copy. Ensure the
rendering uses the t() lookup instead of raw token strings and keep existing
keys for confidence/harmony labels (section.confidence, roleConfidence,
harmonySource, manualOverride) unchanged.
In `@packages/shared-types/src/index.ts`:
- Around line 91-92: The array type-guard isStringArray (and other similar
guards that use Array.every) wrongly accepts sparse arrays because every() skips
holes; modify isStringArray to first confirm Array.isArray(value) and that
Object.keys(value).length === value.length (no holes), then run value.every(item
=> typeof item === "string"); apply the same pattern to the other array-guard
functions with the same structure so parseRehearsalSong and related validators
no longer accept sparse arrays.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0208c697-69a2-48ee-9285-1ff9183b8929
📒 Files selected for processing (8)
ARCHITECTURE.mdapps/desktop/package.jsonapps/desktop/src/App.test.tsxapps/desktop/src/App.tsxapps/desktop/src/locales/en/common.jsonapps/desktop/src/locales/ko/common.jsonpackages/shared-types/src/index.tspackages/shared-types/test/index.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.tsx`:
- Around line 50-55: The UI text for role confidence, harmony source, and manual
overrides lacks a separator between label and value; update the JSX where
confidenceLabels, provenanceLabels, and role.manualOverrides are rendered (the
spans using t("roleConfidence"), t("harmonySource"), and t("manualOverride") and
the map over role.manualOverrides) to insert a colon and space after each
translated label (e.g., change "roleConfidence" to render as
"{t('roleConfidence')}: " and likewise for "harmonySource" and the
manualOverride label) so labels read like "Confidence: Needs ear check",
"Harmony source: Auto-detected", and "Manual override: C#m11".
In `@packages/shared-types/src/index.ts`:
- Around line 91-96: isDenseArray currently counts enumerable keys which allows
sparse arrays with added properties (e.g., new Array(1) with a custom prop) to
pass; update isDenseArray to first ensure Array.isArray(value) then explicitly
verify every index from 0 to value.length-1 actually exists (e.g., using the
"in" operator or Object.prototype.hasOwnProperty.call(value, String(i))) so
holes fail the check; keep isArrayOf using isDenseArray unchanged and add a
regression test that new Array(1) with an attached non-index property does not
pass isDenseArray/isArrayOf for the expected element predicate.
🪄 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: 235cac5b-da7f-4448-8af3-e5a0841b03a5
📒 Files selected for processing (6)
apps/desktop/src/App.test.tsxapps/desktop/src/App.tsxapps/desktop/src/locales/en/common.jsonapps/desktop/src/locales/ko/common.jsonpackages/shared-types/src/index.tspackages/shared-types/test/index.test.ts
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 329-335: The parseRehearsalSong function currently throws a
generic Error when isRehearsalSong fails; update it to provide specific
validation failure details by enhancing the check (or adding a small validator)
to identify which field(s) failed and include that information in the thrown
Error message; reference parseRehearsalSong and isRehearsalSong (or a new helper
like validateRehearsalSong) so the error becomes e.g. "Invalid rehearsal song
contract: missing/invalid field 'xyz'" rather than the generic message.
🪄 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: d1a71eb7-0e83-4cce-879b-af461399c991
📒 Files selected for processing (4)
apps/desktop/src/App.test.tsxapps/desktop/src/App.tsxpackages/shared-types/src/index.tspackages/shared-types/test/index.test.ts
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
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/test/index.test.ts`:
- Around line 71-78: The test uses inconsistent optional chaining which can
throw if roles[2] is undefined; update the property accesses to guard every
dereference — change roles[2]?.harmony.source to roles[2]?.harmony?.source and
change roles[2]?.manualOverrides[0] to roles[2]?.manualOverrides?.[0] (or
roles[2]?.manualOverrides?.[0]) so accesses to harmony.source and
manualOverrides[0] are safe when roles[2] or those nested properties are
undefined.
🪄 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: a255f8b3-e252-4d59-8255-7edc45b12534
📒 Files selected for processing (2)
packages/shared-types/src/index.tspackages/shared-types/test/index.test.ts
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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/test/index.test.ts`:
- Around line 81-91: The test verifying immutability for createDemoRehearsalSong
should defensively use optional chaining when mutating the fixture: change the
mutation call in the test (the line that currently calls
first.sections[0]?.roles[2]?.manualOverrides.splice(...)) to use
manualOverrides?.splice(...) so it safely handles cases where manualOverrides
may be undefined in future fixture shapes; update the expression that mutates
first to call manualOverrides?.splice(0, 1) while keeping all other expectations
intact.
🪄 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: d8c48be1-91b0-42a8-ad40-563b1eae232b
📒 Files selected for processing (3)
ARCHITECTURE.mdpackages/shared-types/src/index.tspackages/shared-types/test/index.test.ts
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai resume |
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
This was referenced Mar 12, 2026
This was referenced Mar 25, 2026
This was referenced Apr 25, 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
song -> section -> roleshared contract baseline with confidence, provenance, manual overrides, export summary, and runtime parsing helpersARCHITECTURE.mdVerification
npm run test --workspaces --if-presentnpm run typecheck --workspaces --if-present./scripts/harness/quickcheck.sh📝 Walkthrough
Walkthrough
packages/shared-types에 리허설 도메인 타입·런타임 검증자·데모 팩토리(createDemoRehearsalSong) 및 파서가 추가되고, 데스크톱 앱은 해당 데모 데이터를 useMemo로 생성해 섹션·역할·화음·신뢰도·수동 오버라이드를 렌더하도록 UI·테스트·로컬라이제이션을 확장했습니다. 문서 타임스탬프와 테스트 스크립트도 소폭 변경되었습니다.
Changes
packages/shared-types/src/index.tspackages/shared-types/test/index.test.tsapps/desktop/src/App.tsxapps/desktop/src/App.test.tsxapps/desktop/src/locales/en/common.json,apps/desktop/src/locales/ko/common.jsonsectionConfidence,roleConfidence,harmonySource,manualOverride,confidenceLevelLow/Medium/High,provenanceSourceModel/User등 다국어 키 추가.ARCHITECTURE.md,apps/desktop/package.jsonapps/desktop/package.json의 test 스크립트를node -e "mkdirSync('coverage/.tmp', { recursive: true })" && vitest run --coverage로 수정하여 커버리지 폴더를 사전 생성하도록 변경.Sequence Diagram(s)
sequenceDiagram participant App as App 컴포넌트 participant Factory as createDemoRehearsalSong (shared-types) participant Validator as isRehearsalSong / parseRehearsalSong participant UI as React 렌더러 App->>Factory: 데모 리허설 노래 생성 요청 Factory-->>App: RehearsalSong 객체 반환 App->>Validator: isRehearsalSong/parseRehearsalSong로 검증 Validator-->>App: 검증 결과 반환 App->>UI: 제목·exportSummary 렌더 App->>UI: 섹션 반복 → 라벨·groove·section confidence 렌더 App->>UI: 역할 반복 → name·harmony·cue·role confidence·manual overrides 렌더 UI-->>App: 렌더 완료Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
Poem