fix(editor): Shift Audio survives save/reopen — persist audio_shift through save, load, and build - #262
Merged
Merged
Conversation
|
Warning Review limit reached
Next review available in: 22 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAudio shift state is parsed from requests, restored from sloppak manifests, sent by editor builds, and persisted across regular saves, Save As, and builds. Explicit zero removes the manifest value, while numeric values are rounded to six decimals. ChangesAudio shift persistence
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Editor
participant routes.py
participant _write_sloppak_pak
participant SloppakManifest
Editor->>routes.py: send audio_shift during save or build
routes.py->>_write_sloppak_pak: pass parsed value in metadata
_write_sloppak_pak->>SloppakManifest: write or remove audio_shift
SloppakManifest-->>routes.py: provide audio_shift on load
routes.py-->>Editor: restore audio_shift
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@routes.py`:
- Around line 88-93: Preserve invalid audio-shift values instead of converting
them to zero: in routes.py:88-93, return the existing absent/invalid sentinel
for parse failures and non-finite values; in routes.py:4256-4266, remove the
manifest only for a genuine zero; in routes.py:3675-3685, validate, sanitize,
and round persisted manifest values before returning them; in
routes.py:7421-7430, reject non-finite metadata before writing; update
tests/test_audio_shift_persist.py:47-52 to verify malformed, NaN, and Infinity
inputs preserve existing state.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e4610cf7-748c-4121-a6d0-4e9964a1893a
📒 Files selected for processing (4)
CHANGELOG.mdroutes.pysrc/create.jstests/test_audio_shift_persist.py
byrongamatos
force-pushed
the
fix/editor-audio-shift-persist
branch
from
July 14, 2026 17:44
6e80809 to
8f0ff49
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_audio_shift_persist.py (1)
77-84: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftExercise the real persistence boundaries.
These tests round-trip a local dictionary and call
_coerce_audio_shift/_apply_audio_shiftdirectly; they do not invoke the production save, Save As, load, or/api/plugins/editor/buildpaths. A regression in that wiring could therefore pass the suite. Add request-level coverage that saves/reopens (including absent vs. zero) and builds a fresh sloppak, then asserts the actual manifest and loaded response.Also applies to: 118-124, 127-136
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_audio_shift_persist.py` around lines 77 - 84, Extend the audio-shift tests beyond the local _round_trip helper and direct _coerce_audio_shift/_apply_audio_shift calls by exercising the production save, Save As, reopen/load, and /api/plugins/editor/build request paths. Cover both absent and zero audio_shift values, then verify the persisted manifest, loaded response, and freshly built sloppak contain the expected values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@node_modules`:
- Line 1: Remove the tracked machine-local node_modules symlink, ensure
node_modules remains ignored, and rely on the project’s package manager and
lockfile to restore dependencies in other environments.
---
Nitpick comments:
In `@tests/test_audio_shift_persist.py`:
- Around line 77-84: Extend the audio-shift tests beyond the local _round_trip
helper and direct _coerce_audio_shift/_apply_audio_shift calls by exercising the
production save, Save As, reopen/load, and /api/plugins/editor/build request
paths. Cover both absent and zero audio_shift values, then verify the persisted
manifest, loaded response, and freshly built sloppak contain the expected
values.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: eea26025-9881-422b-b083-a0443cb3fe19
📒 Files selected for processing (5)
CHANGELOG.mdnode_modulesroutes.pysrc/create.jstests/test_audio_shift_persist.py
🚧 Files skipped from review as they are similar to previous changes (3)
- CHANGELOG.md
- src/create.js
- routes.py
byrongamatos
added a commit
that referenced
this pull request
Jul 14, 2026
…k node_modules Drains the CodeRabbit findings on #262. The real one: at the request boundary a malformed `audio_shift` coerced to 0.0, and zero is a COMMAND — _apply_audio_shift REMOVES the manifest key on zero. So one garbage save body silently wiped alignment the charter had actually saved, while an ABSENT field was safely preserved. Garbage was given more destructive authority than silence, which is backwards. _coerce_audio_shift now takes the invalid-value fallback as a parameter, keeping the single shared coercion this branch deliberately consolidated: - sanitize paths (sloppak load, pack writer) keep the 0.0 default — a corrupt or absent STORED value is just an unshifted song; there is nothing to save. - _parse_audio_shift passes _FIELD_ABSENT — unusable INPUT touches nothing. An explicitly falsy spelling (0/None/"") is still a genuine zero on every path, so "slid back to 0 removes the key" is untouched. The old test pinned the bug (it asserted garbage == 0.0); replaced with the absent-shaped contract plus a regression test that fails with a KeyError on the old code — the popped key IS the data loss. Also: the tracked absolute node_modules symlink. The symptom is the committed blob; the cause is that `node_modules/` (trailing slash) matches only directories, so the worktree symlink was never ignored and `git add -A` swept it in. Slash dropped, blob untracked. Stale on this head, verified already fixed by 8f0ff49 and skipped: "sanitize persisted manifest values on read" (load path already coerces) and "reject non-finite before writing" (writer already coerces). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
|
@coderabbitai review |
✅ Action performedReview finished.
|
The frontend has shipped audio_shift on every save (and read it back on load) since Shift Audio landed, but the backend dropped it — a shifted recording reopened misaligned. New _parse_audio_shift boundary helper (absent = leave persisted value; zero = remove the key; garbage coerces to 0 rather than failing the save); _save_sloppak and _write_sloppak_pak write the manifest extension key (feedpak §4), _load_sloppak surfaces it; create-mode build body now ships the field too. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
…pies
The audio_shift float coercion was pasted three times (request body, sloppak
load, pack writer) and none of the copies guarded non-finite values.
`float("nan")` and `float("inf")` both SUCCEED and are truthy, so a garbage
value sailed past the nonzero write check and landed in manifest.yaml as
`.nan` / `.inf`, poisoning the pack for every reader. Reachable via a string
("nan") and via json.loads, which accepts bare NaN/Infinity.
Collapse all three into one `_coerce_audio_shift` (float + isfinite, 0.0 for
anything unusable) plus `_apply_audio_shift` for the write/remove/no-op
manifest rule, matching the isfinite guard the drum-hit save path already
uses. Net deletion, and the readers and writers can no longer drift apart.
Tests: the suite only exercised the parse helper in isolation, never the
write -> disk -> read path that actually broke. Add the save/load/build round
trip through yaml (the real disk boundary): a saved shift comes back
identical, zero removes the key, an older client omitting the field does not
wipe a persisted shift, and an old pack with no key loads as a clean 0.0.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…k node_modules Drains the CodeRabbit findings on #262. The real one: at the request boundary a malformed `audio_shift` coerced to 0.0, and zero is a COMMAND — _apply_audio_shift REMOVES the manifest key on zero. So one garbage save body silently wiped alignment the charter had actually saved, while an ABSENT field was safely preserved. Garbage was given more destructive authority than silence, which is backwards. _coerce_audio_shift now takes the invalid-value fallback as a parameter, keeping the single shared coercion this branch deliberately consolidated: - sanitize paths (sloppak load, pack writer) keep the 0.0 default — a corrupt or absent STORED value is just an unshifted song; there is nothing to save. - _parse_audio_shift passes _FIELD_ABSENT — unusable INPUT touches nothing. An explicitly falsy spelling (0/None/"") is still a genuine zero on every path, so "slid back to 0 removes the key" is untouched. The old test pinned the bug (it asserted garbage == 0.0); replaced with the absent-shaped contract plus a regression test that fails with a KeyError on the old code — the popped key IS the data loss. Also: the tracked absolute node_modules symlink. The symptom is the committed blob; the cause is that `node_modules/` (trailing slash) matches only directories, so the worktree symlink was never ignored and `git add -A` swept it in. Slash dropped, blob untracked. Stale on this head, verified already fixed by 8f0ff49 and skipped: "sanitize persisted manifest values on read" (load path already coerces) and "reject non-finite before writing" (writer already coerces). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
byrongamatos
force-pushed
the
fix/editor-audio-shift-persist
branch
from
July 14, 2026 19:00
88bef7a to
5333bd1
Compare
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
2 participants
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.
What
A data-loss fix: the frontend has shipped
audio_shiftin every save body — and readdata.audio_shiftback on load — since Shift Audio landed (#236), but the backend silently dropped it. A user who carefully aligned a recording got it back misaligned on every reopen. (This was #236's explicitly deferred follow-up.)_parse_audio_shiftboundary helper with the repo's absent-vs-explicit contract: absent (older client) = leave the persisted value alone; zero = explicitly unshifted, REMOVES the manifest key (a shift slid back to 0 must not leave residue); garbage coerces to 0 rather than 400ing — the field is supplementary alignment state and must never cost the user their note edits._save_sloppak(in-place sloppak save) writes/removesaudio_shiftas a manifest extension key (feedpak §4 ignored-but-preserved — same pattern asorigin);_write_sloppak_pakcarries it viametaso Save As and create-mode builds persist it too;_load_sloppaksurfaces it in the load payload with the same defensive coercion as the existingoffsetpassthrough./buildbody now shipsaudio_shift(the regular save body already did).Tests
tests/test_audio_shift_persist.py(6, fail on main — the helper doesn't exist there): the absent-sentinel identity, explicit-zero spellings, negative shifts, string coercion, garbage-never-fails, and microsecond rounding. Full pytest 245+6 passed, JS suite green, lint 0 errors.Live-verified end-to-end
On a real library pak (:8001 preview, new backend): Shift Audio +120 ms → Save → full page reload + reopen →
S.audioShift = 0.12restored; then shift back to 0 → Save → the manifest key is gone again (checked inside the zip). Zero page errors.🤖 Generated with Claude Code
https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
Summary by CodeRabbit