Skip to content

fix(console): give the Delete Agent dialog the shared dialog action row - #3364

Merged
freeqaz merged 2 commits into
mainfrom
fix-1942
Oct 23, 2026
Merged

freeqaz merged 2 commits into
mainfrom
fix-1942

Conversation

@freeqaz

@freeqaz freeqaz commented Oct 23, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

The Agent page's Delete dialog (agents/deletion.mts) still put Cancel and "Permanently delete Agent" in the page's form-actions row. Every other confirming dialog (Stop, Slack, Create Secret, Namespaces) uses dialog-actions. The Delete dialog only looked the same because of a .dialog > .form-actions alias in console.css, left in #2879 to avoid a conflict with #2524 (finding 1942). #2524 has since landed.

Change

  • The Delete dialog's buttons now sit in dialog-actions.
  • The .dialog > .form-actions alias is removed. No other dialog had a direct form-actions child.
  • The dialog pattern test (tests/browser/console-dialogs.test.ts) now checks every dialog's .dialog-actions row: Cancel first, the confirming action last. Plugins has no such row, because its Done button sits in the header.

No visible change: the shared rule gave the alias the same layout.

before after
1440 before after
390 before after

Verification

  • With main's deletion.mts and console.css, the new check fails with delete: its .dialog-actions row (no row found). With the fix, it passes.
  • tests/browser/console-dialogs.test.ts passed 20/20 at 2 CPUs on the final head.
  • tsc (--build tsconfig.json tests/tsconfig.json), ESLint and Prettier are clean on the touched files.

Risks

Low. A class rename plus removing an alias rule that nothing else matched.

The Agent page's Delete dialog still built its buttons in the page's
form-actions row, kept looking right only through a .dialog > .form-actions
alias. It now uses dialog-actions like every other confirming dialog, and
the alias is gone. The dialog pattern test checks each confirming dialog's
action row (Cancel first); on main it fails with 'delete: []'.

Finding 1942.
@freeqaz
freeqaz requested a review from a team as a code owner October 23, 2026 22:04
@clawsweeper

clawsweeper Bot commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

…ins has none

Review nits on finding 1942's test: every case now names its expected
.dialog-actions buttons, so the confirming action's place is checked too,
and Plugins' empty row says its Done button sits in the header.
@freeqaz-openai

Copy link
Copy Markdown
Collaborator

Free's clanker here. I checked the Delete Agent change at 269427c6 with sandboxed Chromium and a local OCE controller. At 1440, 390, and 320 px wide, the two buttons and their surrounding space matched the base pixel for pixel. At 320 px the buttons stacked and both remained reachable.

Opening with Enter focused Cancel. Escape from either button restored focus to Delete Agent. Neither Escape nor Cancel sent a write. Deliberate keyboard confirmation sent one DELETE for the disposable Agent, received HTTP 202, and moved focus to Refresh deletion status. That checks request acceptance, not completion of cleanup. Both tests in the updated dialog test file passed. I found no regression in this interaction.

@clawsweeper clawsweeper Bot added clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-repro Preliminary submission classification; full source review pending. P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-repro Preliminary submission classification; full source review pending. labels Oct 23, 2026
@clawsweeper

clawsweeper Bot commented Oct 23, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs changes before merge.

What this changes

This PR moves Delete Agent buttons to the shared dialog action class, removes its temporary CSS alias, and extends browser coverage.

Example: Open Delete Agent at 390 × 844.

  • Before: Cancel and Permanently delete Agent appear in the bottom sheet through a temporary CSS alias.
  • After: The same buttons retain their layout through the shared dialog action class.

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The focused production patch appears correct and has relevant screenshots and browser coverage, but required Storybook verification and recording remain incomplete.
Proof confidence 🦪 silver shellfish (2/6) Real behavior proof is not required for maintainer- or bot-authored pull requests.
Patch quality 🐚 platinum hermit (4/6) 1 actionable review finding remain.

Product

Kind: Maintenance · Worth it: Yes · Fix scope: Complete
Reason: The collaborator-authored change removes an expressly temporary exception without adding another owner or changing operator behavior.

Merge readiness

⛔ Needs changes before merge - 1 item remains

Keep this PR open: it completes an explicitly deferred dialog cleanup with no functional regression found, but the matching Storybook update remains required.

Priority: P3
Reviewed head: 269427c6039ed0c104dbf638ffc9d73132859c95

Before merge

  • Update the matching Storybook coverage for the action-row cutover (P3) - AGENTS.md requires matching stories, scenarios, and workflow instructions to be updated with console changes. This PR changes the shared dialog action-row contract but leaves scripts/console-storybook unchanged. Update the existing Components/Dialogs coverage and dialog-pattern-workflow.md to describe and check the completed shared-row contract; reuse the existing Delete confirmation states rather than adding duplicate stories.

Findings

  • [P3] Update the matching Storybook coverage for the action-row cutover — apps/controller/src/console/agents/deletion.mts:415

Tests

  • Missing end-to-end proof: The supplied screenshots and reported base-fail/head-pass run support the change, but final-head rebuilt Storybook inspection and the required interaction recording are not supplied.
Agent review details

How this fits together

The controller serves console modules and CSS; the Agent page builds a native confirmation dialog whose buttons invoke the existing cancellation and deletion handlers.

flowchart TD
  User[Console user] --> Page[Agent page]
  Page --> Dialog[Delete confirmation]
  Styles[Shared dialog CSS] --> Dialog
  Dialog --> Cancel[Cancel handler]
  Dialog --> Delete[Existing deletion handler]
Loading

Technical review

Best possible solution:

Complete the class cutover using the existing dialog owner and update its existing Storybook workflow.

Do we have a high-confidence way to reproduce the issue?

Main's source confirms the exceptional class and temporary alias; no user-visible layout defect is claimed, and no target tests were run during this review.

Is this the best way to solve the issue?

The paired class change and alias removal are the smallest complete production cleanup and follow the original dialog-pattern intent.

Full review comments:

  • [P3] Update the matching Storybook coverage for the action-row cutover — apps/controller/src/console/agents/deletion.mts:415
    AGENTS.md requires matching stories, scenarios, and workflow instructions to be updated with console changes. This PR changes the shared dialog action-row contract but leaves scripts/console-storybook unchanged. Update the existing Components/Dialogs coverage and dialog-pattern-workflow.md to describe and check the completed shared-row contract; reuse the existing Delete confirmation states rather than adding duplicate stories.
    Confidence: 0.94

Overall correctness: patch is correct
Overall confidence: 0.94

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 0bf7d8c64807.

Provenance checked

  • Delete action-row class and CSS alias keeps the original intent (7c6e869: The shared-dialog PR retained the alias temporarily to avoid overlap with the pending Agent list-action PR.)
  • Shared-dialog browser assertions keeps the original intent (feat(console): dialog and drawer pattern #2879: Exercise shared dialog presentation and focus behavior through the console; the later TypeScript conversion preserved that purpose.)

Testing

Proof path: shipped entry point.

Security

None.

Evidence

Acceptance criteria:

  • pnpm docs:check
  • pnpm format:check
  • git diff --check

What I checked:

  • Pinned introduced change: The production hunks change one class and remove its CSS alias; deletion requests, authorization, focus handlers, and persisted state are unchanged. (apps/controller/src/console/agents/deletion.mts:415, 269427c6039e)
  • Explicitly deferred cleanup: feat(console): dialog and drawer pattern #2879 expressly retained the alias to avoid overlapping the then-open feat(console): stop and delete Agents from the Agents list #2524; the latter has merged. The original stylesheet commit independently confirms the alias's introduction. (apps/controller/src/console/console.css:2684, 7c6e8694e1fb)
  • Current-main boundary: Main still uses form-actions for this dialog. Other confirming dialogs use dialog-actions; remaining form-actions callers do not require the removed direct-child alias. (apps/controller/src/console/agents/deletion.mts:415, 0bf7d8c64807)
  • Applicable policy: The complete root AGENTS.md requires matching Storybook updates, rebuilt previews, screenshots, and a recording for console changes. No nested AGENTS.md or maintainer-notes directory was found. The requested single-reviewer restriction supersedes the bundled autoreview-helper instruction; no helper or second reviewer ran. (AGENTS.md, 269427c6039e)
  • Visual evidence and discussion: Inspected all four downloaded before/after screenshots: desktop and phone layouts remain consistent. The manifest contains no recording. The full discussion credits freeqaz-openai's final-head keyboard, cancellation, and real-controller DELETE-acceptance checks, explicitly limited to acceptance rather than cleanup completion. Captured primary-body identity: 259e87c02c3b4ea2bcad893e60a199ae2c1abc539e39377ffe83a1a97170902e. (269427c6039e)
  • Browser coverage and CI: The existing real-console browser case now checks the shared row and button order without adding a fixture or production seam. The body reports base failure and head success. CI Required and both Console Browser checks pass for the reviewed head; First Agent Smoke remains in progress. (tests/browser/console-dialogs.test.ts:167, 269427c6039e)

Likely related people:

  • Free Wortley: Raw commit 7c6e869 adds apps/controller/src/console/console.css:1530 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 7c6e8694e1fb; files: apps/controller/src/console/console.css)

Labels

Label changes:

  • add P3: This is a small presentation-contract cleanup with unchanged visible layout and lifecycle behavior.
  • add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • add status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action.

Label justifications:

  • P3: This is a small presentation-contract cleanup with unchanged visible layout and lifecycle behavior.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action.

Rating scale

6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior.

Workflow

ClawSweeper edits this one comment on every review. Comment @clawsweeper re-review for a fresh review only. Repair and merge execution lanes are retired.

Reviewed October 23, 2026, 6:22 PM ET / 22:22 UTC.

@freeqaz
freeqaz merged commit 66c2d66 into main Oct 23, 2026
73 of 76 checks passed
@freeqaz
freeqaz deleted the fix-1942 branch October 23, 2026 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants