Skip to content

Deduplicate microphone and mix resource handling - #55

Open
johannesschiessl wants to merge 1 commit into
mainfrom
t3code/deduplicate-mix-microphone-services
Open

Deduplicate microphone and mix resource handling#55
johannesschiessl wants to merge 1 commit into
mainfrom
t3code/deduplicate-mix-microphone-services

Conversation

@johannesschiessl

Copy link
Copy Markdown
Owner

Summary

  • Extract shared numbered-resource editing UI and service logic.
  • Reuse common contracts and frontend atoms for microphones and mixes.
  • Preserve mix-specific behavior, including the protected main mix.

Testing

  • Not run.

- Share numbered-resource contracts, services, atoms, and editor UI
- Preserve microphone and mix-specific behavior through configuration

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 surfacing save() errors so failed edits are explicit.
  • In apps/web/src/components/numbered-resources/NumberedResourceEditor.tsx, save entry points send only one changed field while save reconstructs 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
Loading

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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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"],

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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>

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