fix(spec-builder): close delete, review, and i18n gaps

[kyleseaman]

Summary

• Add a confirm-to-remove path so a spec can leave Spec Builder without wiping the project's markdown.
• Keep approve / build / pause on the fullscreen review overlay; expanding a document no longer hides the only way to advance it.
• Translate leftover English (ACTIVE, mashed-together decision names, overlay title), show the derived spec name and empty-filter state, and drop lucide size={N}.
• Stop the overlay from remounting a second markdown tree and from stealing comment focus on every poll.
• Also rides along in touched files (declared per First Principles review): hide the working-directory path on mobile in the SpecDetail header (it truncated uselessly at narrow widths), and clean up the divider-drag listeners on unmount plus swap size={N} → lucide-inline on the four icons in edited lines.

Why no screenshot: Shared Btn/Modal chrome and catalog copy; no new layout, theme, or component surface beyond the existing Spec Builder header and confirm dialog.

Test plan

• First-run empty state → create a spec; confirm the derived name preview and folder picker.
• Filter the rail to a name that matches nothing; confirm the empty-filter copy.
• Expand a document for review and approve / start building / pause from the overlay.
• Remove a spec from the header; confirm the dialog, that the spec leaves the rail, and that .md files stay on disk.
• Switch the dashboard language and check the rail ACTIVE group, decision option names, and overlay title.
• cd website && npx vitest run src/test/SpecBuilder src/i18n/style/koStyle.test.ts src/i18n/deadKeys.test.ts --reporter=dot
• cd website && npm run i18n:check

Pattern harvest

Rule candidate: Never interpolate user-controlled names into fixed-width UI controls (buttons, dialog footers) — restate the name in wrapping body text and keep the control label static, so an unbroken long name cannot clip the dialog's actions at narrow viewports.

[github-actions]

Design Review (Fable 5) — ✅ PASS

Design-level review of 8f4736a36612194439f8894a2ca9d4b6d8f4f0c0 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

The backend confirms the design contract: _client_claim's docstring states "deleting a spec leaves its documents on disk by design," and the frontend's pendingRemove snapshot of spec_dir+slot_key matches the backend's stale-client identity-pinning protocol exactly. Every description claim has backing code, the ride-alongs are declared, the composer lift is the root-cause fix for the doubled markdown tree, and delete failure paths route through the existing error surface. No design-level concerns survive the kill-filter.

Design-Verdict: PASS

Delete wires an existing identity-pinned endpoint correctly, the composer lift fixes the doubled-render root cause, and every claimed behavior has backing code.

[DESIGN-REVIEWED] 8f4736a

[github-actions]

UX Review (Fable 5) — 🟡 CONCERNS

UX-level review of 8f4736a36612194439f8894a2ca9d4b6d8f4f0c0 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

UX-Verdict: CONCERNS

The new remove-spec confirm quotes the raw spec name, not the display label the user sees everywhere else — at a destructive decision point.

Watch

• Confirm dialog names the wrong identifier. remove_spec_body interpolates { name } ("“{{name}}” will leave Spec Builder"), but the header, rail, and rename flow all render detail?.title || name (SpecDetail.tsx:583, SpecRail.tsx:187). A user who renamed their spec's label is asked to confirm removing a folder name they may never have seen — the one job of a destructive confirm (verify the target) is undermined. Moderate frequency (rename lives in the same menu), confusion at a destructive point, every time. Fix: pass detail?.title || name to remove_spec_body and the delete_spec_named aria-label.

Suggestions

• Rail empty-filter state "No specs match that filter" could carry an inline "Clear filter" action so recovery is one click instead of re-editing the input.

[UX-REVIEWED] 8f4736a

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 8f4736a36612194439f8894a2ca9d4b6d8f4f0c0 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 8f4736a

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 8f4736a36612194439f8894a2ca9d4b6d8f4f0c0: <one-sentence reason>

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 8f4736a36612194439f8894a2ca9d4b6d8f4f0c0 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All checks are done. The delete UI wires a pre-existing zero-consumer specApi.remove (api.ts:273), removed i18n keys have no remaining consumers, Modal's Escape contract matches the diff's claim, the empty-state doesn't duplicate a shared component, and the icon swap is mandated by website/AGENTS.md:75 — with 10 size={N} siblings left in spec-builder.

First-Principles-Verdict: CONCERNS

A fix: PR ships two new capabilities and removes the mobile working-dir display on nobody's reported failure — all declared, none blocking.

What this change ships

Intent: let a spec be removed, reviewed, and read in any language without dead ends — mostly a FIX, carrying declared additions.

1. Remove-spec menu item + confirm dialog — justified: first consumer of specApi.remove (api.ts:273, 0 consumers at base), but a capability shipped under fix:
2. Approve/Build/Pause now on the fullscreen overlay — justified (expanding hid the only advance path)
3. Overlay no longer mounts a second markdown tree — justified (cause-level: dual mount was the mechanism)
4. Overlay stops stealing composer focus on each poll — justified (cause named: inline callback ref)
5. Comment draft survives expand/collapse (lifted composer) — justified, required by item 3
6. Rail ACTIVE, decision labels, overlay title translated; concatenation removed — justified, CI-gated invariant
7. Derived spec-name preview while typing — declared addition; no named person who was surprised
8. Empty-filter message in the rail — declared addition, no shared component duplicated (ListEmptyState is issue-radar-local)
9. Working-dir path hidden on mobile — rides along; visibility removal with only "truncated uselessly" as support
10. Divider-drag cleanup + 4 icon size={N} swaps — rides along, declared; icon rule is website/AGENTS.md:75

Watch

• Item 9 removes the only place a phone user sees which project a spec targets. For a removal the bar is who was failing — "it truncated uselessly at narrow widths" names no report. Zero option: leave it truncated; the title tooltip and ellipsis already existed.
• Framing: title says fix(spec-builder) while the description's first line says "Add a confirm-to-remove path" — item 1 is a feature completing a half-shipped surface, not a defect repair. Advisory only; the description itself is honest.
• Point patch, declared: grep size=\{ in website/src/apps/spec-builder leaves 10 siblings (7 in SpecDetail.tsx, 3 in TaskList.tsx) after fixing the 4 on edited lines. Accepted-and-deferred.

[FIRST-PRINCIPLES-REVIEWED] 8f4736a

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 8f4736a36612194439f8894a2ca9d4b6d8f4f0c0 — this comment is updated in place on each push.

Review details

No findings.

FINDING — website/src/apps/spec-builder/components/NewSpecView.tsx:93 — the preview i18nT('...this_spec_will_be_called', { name: autoName }) shows autoName, but on a name collision create() instead creates autoName.slice(0,44) + '-' + (Date.now()%1000), so the promised name is not the one created → Fix: phrase the preview as approximate, or suppress/adjust it once a collision suffix is applied.

[OPUS-REVIEWED] 8f4736a

Verdict parsed from the review's SHA-scoped output markers for commit 8f4736a36612194439f8894a2ca9d4b6d8f4f0c0.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 8f4736a36612194439f8894a2ca9d4b6d8f4f0c0: <one-sentence reason>

[bolichen97]

Approving per triage sweep: readiness passed, no blocking reviews, fix-type change. Auto-merge will be enabled; branch protection still gates.

[bolichen97]

Re-approved after conflict resolution against the latest main; focused validation passed.

[NicholasRBowers]

🤖 Kiro Crew [operator: NicholasRBowers#a942f9ca]

Review-ready ✅ — head a59eef4c7a7e535566f21aacce9dea26724552b1

• PR Readiness: passed (68 checks, 0 failing)
• GPT 5.6 Review: clean on current head (fresh [GPT-REVIEWED] stamp)
• Opus Review: clean on current head
• Unresolved review threads: 0
• Every advisory concern answered: 2 UX findings accepted-and-deferred → spec-builder: surface delete failure inside the confirm dialog; add Clear-filter action to empty rail state #7662; 3 First Principles concerns dispositioned (stale-diff fixed by rebase, icon-sweep proportionality rebuttal, undeclared riders declared in body)

History this drive: rebased 926 commits onto main (bundle-size breach resolved by the rebase), fixed GPT's [BLOCK-MERGE] on delete-control clipping at 320px (static confirm label + break-words, dead remove_spec key dropped from 13 locales), then rode out three GPT-lane infra attempts until attempt 3 completed clean.

Drive-to-green never merges — handing off to a human for final review/merge.

[NicholasRBowers]

🤖 Kiro Crew [operator: NicholasRBowers#a942f9ca]

Conflict-resolution round — a59eef4c7 → 8f4736a36 (rebased onto main @ 9af88218)

The PR went CONFLICTING after main landed the #7891 last-paragraph selection fix, which rewrote the same region of DocView.tsx this PR touches. Resolution:

• Kept main's newer selection handling: the hoisted onSelectionSettled using containedSelectionRange (and its dep-less effect wiring) — this PR's older in-effect handler with plain ancestor containment was dropped so Selection containment by commonAncestorContainer still dismisses last-line multi-click at UserMessage.tsx and DocView.tsx #7891 does not regress.
• Kept this PR's composer lift: export interface Selection, the DocComposer interface, and the required composer prop.
• Updated main's two new Selection containment by commonAncestorContainer still dismisses last-line multi-click at UserMessage.tsx and DocView.tsx #7891 tests in SpecBuilderDocView.test.tsx to render through the existing Harness (they instantiated DocView without the now-required composer prop and crashed on destructure).

Gates on the resolved tree: tsc -b 0 · vitest 28,368 passed / 0 failed (1796 files) · i18n:check 0 · bundle gate 720 chunks within budget. Single commit, Kyle Seaman authorship + both co-author trailers preserved.

[NicholasRBowers]

🤖 Kiro Crew [operator: NicholasRBowers#a942f9ca]

• Preview name may differ from created name on collision — span=c0a47e9e4cb6 — rebutted (holds, but disproportional to fix in this PR)

the preview i18nT('...this_spec_will_be_called', { name: autoName }) shows autoName, but on a name collision create() instead creates autoName.slice(0,44) + '-' + (Date.now()%1000) , so the promised name is not the one created

The observation is accurate: the collision retry (added for the "already exists" fallback) can create a suffixed name that differs from the previewed one. We are keeping the copy as-is because:

1. The mismatch is self-correcting within one interaction. The retry path calls onCreated(alt), which navigates straight to the spec under its actual name — the user sees the real name immediately and is never left acting on the stale promise.
2. The trigger is a rare edge case: it fires only when the backend reports a same-name spec already exists, and the previewed name is correct in every non-collision case.
3. The remedy is disproportionate here: softening the copy ("will be called approximately…") changes a user-visible i18n string across all 13 locales plus the context file and re-triggers screenshot evidence on a PR that is otherwise converged — weakening the common-case message to hedge a rare, self-revealing fallback.

If maintainers prefer hedged phrasing, it is a clean one-string follow-up alongside the UX polish already tracked in #7662.

[NicholasRBowers]

🤖 Kiro Crew [operator: NicholasRBowers#a942f9ca]

Review-ready again ✅ — head 8f4736a36612 (post conflict-resolution rebase)

• PR Readiness: passed; mergeable, no conflicts
• GPT + Opus: both fresh and non-blocking on the current head
• Opus's one advisory finding (preview name vs. collision-suffix, span=c0a47e9e4cb6) dispositioned as rebutted-disproportional above
• 0 unresolved threads; all 6 disposition records verified

Drive-to-green never merges — over to a human for final review/merge.

[bolichen97]

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

• This PR is PARTIALLY_COVERED with PR #3859. Coverage is explicitly incomplete; this finding is not a completion or closure claim. Recommended action for PR #5111: KEEP. The merged PR mitigated the symptom at one viewport and recorded the remaining gap; this PR fixes the cause at every viewport. Files: website/src/apps/spec-builder/components/SpecDetail.tsx.
• This PR is PARTIALLY_COVERED with PR #4565. Coverage is explicitly incomplete; this finding is not a completion or closure claim. Recommended action for PR #5111: KEEP. Only the API client and backend halves of spec deletion are on main; the UI half exists nowhere else and is what this PR adds. Files: website/src/apps/spec-builder/api.ts, src/kiro_crew/apps/builtins/spec_builder/backend/routes.py.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.

[bolichen97]

Re-approving after the conflict-resolution rebase. The twice-approved content is intact: removal is confirm-gated and calls the pre-existing specApi.remove, which leaves the project's markdown on disk; the fullscreen review overlay carries Approve/Start building/Pause and only one phaseActions() instance is in the tree; the #7891 last-paragraph selection fix is untouched (no duplicated or dropped hunk in DocView.tsx). All 12 new keys land in 13 catalogs. Two Low notes for a follow-up: 9 locales now use a second term for "spec" beside the existing one, and the overlay test asserts Approve -> Design without within(dialog) so it would pass on old code too.