Deduplicate microphone and mix resource handling - #55
Conversation
- Share numbered-resource contracts, services, atoms, and editor UI - Preserve microphone and mix-specific behavior through configuration
There was a problem hiding this comment.
3 issues found across 16 files
Confidence score: 3/5
- In
packages/backend/src/numbered-resources/NumberedResourceService.ts, concurrent microphone/mix updates can race with deletion or newer edits, potentially resurrecting deleted resources or overwriting the latest user state. Rebuild from the freshest persisted item and re-validate that it is still active before writing to avoid lost updates. - In
apps/web/src/components/numbered-resources/NumberedResourceEditor.tsx, the optimistic rollback after failed microphone/mix saves can clear the RPC failure message, so users may see a silent revert and not know their change failed. Remove that reset path and keep surfacingsave()errors so failed edits are explicit. - In
apps/web/src/components/numbered-resources/NumberedResourceEditor.tsx, save entry points send only one changed field whilesavereconstructs a full payload from local state, which risks persisting stale values from other fields. Align save calls/payload construction to use a single authoritative snapshot (or send full validated state) to prevent cross-field clobbering.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/backend/src/numbered-resources/NumberedResourceService.ts">
<violation number="1" location="packages/backend/src/numbered-resources/NumberedResourceService.ts:118">
P1: Concurrent microphone/mix edits can resurrect a resource after deletion or overwrite a newer edit, so the user's latest state may be lost. Building `resource` from the current item and rechecking that it is still active inside the `repository.update` callback would make the operation atomic.</violation>
</file>
<file name="apps/web/src/components/numbered-resources/NumberedResourceEditor.tsx">
<violation number="1" location="apps/web/src/components/numbered-resources/NumberedResourceEditor.tsx:153">
P2: Failed microphone or mix edits can revert without showing the RPC error to the user. The optimistic rollback changes these dependencies after `save()` assigns the failure message, so this reset should be removed; `save()` already clears the previous error before each new attempt.</violation>
<violation number="2" location="apps/web/src/components/numbered-resources/NumberedResourceEditor.tsx:160">
P2: In the new shared NumberedResourceCard, every save path (color change, number commit, and name blur) calls `save` with only the single field that changed, yet `save` reconstructs the full payload using the current `number`/`name`/`color` state as fallbacks. As a result, a color-only change also persists whatever is currently typed (but not yet committed) in the number and name fields, and a name-blur + color-click pair can fire two asynchronous edits that race on the same atom. Consider scoping each save to the field actually being edited (only build the payload from `next.*` values and the committed `item` values, not the live editable state) so unrelated edits don't accidentally commit half-typed input.</violation>
</file>
Architecture diagram
sequenceDiagram
participant UI as React UI Routes
participant NRE as NumberedResourceEditor
participant Atoms as NumberedResourceAtoms
participant API as RPC Client
participant Svc as NumberedResourceService
participant Repo as ShowRepository
Note over UI,Repo: Shared Numbered Resource Flow (Microphones and Mixes)
UI->>NRE: Render with config (singular/plural/EmptyIcon)
NRE->>Atoms: Get items AsyncResult via useAtomValue
alt Loading (initial)
Atoms-->>NRE: AsyncResult initial
NRE-->>UI: Show spinner
else Load error and no items
Atoms-->>NRE: AsyncResult failure
NRE-->>UI: Show error state
else Success with no items
Atoms-->>NRE: Empty list
NRE-->>UI: Show empty state with config.EmptyIcon
else Success with items
Atoms-->>NRE: Resource list
NRE-->>UI: Render resource cards
end
Note over NRE,Svc: Edit Flow (number, name, or color)
NRE->>NRE: User edits field (blur/click)
NRE->>NRE: const save = async (next)
NRE->>Atoms: call edit mutation (optimistic)
Atoms->>Atoms: Apply optimistic edit via applyOptimisticNamedItemEdit
Atoms->>API: RPC mutation "microphones.edit"/"mixes.edit"
API->>Svc: Edit(showId, id, number, color, name)
alt Resource not found
Svc-->>API: RpcError "not found"
API-->>Atoms: Failure
Atoms-->>NRE: Exit failure
NRE->>NRE: Revert optimistic change
else Success
Svc->>Repo: Update document with edited resource
Repo-->>Svc: Updated show document
Svc-->>API: Updated resource
API-->>Atoms: Success
Atoms-->>NRE: Exit success
NRE-->>UI: Commit change
end
Note over NRE,Svc: Delete Flow
NRE->>NRE: User clicks delete button
NRE->>Atoms: call delete mutation (optimistic)
Atoms->>Atoms: Remove item from list optimistically
Atoms->>API: RPC mutation "microphones.delete"/"mixes.delete"
API->>Svc: Delete(showId, id)
alt Delete blocked (main mix only)
Svc->>Svc: Check deleteBlockedMessage for mainMixId
Svc-->>API: RpcError "main mix cannot be deleted"
API-->>Atoms: Failure
Atoms-->>NRE: Exit failure
NRE->>NRE: Restore deleted item
else Resource not found
Svc-->>API: RpcError "not found"
API-->>Atoms: Failure
Atoms-->>NRE: Exit failure
NRE->>NRE: Restore deleted item
else Success
Svc->>Repo: Soft delete (set deletedAt)
Repo-->>Svc: Updated show document
Svc-->>API: Void success
API-->>Atoms: Success
end
Note over Atoms,Repo: Create Flow (not shown in editor, but shared logic)
Atoms->>Atoms: Create optimistic item with temp id + nextNumber
Atoms->>API: RPC mutation "microphones.create"/"mixes.create"
API->>Svc: Create(showId, color)
Svc->>Svc: Generate id via Ids, compute nextNumber
Svc->>Repo: Update document with new resource
Repo-->>Svc: Updated show document
Svc-->>API: Created resource
API-->>Atoms: Success
Atoms->>Atoms: Replace optimistic item with real one
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| const trimmedName = params.name?.trim(); | ||
| const now = yield* DateTime.now; | ||
| const existingForUpdate = params.name === undefined ? existing : removeName(existing); |
There was a problem hiding this comment.
P1: Concurrent microphone/mix edits can resurrect a resource after deletion or overwrite a newer edit, so the user's latest state may be lost. Building resource from the current item and rechecking that it is still active inside the repository.update callback would make the operation atomic.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/backend/src/numbered-resources/NumberedResourceService.ts, line 118:
<comment>Concurrent microphone/mix edits can resurrect a resource after deletion or overwrite a newer edit, so the user's latest state may be lost. Building `resource` from the current item and rechecking that it is still active inside the `repository.update` callback would make the operation atomic.</comment>
<file context>
@@ -0,0 +1,174 @@
+
+ const trimmedName = params.name?.trim();
+ const now = yield* DateTime.now;
+ const existingForUpdate = params.name === undefined ? existing : removeName(existing);
+ const resource = {
+ ...existingForUpdate,
</file context>
| setNumber(String(item.number)); | ||
| setName(item.name ?? ""); | ||
| setColor(item.color); | ||
| setSaveError(undefined); |
There was a problem hiding this comment.
P2: Failed microphone or mix edits can revert without showing the RPC error to the user. The optimistic rollback changes these dependencies after save() assigns the failure message, so this reset should be removed; save() already clears the previous error before each new attempt.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/components/numbered-resources/NumberedResourceEditor.tsx, line 153:
<comment>Failed microphone or mix edits can revert without showing the RPC error to the user. The optimistic rollback changes these dependencies after `save()` assigns the failure message, so this reset should be removed; `save()` already clears the previous error before each new attempt.</comment>
<file context>
@@ -0,0 +1,358 @@
+ setNumber(String(item.number));
+ setName(item.name ?? "");
+ setColor(item.color);
+ setSaveError(undefined);
+ }, [item.color, item.name, item.number]);
+
</file context>
| setSaveError(undefined); | ||
| const result = await onEdit({ | ||
| id: item.id, | ||
| number: ((next.number ?? number.trim()) || item.number) as Item["number"], |
There was a problem hiding this comment.
P2: In the new shared NumberedResourceCard, every save path (color change, number commit, and name blur) calls save with only the single field that changed, yet save reconstructs the full payload using the current number/name/color state as fallbacks. As a result, a color-only change also persists whatever is currently typed (but not yet committed) in the number and name fields, and a name-blur + color-click pair can fire two asynchronous edits that race on the same atom. Consider scoping each save to the field actually being edited (only build the payload from next.* values and the committed item values, not the live editable state) so unrelated edits don't accidentally commit half-typed input.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/components/numbered-resources/NumberedResourceEditor.tsx, line 160:
<comment>In the new shared NumberedResourceCard, every save path (color change, number commit, and name blur) calls `save` with only the single field that changed, yet `save` reconstructs the full payload using the current `number`/`name`/`color` state as fallbacks. As a result, a color-only change also persists whatever is currently typed (but not yet committed) in the number and name fields, and a name-blur + color-click pair can fire two asynchronous edits that race on the same atom. Consider scoping each save to the field actually being edited (only build the payload from `next.*` values and the committed `item` values, not the live editable state) so unrelated edits don't accidentally commit half-typed input.</comment>
<file context>
@@ -0,0 +1,358 @@
+ setSaveError(undefined);
+ const result = await onEdit({
+ id: item.id,
+ number: ((next.number ?? number.trim()) || item.number) as Item["number"],
+ color: next.color ?? color,
+ ...(next.name !== undefined
</file context>
Summary
Testing