Skip to content

Improve React code quality#34

Open
johannesschiessl wants to merge 4 commits into
mainfrom
t3code/improve-react-code-quality
Open

Improve React code quality#34
johannesschiessl wants to merge 4 commits into
mainfrom
t3code/improve-react-code-quality

Conversation

@johannesschiessl

Copy link
Copy Markdown
Member

Summary

  • eliminate React render-purity, effect lifecycle, derived-state, list-key, and callback subscription issues
  • consolidate related song and connection state into reducers and preserve predictable dialog resets
  • improve native list/control semantics and optimize repeated lookups and iterations
  • break the chat navigation import cycle and split large route components into focused modules
  • remove unreachable files and unused exports

Why

React Doctor identified state synchronization and render-time patterns that could produce stale UI, unnecessary subscriptions, extra renders, or order-dependent initialization. The structural extractions keep those fixes maintainable and reduce the risk of future regressions in show-critical workflows.

Validation

  • npx react-doctor@latest . --verbose — 100/100, no issues found
  • changed-file vp check --fix — formatting, lint, and type checks pass
  • vp check --no-fmt — full repository lint and type checks pass
  • vp run -r test — all monorepo tests pass
  • pnpm --filter @showtime/web build — passes
  • pnpm --filter @showtime/desktop build — passes, including Windows installer packaging

@johannesschiessl
johannesschiessl marked this pull request as ready for review July 16, 2026 13:00

@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.

16 issues found across 30 files

Confidence score: 2/5

  • In apps/web/src/components/songs/SongDetail.tsx, saving while mixes/microphones are still loading can treat empty fallback data as real and wipe saved assignments/mic overrides, which is direct data-loss risk for song configuration — preserve prior persisted mappings until dependent data has actually loaded.
  • In apps/web/src/routes/shows/$showId/setlist/$songId.tsx and apps/web/src/components/songs/SongDetail.tsx, optimistic remounts and reset-on-error paths can allow overlapping saves and then roll back newer edits, so users can lose recent changes or see inconsistent form state — gate concurrent mutations and avoid whole-form resets for single-field failures.
  • In apps/web/src/components/mixes/MixCard.tsx and apps/web/src/components/songs/MicrophoneName.tsx, concurrent/stale event flows can submit outdated payloads (number vs color races, Escape still triggering blur-save), causing one user edit to overwrite another — serialize or patch only changed fields and suppress blur-save after cancel/escape.
  • In apps/web/src/components/microphones/MicrophoneCard.tsx, apps/web/src/components/shows/ShowDeleteDialog.tsx, apps/web/src/components/chats/ChatPresetDialog.tsx, apps/web/src/components/chats/ChatWorkspace.tsx, and apps/web/src/components/microphones/MicrophoneDeleteDialog.tsx, pending/delete dialog and selection state transitions have timing gaps that can leave stale IDs, incorrect navigation/close behavior, or unreported failures — tighten pending-state guards and route all close/selection updates through a single synchronized path.
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="apps/web/src/components/microphones/MicrophoneCard.tsx">

<violation number="1" location="apps/web/src/components/microphones/MicrophoneCard.tsx:25">
P3: MicrophoneCard now duplicates MixCard's editable-card behavior, so lifecycle and reliability fixes can diverge between microphones and mixes. A shared named-item card or shared edit-field logic would keep these workflows synchronized while retaining their entity-specific labels and mutations.</violation>

<violation number="2" location="apps/web/src/components/microphones/MicrophoneCard.tsx:89">
P2: An optimistic microphone can still be edited while its create mutation is pending, so these controls can submit edits against the temporary ID and show avoidable failures or conflicting optimistic state. The pending guard would be more reliable if it also disabled the number/name inputs, the popover trigger, and color buttons, as `MixCard` does.</violation>
</file>

<file name="apps/web/src/components/mixes/MixDeleteDialog.tsx">

<violation number="1" location="apps/web/src/components/mixes/MixDeleteDialog.tsx:23">
P3: The mix delete workflow now duplicates the existing microphone delete dialog, so fixes to shared deletion behavior can diverge between these screens. A shared resource-agnostic delete dialog or hook would keep the two flows consistent.</violation>
</file>

<file name="apps/web/src/components/connections/ConnectionDialog.tsx">

<violation number="1" location="apps/web/src/components/connections/ConnectionDialog.tsx:94">
P3: Pairing selection and reset behavior can regress without a focused test for the new reducer, especially around preserving a valid selection versus clearing the QR/copy state. Adding reducer-level tests for `loaded`, `select`, `reset`, and error transitions would make this refactor safer to maintain.</violation>
</file>

<file name="apps/web/src/components/shows/ShowDeleteDialog.tsx">

<violation number="1" location="apps/web/src/components/shows/ShowDeleteDialog.tsx:31">
P2: A fast deletion completion can observe a stale `dialogRef` during the gap before this passive effect runs, causing a closed or replaced dialog to be treated as the original delete and triggering incorrect close/navigation behavior. Synchronizing this latest-value ref in a layout effect (or another commit-synchronous path) preserves render purity without leaving that window.</violation>
</file>

<file name="apps/web/src/components/songs/MicrophoneName.tsx">

<violation number="1" location="apps/web/src/components/songs/MicrophoneName.tsx:56">
P2: Pressing Escape can still save the edited microphone name: this branch closes the editor without suppressing `onBlur`, whose stale `value` may be submitted when focus is lost. Tracking a `suppressNextBlurSave` ref (as `ProfileSwitcher` does) and skipping the next blur save would preserve Escape as a cancel action.</violation>
</file>

<file name="apps/web/src/components/songs/SongDetail.tsx">

<violation number="1" location="apps/web/src/components/songs/SongDetail.tsx:106">
P1: Editing a song while mixes or microphones are still loading can silently clear every saved assignment and microphone override. The empty fallback lists should not be treated as authoritative; preserving the existing payload or deferring saves until both lists are loaded avoids this data loss.</violation>

<violation number="2" location="apps/web/src/components/songs/SongDetail.tsx:180">
P2: A microphone-name save can fail after the user has edited another song field, and the failure handler then discards those newer local edits by resetting the entire form to the original song. Serializing this request with the other saves, or reconciling only the failed microphone-name change, prevents that data loss.</violation>
</file>

<file name="apps/web/src/components/songs/SongMixAssignments.tsx">

<violation number="1" location="apps/web/src/components/songs/SongMixAssignments.tsx:66">
P3: The assignment count can include microphones that have since been deleted, causing the UI to display e.g. “1 mic on” while every visible microphone is unselected. Filtering assignment IDs against the current `microphones` list before constructing `selected` keeps the count consistent with the controls rendered below.</violation>
</file>

<file name="apps/web/src/components/chats/ChatPresetDialog.tsx">

<violation number="1" location="apps/web/src/components/chats/ChatPresetDialog.tsx:145">
P2: After a successful preset send, reopening the dialog shows the previously used preset instead of the preset list because the close callback bypasses `changeOpen` and the reset effect is gone. Routing this close through `changeOpen(false)` (and preserving a reset for externally controlled closes) would keep dialog sessions predictable.</violation>
</file>

<file name="apps/web/src/components/chats/ChatMessageBody.tsx">

<violation number="1" location="apps/web/src/components/chats/ChatMessageBody.tsx:15">
P2: Empty adjacent parts can still produce duplicate React keys, so React may warn and reconcile these sibling spans unpredictably. Including the part index as a uniqueness fallback (or otherwise assigning an ID) would preserve unique keys for all contract-valid `ChatMessagePart` arrays.</violation>
</file>

<file name="apps/web/src/routes/shows/$showId/setlist/$songId.tsx">

<violation number="1" location="apps/web/src/routes/shows/$showId/setlist/$songId.tsx:75">
P1: Saving an edit can immediately remount `SongDetail` before the RPC resolves because the optimistic edit changes `updatedAt`; the new instance recreates `isSaving`/`isSavingRef` as false, allowing overlapping edits, and failure updates target the discarded instance so the save error can be lost. A stable component identity plus explicit synchronization of server changes would preserve the in-flight save state without reintroducing stale drafts.</violation>
</file>

<file name="apps/web/src/components/chats/ChatWorkspace.tsx">

<violation number="1" location="apps/web/src/components/chats/ChatWorkspace.tsx:134">
P2: Deleting the active channel now displays the first remaining channel without notifying `onSelectedChannelChange`, so `ChatDrawer` retains the deleted channel ID as its requested selection. The selection fallback needs to update the owning selection state (or the delete flow needs to explicitly select and publish the replacement channel) rather than only changing the rendered value.</violation>
</file>

<file name="apps/web/src/components/mixes/MixCard.tsx">

<violation number="1" location="apps/web/src/components/mixes/MixCard.tsx:71">
P3: The duplicate-label warning misses existing mix labels with surrounding whitespace because `trimmedNumber` is normalized but `other.number` is not. Comparing `other.number.trim()` would match the normalization performed by `commitNumber` and the existing microphone UI.</violation>

<violation number="2" location="apps/web/src/components/mixes/MixCard.tsx:141">
P2: Changing a mix number and immediately choosing a color can lose one of the two edits: both handlers issue full `mixes.edit` payloads, so whichever request runs last overwrites the other field with its stale snapshot. Serializing edits or changing the mutation to merge/patch fields would preserve both user changes.</violation>
</file>

<file name="apps/web/src/components/microphones/MicrophoneDeleteDialog.tsx">

<violation number="1" location="apps/web/src/components/microphones/MicrophoneDeleteDialog.tsx:57">
P2: The dialog can still be dismissed through the header X or Escape while `isDeleting` is true; only the footer Cancel button is disabled. That unmounts the component while the delete continues, so a failed request has no visible error and the user can reopen the item and start another deletion; keep the dialog open for all dismiss paths while deletion is in flight.</violation>
</file>
Architecture diagram
sequenceDiagram
    participant App as App Bootstrap (main.tsx)
    participant ChatNav as ChatNavigation module
    participant Router as TanStack Router
    participant Route as Route Component ($songId.tsx)
    participant SongDetail as SongDetail component
    participant MicrophoneCard as MicrophoneCard component
    participant MixCard as MixCard component
    participant DeleteDialog as DeleteDialog (Microphone/Mix/Song)
    participant UIItem as UI Item component
    participant Profiles as currentProfilesState
    participant AtomLayer as Effect Atom Layer
    participant Backend as Backend API

    Note over App,ChatNav: NEW: Chat navigation configuration extracted to bootstrap

    App->>ChatNav: configureChatNavigation({ getActiveShowId, navigateToShow })
    ChatNav->>ChatNav: Create internal makeChatNavigation instance
    App->>Router: router.state.matches (deferred)

    Note over App,AtomLayer: Connection dialog state management consolidated

    ConnectionDialog->>ConnectionDialog: useReducer(reducePairClientState, initial)
    loop Poll pairing info
        ConnectionDialog->>Backend: fetchPairingInfo(client)
        Backend-->>ConnectionDialog: { discovery, candidates }
        ConnectionDialog->>ConnectionDialog: dispatch({ type: "loaded", ... })
        alt Candidates empty
            ConnectionDialog->>ConnectionDialog: dispatch({ type: "error", message: "No local network found" })
        end
    end

    Note over Route,SongDetail: NEW: SongDetail extracted with useReducer

    Route->>AtomLayer: songAtoms(showId).songs
    Route->>AtomLayer: microphoneAtoms(showId)
    Route->>AtomLayer: mixAtoms(showId)
    Route-->>SongDetail: Pass data via props

    SongDetail->>SongDetail: useReducer(reduceSongDetailState, initialState)
    SongDetail->>AtomLayer: useAtomSet(songAtoms.edit)
    Note over SongDetail: CHANGED: derived state from useReducer, not multiple useStates

    SongDetail->>SongDetail: save() with field normalization
    SongDetail->>AtomLayer: edit(payload, { reactivityKeys })
    alt Success
        AtomLayer-->>SongDetail: Exit.success
    else Failure
        AtomLayer-->>SongDetail: Exit.failure
        SongDetail->>SongDetail: Rollback to original values
    end

    Note over SongDetail,UIItem: CHANGED: list item semantics (li/ul) for correct accessibility

    UIItem->>UIItem: Changed defaultTagName from "div" to "li"
    ItemGroup->>ItemGroup: Changed from "div role=list" to "ul"

    Note over Route,DeleteDialog: NEW: Delete dialogs extracted as separate components

    DeleteDialog->>AtomLayer: useAtomSet(microphoneAtoms.delete) / mixAtoms.delete / songAtoms.delete
    DeleteDialog->>DeleteDialog: confirmDelete()
    DeleteDialog->>AtomLayer: delete(payload, { reactivityKeys })
    alt Success
        AtomLayer-->>DeleteDialog: Exit.success
        DeleteDialog->>DeleteDialog: onClose()
    else Failure
        AtomLayer-->>DeleteDialog: Exit.failure
        DeleteDialog->>DeleteDialog: Display error, keep dialog open
    end

    Note over MicrophoneCard,MixCard: NEW: Extracted from route files

    MicrophoneCard->>AtomLayer: useAtomSet(microphoneAtoms.edit)
    MicrophoneCard->>MicrophoneCard: Use Map lookup for duplicate check
    Note over MicrophoneCard: CHANGED: MicrophoneById map avoids linear find()

    Note over Profiles: CHANGED: currentProfilesState extracted to dedicated module

    ProfilesSwitcher->>Profiles: import from /profiles/currentProfilesState
    Profiles->>Profiles: Handle AsyncResult.Success & Failure with fallback

    Note over App,Backend: Preset dialog reset moved from useEffect to callback

    ChatPresetDialog->>ChatPresetDialog: changeOpen() resets mode synchronously
    Note over ChatPresetDialog: CHANGED: No useEffect dependency on "open"

    Note over Route,AtomLayer: SongDetail re-keyed on update to force fresh instance

    Route->>Route: key={`${song.id}:${DateTime.toEpochMillis(song.updatedAt)}`}
    Route-->>SongDetail: Force remount on update

    Note over App,ChatNav: ChatWorkspace: derived state replaces useEffect sync

    ChatWorkspace->>ChatWorkspace: selectedChannelId = requestedChannelId ?? internalSelectedChannelId
    Note over ChatWorkspace: CHANGED: Removed two useEffects, replaced with derived computation
Loading

Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.

Re-trigger cubic


return (
<SongDetail
key={`${song.id}:${DateTime.toEpochMillis(song.updatedAt)}`}

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: Saving an edit can immediately remount SongDetail before the RPC resolves because the optimistic edit changes updatedAt; the new instance recreates isSaving/isSavingRef as false, allowing overlapping edits, and failure updates target the discarded instance so the save error can be lost. A stable component identity plus explicit synchronization of server changes would preserve the in-flight save state without reintroducing stale drafts.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/routes/shows/$showId/setlist/$songId.tsx, line 75:

<comment>Saving an edit can immediately remount `SongDetail` before the RPC resolves because the optimistic edit changes `updatedAt`; the new instance recreates `isSaving`/`isSavingRef` as false, allowing overlapping edits, and failure updates target the discarded instance so the save error can be lost. A stable component identity plus explicit synchronization of server changes would preserve the in-flight save state without reintroducing stale drafts.</comment>

<file context>
@@ -110,6 +72,7 @@ function RouteComponent() {
 
   return (
     <SongDetail
+      key={`${song.id}:${DateTime.toEpochMillis(song.updatedAt)}`}
       showId={typedShowId}
       song={song}
</file context>

const nextName = (next?.name ?? name).trim();
const nextArtist = (next?.artist ?? artist).trim();
if (blockUi && isSavingRef.current) return false;
const activeMixIds = new Set(mixes.map((mix) => mix.id));

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: Editing a song while mixes or microphones are still loading can silently clear every saved assignment and microphone override. The empty fallback lists should not be treated as authoritative; preserving the existing payload or deferring saves until both lists are loaded avoids this data loss.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/components/songs/SongDetail.tsx, line 106:

<comment>Editing a song while mixes or microphones are still loading can silently clear every saved assignment and microphone override. The empty fallback lists should not be treated as authoritative; preserving the existing payload or deferring saves until both lists are loaded avoids this data loss.</comment>

<file context>
@@ -0,0 +1,272 @@
+    const nextName = (next?.name ?? name).trim();
+    const nextArtist = (next?.artist ?? artist).trim();
+    if (blockUi && isSavingRef.current) return false;
+    const activeMixIds = new Set(mixes.map((mix) => mix.id));
+    const activeMicrophoneIds = new Set(microphones.map((microphone) => microphone.id));
+    const normalizedAssignments = (next?.assignments ?? assignments).flatMap((assignment) => {
</file context>

variant="destructive"
size="icon-sm"
aria-label={`Delete microphone ${number}`}
disabled={microphone.pending}

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: An optimistic microphone can still be edited while its create mutation is pending, so these controls can submit edits against the temporary ID and show avoidable failures or conflicting optimistic state. The pending guard would be more reliable if it also disabled the number/name inputs, the popover trigger, and color buttons, as MixCard does.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/components/microphones/MicrophoneCard.tsx, line 89:

<comment>An optimistic microphone can still be edited while its create mutation is pending, so these controls can submit edits against the temporary ID and show avoidable failures or conflicting optimistic state. The pending guard would be more reliable if it also disabled the number/name inputs, the popover trigger, and color buttons, as `MixCard` does.</comment>

<file context>
@@ -0,0 +1,182 @@
+        variant="destructive"
+        size="icon-sm"
+        aria-label={`Delete microphone ${number}`}
+        disabled={microphone.pending}
+        onClick={onDelete}
+        className="absolute top-2 right-2 md:opacity-0 md:transition-opacity md:group-hover/card:opacity-100 md:focus-visible:opacity-100"
</file context>


return (
<Dialog open={open} onOpenChange={onOpenChange}>
<Dialog open={open} onOpenChange={changeOpen}>

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: After a successful preset send, reopening the dialog shows the previously used preset instead of the preset list because the close callback bypasses changeOpen and the reset effect is gone. Routing this close through changeOpen(false) (and preserving a reset for externally controlled closes) would keep dialog sessions predictable.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/components/chats/ChatPresetDialog.tsx, line 145:

<comment>After a successful preset send, reopening the dialog shows the previously used preset instead of the preset list because the close callback bypasses `changeOpen` and the reset effect is gone. Routing this close through `changeOpen(false)` (and preserving a reset for externally controlled closes) would keep dialog sessions predictable.</comment>

<file context>
@@ -142,7 +142,7 @@ export function ChatPresetDialog({
 
   return (
-    <Dialog open={open} onOpenChange={onOpenChange}>
+    <Dialog open={open} onOpenChange={changeOpen}>
       <DialogContent className="sm:max-w-xl">
         <DialogHeader>
</file context>

Comment on lines +15 to +16
return parts.map((part) => {
const key = `${part.type}:${characterOffset}:${part.text}`;

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: Empty adjacent parts can still produce duplicate React keys, so React may warn and reconcile these sibling spans unpredictably. Including the part index as a uniqueness fallback (or otherwise assigning an ID) would preserve unique keys for all contract-valid ChatMessagePart arrays.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/components/chats/ChatMessageBody.tsx, line 15:

<comment>Empty adjacent parts can still produce duplicate React keys, so React may warn and reconcile these sibling spans unpredictably. Including the part index as a uniqueness fallback (or otherwise assigning an ID) would preserve unique keys for all contract-valid `ChatMessagePart` arrays.</comment>

<file context>
@@ -11,14 +11,17 @@ export function ChatMessageBody({
-  return parts.map((part, index) => {
-    if (part.type === "text") return <span key={index}>{part.text}</span>;
+  let characterOffset = 0;
+  return parts.map((part) => {
+    const key = `${part.type}:${characterOffset}:${part.text}`;
+    characterOffset += part.text.length;
</file context>
Suggested change
return parts.map((part) => {
const key = `${part.type}:${characterOffset}:${part.text}`;
return parts.map((part, index) => {
const key = `${part.type}:${characterOffset}:${part.text}:${index}`;

DialogTitle,
} from "@/components/ui/dialog";

export function MixDeleteDialog({

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.

P3: The mix delete workflow now duplicates the existing microphone delete dialog, so fixes to shared deletion behavior can diverge between these screens. A shared resource-agnostic delete dialog or hook would keep the two flows consistent.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/components/mixes/MixDeleteDialog.tsx, line 23:

<comment>The mix delete workflow now duplicates the existing microphone delete dialog, so fixes to shared deletion behavior can diverge between these screens. A shared resource-agnostic delete dialog or hook would keep the two flows consistent.</comment>

<file context>
@@ -0,0 +1,77 @@
+  DialogTitle,
+} from "@/components/ui/dialog";
+
+export function MixDeleteDialog({
+  mix,
+  showId,
</file context>

| { readonly type: "copied" }
| { readonly type: "error"; readonly message: string };

const reducePairClientState = (

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.

P3: Pairing selection and reset behavior can regress without a focused test for the new reducer, especially around preserving a valid selection versus clearing the QR/copy state. Adding reducer-level tests for loaded, select, reset, and error transitions would make this refactor safer to maintain.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/components/connections/ConnectionDialog.tsx, line 94:

<comment>Pairing selection and reset behavior can regress without a focused test for the new reducer, especially around preserving a valid selection versus clearing the QR/copy state. Adding reducer-level tests for `loaded`, `select`, `reset`, and error transitions would make this refactor safer to maintain.</comment>

<file context>
@@ -56,11 +56,70 @@ import {
+  | { readonly type: "copied" }
+  | { readonly type: "error"; readonly message: string };
+
+const reducePairClientState = (
+  state: PairClientState,
+  action: PairClientAction,
</file context>

Comment thread apps/web/src/components/connections/ConnectionDialog.tsx Outdated
};

const trimmedNumber = number.trim();
const duplicate = mixes.some((other) => other.id !== mix.id && other.number === trimmedNumber);

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.

P3: The duplicate-label warning misses existing mix labels with surrounding whitespace because trimmedNumber is normalized but other.number is not. Comparing other.number.trim() would match the normalization performed by commitNumber and the existing microphone UI.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/components/mixes/MixCard.tsx, line 71:

<comment>The duplicate-label warning misses existing mix labels with surrounding whitespace because `trimmedNumber` is normalized but `other.number` is not. Comparing `other.number.trim()` would match the normalization performed by `commitNumber` and the existing microphone UI.</comment>

<file context>
@@ -0,0 +1,189 @@
+  };
+
+  const trimmedNumber = number.trim();
+  const duplicate = mixes.some((other) => other.id !== mix.id && other.number === trimmedNumber);
+  const commitNumber = async () => {
+    const valid = trimmedNumber || mix.number;
</file context>
Suggested change
const duplicate = mixes.some((other) => other.id !== mix.id && other.number === trimmedNumber);
const duplicate = mixes.some((other) => other.id !== mix.id && other.number.trim() === trimmedNumber);

<div className="grid gap-3 md:grid-cols-2">
{orderedMixes.map((mix, index) => {
const assignment = assignments.find((item) => item.mixId === mix.id);
const selected = new Set(assignment?.microphoneIds ?? []);

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.

P3: The assignment count can include microphones that have since been deleted, causing the UI to display e.g. “1 mic on” while every visible microphone is unselected. Filtering assignment IDs against the current microphones list before constructing selected keeps the count consistent with the controls rendered below.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/components/songs/SongMixAssignments.tsx, line 66:

<comment>The assignment count can include microphones that have since been deleted, causing the UI to display e.g. “1 mic on” while every visible microphone is unselected. Filtering assignment IDs against the current `microphones` list before constructing `selected` keeps the count consistent with the controls rendered below.</comment>

<file context>
@@ -0,0 +1,154 @@
+          <div className="grid gap-3 md:grid-cols-2">
+            {orderedMixes.map((mix, index) => {
+              const assignment = assignments.find((item) => item.mixId === mix.id);
+              const selected = new Set(assignment?.microphoneIds ?? []);
+              return (
+                <Card
</file context>
Suggested change
const selected = new Set(assignment?.microphoneIds ?? []);
const selected = new Set(
assignment?.microphoneIds.filter((id) => microphones.some((microphone) => microphone.id === id)) ?? [],
);

@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.

11 issues found across 34 files

Confidence score: 2/5

  • The highest-risk pattern is optimistic UI unmounting components before mutations settle in apps/web/src/components/songs/SongDeleteDialog.tsx and apps/web/src/routes/shows/$showId/mixes.tsx: failed delete/edit paths lose their error and rollback handlers because SongDetail/MixCard are gone, so users can see silent failures or stale state — keep failure UI/rollback state mounted until the request resolves (or decouple keys from optimistic timestamps).
  • Deletion dialogs remain dismissible while requests are in flight in apps/web/src/components/microphones/MicrophoneDeleteDialog.tsx, apps/web/src/components/mixes/MixDeleteDialog.tsx, and apps/web/src/components/songs/SongDeleteDialog.tsx, which can route callbacks to a now-closed/new dialog and trigger unexpected close/navigation behavior — block X/Escape/backdrop dismissal until the mutation completes.
  • apps/web/src/components/microphones/MicrophoneCard.tsx allows edits while microphone.pending, so submits can use a temporary ID and fail with “Microphone not found,” creating a clear create-then-edit regression for new items — disable editable fields until creation finishes.
  • Concurrent/local-edit synchronization looks fragile in apps/web/src/components/songs/SongDetail.tsx and apps/web/src/components/songs/MicrophoneName.tsx: out-of-order full-record saves and effect-driven value resets can overwrite newer microphone-name input, leading to lost user edits — serialize/patch saves and avoid syncing remote value while actively editing.
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="apps/web/src/components/mixes/MixDeleteDialog.tsx">

<violation number="1" location="apps/web/src/components/mixes/MixDeleteDialog.tsx:58">
P2: An in-flight removal can still be dismissed through the dialog's X, Escape, or backdrop. Reopening another mix before the first request resolves lets that first success call `onClose()` and close the newer confirmation; keep the controlled dialog open while `isDeleting`.</violation>
</file>

<file name="apps/web/src/components/microphones/MicrophoneDeleteDialog.tsx">

<violation number="1" location="apps/web/src/components/microphones/MicrophoneDeleteDialog.tsx:23">
P3: Microphone and mix removal flows now have near-identical dialog implementations, so behavior fixes (including pending-close handling) must be made twice. A shared confirmation dialog or shared delete-flow hook would keep loading/error handling consistent while callers supply entity labels and mutation callbacks.</violation>

<violation number="2" location="apps/web/src/components/microphones/MicrophoneDeleteDialog.tsx:60">
P2: Closing via the title-bar X while removal is pending hides the dialog but does not cancel `deleteMicrophone`; a failed request then sets an error in a newly closed dialog. Keep the dialog open until the in-flight deletion resolves, matching the disabled Cancel button's behavior.</violation>
</file>

<file name="apps/web/src/components/songs/SongDeleteDialog.tsx">

<violation number="1" location="apps/web/src/components/songs/SongDeleteDialog.tsx:45">
P1: A rejected delete cannot show this error: optimistic removal unmounts `SongDetail` and this dialog before `deleteSong` resolves. Keep failure UI mounted outside the detail route or retain the detail while the mutation settles, so users can retry instead of only seeing “Song not found.”</violation>

<violation number="2" location="apps/web/src/components/songs/SongDeleteDialog.tsx:55">
P2: While deletion is pending, the dialog can still be closed with its X button (and other dialog dismissal gestures), then successful completion forces navigation to the setlist. Prevent dismissal while `isDeleting`, or cancel/ignore the pending completion after dismissal, so closing the dialog does not unexpectedly override navigation.</violation>
</file>

<file name="apps/web/src/routes/shows/$showId/mixes.tsx">

<violation number="1" location="apps/web/src/routes/shows/$showId/mixes.tsx:64">
P2: A failed mix edit no longer reports its error: the optimistic edit changes `updatedAt` immediately, which changes this key and unmounts `MixCard` before `edit()` resolves, so `setSaveError` and the rollback setters run against the discarded instance. Keeping the card mounted through the mutation and synchronizing/resetting its draft state after the result would preserve the failure feedback.</violation>
</file>

<file name="apps/web/src/components/microphones/MicrophoneCard.tsx">

<violation number="1" location="apps/web/src/components/microphones/MicrophoneCard.tsx:76">
P2: On medium-and-larger screens, the delete control remains transparent on mouse hover because no `group/card` parent exists. Add the named group class to this card so the visible-on-hover affordance works.</violation>

<violation number="2" location="apps/web/src/components/microphones/MicrophoneCard.tsx:110">
P2: Editing a newly added microphone before its create request finishes submits its temporary ID and fails with “Microphone not found.” Disable number, color, and name editing while `microphone.pending`, as `MixCard` does.</violation>
</file>

<file name="apps/web/src/components/songs/SongDetail.tsx">

<violation number="1" location="apps/web/src/components/songs/SongDetail.tsx:180">
P2: Rapidly renaming two microphones can lose the later override: this non-blocking full-record save may complete after a newer save and overwrite its `microphoneNames`. Serialize these edits (or make the mutation patch a single override) so requests cannot apply stale snapshots out of order.</violation>
</file>

<file name="apps/web/src/components/songs/MicrophoneName.tsx">

<violation number="1" location="apps/web/src/components/songs/MicrophoneName.tsx:22">
P2: An incoming microphone-name update can erase text currently being typed because this effect resets `value` even while `editing` is true. Sync the displayed value only outside edit mode so the local draft remains stable until blur/Escape.</violation>
</file>

<file name="apps/web/src/components/connections/ConnectToShowtime.tsx">

<violation number="1" location="apps/web/src/components/connections/ConnectToShowtime.tsx:41">
P2: Dialog snaps shut instead of animating out. Changing `key` on every `connectOpen` transition causes React to immediately unmount the dialog when closing, which interrupts the CSS close animation (`data-closed:animate-out`, `data-closed:fade-out-0`, `data-closed:zoom-out-95`, 100ms duration) defined in the Base UI dialog overlay and content.</violation>
</file>
Architecture diagram
sequenceDiagram
    participant App as main.tsx (App Root)
    participant ChatNav as ChatNavigation module
    participant ChatDrawer as ChatDrawer component
    participant ChatPreset as ChatPresetDialog
    participant PairClient as PairClientDialog (ConnectionDialog)
    participant SongDet as SongDetail component
    participant MicroList as Microphones route
    participant MixList as Mixes route
    participant Item as Item/ItemGroup UI
    participant NotifCenter as NotificationCenter

    Note over App,NotifCenter: NEW: Chat navigation configuration moved to app root

    App->>ChatNav: NEW: configureChatNavigation({getActiveShowId, navigateToShow})
    ChatNav->>ChatNav: creates/assigns singleton chatNavigation

    Note over ChatNav,ChatDrawer: CHANGED: ChatDrawer uses useEffectEvent for stable callback
    ChatDrawer->>ChatDrawer: CHANGED: wraps consumeChatOpenRequest in useEffectEvent
    ChatDrawer->>ChatDrawer: CHANGED: empty dependency array on useEffect (no reactivity to showId)

    Note over ChatPreset: CHANGED: Dialog reset on close via onOpenChange wrapper
    ChatPreset->>ChatPreset: back() on close → reset mode to "list"

    Note over PairClient: CHANGED: useReducer replaces multiple useState calls
    PairClient->>PairClient: dispatch({type:"reset"}) on mount
    loop polling
        PairClient->>PairClient: CHANGED: dispatch({type:"loaded", ...}) with aggregated state
        alt failure
            PairClient->>PairClient: dispatch({type:"error", message})
        end
    end
    PairClient->>PairClient: dispatch({type:"prepare-qr-code"}) before QR generation

    Note over SongDet: CHANGED: useReducer for song detail state
    SongDet->>SongDet: updatedAt key forces remount on data change
    SongDet->>SongDet: try/finally around async edit operations
    alt RPC failure
        SongDet->>SongDet: restore to original song state
    end

    Note over MicroList,MixList: EXTRACTED: MicrophoneCard, MixCard to separate components
    MicroList->>MicroList: key={`${id}:${updatedAt}`}
    MixList->>MixList: key={`${id}:${updatedAt}`}

    Note over Item: CHANGED: semantic HTML (ul/li replaces role=list)
    Item->>Item: CHANGED: aria attributes (aria-label on Switch)

    Note over NotifCenter: REMOVED: subscribeNotifications listener set
    NotifCenter->>NotifCenter: CHANGED: no longer iterates listeners on publish

    Note over PairClient,SongDet: CHANGED: all async mutations wrapped in try/finally
    PairClient->>PairClient: try/finally ensures setSaving(false) on error or success
    SongDet->>SongDet: try/finally prevents stuck saving state

    Note over App,NotifCenter: KEY: no stale closure bugs due to useEffectEvent/reducer patterns
Loading

Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.

Re-trigger cubic

reactivityKeys: songsRpcReactivityKey(showId),
});
if (Exit.isFailure(result)) {
setError(rpcErrorMessageFromCause(result.cause));

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: A rejected delete cannot show this error: optimistic removal unmounts SongDetail and this dialog before deleteSong resolves. Keep failure UI mounted outside the detail route or retain the detail while the mutation settles, so users can retry instead of only seeing “Song not found.”

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/components/songs/SongDeleteDialog.tsx, line 45:

<comment>A rejected delete cannot show this error: optimistic removal unmounts `SongDetail` and this dialog before `deleteSong` resolves. Keep failure UI mounted outside the detail route or retain the detail while the mutation settles, so users can retry instead of only seeing “Song not found.”</comment>

<file context>
@@ -0,0 +1,79 @@
+        reactivityKeys: songsRpcReactivityKey(showId),
+      });
+      if (Exit.isFailure(result)) {
+        setError(rpcErrorMessageFromCause(result.cause));
+        return;
+      }
</file context>

const label = mix?.name?.trim() ? `“${mix.name}” (${mix.number})` : `mix ${mix?.number ?? ""}`;

return (
<Dialog open={mix !== undefined} onOpenChange={(open) => !open && onClose()}>

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: An in-flight removal can still be dismissed through the dialog's X, Escape, or backdrop. Reopening another mix before the first request resolves lets that first success call onClose() and close the newer confirmation; keep the controlled dialog open while isDeleting.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/components/mixes/MixDeleteDialog.tsx, line 58:

<comment>An in-flight removal can still be dismissed through the dialog's X, Escape, or backdrop. Reopening another mix before the first request resolves lets that first success call `onClose()` and close the newer confirmation; keep the controlled dialog open while `isDeleting`.</comment>

<file context>
@@ -0,0 +1,80 @@
+  const label = mix?.name?.trim() ? `“${mix.name}” (${mix.number})` : `mix ${mix?.number ?? ""}`;
+
+  return (
+    <Dialog open={mix !== undefined} onOpenChange={(open) => !open && onClose()}>
+      <DialogContent>
+        <DialogHeader>
</file context>

: `microphone ${microphone?.number ?? ""}`;

return (
<Dialog open={microphone !== undefined} onOpenChange={(open) => !open && onClose()}>

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: Closing via the title-bar X while removal is pending hides the dialog but does not cancel deleteMicrophone; a failed request then sets an error in a newly closed dialog. Keep the dialog open until the in-flight deletion resolves, matching the disabled Cancel button's behavior.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/components/microphones/MicrophoneDeleteDialog.tsx, line 60:

<comment>Closing via the title-bar X while removal is pending hides the dialog but does not cancel `deleteMicrophone`; a failed request then sets an error in a newly closed dialog. Keep the dialog open until the in-flight deletion resolves, matching the disabled Cancel button's behavior.</comment>

<file context>
@@ -0,0 +1,84 @@
+    : `microphone ${microphone?.number ?? ""}`;
+
+  return (
+    <Dialog open={microphone !== undefined} onOpenChange={(open) => !open && onClose()}>
+      <DialogContent>
+        <DialogHeader>
</file context>

}
};
return (
<Dialog open={open} onOpenChange={onOpenChange}>

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: While deletion is pending, the dialog can still be closed with its X button (and other dialog dismissal gestures), then successful completion forces navigation to the setlist. Prevent dismissal while isDeleting, or cancel/ignore the pending completion after dismissal, so closing the dialog does not unexpectedly override navigation.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/components/songs/SongDeleteDialog.tsx, line 55:

<comment>While deletion is pending, the dialog can still be closed with its X button (and other dialog dismissal gestures), then successful completion forces navigation to the setlist. Prevent dismissal while `isDeleting`, or cancel/ignore the pending completion after dismissal, so closing the dialog does not unexpectedly override navigation.</comment>

<file context>
@@ -0,0 +1,79 @@
+    }
+  };
+  return (
+    <Dialog open={open} onOpenChange={onOpenChange}>
+      <DialogContent>
+        <DialogHeader>
</file context>

{mixes.map((mix) => (
<MixCard
key={mix.id}
key={`${mix.id}:${DateTime.toEpochMillis(mix.updatedAt)}`}

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: A failed mix edit no longer reports its error: the optimistic edit changes updatedAt immediately, which changes this key and unmounts MixCard before edit() resolves, so setSaveError and the rollback setters run against the discarded instance. Keeping the card mounted through the mutation and synchronizing/resetting its draft state after the result would preserve the failure feedback.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/routes/shows/$showId/mixes.tsx, line 64:

<comment>A failed mix edit no longer reports its error: the optimistic edit changes `updatedAt` immediately, which changes this key and unmounts `MixCard` before `edit()` resolves, so `setSaveError` and the rollback setters run against the discarded instance. Keeping the card mounted through the mutation and synchronizing/resetting its draft state after the result would preserve the failure feedback.</comment>

<file context>
@@ -83,7 +61,7 @@ function RouteComponent() {
           {mixes.map((mix) => (
             <MixCard
-              key={mix.id}
+              key={`${mix.id}:${DateTime.toEpochMillis(mix.updatedAt)}`}
               mix={mix}
               mixes={mixes}
</file context>


const colors = microphoneColorClassNames[color];
return (
<Card className="relative">

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: On medium-and-larger screens, the delete control remains transparent on mouse hover because no group/card parent exists. Add the named group class to this card so the visible-on-hover affordance works.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/components/microphones/MicrophoneCard.tsx, line 76:

<comment>On medium-and-larger screens, the delete control remains transparent on mouse hover because no `group/card` parent exists. Add the named group class to this card so the visible-on-hover affordance works.</comment>

<file context>
@@ -0,0 +1,175 @@
+
+  const colors = microphoneColorClassNames[color];
+  return (
+    <Card className="relative">
+      <Button
+        type="button"
</file context>

...(override ? [{ microphoneId: microphone.id, name: override }] : []),
];
update({ microphoneNames: nextMicrophoneNames });
void save({ microphoneNames: nextMicrophoneNames }, false);

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: Rapidly renaming two microphones can lose the later override: this non-blocking full-record save may complete after a newer save and overwrite its microphoneNames. Serialize these edits (or make the mutation patch a single override) so requests cannot apply stale snapshots out of order.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/components/songs/SongDetail.tsx, line 180:

<comment>Rapidly renaming two microphones can lose the later override: this non-blocking full-record save may complete after a newer save and overwrite its `microphoneNames`. Serialize these edits (or make the mutation patch a single override) so requests cannot apply stale snapshots out of order.</comment>

<file context>
@@ -0,0 +1,272 @@
+      ...(override ? [{ microphoneId: microphone.id, name: override }] : []),
+    ];
+    update({ microphoneNames: nextMicrophoneNames });
+    void save({ microphoneNames: nextMicrophoneNames }, false);
+  };
+
</file context>

const [editing, setEditing] = React.useState(false);
const [value, setValue] = React.useState(displayedName);

React.useEffect(() => setValue(displayedName), [displayedName]);

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: An incoming microphone-name update can erase text currently being typed because this effect resets value even while editing is true. Sync the displayed value only outside edit mode so the local draft remains stable until blur/Escape.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/components/songs/MicrophoneName.tsx, line 22:

<comment>An incoming microphone-name update can erase text currently being typed because this effect resets `value` even while `editing` is true. Sync the displayed value only outside edit mode so the local draft remains stable until blur/Escape.</comment>

<file context>
@@ -0,0 +1,65 @@
+  const [editing, setEditing] = React.useState(false);
+  const [value, setValue] = React.useState(displayedName);
+
+  React.useEffect(() => setValue(displayedName), [displayedName]);
+
+  if (!editing) {
</file context>

</Empty>
<ConnectionLinkDialog open={connectOpen} onOpenChange={setConnectOpen} />
<ConnectionLinkDialog
key={connectOpen ? "open" : "closed"}

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: Dialog snaps shut instead of animating out. Changing key on every connectOpen transition causes React to immediately unmount the dialog when closing, which interrupts the CSS close animation (data-closed:animate-out, data-closed:fade-out-0, data-closed:zoom-out-95, 100ms duration) defined in the Base UI dialog overlay and content.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/components/connections/ConnectToShowtime.tsx, line 41:

<comment>Dialog snaps shut instead of animating out. Changing `key` on every `connectOpen` transition causes React to immediately unmount the dialog when closing, which interrupts the CSS close animation (`data-closed:animate-out`, `data-closed:fade-out-0`, `data-closed:zoom-out-95`, 100ms duration) defined in the Base UI dialog overlay and content.</comment>

<file context>
@@ -37,7 +37,11 @@ export function ConnectToShowtime({ error }: { readonly error?: string }) {
       </Empty>
-      <ConnectionLinkDialog open={connectOpen} onOpenChange={setConnectOpen} />
+      <ConnectionLinkDialog
+        key={connectOpen ? "open" : "closed"}
+        open={connectOpen}
+        onOpenChange={setConnectOpen}
</file context>

DialogTitle,
} from "@/components/ui/dialog";

export function MicrophoneDeleteDialog({

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.

P3: Microphone and mix removal flows now have near-identical dialog implementations, so behavior fixes (including pending-close handling) must be made twice. A shared confirmation dialog or shared delete-flow hook would keep loading/error handling consistent while callers supply entity labels and mutation callbacks.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/components/microphones/MicrophoneDeleteDialog.tsx, line 23:

<comment>Microphone and mix removal flows now have near-identical dialog implementations, so behavior fixes (including pending-close handling) must be made twice. A shared confirmation dialog or shared delete-flow hook would keep loading/error handling consistent while callers supply entity labels and mutation callbacks.</comment>

<file context>
@@ -0,0 +1,84 @@
+  DialogTitle,
+} from "@/components/ui/dialog";
+
+export function MicrophoneDeleteDialog({
+  microphone,
+  showId,
</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