feat: add setbuilder document history#427
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR implements snapshot-based undo/redo, autosave, and destructive-action confirmations for the set builder ( ChangesSet Builder Document History System
Sequence Diagram(s)sequenceDiagram
participant DJ as DJ (Browser)
participant BuilderPage
participant useSetDocumentHistory
participant ApiClient
participant Server
DJ->>BuilderPage: performs mutating action (e.g. move slot)
BuilderPage->>useSetDocumentHistory: commit("Move slot 3 → 5", action)
useSetDocumentHistory->>useSetDocumentHistory: clone snapshot → undoStack
useSetDocumentHistory->>ApiClient: action() mutating API call
useSetDocumentHistory->>ApiClient: getSetDocument(setId)
ApiClient->>Server: GET /api/setbuilder/sets/{id}/document
Server-->>ApiClient: SetDocumentSnapshot
useSetDocumentHistory-->>BuilderPage: isDirty=true, undoDepth+1
DJ->>BuilderPage: clicks Undo (or ⌘Z)
BuilderPage->>useSetDocumentHistory: undo()
useSetDocumentHistory->>ApiClient: putSetDocument(setId, undoSnapshot)
ApiClient->>Server: PUT /api/setbuilder/sets/{id}/document
Server-->>ApiClient: SetDocumentSnapshot (restored)
useSetDocumentHistory-->>BuilderPage: snapshot updated, toast "Undid Move slot 3 → 5"
DJ->>BuilderPage: autosave interval fires (30s, dirty)
BuilderPage->>useSetDocumentHistory: saveNow()
useSetDocumentHistory->>ApiClient: putSetDocument(setId, currentSnapshot)
ApiClient->>Server: PUT /api/setbuilder/sets/{id}/document
Server-->>ApiClient: SetDocumentSnapshot
useSetDocumentHistory-->>BuilderPage: isDirty=false, lastSavedAt updated
Estimated code review effort🎯 4 (Complex) | ⏱️ ~90 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
dashboard/app/(dj)/setbuilder/components/PoolPanel.tsx (1)
87-101:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCancel stale
getPoolresponses in the snapshot-loading effect.Line 96 starts
api.getPool(setId)without cancellation. Ifsnapshotbecomes available before that request resolves, the late response can overwrite the snapshot-derived pool state.Suggested fix
useEffect(() => { + let cancelled = false; setVibes(new Map()); setShowVibes(false); setVibesLoaded(false); + setLoaded(false); if (snapshot) { setPool(snapshot.pool); setLoaded(true); - return; + return () => { + cancelled = true; + }; } api .getPool(setId) - .then(setPool) - .catch(() => setToast('Failed to load pool')) - .finally(() => setLoaded(true)); + .then((next) => { + if (!cancelled) setPool(next); + }) + .catch(() => { + if (!cancelled) setToast('Failed to load pool'); + }) + .finally(() => { + if (!cancelled) setLoaded(true); + }); + return () => { + cancelled = true; + }; }, [setId, snapshot, snapshotVersion]);🤖 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 `@dashboard/app/`(dj)/setbuilder/components/PoolPanel.tsx around lines 87 - 101, The useEffect hook in PoolPanel.tsx initializes an async api.getPool(setId) request without any cancellation mechanism. If the snapshot dependency changes before the request completes, the stale response will overwrite the pool state derived from the snapshot. Add an AbortController to the api.getPool call and implement proper cleanup in the useEffect return statement to cancel the pending request when dependencies change or the component unmounts. Check whether the request was aborted before calling setPool to prevent stale state updates.dashboard/app/(dj)/setbuilder/components/CurvePanel.tsx (1)
162-183:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent stale
getVibeWindowsresponses from overwriting newer state.Line 162 starts an async fetch that is never cancelled/guarded; if
snapshotarrives orsetIdchanges before it resolves, Line 170 can still callsetWindows(...)with stale data.Suggested fix
- useEffect(() => { - if (snapshot || totalSec <= 0 || windowsLoadedFor.current === setId) return; + useEffect(() => { + if (snapshot || totalSec <= 0 || windowsLoadedFor.current === setId) return; + let cancelled = false; // Mark eagerly so in-flight fetches dedupe; roll back on failure so a // transient error doesn't block the load for the rest of the session. windowsLoadedFor.current = setId; api .getVibeWindows(setId) .then((resp) => { + if (cancelled) return; setWindows( resp.windows.map((w, i) => ({ id: `w-${i}-${w.t0_sec}`, t0: Math.min(1, w.t0_sec / totalSec), t1: Math.min(1, w.t1_sec / totalSec), label: w.label, })), ); }) .catch(() => { + if (cancelled) return; if (windowsLoadedFor.current === setId) windowsLoadedFor.current = null; setWindows([]); }); - }, [setId, totalSec]); + return () => { + cancelled = true; + }; + }, [setId, totalSec, snapshot]);🤖 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 `@dashboard/app/`(dj)/setbuilder/components/CurvePanel.tsx around lines 162 - 183, The async getVibeWindows API call in the useEffect hook can result in stale data being written to state if snapshot arrives or setId changes before the promise resolves. Guard the setWindows call in the .then() callback by adding a condition that checks whether the current setId still matches the one that initiated the fetch, and similarly ensure the guard accounts for snapshot changes. Use the existing windowsLoadedFor.current pattern or add a comparable check to prevent the .then() callback from calling setWindows if the component dependencies have shifted, ensuring only fresh data updates the state.
🤖 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 `@dashboard/app/`(dj)/setbuilder/[setId]/page.tsx:
- Around line 93-103: The requestConfirmation function overwrites the
confirmResolver without resolving any existing pending promise, causing the
caller to hang indefinitely. Before creating a new promise in
requestConfirmation, check if confirmResolver is already set and if so, resolve
the pending confirmation by calling closeConfirm(false) to clean up the previous
state before setting the new action and resolver.
In `@dashboard/app/`(dj)/setbuilder/components/useSetDocumentHistory.ts:
- Around line 55-75: The useEffect hook in useSetDocumentHistory resets the
undo/redo stacks and autosave state when setId changes, but it does not reset
the snapshot/snapshotRef state. This creates a race condition where if a
mutation runs between the state reset and the new document fetch completing,
fetchCurrent() could use the previous set's snapshot as history input. Reset the
snapshot and snapshotRef state at the beginning of the useEffect hook (along
with the other state resets) to ensure the old snapshot is cleared before the
new fetch begins, preventing cross-set snapshot reuse.
- Around line 100-176: The commit, restore, and saveNow functions can execute
concurrently if the user rapidly triggers them (via keyboard shortcuts or button
clicks), causing interleaved state updates and inconsistent undo/redo stacks.
Add an in-flight guard using useRef to track whether an operation is currently
executing. At the start of each of these three functions, check the guard flag
and return early if an operation is already in progress. Set the guard to true
before performing async work and reset it to false in the finally block of each
function to ensure the lock is properly released even if errors occur.
In `@dashboard/app/`(dj)/setbuilder/setbuilder.module.css:
- Around line 748-749: The keyframe name `savePulse` uses camelCase but
stylelint requires kebab-case for keyframe names. Rename the keyframe from
`savePulse` to `save-pulse` in two locations: update the keyframe definition at
line 945 and update the animation reference in the animation property at line
748-749 to use `save-pulse` instead of `savePulse`.
In `@server/app/services/setbuilder/document_snapshot.py`:
- Around line 110-175: The code directly persists client-supplied ID values as
primary keys for SetPoolSource, SetPoolTrack, SetSlot, and SetCurvePoint
records, which allows ID collisions that cause integrity errors. Remove the id
parameter assignments from all four object creations to let the database
auto-generate primary keys. Maintain a mapping dictionary that tracks the
relationship between the snapshot's original source IDs and the newly generated
database IDs for sources. When creating SetPoolTrack records, replace the direct
track.source_id reference with a lookup into this mapping dictionary to use the
correct new source ID. Apply the same pattern for SetSlot by maintaining a track
ID mapping and using it to resolve track_id foreign key references. This ensures
foreign key consistency while preventing ID collision attacks.
In `@server/openapi.json`:
- Around line 7649-7716: The SetDocumentSnapshot-Input schema requires only
settings and pool, while SetDocumentSnapshot-Output requires all four fields
including curve_points and slots, breaking round-trip compatibility for the
replace-all snapshot API. Update the required fields array in
SetDocumentSnapshot-Input to include curve_points and slots alongside settings
and pool so the input schema matches the output schema requirements. After
making this change, regenerate all downstream types that depend on these
schemas.
- Around line 16683-16759: The GET and PUT endpoints for document snapshots
(operationId values
get_document_snapshot_api_setbuilder_sets__set_id__document_get and
put_document_snapshot_api_setbuilder_sets__set_id__document_put) are missing the
404 response definition in their responses objects. Add a 404 response entry to
both endpoints that references the HTTPValidationError schema with an
appropriate description indicating the set was not found or is not accessible,
ensuring the generated dashboard client has the complete error contract for
owner-isolation scenarios.
In `@server/tests/test_setbuilder_document_snapshot_api.py`:
- Around line 114-137: The test performs three destructive mutations (PATCH on
the target endpoint, DELETE on the sources endpoint, and PUT on the vibe-windows
endpoint) but never checks their response status codes. If any of these
operations fail, the subsequent restore assertions can still pass without
proving an actual mutate-then-restore round trip. Add status code assertions
(checking for 200 or appropriate success code) immediately after each of the
three destructive client calls, before the restore operation. Additionally, add
an assertion to verify that the restored settings match the original settings
(assert restored["settings"] == original["settings"]) alongside the existing
assertions for slots, pool, and curve_points to fully cover the snapshot
contract.
---
Outside diff comments:
In `@dashboard/app/`(dj)/setbuilder/components/CurvePanel.tsx:
- Around line 162-183: The async getVibeWindows API call in the useEffect hook
can result in stale data being written to state if snapshot arrives or setId
changes before the promise resolves. Guard the setWindows call in the .then()
callback by adding a condition that checks whether the current setId still
matches the one that initiated the fetch, and similarly ensure the guard
accounts for snapshot changes. Use the existing windowsLoadedFor.current pattern
or add a comparable check to prevent the .then() callback from calling
setWindows if the component dependencies have shifted, ensuring only fresh data
updates the state.
In `@dashboard/app/`(dj)/setbuilder/components/PoolPanel.tsx:
- Around line 87-101: The useEffect hook in PoolPanel.tsx initializes an async
api.getPool(setId) request without any cancellation mechanism. If the snapshot
dependency changes before the request completes, the stale response will
overwrite the pool state derived from the snapshot. Add an AbortController to
the api.getPool call and implement proper cleanup in the useEffect return
statement to cancel the pending request when dependencies change or the
component unmounts. Check whether the request was aborted before calling setPool
to prevent stale state updates.
🪄 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: 44892d82-7078-4661-b79e-a02850d58ed0
📒 Files selected for processing (21)
dashboard/app/(dj)/setbuilder/[setId]/page.tsxdashboard/app/(dj)/setbuilder/components/BuilderBrandMenu.tsxdashboard/app/(dj)/setbuilder/components/BuilderSettingsModal.tsxdashboard/app/(dj)/setbuilder/components/BuilderWorkspace.tsxdashboard/app/(dj)/setbuilder/components/ConfirmActionDialog.tsxdashboard/app/(dj)/setbuilder/components/CurvePanel.tsxdashboard/app/(dj)/setbuilder/components/HistoryControls.tsxdashboard/app/(dj)/setbuilder/components/ImportModal.tsxdashboard/app/(dj)/setbuilder/components/PoolPanel.tsxdashboard/app/(dj)/setbuilder/components/__tests__/ImportModal.test.tsxdashboard/app/(dj)/setbuilder/components/__tests__/useSetDocumentHistory.test.tsxdashboard/app/(dj)/setbuilder/components/useSetDocumentHistory.tsdashboard/app/(dj)/setbuilder/setbuilder.module.cssdashboard/lib/api-types.generated.tsdashboard/lib/api-types.tsdashboard/lib/api.tsserver/app/api/setbuilder.pyserver/app/schemas/setbuilder.pyserver/app/services/setbuilder/document_snapshot.pyserver/openapi.jsonserver/tests/test_setbuilder_document_snapshot_api.py
Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@dashboard/app/`(dj)/setbuilder/components/BuilderWorkspace.tsx:
- Around line 81-95: The catch block in the loadSlots useCallback function
clears both pool and slots to empty arrays on fetch failure, which loses
workspace state even though a history snapshot is available for restoration.
Instead of clearing these states with setPool([]) and setSlots([]), restore the
workspace from the history snapshot that the component already receives, making
undo/redo restoration resilient to transient network failures. Replace the error
handler logic to use the snapshot as the restore source rather than clearing to
empty arrays.
In `@dashboard/app/`(dj)/setbuilder/setbuilder.module.css:
- Around line 511-553: The CSS selectors .confirmDialog, .confirmHeader,
.confirmIcon, and .confirmBody are defined twice in the file, causing styling
collisions where legacy positioning properties leak into the new modal
definition. Either rename the selectors in this block to unique names (e.g.,
.newConfirmDialog, .newConfirmHeader, .newConfirmIcon, .newConfirmBody) to avoid
the collision entirely, or if keeping the names, ensure this definition includes
explicit CSS resets (like resetting top, left, transform if they exist in the
legacy definition) to fully override all legacy positioning properties and
prevent misplacement of the ConfirmActionDialog modal.
In `@server/app/api/setbuilder.py`:
- Around line 712-737: The SetDocumentSnapshot schema used in the response_model
for get_document_snapshot and put_document_snapshot endpoints does not include
pairing state, which prevents pairings from being fully round-tripped through
undo/redo/autosave operations. Extend the SetDocumentSnapshot schema to include
a field for pairing state, update the document_snapshot.build_snapshot function
to serialize the pairing state from the set_obj when building the snapshot, and
update the document_snapshot.restore_snapshot function to deserialize and
restore the pairing state along with the other document state. The endpoint
response models will automatically work with the extended schema once these
changes are made.
🪄 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: 71027893-aad1-4694-b3c7-deccf76f714f
📒 Files selected for processing (21)
dashboard/app/(dj)/setbuilder/[setId]/page.tsxdashboard/app/(dj)/setbuilder/components/BuilderBrandMenu.tsxdashboard/app/(dj)/setbuilder/components/BuilderSettingsModal.tsxdashboard/app/(dj)/setbuilder/components/BuilderWorkspace.tsxdashboard/app/(dj)/setbuilder/components/ConfirmActionDialog.tsxdashboard/app/(dj)/setbuilder/components/CurvePanel.tsxdashboard/app/(dj)/setbuilder/components/HistoryControls.tsxdashboard/app/(dj)/setbuilder/components/ImportModal.tsxdashboard/app/(dj)/setbuilder/components/PoolPanel.tsxdashboard/app/(dj)/setbuilder/components/__tests__/ImportModal.test.tsxdashboard/app/(dj)/setbuilder/components/__tests__/useSetDocumentHistory.test.tsxdashboard/app/(dj)/setbuilder/components/useSetDocumentHistory.tsdashboard/app/(dj)/setbuilder/setbuilder.module.cssdashboard/lib/api-types.generated.tsdashboard/lib/api-types.tsdashboard/lib/api.tsserver/app/api/setbuilder.pyserver/app/schemas/setbuilder.pyserver/app/services/setbuilder/document_snapshot.pyserver/openapi.jsonserver/tests/test_setbuilder_document_snapshot_api.py
💤 Files with no reviewable changes (1)
- server/openapi.json
✅ Files skipped from review due to trivial changes (1)
- dashboard/lib/api-types.generated.ts
🚧 Files skipped from review as they are similar to previous changes (15)
- dashboard/app/(dj)/setbuilder/components/tests/ImportModal.test.tsx
- dashboard/app/(dj)/setbuilder/components/tests/useSetDocumentHistory.test.tsx
- dashboard/lib/api-types.ts
- dashboard/app/(dj)/setbuilder/components/ConfirmActionDialog.tsx
- dashboard/app/(dj)/setbuilder/components/HistoryControls.tsx
- dashboard/app/(dj)/setbuilder/components/BuilderBrandMenu.tsx
- dashboard/lib/api.ts
- server/app/services/setbuilder/document_snapshot.py
- dashboard/app/(dj)/setbuilder/components/BuilderSettingsModal.tsx
- dashboard/app/(dj)/setbuilder/components/useSetDocumentHistory.ts
- dashboard/app/(dj)/setbuilder/components/PoolPanel.tsx
- server/tests/test_setbuilder_document_snapshot_api.py
- server/app/schemas/setbuilder.py
- dashboard/app/(dj)/setbuilder/components/ImportModal.tsx
- dashboard/app/(dj)/setbuilder/components/CurvePanel.tsx
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🤖 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 `@dashboard/app/`(dj)/setbuilder/components/BuilderWorkspace.tsx:
- Around line 81-95: The catch block in the loadSlots useCallback function
clears both pool and slots to empty arrays on fetch failure, which loses
workspace state even though a history snapshot is available for restoration.
Instead of clearing these states with setPool([]) and setSlots([]), restore the
workspace from the history snapshot that the component already receives, making
undo/redo restoration resilient to transient network failures. Replace the error
handler logic to use the snapshot as the restore source rather than clearing to
empty arrays.
In `@dashboard/app/`(dj)/setbuilder/setbuilder.module.css:
- Around line 511-553: The CSS selectors .confirmDialog, .confirmHeader,
.confirmIcon, and .confirmBody are defined twice in the file, causing styling
collisions where legacy positioning properties leak into the new modal
definition. Either rename the selectors in this block to unique names (e.g.,
.newConfirmDialog, .newConfirmHeader, .newConfirmIcon, .newConfirmBody) to avoid
the collision entirely, or if keeping the names, ensure this definition includes
explicit CSS resets (like resetting top, left, transform if they exist in the
legacy definition) to fully override all legacy positioning properties and
prevent misplacement of the ConfirmActionDialog modal.
In `@server/app/api/setbuilder.py`:
- Around line 712-737: The SetDocumentSnapshot schema used in the response_model
for get_document_snapshot and put_document_snapshot endpoints does not include
pairing state, which prevents pairings from being fully round-tripped through
undo/redo/autosave operations. Extend the SetDocumentSnapshot schema to include
a field for pairing state, update the document_snapshot.build_snapshot function
to serialize the pairing state from the set_obj when building the snapshot, and
update the document_snapshot.restore_snapshot function to deserialize and
restore the pairing state along with the other document state. The endpoint
response models will automatically work with the extended schema once these
changes are made.
🪄 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: 71027893-aad1-4694-b3c7-deccf76f714f
📒 Files selected for processing (21)
dashboard/app/(dj)/setbuilder/[setId]/page.tsxdashboard/app/(dj)/setbuilder/components/BuilderBrandMenu.tsxdashboard/app/(dj)/setbuilder/components/BuilderSettingsModal.tsxdashboard/app/(dj)/setbuilder/components/BuilderWorkspace.tsxdashboard/app/(dj)/setbuilder/components/ConfirmActionDialog.tsxdashboard/app/(dj)/setbuilder/components/CurvePanel.tsxdashboard/app/(dj)/setbuilder/components/HistoryControls.tsxdashboard/app/(dj)/setbuilder/components/ImportModal.tsxdashboard/app/(dj)/setbuilder/components/PoolPanel.tsxdashboard/app/(dj)/setbuilder/components/__tests__/ImportModal.test.tsxdashboard/app/(dj)/setbuilder/components/__tests__/useSetDocumentHistory.test.tsxdashboard/app/(dj)/setbuilder/components/useSetDocumentHistory.tsdashboard/app/(dj)/setbuilder/setbuilder.module.cssdashboard/lib/api-types.generated.tsdashboard/lib/api-types.tsdashboard/lib/api.tsserver/app/api/setbuilder.pyserver/app/schemas/setbuilder.pyserver/app/services/setbuilder/document_snapshot.pyserver/openapi.jsonserver/tests/test_setbuilder_document_snapshot_api.py
💤 Files with no reviewable changes (1)
- server/openapi.json
✅ Files skipped from review due to trivial changes (1)
- dashboard/lib/api-types.generated.ts
🚧 Files skipped from review as they are similar to previous changes (15)
- dashboard/app/(dj)/setbuilder/components/tests/ImportModal.test.tsx
- dashboard/app/(dj)/setbuilder/components/tests/useSetDocumentHistory.test.tsx
- dashboard/lib/api-types.ts
- dashboard/app/(dj)/setbuilder/components/ConfirmActionDialog.tsx
- dashboard/app/(dj)/setbuilder/components/HistoryControls.tsx
- dashboard/app/(dj)/setbuilder/components/BuilderBrandMenu.tsx
- dashboard/lib/api.ts
- server/app/services/setbuilder/document_snapshot.py
- dashboard/app/(dj)/setbuilder/components/BuilderSettingsModal.tsx
- dashboard/app/(dj)/setbuilder/components/useSetDocumentHistory.ts
- dashboard/app/(dj)/setbuilder/components/PoolPanel.tsx
- server/tests/test_setbuilder_document_snapshot_api.py
- server/app/schemas/setbuilder.py
- dashboard/app/(dj)/setbuilder/components/ImportModal.tsx
- dashboard/app/(dj)/setbuilder/components/CurvePanel.tsx
🛑 Comments failed to post (3)
dashboard/app/(dj)/setbuilder/components/BuilderWorkspace.tsx (1)
81-95:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
snapshotas a restore source instead of clearing workspace on fetch failure.On Line 93-94, a transient
getSetSlots/getPoolfailure dropsslotsandpoolto empty, even though this component already receives historysnapshot. That makes undo/redo restoration brittle during network blips.🤖 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 `@dashboard/app/`(dj)/setbuilder/components/BuilderWorkspace.tsx around lines 81 - 95, The catch block in the loadSlots useCallback function clears both pool and slots to empty arrays on fetch failure, which loses workspace state even though a history snapshot is available for restoration. Instead of clearing these states with setPool([]) and setSlots([]), restore the workspace from the history snapshot that the component already receives, making undo/redo restoration resilient to transient network failures. Replace the error handler logic to use the snapshot as the restore source rather than clearing to empty arrays.dashboard/app/(dj)/setbuilder/setbuilder.module.css (1)
511-553:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid selector collisions between legacy and current confirm dialog styles.
ConfirmActionDialoguses.confirmDialog,.confirmHeader,.confirmIcon, and.confirmBody, but those selectors are defined twice. The later block does not reset all earlier positioning fields, so legacy offsets can leak into the new modal and misplace it.Suggested minimal fix
-.confirmDialog { +.confirmWrap .confirmDialog { position: absolute; left: 50%; top: 45%; transform: translate(-50%, -50%); width: min(440px, calc(100vw - 2rem)); overflow: hidden; border: 1px solid rgba(245, 158, 11, 0.35); border-radius: 12px; background: var(--card); box-shadow: 0 24px 60px rgba(0, 0, 0, 0.55); } -.confirmHeader { +.confirmWrap .confirmHeader { display: flex; align-items: center; gap: 0.75rem; padding: 1rem 1.1rem 0; } -.confirmIcon { +.confirmWrap .confirmIcon { display: grid; place-items: center; width: 2.2rem; height: 2.2rem; border-radius: 10px; background: rgba(245, 158, 11, 0.12); color: `#f59e0b`; font-weight: 800; } -.confirmBody { +.confirmWrap .confirmBody { padding: 0.8rem 1.1rem 1rem; color: var(--text-secondary); font-size: 0.8125rem; line-height: 1.55; }Also applies to: 1936-2066
🤖 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 `@dashboard/app/`(dj)/setbuilder/setbuilder.module.css around lines 511 - 553, The CSS selectors .confirmDialog, .confirmHeader, .confirmIcon, and .confirmBody are defined twice in the file, causing styling collisions where legacy positioning properties leak into the new modal definition. Either rename the selectors in this block to unique names (e.g., .newConfirmDialog, .newConfirmHeader, .newConfirmIcon, .newConfirmBody) to avoid the collision entirely, or if keeping the names, ensure this definition includes explicit CSS resets (like resetting top, left, transform if they exist in the legacy definition) to fully override all legacy positioning properties and prevent misplacement of the ConfirmActionDialog modal.server/app/api/setbuilder.py (1)
712-737:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftInclude pairings in the snapshot contract used by document GET/PUT.
At Line 712 and Line 729, the endpoint contract is fixed to
SetDocumentSnapshot, but that schema does not carry pairing state. Given the history requirements, pairing mutations cannot be fully round-tripped through undo/redo/autosave via this API surface. Extend the snapshot schema/service and this endpoint contract so pairings are serialized and restored with the rest of the document state.🤖 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 `@server/app/api/setbuilder.py` around lines 712 - 737, The SetDocumentSnapshot schema used in the response_model for get_document_snapshot and put_document_snapshot endpoints does not include pairing state, which prevents pairings from being fully round-tripped through undo/redo/autosave operations. Extend the SetDocumentSnapshot schema to include a field for pairing state, update the document_snapshot.build_snapshot function to serialize the pairing state from the set_obj when building the snapshot, and update the document_snapshot.restore_snapshot function to deserialize and restore the pairing state along with the other document state. The endpoint response models will automatically work with the extended schema once these changes are made.
Collaborator
Author
|
CodeRabbit body-only items addressed in bf08d25: PoolPanel now ignores late getPool responses after snapshot or set changes, and CurvePanel now ignores late getVibeWindows responses under the same conditions. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
server/tests/test_setbuilder_document_snapshot_api.py (2)
130-130: ⚡ Quick winAssert setup-call success before consuming response JSON.
Line [130], Line [191], and Line [238] consume
.json()without first asserting expected status codes. Add explicit status assertions so failures are reported at the failing call site instead of surfacing later as downstream assertion/key errors.Suggested hardening patch
- original = client.get(f"/api/setbuilder/sets/{set_id}/document", headers=auth_headers).json() + original_resp = client.get(f"/api/setbuilder/sets/{set_id}/document", headers=auth_headers) + assert original_resp.status_code == 200, original_resp.json() + original = original_resp.json() ... - payload = client.get(f"/api/setbuilder/sets/{set_id}/document", headers=auth_headers).json() + payload_resp = client.get(f"/api/setbuilder/sets/{set_id}/document", headers=auth_headers) + assert payload_resp.status_code == 200, payload_resp.json() + payload = payload_resp.json() ... created = client.post( "/api/setbuilder/sets", json={"name": "Bad Snapshot"}, headers=auth_headers ) + assert created.status_code == 201, created.json() set_id = created.json()["id"]Also applies to: 191-191, 235-239
🤖 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 `@server/tests/test_setbuilder_document_snapshot_api.py` at line 130, Add explicit status code assertions before consuming the response JSON at three locations in the test file. At line 130 where the original variable is assigned from the GET request to the document endpoint, at line 191, and at lines 235-239, insert status code assertions (likely assert_status_code(200) or similar) immediately after each client call and before calling .json() on the response. This ensures that request failures are caught and reported at the failing call site rather than surfacing later as downstream errors when trying to access missing keys in the response JSON.
195-207: ⚡ Quick winAssert
source_idremapping explicitly in the primary-key restore test.Lines [195]-[207] verify regenerated row IDs, but they do not explicitly verify that restored tracks reference restored source IDs (not the injected client
source_id). Adding this closes a key data-integrity assertion for the remap contract.Suggested assertion addition
restored = restore.json() + restored_source_ids = {source["id"] for source in restored["pool"]["sources"]} assert restored["slots"][0]["id"] != 9901 assert restored["curve_points"][0]["id"] != 9902 assert restored["pool"]["sources"][0]["id"] != 9903 assert all(track["id"] != 9904 for track in restored["pool"]["tracks"]) + assert all(track["source_id"] in restored_source_ids for track in restored["pool"]["tracks"]) + assert all(track["source_id"] != 9903 for track in restored["pool"]["tracks"])🤖 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 `@server/tests/test_setbuilder_document_snapshot_api.py` around lines 195 - 207, The test in the file server/tests/test_setbuilder_document_snapshot_api.py (lines 195-207) verifies that injected IDs for slots, curve_points, sources, and tracks are regenerated, but it does not explicitly assert that the restored tracks have their source_id remapped away from the injected value of 9903. Add an assertion after the existing track assertions to verify that all restored tracks do not have source_id equal to 9903, following the same pattern used for the track id assertion (all(track["source_id"] != 9903 for track in restored["pool"]["tracks"])). This closes the data-integrity verification gap for the source_id remapping contract.
🤖 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.
Nitpick comments:
In `@server/tests/test_setbuilder_document_snapshot_api.py`:
- Line 130: Add explicit status code assertions before consuming the response
JSON at three locations in the test file. At line 130 where the original
variable is assigned from the GET request to the document endpoint, at line 191,
and at lines 235-239, insert status code assertions (likely
assert_status_code(200) or similar) immediately after each client call and
before calling .json() on the response. This ensures that request failures are
caught and reported at the failing call site rather than surfacing later as
downstream errors when trying to access missing keys in the response JSON.
- Around line 195-207: The test in the file
server/tests/test_setbuilder_document_snapshot_api.py (lines 195-207) verifies
that injected IDs for slots, curve_points, sources, and tracks are regenerated,
but it does not explicitly assert that the restored tracks have their source_id
remapped away from the injected value of 9903. Add an assertion after the
existing track assertions to verify that all restored tracks do not have
source_id equal to 9903, following the same pattern used for the track id
assertion (all(track["source_id"] != 9903 for track in
restored["pool"]["tracks"])). This closes the data-integrity verification gap
for the source_id remapping contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a2a906b2-def2-42a2-94f6-ec6053239f4c
📒 Files selected for processing (11)
dashboard/app/(dj)/setbuilder/[setId]/page.tsxdashboard/app/(dj)/setbuilder/components/CurvePanel.tsxdashboard/app/(dj)/setbuilder/components/PoolPanel.tsxdashboard/app/(dj)/setbuilder/components/useSetDocumentHistory.tsdashboard/app/(dj)/setbuilder/setbuilder.module.cssdashboard/lib/api-types.generated.tsserver/app/api/setbuilder.pyserver/app/schemas/setbuilder.pyserver/app/services/setbuilder/document_snapshot.pyserver/openapi.jsonserver/tests/test_setbuilder_document_snapshot_api.py
✅ Files skipped from review due to trivial changes (1)
- dashboard/lib/api-types.generated.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- dashboard/app/(dj)/setbuilder/components/PoolPanel.tsx
- dashboard/app/(dj)/setbuilder/[setId]/page.tsx
- server/app/schemas/setbuilder.py
- server/app/api/setbuilder.py
- server/openapi.json
- dashboard/app/(dj)/setbuilder/components/useSetDocumentHistory.ts
- dashboard/app/(dj)/setbuilder/components/CurvePanel.tsx
- server/app/services/setbuilder/document_snapshot.py
Collaborator
Author
|
CodeRabbit body-only test nitpicks addressed in 27fb1dc: added status assertions before consuming snapshot/setup response JSON and asserted restored tracks reference remapped source ids rather than the injected source id. |
This was referenced Jun 15, 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.
Why
WrzDJSet builder edits are destructive and need trustable document history, visible save state, keyboard recovery, and confirmation around dangerous actions.
Closes #395
What
Testing
dashboard:npm run lintdashboard:npx tsc --noEmitdashboard:npm test -- --runserver:/home/adam/github/WrzDJ/server/.venv/bin/ruff check .server:/home/adam/github/WrzDJ/server/.venv/bin/ruff format --check .server:/home/adam/github/WrzDJ/server/.venv/bin/bandit -r app -c pyproject.toml -qserver:/home/adam/github/WrzDJ/server/.venv/bin/pytest --tb=short -q(2802 passed, coverage gate reached at 88.71%)server:/home/adam/github/WrzDJ/server/.venv/bin/alembic upgrade head && /home/adam/github/WrzDJ/server/.venv/bin/alembic checkcould not run locally because PostgreSQL rejected the configuredwrzdjcredentials; no migrations/model columns were added.Design decisions
Claude assisted with implementation and test coverage.
Summary by CodeRabbit
Release Notes
New Features
Tests