Skip to content

feat(workspace): name tonight's first ritardando plan on the map - #1047

Open
seonghobae wants to merge 19 commits into
developfrom
feat/workspace-first-ritardando-plan
Open

feat(workspace): name tonight's first ritardando plan on the map#1047
seonghobae wants to merge 19 commits into
developfrom
feat/workspace-first-ritardando-plan

Conversation

@seonghobae

@seonghobae seonghobae commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator

Summary

Name tonight's first ritardando plan on the mounted rehearsal map so the vocal or bass that eases into a slower tempo can open that landing.

  • Engine emits ritardandoPlan only when existing tempo-stability reports a sustained slowing (to_bpm < from_bpm) that is not a half-time (~0.45–0.55) or double-time (~2.0) feel flip.
  • The owned copy lands on the highest-priority active named vocal or bass in the section that contains the change. Vocal is preferred on priority ties. Accompaniment never owns. Heuristic/demo topology stays unnamed.
  • Workspace callout opens the matching rendered map section. Fail-closed resolvers ignore inherited, accessor, or Proxy metadata. Customer copy names the next action in EN/KO.
  • This is not a new MIR product. test(analysis): govern real YouTube known-stem benchmark #828 owns [Product Gap] Add real-audio MIR accuracy acceptance benchmarks #770. Beat times reuse existing librosa.beat.beat_track / analyze_tempo_stability.

Protected base: develop@749511c3ad4000090048718f685c6bee6b3d2c25.
Exact current head: bac0d90d1f1ae88141c93dd50483eebb63ed1e85.

Do not mix with #811, #828, or #897.

Test plan

  • Python ritardando tests (18) at 100% module coverage
  • Rust ritardando-plan contract tests
  • Vitest resolver, i18n, provenance, callout, particle, and workspace armed-state tests

Do not self-approve. Do not wait on checks.


Devin Review

Summary by CodeRabbit

  • 새로운 기능

    • 실시간 분석에서 지속적인 감속을 감지하면 첫 리타르단도 연습 계획을 제공합니다.
    • 계획 안내에서 담당 보컬 또는 베이스, BPM 변화, 위치와 시간을 확인할 수 있습니다.
    • 안내 버튼을 통해 해당 곡 구조의 지도 섹션을 바로 열 수 있습니다.
    • 영어와 한국어 안내 문구 및 섹션 라벨 현지화를 지원합니다.
  • 개선 사항

    • 연습 진행률 변경 후에도 열어 둔 리타르단도 안내가 유지됩니다.
    • 잘못되거나 불완전한 계획 정보는 표시하지 않도록 안정성이 향상되었습니다.
    • 로컬 오디오 분석에서 템포와 비트 정보가 더 효율적으로 재사용됩니다.

Stamp one owned ritardandoPlan from existing tempo-stability slowing
onto the highest-priority active named vocal or bass, and open that
landing on the rehearsal map. Half-time and double-time feel flips
stay unnamed. Heuristic demo topology stays unnamed. This is not a
new MIR product.
@coderabbitai

coderabbitai Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

Next included review available in 20 minutes.

View limit details

Limit 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.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 95465bc9-f513-4587-ab82-b55b1dead512

📥 Commits

Reviewing files that changed from the base of the PR and between 2be5695 and bc63d8a.

📒 Files selected for processing (9)
  • apps/desktop/core/src/lib.rs
  • apps/desktop/src/features/workspace/FirstRitardandoCallout.particle.test.tsx
  • apps/desktop/src/features/workspace/Workspace.ritardando-state.test.tsx
  • apps/desktop/src/features/workspace/Workspace.tsx
  • apps/desktop/src/features/workspace/firstRitardando.test.ts
  • apps/desktop/src/features/workspace/firstRitardando.ts
  • apps/desktop/src/locales/ko/common.json
  • services/analysis-engine/tests/test_api.py
  • services/analysis-engine/tests/test_branch_coverage_contract.py
📝 Walkthrough

Walkthrough

첫 ritardando 계획을 위한 역할 계약, 템포 분석, 캐시 전달, 데스크톱 resolver, 워크스페이스 안내와 지도 탐색이 추가되었습니다. 지속 감속만 처리하며, 반감·배속 전환과 휴리스틱 데모 토폴로지는 제외합니다.

Changes

첫 ritardando 계획

Layer / File(s) Summary
역할 계약과 출처 검증
packages/shared-types/..., apps/desktop/core/...
ritardandoPlanritardandoPlanSource 필드를 추가하고 함께 존재하는지 검증합니다.
템포 분석과 엔진 스탬프
services/analysis-engine/src/bandscope_analysis/...
BPM과 비트 시간을 전달하고, 첫 지속 감속을 적합한 활성 보컬 또는 베이스 역할에 기록합니다.
첫 계획 resolver
apps/desktop/src/features/workspace/firstRitardando.ts
계획, 섹션, 시간, 역할을 검증하고 가장 이른 유효 계획을 선택합니다.
워크스페이스 안내와 탐색
apps/desktop/src/features/workspace/..., apps/desktop/src/i18n/..., apps/desktop/src/locales/...
안내 문구를 현지화하고 지도 섹션으로 이동합니다. 로컬 진행률 변경 뒤에도 열린 상태를 유지합니다.

Estimated code review effort: 4 (Complex) | ~45 minutes

Merge Risk: 🟡 Moderate · up to 2be56

The PR adds ritardando guidance and workspace navigation, but oversized plan text can cause excessive desktop processing, malformed metadata can produce projects that fail to reload, and local updates may reset the armed callout. These bounded correctness and availability issues should be fixed or explicitly accepted before merging.

Sequence Diagram(s)

sequenceDiagram
  participant 오디오분리
  participant 분석엔진
  participant 계획Resolver
  participant 워크스페이스
  participant 지도섹션
  오디오분리->>분석엔진: BPM과 beat_times 제공
  분석엔진->>계획Resolver: 지속 감속 계획 데이터 제공
  계획Resolver->>워크스페이스: 첫 계획과 landing role 반환
  워크스페이스->>지도섹션: 일치하는 section index로 이동
  지도섹션-->>워크스페이스: 열린 계획 상태 표시
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed 제목은 지도에서 첫 ritardando 계획의 소유 역할을 명명하는 핵심 변경을 정확히 요약합니다. 간결하고 구체적이며 변경 사항과 직접 관련됩니다.
Docstring Coverage ✅ Passed Docstring coverage is 91.94% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 124 functions across 25 files. (6 skipped: …
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 91.94% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 124 functions across 25 files. (6 skipped: 6 unsupported.)

✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/workspace-first-ritardando-plan

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

Copy link
Copy Markdown
Collaborator Author

@opencode-agent Work only on the existing canonical branch feat/workspace-first-ritardando-plan for PR #1047, starting from exact current head bac0d90d1f1ae88141c93dd50483eebb63ed1e85 against protected develop@749511c3ad4000090048718f685c6bee6b3d2c25, and only if that head is still current immediately before mutation. Refetch the branch/base/target blobs/review threads/checks before every write; adapt if compatible movement occurred. Apply receiving-code-review + systematic-debugging + test-driven-development + verification-before-completion. Never force-push, self-approve, weaken gates, or touch foreign repositories.

Two current-head findings have been independently revalidated against production source and are BandScope/#1047-owned:

  1. Untrusted persisted model copy can masquerade as a ritardando. Current boundedGeneratedRitardandoPlan in apps/desktop/src/features/workspace/firstRitardando.ts validates only the template and numeric tokens. It therefore accepts a speed-up such as Ease this part from 80 BPM into 120 BPM; let the next downbeat land later. and a half-time feel flip such as 120→60 even though the engine's authoritative is_ritardando_change requires finite positive to_bpm < from_bpm and rejects ratios 0.45..0.55. Treat thread PRRT_kwDORjvEXs6dGvff as valid. RED first: add realistic resolver regressions for at least speed-up and half-time model copy while preserving valid 120→80 and arbitrary bounded user copy. Then make the narrow resolver fix by parsing the already-regex-validated numeric tokens and enforcing the same slowing/half-time invariant before returning model guidance. Do not change user provenance behavior.

  2. The real local-audio pipeline never supplies the beat_times feature consumed by _apply_ritardando. Current _coerce_beat_times looks for audio_features["beat_times"], but _build_local_audio_features does not populate it, so the fallback always re-runs beat tracking on the normalized reconstructed stem sum rather than reusing the decoded/temporal-analysis signal. Treat thread PRRT_kwDORjvEXs6dGvgv as valid only after reproducing it on the fresh head. Compare the existing working TemporalAnalyzer/feature construction path and choose the smallest owning fix that reuses one authoritative beat-time result without introducing a second MIR product or changing source-separation authority. RED first with a pipeline-level regression proving the real local-audio features path supplies/reuses authoritative beat times; then make the narrow causal fix and preserve fail-closed malformed-feature fallback.

Do not duplicate unresolved thread PRRT_kwDORjvEXs6dGNt7's optional Rust role fields here: that persisted-contract defect already has the earlier canonical owner #1040 and must be repaired/merged dependency-root first rather than copied into this ritardando branch.

Verification: focused Vitest resolver tests; focused Python ritardando/pipeline tests; Rust contract tests applicable to this branch; then the canonical full quickcheck and all exposed exact owned statement/branch/function/line coverage, formatting/lint, security and build gates. Resolve only the two #1047 threads actually repaired. Refetch exact new head/live base after push. Do not mark merge-ready or merge without unchanged-head required checks plus qualifying independent approval.

devin-ai-integration[bot]

This comment was marked as resolved.

Copy link
Copy Markdown
Collaborator Author

@opencode-agent Please perform the required independent formal review on exact current head be9f10f465145bc2094c1b9ad69fee4f27fba6a5 against protected develop@749511c3ad4000090048718f685c6bee6b3d2c25. Review only; do not mutate the branch. This head includes the test-first model-copy tempo-semantics repair, section-local role-stamp isolation, and coverage regressions for runtime role-list drift. Verify the exact head and submit APPROVED or CHANGES_REQUESTED anchored to this SHA. Do not use predecessor-head evidence.

devin-ai-integration[bot]

This comment was marked as resolved.

Copy link
Copy Markdown
Collaborator Author

@opencode-agent Repair PR #1047 on its existing canonical branch only. Refetch immediately before every mutation; current evidence is exact head be9f10f465145bc2094c1b9ad69fee4f27fba6a5 against protected develop@749511c3ad4000090048718f685c6bee6b3d2c25. Apply receiving-code-review, systematic-debugging, test-driven-development, and verification-before-completion. Do not race or overwrite a moved head.

Two current-head BandScope-owned gaps remain. First, ownedRitardandoPlan() still accepts a nonblank ritardandoPlan when ritardandoPlanSource is absent, even though shared-types and native persistence now enforce plan/source presence parity. Add the smallest RED resolver/callout regression for source-less valid-looking copy, then fail closed unless source is exactly model or user; preserve valid user copy and the model slowing/half-time/template checks already repaired. Resolve PRRT_kwDORjvEXs6dKgTf only after successor-head proof.

Second, review thread PRRT_kwDORjvEXs6dGvgv remains valid at the production boundary: _apply_ritardando consumes audio_features['beat_times'], while the real _build_local_audio_features path does not populate authoritative beat times, so the normal path silently re-derives a beat grid from the reconstructed normalized stem sum instead of reusing the decoded-audio/TemporalAnalyzer timing source. Write the smallest realistic integration regression through the real local-audio feature construction/apply boundary proving the authoritative beat grid is carried and consumed; then plumb that existing timing evidence through the feature payload rather than adding a second tracker or success-shaped mock. Preserve bounded/fail-closed handling for malformed beat grids and the current explicit fallback only where no authoritative beat evidence legitimately exists.

Run focused ritardando unit/integration tests first, then repository-pinned lint/format and canonical relevant/full analysis/desktop/shared-types verification. Do not touch the native optional-field parity owned by #1040, central workflows, unrelated fade/accelerando lanes, or security/review gates. No force-push, destructive rebase, self-approval, or gate weakening.

devin-ai-integration[bot]

This comment was marked as resolved.

Copy link
Copy Markdown
Collaborator Author

@OpenCode Repair only the existing canonical branch feat/workspace-first-ritardando-plan. Refetch immediately and require exact head 02b58dd5144a1168279b3ade31a51756d9dbf090 against protected develop@749511c3ad4000090048718f685c6bee6b3d2c25; if either moved, inspect/remap the intervening delta rather than overwriting it. Apply receiving-code-review + systematic-debugging + TDD + verification-before-completion.

Current unresolved thread PRRT_kwDORjvEXs6dNjJ3 is valid. Exact source proves the fresh local path decodes the original audio in AudioStemSeparator._load_audio, then _temporal_features_for_request opens/decodes the same source again via TemporalAnalyzer.analyze. The reusable-feature cache also stores/loads only stems/sample rate/separation/role metadata, so every cache hit calls TemporalAnalyzer.analyze and re-decodes the source again. This is a BandScope-owned fast-path/performance/reliability defect, not a correctness excuse to revert source-authoritative beat evidence.

I added the smallest RED at exact head 02b58dd5144a1168279b3ade31a51756d9dbf090: services/analysis-engine/tests/test_temporal_feature_cache.py::test_feature_cache_round_trips_authoritative_tempo_grid. Keep it. It requires source-derived bpm and beat_times to survive feature-cache store/load; current code drops them.

Repair the owning boundary rather than reintroducing reconstructed-stem timing. Acceptance has two parts:

  1. Cache hit: extend the versioned feature-cache metadata contract to validate/store/load finite positive BPM and a finite non-negative beat grid when present, so a valid cached original-source temporal grid is reused without invoking TemporalAnalyzer.analyze again. Malformed temporal metadata must fail closed as a cache miss or omit unusable temporal evidence; never accept NaN/Inf/negative beats.
  2. Fresh analysis: eliminate the second full source decode. Reuse the audio that the separation boundary has already decoded, ideally by extracting a shared in-memory temporal-analysis helper from TemporalAnalyzer and computing compact BPM/beat evidence from that same decoded source before/alongside separation. Return only compact temporal metadata across the worker boundary; do not send raw audio through IPC, do not use reconstructed stem sum as the authoritative signal, and do not add network/model authority. If the cleanest root fix requires AudioStemSeparator to expose the already-decoded signal to the worker-local temporal helper, keep that change narrow and private.

RED first for both paths: retain the cache round-trip regression and add a realistic orchestration regression proving a reusable-feature cache hit does not call the file-decoding TemporalAnalyzer.analyze; add a fresh-path regression that proves one source decode owns both separation and temporal evidence (or an equivalent falsifiable call-count boundary). Preserve bounded file-size/duration/path validation, process timeout, feature-cache schema/version integrity and fallback behavior.

Run focused Python tests, pinned Ruff lint/format, exact 100% owned analysis-engine coverage and full ./scripts/harness/quickcheck.sh; report exact commands/results and successor head. Do not touch central workflows, #828/#811/#897, do not force-push/rebase, weaken gates, or self-approve. Resolve only PRRT_kwDORjvEXs6dNjJ3 after successor exact-head GREEN proves both no repeated cache-hit decode and no duplicate fresh-path decode.

devin-ai-integration[bot]

This comment was marked as resolved.

github-code-quality[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

coderabbitai[bot]

This comment was marked as resolved.

Copy link
Copy Markdown
Collaborator Author

Fresh ownership handoff from #1049 review, verified against current #1047 head 11b88488305a28ca2a1e5f3a61ae119f45c23b2a and protected develop@749511c3ad4000090048718f685c6bee6b3d2c25.

The shared feature-cache path is still coupled to the analysis-result schema: _feature_cache_paths() derives its base from _analysis_cache_path(). That means an ANALYSIS_CACHE_SCHEMA_VERSION bump changes the digest/directory for stem arrays even when FEATURE_CACHE_SCHEMA_VERSION and the separator payload contract are unchanged. #1049 hit this as a current-head review finding after its result-schema bump; #1047 is the earlier active owner because it already owns the shared temporal/feature-cache path and is currently carrying FEATURE_CACHE_SCHEMA_VERSION plus beat-grid persistence.

Please keep this repair in #1047 rather than duplicating it in #1049. Smallest regression: prove a feature-cache path is governed only by FEATURE_CACHE_SCHEMA_VERSION/source identity and does not change when only ANALYSIS_CACHE_SCHEMA_VERSION changes, while the analysis-result path does change. Narrow fix: give feature-cache metadata/arrays their own versioned digest/path instead of deriving them from _analysis_cache_path; preserve current app-owned cacheRoot, source/project identity, fail-closed metadata validation, and existing temporal feature-cache behavior. Focused GREEN should include feature-cache store/load and the temporal no-redecode contracts. After this dependency-root repair is integrated, #1049 should be deliberately restacked/revalidated; predecessor checks/reviews must not transfer.

Copy link
Copy Markdown
Collaborator Author

Ownership correction after a fresh whole-stack pass: do not implement the shared feature-cache schema decoupling in #1047. #1045 is the earlier canonical BandScope owner and its current exact head 284d0a39bd0fe2483eaf2d0d43774eff5a44081e already contains the RED regression (test(cache): preserve stems across result schema bumps) while its current production _feature_cache_paths() remains coupled to _analysis_cache_path(). I have routed the causal fix there against its exact api.py blob.

Keep #1047 scoped to ritardando/temporal behavior and, once #1045 lands the cache repair, deliberately restack/revalidate this branch on that dependency-root result. Do not copy the cache fix here and do not transfer #1045 predecessor checks/reviews.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 new potential issue.

Devin Review

Comment thread services/analysis-engine/src/bandscope_analysis/api.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant