Frontend polish items surfaced during PR #216 review (issue #215). All Minor — none block #216 merge; filed as follow-up per reviewer recommendation.
Reviewer: frontend-reviewer (PR #216 review, Task #17). Cite verbatim below.
F1 — StateEditModal silently ignores name-cleared-to-empty
File: src/components/workspace/StatesSectionRenderer.ts:382-393 (buildPatch)
buildPatch gates on if (trimmedName && trimmedName !== (this.state.name || '')) so a user who deliberately clears the name field then clicks Save sees a 'State updated' Notice but the name is unchanged (and there's no inline validation telling them empty names are not allowed).
Fix: In StateEditModal.onOpen, gate the Save button's enabled state on nameInput.getValue().trim().length > 0, OR in the Save click handler show new Notice('Name cannot be empty') and return without closing.
F2 — Empty-patch save still fires a no-op updateState + 'State updated' Notice
File: src/components/workspace/StatesSectionRenderer.ts:370-379 (Save button click)
If the user opens the edit modal and clicks Save without changing anything, buildPatch returns {} and onSave({}) is invoked. MemoryService.updateState writes a state_updated event with no actual changes (JSONL append + SQLite touch + cache invalidation), then the user sees 'State updated'. Wasted work + misleading UX.
Fix: In the Save handler at line 370: `const patch = this.buildPatch(); if (Object.keys(patch).length === 0) { this.close(); return; } void this.onSave(patch);` — or skip the close entirely and show a 'No changes' notice.
F3 — Double-click race on Archive/Restore (and Delete) buttons — no in-flight disable
Files: src/components/workspace/StatesSectionRenderer.ts:254-270 (toggleArchive), 272-290 (confirmAndDelete), 106-108 (Refresh)
The archive flow is read-modify-write: getState then updateState. Two rapid clicks fire two concurrent read-modify-writes with the SAME pre-toggle `existing` snapshot. If a state is rapidly clicked archive then restore before the first round-trip lands, the second read sees the pre-toggle snapshot and re-archives. Same shape applies to confirmAndDelete (two clicks open two confirmation modals). No `archiveBtn.disabled = true` while the round-trip is in flight, no row-level loading marker. The Refresh button has the same problem.
Fix: Track an in-flight set keyed by stateId; on toggleArchive/confirmAndDelete entry, bail if the id is already in-flight (or disable the buttons + show a spinner on the row). Same for the Refresh button — disable while loadAndRender is running.
F4 — WorkspaceDetailRenderer.renderStatesSection is fire-and-forget — stale-promise stomp on rapid re-render
File: src/components/workspace/WorkspaceDetailRenderer.ts:252-274
Each call to renderStatesSection captures `sectionHost` in the outer closure and resolves `getStatesService()` asynchronously. If the user navigates between workspaces (or the detail is re-rendered) before the previous service resolves, the older promise resolves second and stomps the newer section's contents. There's no AbortController / mounted-flag guard. Severity is Minor because (a) getStatesService is currently fast and (b) sectionHost.empty() only touches the older host. However if MemoryService is lazy-initialized in a future change this turns into a real bug.
Fix: Use a per-render token (e.g. `const renderToken = ++this.statesRenderToken;`) and inside `.then` check `if (renderToken !== this.statesRenderToken) return;`.
Context
Frontend polish items surfaced during PR #216 review (issue #215). All Minor — none block #216 merge; filed as follow-up per reviewer recommendation.
Reviewer: frontend-reviewer (PR #216 review, Task #17). Cite verbatim below.
F1 — StateEditModal silently ignores name-cleared-to-empty
File:
src/components/workspace/StatesSectionRenderer.ts:382-393(buildPatch)buildPatchgates onif (trimmedName && trimmedName !== (this.state.name || ''))so a user who deliberately clears the name field then clicks Save sees a 'State updated' Notice but the name is unchanged (and there's no inline validation telling them empty names are not allowed).Fix: In
StateEditModal.onOpen, gate the Save button's enabled state onnameInput.getValue().trim().length > 0, OR in the Save click handler shownew Notice('Name cannot be empty')and return without closing.F2 — Empty-patch save still fires a no-op updateState + 'State updated' Notice
File:
src/components/workspace/StatesSectionRenderer.ts:370-379(Save button click)If the user opens the edit modal and clicks Save without changing anything,
buildPatchreturns{}andonSave({})is invoked.MemoryService.updateStatewrites a state_updated event with no actual changes (JSONL append + SQLite touch + cache invalidation), then the user sees 'State updated'. Wasted work + misleading UX.Fix: In the Save handler at line 370: `const patch = this.buildPatch(); if (Object.keys(patch).length === 0) { this.close(); return; } void this.onSave(patch);` — or skip the close entirely and show a 'No changes' notice.
F3 — Double-click race on Archive/Restore (and Delete) buttons — no in-flight disable
Files:
src/components/workspace/StatesSectionRenderer.ts:254-270(toggleArchive),272-290(confirmAndDelete),106-108(Refresh)The archive flow is read-modify-write:
getStatethenupdateState. Two rapid clicks fire two concurrent read-modify-writes with the SAME pre-toggle `existing` snapshot. If a state is rapidly clicked archive then restore before the first round-trip lands, the second read sees the pre-toggle snapshot and re-archives. Same shape applies toconfirmAndDelete(two clicks open two confirmation modals). No `archiveBtn.disabled = true` while the round-trip is in flight, no row-level loading marker. The Refresh button has the same problem.Fix: Track an in-flight set keyed by stateId; on toggleArchive/confirmAndDelete entry, bail if the id is already in-flight (or disable the buttons + show a spinner on the row). Same for the Refresh button — disable while loadAndRender is running.
F4 — WorkspaceDetailRenderer.renderStatesSection is fire-and-forget — stale-promise stomp on rapid re-render
File:
src/components/workspace/WorkspaceDetailRenderer.ts:252-274Each call to
renderStatesSectioncaptures `sectionHost` in the outer closure and resolves `getStatesService()` asynchronously. If the user navigates between workspaces (or the detail is re-rendered) before the previous service resolves, the older promise resolves second and stomps the newer section's contents. There's no AbortController / mounted-flag guard. Severity is Minor because (a)getStatesServiceis currently fast and (b)sectionHost.empty()only touches the older host. However if MemoryService is lazy-initialized in a future change this turns into a real bug.Fix: Use a per-render token (e.g. `const renderToken = ++this.statesRenderToken;`) and inside `.then` check `if (renderToken !== this.statesRenderToken) return;`.
Context