Repository navigation
Retire the legacy menu mirror and make MenuDocument the single authored model - #116
Conversation
Spindle JS tests239 tests +6 239 ✅ +6 5s ⏱️ ±0s Results for commit beb2bf0. ± Comparison against base commit c494f14. This pull request removes 5 and adds 11 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Spindle Rust tests232 tests +10 232 ✅ +10 3s ⏱️ -1s Results for commit beb2bf0. ± Comparison against base commit c494f14. This pull request removes 3 and adds 13 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06625b09bd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
06625b0 to
7685fcd
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3753520619
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let mut project: SpindleProjectFile = serde_json::from_value(raw)?; | ||
| project.migrate_all_menus(); | ||
| Ok(project) |
There was a problem hiding this comment.
Stamp migrated projects with the current schema
When a version-1 project is opened, this migrates its menus but leaves project.schema_version equal to 1; saveProject then passes that object unchanged to serialise_project, which emits the new document-only menu shape under the old version stamp. An older binary therefore accepts the saved file and fails during deserialization instead of rejecting it as version 2. Fresh evidence after the earlier schema-version fix is the new parse_project_accepts_schema_version_one path, which deliberately preserves the parsed version rather than upgrading it to SCHEMA_VERSION.
Useful? React with 👍 / 👎.
| function wireVerticalListNav(items: GeneratedButtonSpec[]): void { | ||
| for (let i = 0; i < items.length - 1; i++) { | ||
| linkNav(items[i], 'navDown', items[i + 1]); | ||
| } |
There was a problem hiding this comment.
Order setup-menu navigation by visual position
For generated audio or subtitle menus with enough tracks and a return target, the fixed-position Back button is appended after every track even though it sits at y = 420; later track or “Subtitles Off” buttons can have larger y values. Wiring the array in insertion order then makes Down from the visually lowest item jump upward to Back (for example, six audio tracks place the last track at y = 492), so the newly generated remote navigation contradicts the displayed layout. Sort by vertical geometry or wire the utility button separately.
Useful? React with 👍 / 👎.
…Fields
Moves the nine legacy flat-menu fields (`buttons`, `backgroundAssetId`,
`defaultButtonId`, `highlightColours`, `backgroundMode`,
`motionDurationSecs`, `motionAudioAssetId`, `motionLoopCount`,
`timeoutAction`) into a private `LegacyMenuFields` struct on `Menu`,
deserialise-only via `#[serde(flatten, default, skip_serializing)]`.
Old project files keep loading; saves stop emitting the legacy keys.
Privacy is the enforcement mechanism — it forces every remaining
reader in `build/` and `validation/` through the compiler.
`Menu::migrate_to_document` now consumes `self.legacy` (via
`mem::take`) instead of reading the public fields directly, and adds
`Menu::doc()`/`doc_mut()` as the infallible accessor for callers
downstream of `SpindleProjectFile::migrate_all_menus` (which runs at
load time in `parse_project`).
`MenuTiming` gains `audioAssetId` — the missing document home for what
was `motionAudioAssetId`; without it, deleting the legacy mirror would
have silently dropped users' motion menu audio. `migrate_to_document`
lifts it, and `Menu::resolved_motion_audio_asset_id` now reads it
instead of the old always-legacy field.
Adds `MenuDocument::motion_loop_duration()` (`None` when
`loop_duration_secs <= 0.0`) and `MenuDocument::buttons()` — a single
shared top-level button-collection helper (scene `Button` nodes joined
with their interaction-graph nodes) that both the build pipeline and
validation now use, so they can't disagree about what counts as a
button.
Since `legacy` is private, `Menu { .., ..Menu::default() }`
struct-update syntax no longer compiles outside `models::menu` (Rust
requires field access for every field in that pattern, even unnamed
ones). Adds `Menu::new(id, name)` and the builder `.with_document(doc)`
as the public construction path for the rest of the crate.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01R9LH9iQtUrLFS2QDLsPZy9
`AuthorableMenuRef` (`build/menu.rs`) is authored-first-with-legacy-
fallback no longer — the document is guaranteed, so every accessor
(`name`, `background_asset_id`, `highlight_colours`, `timeout_action`,
`motion_loop_count`, `default_button_id`, `scene_nodes`, `buttons`)
reads straight off `Menu::doc()`. `AuthorableButtonRef` is now a type
alias for `MenuButtonView`, the shared button view.
`build/navigation.rs`'s `auto_generate_navigation` drops its dead
legacy branch entirely — it only ever touches the document now.
`build/capacity.rs`'s disc-capacity estimate read `menu.background_mode`/
`menu.motion_duration_secs` directly, ignoring document-authored motion
menus entirely (an existing sizing bug: such menus were estimated as
stills). It now reads `menu.doc().background_mode` and
`doc.motion_loop_duration()`.
`build/executor/mod.rs`'s menu-render path switches its `Menu { .. }`
literal to `Menu::new(..).with_document(..)` (see prior commit).
`validation/scene.rs`'s `validate_button_video_usage` iterates
`menu.doc().buttons()` (the shared helper) instead of the legacy
`menu.buttons`.
`validation/menu.rs`'s `validate_menus` is rewritten against the
document (closes #29 — scene-only menus failing validation): button-
count checks (36 hard limit, 18 density warning) and per-button checks
(no-action, dangling-nav, no-navigation) now run off
`doc().buttons()`; menu-level checks (background asset, motion
duration/audio, timeout action) always run regardless of button count;
`menu.no-buttons` stays a Warning but no longer `continue`s past the
rest of the per-menu checks. The separate authored-vs-legacy action
validation split collapses into one path since buttons only ever come
from the document now.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01R9LH9iQtUrLFS2QDLsPZy9
`Menu`'s legacy fields are now deserialise-only and private outside `models::menu`, so every test fixture that built a menu via a struct literal with the nine legacy fields (or relied on `..Menu::default()` from outside that module) now builds a document via `Menu::new(id, name).with_document(doc)` instead. `build/test_support.rs`'s `test_menu`/`test_menu_with_action` build a single-button authored document (mirroring what `migrate_to_document` would have lifted) rather than legacy fields. Adds `push_button` and `focus_node_mut` helpers standing in for the old `menu.buttons.push(..)` / `menu.buttons[i].nav_* = ..` patterns used throughout `build/authoring/tests.rs`. Replaces `menu_migration_lifts_legacy_fields` (`models/mod.rs`) with `legacy_menu_json_migrates_every_field_and_never_serialises_legacy_keys_again`: a full migration round-trip built from legacy-shaped project JSON (deserialise is the only way to construct legacy fields now), covering all nine fields including `motionAudioAssetId` landing in `timing.audioAssetId`, and asserting the re-serialised menu object contains only `id`/`name`/`authoredDocument` — no legacy keys. `build/authoring/tests.rs`'s aspect tests no longer call `migrate_to_document` on an already-migrated `test_menu()` (a no-op now that it always builds with a document) — they set `doc_mut().compile_policy.display_aspect` directly instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R9LH9iQtUrLFS2QDLsPZy9
Removes the nine legacy flat-menu fields from the TS `Menu` interface — `authoredDocument` is now the only per-menu payload besides `id`/`name`, and is no longer optional (guaranteed present for any menu that came from `parseProject` or was created in-app). Adds the missing `aspect: AspectMode` field to `MenuSize` and `audioAssetId` to `MenuTiming`, matching the Rust schema. `apps/spindle/src/types/project.ts` re-exports these types unchanged, so `tsc` will now surface every remaining TS reader/writer of the retired fields as a compile error. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R9LH9iQtUrLFS2QDLsPZy9
…nuEditor `project-store.ts`'s `updateMenuDocument` no longer lift-on-demand initialises a document from legacy fields, nor writes a `syncMenu` mirror back to them — `authoredDocument` is guaranteed present (loaded projects are migrated at parse time; in-app menu creation always builds one), so the updater just runs against `menu.authoredDocument` directly and the result is written straight back. `MenuEditor.tsx`'s ten mutation handlers (`handleUpdateButton`, `handleAddButton`, `handleRemoveButton`, `handleUpdateHighlightColours`, `handleSetDefaultFocus`, `handleBackgroundChange`, `handleBackgroundModeChange`, `handleMotionAudioChange`, `handleMotionDurationChange`, `handleMotionLoopCountChange`) drop their hand-rolled legacy-field mirror and route through the store's `updateMenuDocument`, following the pattern the display-aspect and name handlers already used. `handleMotionAudioChange` now writes `timing.audioAssetId` (the document's new home for motion audio). Derived read values (`sceneNodes`, `currentButtons`, `backgroundAsset`, `highlightColours`, `defaultFocusId`) drop their legacy fallbacks and read `authoredDocument` directly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R9LH9iQtUrLFS2QDLsPZy9
…ional nav Closes #36. The chapter-grid/audio-setup/subtitle-setup generators in `menuGenerators.ts` used to build a `MenuButton[]` and derive the document from it via `createGeneratedMenuFromButtons` — that intermediary is gone (`MenuButton[]` isn't a `Menu` field to derive from any more). Generators now build scene nodes + a `MenuInteractionGraph` directly via a new local `GeneratedButtonSpec` working shape, lowered into a `MenuDocument` by `createGeneratedMenuDocument`. While inverting the builder, wires up directional navigation that was previously left `null` everywhere (issue #36 — generated chapter grids shipped with no nav, requiring a manual Auto Nav pass on every page): - `wireGridNav` derives up/down/left/right for the chapter grid from its row/column geometry, then anchors an optional trailing utility row (Previous/Next/Back) onto the grid's last row and chains the utility row's own left/right links. - `wireVerticalListNav` chains up/down through the audio/subtitle setup menus' single-column lists. - `linkNav` sets both a link and its opposite together, so every generated link is bidirectionally traversable — the only way to guarantee no button ends up unreachable from the default focus (chapter-grid button 0 / first list item). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R9LH9iQtUrLFS2QDLsPZy9
Menu construction: `MenusPage.tsx`'s `createMenu` no longer writes the nine legacy fields alongside the document, and aligns its document defaults with the Rust migration's — background colour `#101014` (`DEFAULT_MENU_BACKGROUND_COLOUR`, new in `types/project.ts`) and `safeAreaMode: 'action-safe'` (`createDefaultMenuCompilePolicy` was `'title-safe'`) — so there is one canonical set of authored-document defaults instead of the Rust and TS sides quietly disagreeing. Adds `getMenuButtons(menu)` to `menuProjectHelpers.ts`: the shared "what counts as a button" view (top-level scene `button` nodes joined with their interaction-graph nodes), mirroring `MenuDocument::buttons()` on the Rust side. `LogsPage.tsx` (button count) and `PlannerPage.tsx` (button count, motion mode/duration) now use it instead of reading the legacy `buttons`/`backgroundMode`/ `motionDurationSecs` fields; `menuProjectHelpers.ts`'s own connection- count graph walk uses it too instead of its authored-vs-legacy button-source branch. Drops the remaining authored-first-with-legacy-fallback reads in `MenuMap.tsx` (edge extraction, background mode) and `MenuListItem.tsx` (preview blocks, mode label) — `authoredDocument` is read directly, no `?? menu.<legacyField>` fallback. `MenuLevelInspector.tsx`'s background/motion inspector section (previously reading `menu.backgroundMode`/`backgroundAssetId`/`motionAudioAssetId`/ `motionDurationSecs`/`motionLoopCount` throughout) now reads the `document` prop it's already given, including `timing.audioAssetId` for the motion audio picker; its background-colour input fallback moves from `#0f0e1a` to the canonical `DEFAULT_MENU_BACKGROUND_COLOUR`. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R9LH9iQtUrLFS2QDLsPZy9
… nav test
Rewrites every menu fixture across the vitest suite
(SceneEditor.test.tsx, project-store.test.ts, MenusPage.test.tsx,
PlannerPage.test.tsx, MenuMap.test.tsx, LogsPage.test.tsx) to build
`authoredDocument` directly instead of the retired legacy fields,
matching the new `Menu`/`MenuSize`/`MenuTiming` shapes.
`project-store.test.ts`'s `updateMenuDocument` describe block drops
the tests for behavior that no longer exists (lift-on-demand
document initialisation from legacy fields, NTSC/PAL-aware
initialisation, syncing scene changes back to a legacy buttons array)
and adds one for the new no-op contract: a menu with no
`authoredDocument` is left untouched.
`LogsPage.test.tsx`'s button-count fixture was `{ buttons: [...] } as
any` — type-safe under the old code, but would have silently reported
zero buttons at runtime under the new `getMenuButtons`-based counting
since it carried no `authoredDocument`. Rebuilt as a real two-button
document.
Adds `menuGenerators.test.ts` (issue #36): drives
`buildChapterMenusForTitleset` and proves, via an actual directed-graph
BFS over each button's four nav pointers from `defaultFocusId`, that
every generated button is reachable — covering both a multi-page grid
with a Previous/Next/Back utility row and a single-page grid with none.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01R9LH9iQtUrLFS2QDLsPZy9
`pnpm validate` runs `format:check` before `build`, so CI's fresh checkout never sees the rollup output — but any local tree that has built the plugins fails the check on `plugins/*/dist-js/*`. The directory is already git-ignored; align prettier with that. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R9LH9iQtUrLFS2QDLsPZy9
`Menu::migrate_to_document` returned early whenever `authored_document` was already present, so a menu carrying both a document (from the old editor's mirror-on-open sync layer) and a legacy `motionAudioAssetId` lost the audio asset on load — the sync layer mirrored every legacy field into `authoredDocument` except motion audio, since old `MenuTiming` had no audio field to mirror it into. Backfill `doc.timing.audio_asset_id` from the legacy field in that case (without clobbering audio already authored directly on the document), and add `Menu::ensure_document` as the single per-menu migration entry point (`migrate_to_document` + the two backfills that already followed it) so callers can't invoke one step without the others. Covered by `migrate_to_document_backfills_motion_audio_when_document_already_exists`, which round-trips a project with both a document (no audio) and legacy audio and asserts the audio lands in the document with no legacy keys resurfacing on save. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R9LH9iQtUrLFS2QDLsPZy9
…ands `Menu::doc()`/`doc_mut()` assume migration already ran, but the guest-js `Menu`/`SpindleProjectFile` types still allow `authoredDocument: null`, so a payload built or mutated directly in the webview can reach an IPC command unmigrated and panic inside `doc()`. Only `parse_project` (the load path) ran `migrate_all_menus`/`ensure_document` before this. Call the same migration defensively on entry to every command that reaches `doc()`/`doc_mut()`, directly or via the build/validation pipelines: `validate_project`, `estimate_disc_capacity`, `generate_build_plan`, `execute_build`, `export_menu_render_preview` (all via `migrate_all_menus`), and `auto_generate_menu_nav` (via `ensure_document`, since it only receives a single `Menu` with no project context). `export_diagnostics` only reads plain summary fields and never calls `doc()`, so it's left alone. Idempotent in all cases — a no-op for an already-migrated payload. Tests cover `validate_project`, `estimate_disc_capacity`, and `auto_generate_menu_nav` with an unmigrated (`authored_document: None`) payload, asserting sensible behaviour instead of a panic. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R9LH9iQtUrLFS2QDLsPZy9
…n counts Two regressions from the same root cause — reading only top-level scene buttons instead of the full interaction graph — reappeared during the `MenuDocument` inversion: - `validate_menus` validated action targets only via the per-button loop over `doc.buttons()`, which only sees top-level `Button` scene nodes. A button nested inside a `Group`, or a focus node orphaned after its scene node was deleted, escaped target validation entirely — a dangling `showMenu`/`playTitle`/etc. target on either went unreported. Restore a pass over every `interaction.nodes[*]` entry that validates its action's target, keeping the per-button loop for dead-end and nav-link checks (which need `doc.buttons()` geometry/labels). Each action is target-validated exactly once. Covered by `dangling_actions_on_group_nested_and_orphaned_interaction_nodes_are_flagged`. - `computeMenuConnectionCounts` (TS) summed edges via `getMenuButtons`, which has the same top-level-only blind spot, so a group-nested button's outgoing edge was invisible to connection counts even though `MenuMap` draws it — showing a false "unconnected" badge for a menu that's actually reachable. Walk `interaction.nodes` directly instead. Covered by a test asserting a group-nested edge yields `incoming >= 1` on the target menu. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R9LH9iQtUrLFS2QDLsPZy9
`auto_generate_navigation` re-derived "what counts as a button" with its own inline `filter_map` over `scene.nodes`, duplicating (and drifting from) the canonical definition in `MenuDocument::buttons()` that the build pipeline and validation already share. Use `buttons()` instead, so auto-nav doesn't silently skip grouped buttons once group flattening lands there. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R9LH9iQtUrLFS2QDLsPZy9
…oin unification Three MenuEditor-adjacent fixes: - Clearing the motion-duration input snapped back to the previous value instead of clearing, because `handleMotionDurationChange` fell back to `document.timing.loopDurationSecs` on `null`. Write the `0.0` "unset" sentinel instead (`MenuDocument::motion_loop_duration` already treats `<= 0.0` as not-authored on the Rust side), and have `MenuLevelInspector` render that sentinel as an empty input rather than a literal `0`. - Deleting a button left other buttons' `navUp`/`navDown`/`navLeft`/ `navRight` pointing at the deleted id, surfacing only later as a `menu.dangling-nav-ref` validation error instead of being cleaned up at delete time. `handleRemoveButton` now nulls any surviving interaction node's nav ref that pointed at the deleted button. - `getMenuButtons` (`menuProjectHelpers.ts`) gains the geometry/ highlight/video fields the legacy `MenuButton` shape carried (`bounds`, `highlightMode`, `highlightKeyframes`, `videoAssetId`), and `MenuEditor`'s `currentButtons` now consumes it directly instead of re-deriving its own scene-node/interaction-node join — a single "what counts as a button" view shared with `MenuMap`'s button-count display (`getMenuButtons(...).length` in place of ad-hoc `scene.nodes` filtering). Covered by unit tests in `menuProjectHelpers.test.ts` (the extended view's fields, its defaults when a scene node omits them, and that it still only sees top-level buttons) and integration tests in `MenusPage.test.tsx` (clearing to the sentinel, nav-ref cleanup on delete). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R9LH9iQtUrLFS2QDLsPZy9
…pped store edits - Pin `estimate_disc_capacity`'s existing behaviour for a motion menu whose loop duration is unset (`<= 0.0`): it falls back to `STILL_MENU_BYTES` rather than contributing 0 bytes, an intentional choice (an unset duration is an authoring gap, not "no menu overhead") that had no test guarding it. - `updateMenuDocument`'s early return for a menu with no `authoredDocument` — which should not happen in practice, since every menu loaded via `parseProject` or created in-app has one — now logs via `console.error` instead of silently dropping the edit, giving a diagnostic trail instead of what looks like a dead editor. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R9LH9iQtUrLFS2QDLsPZy9
The document-only menu shape landed on this branch stopped emitting the nine legacy flat-menu fields (`backgroundAssetId`, `buttons`, etc.) on save, but `schemaVersion` stayed at `1`. A project saved by this binary and opened by the preceding release would still pass that release's version guard, then fail deserialisation with a confusing "malformed project" error instead of a clean "unsupported schema" rejection. Bumping `SCHEMA_VERSION` to `2` fixes this going forward: older binaries reject v2 files cleanly via `SchemaVersionTooNew`. The loader already accepts both versions unconditionally (the guard only rejects versions *newer* than `SCHEMA_VERSION`), and `migrate_all_menus` already runs regardless of the stamped version, so v1 files keep loading and migrating exactly as before. Extracted `parse_project`'s body into a free `parse_project_json` function so the version guard and round-trip behaviour are unit testable without an `AppHandle`, and added tests covering: a v1 file loading under v2, a current-version file round-tripping, and a newer-than-supported version being rejected.
Autogenerated chapter menus now ship with directional remote-navigation links and a default focus button instead of unconnected buttons, but that user-visible behaviour change wasn't described anywhere. Add SPEC.md section 14.5 covering the wiring generated menus get (geometry-derived directional links, guaranteed bidirectionality so no button is stranded, utility-row anchoring, and default-focus selection) so it's documented alongside the rest of the menu authoring system. README.md's feature list stays high-level and doesn't itemise individual generator behaviours, so no change was needed there.
Silences `clippy::field_reassign_with_default` on the two new `parse_project_json` tests — no behaviour change.
3753520 to
beb2bf0
Compare
## Summary - Prepare the v0.4.0 release: bump all release-facing manifest versions (root and desktop `package.json`, `tauri.conf.json`, both Cargo manifests, `Cargo.lock`) from 0.3.0 to 0.4.0. - Since 0.3.0, this brings in the full motion-menu authoring stack (per-format constraint profiles, semantic menu roles, colour-management rules, motion menus with animated button highlights, a timeline editor, and supporting docs — PRs #116–#128) and the audio/DVD-compatibility fixes (bitrate ceilings, DTS refusal, authored audio track order, titleset audio consistency validation — PRs #137–#138). ## Test plan - [x] `pnpm release:version:check` — all manifests synchronised at 0.4.0 - [x] `pnpm validate` (format:check, vitest, tsc build, cargo fmt/clippy -D warnings, cargo nextest) — all green
Summary
Data model
MenuDocumentis now the single authored model. The nine legacy flatMenufields become a private, deserialize-only shape; old project files keep loading via a one-way migration on load, and saves stop emitting the legacy keys.timing.audioAssetId), with a migration backfill that also covers mirror-era files carrying both a document and the legacy audio field — previously that shape silently lost the user's audio bed on the next save.Editor and generators
Validation and build
Test plan
pnpm validategreen in the dev container (prettier, tsc, vitest, clippy-D warnings, nextest)Closes #29
Closes #36
🤖 Generated with Claude Code
https://claude.ai/code/session_01R9LH9iQtUrLFS2QDLsPZy9
Stack
Part of the rich menu editor stack — merge bottom → top: