fix(aws-control): implement the UX review debt across accounts and drive - #8716
Conversation
UX Review (Fable 5) — 🟡 CONCERNSUX-level review of I have everything I need: the blind read validated the PR's main-path controls, and I've reconciled every added/changed control against the report, the diff, and the screenshots. Two details stand out — the disabled "Move to folder…" item gives no reason (shot-11), and the committed UX-Verdict: CONCERNS Cold reader understood every main-path control — move flow, renames, busy states — but a disabled Move item is unexplained and one "after" screenshot is stale. Watch
Evidence gaps
Suggestions
[UX-REVIEWED] 7b7b832 |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsFINDING -- website/src/apps/aws-control/DrivePage.tsx:1912 -- deletion completing after navigation does not retrigger False positive or not applicable? A repository writer can comment: |
Design Review (Fable 5) — 🟡 CONCERNSDesign-level review of Design-Verdict: CONCERNS Sound debt-paydown, but the Share dialog's mid-mint lockout rebuilds the exact modal trap the Move picker was redesigned to avoid. Watch
Suggestions
[DESIGN-REVIEWED] 7b7b832 |
First Principles Review (Fable 5) — ✅ PASSPremise-level review of All evidence is gathered: the patch decomposed, consumers counted ( First-Principles-Verdict: PASS Seven recorded review findings paid down, with the standout items being deletions: the move-concurrency class, a dead navigation branch, and a hand-rolled popover. What this change shipsIntent: make the aws-control app's front-door surfaces (rail, accounts list, file table) answer the seven UX-review reports that merged as deferred debt — a FIX.
More items exist (inline cost-dash reason, compact consent credential source, library empty-vs-no-match split, catalog polish, screenshots and capture scripts per repo convention); each traces to a named source finding or documented convention. Every item carries a nameable harm from a cited report, reuses the existing mechanism where one exists ( [FIRST-PRINCIPLES-REVIEWED] 7b7b832 |
Opus 4.8 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
|
First Principles Watch —
Agreed on both counts: the pattern is duplicated, and the shared helper belongs in a change that also migrates |
b0cb8ff to
176d1b6
Compare
|
Design Watch —
Agreed — nothing is lost by dismissing. |
|
First Principles Watch — undeclared
Correct — it was in the diff and not in the ledger. The body's |
176d1b6 to
0e50bd2
Compare
Correct: |
0e50bd2 to
c69f653
Compare
Correct, and the adjudication's reading of the rule's scope text is the right one: |
|
FP Watch — focus-return patched 2 of the 3 menus that open
Correct, and a completeness gap in this PR's own fix rather than a new design: FP Watch —
Also correct: the search table sits directly on the page surface with no card wrapper. It now passes FP Subtraction — fold
The two scripts stub different worlds on purpose: the first is a before/after pair on one flat listing; the second carries mode switches, path-keyed nested listings, a late-409 and a hanging mint, backup remotes and a deletable folder. Sharing the base constants would need a third module plus per-script overrides, for two evidence generators that are run by hand and never by CI, in a directory whose convention (407 scripts) is self-contained files. Drift between them cannot break anything a user sees. Kept separate. |
|
UX Watch —
Legitimate, and a new affordance with a design choice in it (an action inside a picker that today has no write actions, vs. handing focus to New folder on close). This PR's Move work adds the missing move path for keyboard/touch users; what the picker does when there is nowhere to move is a follow-up, tracked with the two items below. UX Watch — "Ask the agent" is the only visible exit on a move refusal — disposition: accepted-and-deferred → #8722.
Agreed on the copy; it is a 13-catalog wording change on the shared refusal strings plus a decision about which sentence carries it. Tracked in #8722 alongside the empty-state item so the picker's copy is revised in one pass. UX Watch — disabled "Move to folder…" carries no reason during a serialized move — disposition: accepted-and-deferred → #8722.
Agreed; needs a new key ×13 and a disabled-item affordance the shared dropdown does not currently have. Same follow-up. Evidence gaps — both captured on 21f2bf9 and added to the body's second |
21f2bf9 to
cb6dcd0
Compare
Correct. |
Correct — reachable when a delete's confirmation has moved to another row and the picker is open for a third file when the delete fails. Page-level delete/rename failures now have their own |
Correct; the earlier wording "leaving retires it" described hiding, not retiring. A |
|
UX Watch — "Ask the agent" carries no outcome clause — disposition: rebutted (out of this PR's scope).
UX Watch — picker rows commit on click; mid-move disabled state is opacity-only — disposition: rebutted.
While a move runs the picker's own body says "Moving…" ( Evidence gaps — both captured on cb6dcd0 and added to the body: the breadcrumb ancestor menu open two levels down ( Suggestion — |
cb6dcd0 to
4be6145
Compare
|
UX Watch — the picker's refusal notice wraps around the right-floated "Ask the agent" link; the hand-off should name its scope — disposition: accepted-and-deferred → #8722 (
Both halves are UX Watch —
The sentence is the one-way-door line the earlier UX rounds on the source PRs asked for, promoted to body tone in this PR because it was the dialog's only cost disclosure and had been styled as a caption. Its content — that a copy occupies storage in the reader's own bucket — is what the dialog is about; the proposed rewording adds "which AWS bills", which is true of every object on this page and is what the Usage & costs pane exists to show. Wording on a line that eleven rounds of this lane have already passed is not changing at round 12. Evidence gaps — These are copy-only changes to strings whose surfaces this PR does not restyle: the bad-name upload error ( Suggestion — a "New folder" action inside the empty picker — already accepted-and-deferred in the round-9 disposition (see #8722, "Move picker / refusal copy"). No change here. |
4be6145 to
7b7b832
Compare
Correct, and introduced by the previous round's fix (the shared |
|
UX Watch — disabled "Move to folder…" gives no reason — disposition: accepted-and-deferred → #8722 (already recorded there from the round-9 disposition: "a reason on the disabled Move item"; the UX Watch — mid-move close reads as maybe-cancel — disposition: accepted-and-deferred → #8722 (task added).
Agreed on the copy; it is a 13-catalog change to the picker's pending line, and the picker's copy is being revised in one pass under #8722 (empty state, refusal wording, disabled reason). Added there as "Moving… — closing this does not cancel it." UX Watch — destination click commits with no confirm — disposition: rebutted, as the lane itself frames it: the design (no confirm, visible "Moving…" state, trivially reversible move-back) is the intended one and the lane is flagging the reader's hesitation for a human, not asking for a confirm dialog. Left as is; a maintainer reading this PR sees the note here. Suggestion — "New folder" inside the empty picker — already accepted-and-deferred (round 9, #8722). No change. |
|
Design Watch — the Share dialog refuses dismissal mid-mint — disposition: rebutted (same item the UX lane raised on head 3ccc844; the round-8 disposition stands, restated here for this lane).
The Move picker may close mid-flight because a move's outcome lands on the row: nothing the reader needs is lost with the dialog. A mint's outcome is a URL, and the Share dialog is the only surface that ever shows it — the Share links pane stores the record and, by design, never the URL ( Design Suggestion — fold the three-point menu-opens-dialog handshake into one shared wrapper — disposition: accepted-and-deferred → #8722 ( |
|
UX Watch — disabled "Move to folder…" gives no reason — already accepted-and-deferred (round 9 and round 12 dispositions; #8722 carries the task including the UX Watch — UX Watch — "Ask the agent" on the two new refusal surfaces — already accepted-and-deferred (round 11; #8722: scoped hand-off label on the shared Evidence gaps — Suggestions — New folder inside the empty picker; |
The sequence in the finding is: delete |
Problem / Motivation
The UX Review lane raised CONCERNS on seven aws-control PRs (#5517, #6446, #6780, #7801, #7741, #7972, #7983) and each PR merged with its Watch items dispositioned as accepted-and-deferred or left unanswered. The debt is user-visible on every visit to the app: the rail says "Access" above "Accounts & credentials", the totals strip counts "2 healthy" of nothing, one thing is called keys / profiles / Connections on the same page, the pinned Actions column is a permanently tinted stripe, a file can only be moved by dragging it, and the Share dialog and breadcrumb menu ignore Escape and never return focus.
Why it matters
These are the front-door surfaces of the app (rail, accounts list, file table). Each item alone is small; together they make a first-run reader mis-navigate ("Acc…" twice), misread account health, and — for keyboard and touch users — make moving a file impossible. The lane already found them; leaving them deferred with no owner is how advisory findings ship anyway.
What changed (motivation → approach → change)
Every Watch item from the seven source PRs, with the change that answers it. Items already fixed on main (by #8320 / #8475) are listed so the reader sees the whole ledger.
#7801 — flatten the drive into an in-app rail
access_titleis now "Share links" (rail and pane header share the key; 13 catalogs).useAccountonly sets the stored account; the reader stays on Accounts and the keys section below re-renders for the picked account. The now-unreachable push-marker branch in the row handler is deleted.keys_for_account), naming the account whose keys it lists.#5517 — account portal + cloud drive
~/.awsname in Add accounts. Copy aligned acrossAwsControlPage,ConsoleView, and the consent receipt.empty_body; this PR adds the second half: when the account list is empty, Add accounts opens itself (autoOpen, edge-triggered so a reader who collapsed it keeps their choice).ShareDialognow usesuseDialogFocusTrap(Escape closes — except mid-mint — and focus returns to the menu that opened it — all three openers: list row, grid tile and search hit). The breadcrumb-ancestor menu is RadixDropdownMenu. The Share dialog also names the file it is about to publish (share_title→ "Share “{{name}}”", 13 catalogs), matching the Move picker — a wrong-file share link is unrecoverable until it expires (UX Review, round 7).shareMut/nightlyMut/restoreMut/forgetMut/removeMut) → already on main via fix(aws-control): route every error state through the shared notice #8320 (AwsErrorNotice, "route every error state through the shared notice"). Not duplicated here; every error surface this PR adds (the Move picker's refusal) also renders throughAwsErrorNotice.#7741 — density / disclosure polish
totals_summaryreads "{{accounts}} accounts · {{healthy}} of {{keys}} keys healthy". The health word on each row takes the status dot's colour, and the account switcher's dropdown says "Needs attention" in words for a non-healthy account, not just a dot.closeFolderDisclosurefocuses the "New folder" toggle after the input unmounts.AwsConsentGaterow shows the credential source inline.AwsErrorNoticeunder the card already reports it with the hand-off (errors-use-error-notice).#7972 — icon-only view toggle, borderless table
LibraryTableHeadtakes asurfaceparameter (bg/card); the drive table passesbg, the library keepscard. Pinned by a test that asserts nobg-cardon the drive's sticky cells. Both drive tables on the page surface passsurface="bg"— the folder listing and the search-results table.#7983 — drag and drop
share_activerefusal is reported inside the picker, which stays open for another pick.opacity-50), reportsaria-busy, and says "Moving…" in words where its Modified cell (list) or size line (grid) was, so the state is not encoded by opacity alone once the picker is gone; the picker shows "Moving…" too. The picker can be dismissed at any point, mid-move included — the copy keeps running, the row stays busy until it lands, and a refusal that arrives after dismissal is reported on the pane's error strip — so a slow or hung copy never traps the reader in a modal (Design Review finding on the first head). Moves are serialized — one at a time. While a copy runs, the other rows are not draggable, their "Move to folder…" item is disabled, and a drop is ignored. This is a product decision taken after review rounds 2, 3 and 6 each surfaced another sibling of the same class (two overlapping moves sharing one busy marker / one error slot / one "picker open" flag): the original finding asked for a visible busy state, never for concurrent moves, so the concurrency class — and the per-key state it needed — is removed rather than made correct.#6446 — the cloud drive becomes a page
folder_deletedparenthetical plural → real_one/_otherkeys registered inpluralKeys.json, 13 catalogs, and the line now names the folder ("Deleted “archive”: 14 items.") so a reader can tell how one click became fourteen. It is scoped to the folder the delete happened in — the deleted folder's parent, taken from the request rather than from wherever the reader is when the response lands — shows only while that folder is the current path, and leaving the folder retires it (rather than merely hiding it), so a later return does not replay it. Separately, a delete or rename that fails after its row's strip is gone has its own page-level slot AND its own notice (pageError,drive-page-error) instead of borrowing the Move picker's: sharing the slot would have shown "could not delete X" inside a picker moving Y, and sharing the notice would have let a stale drag refusal stand in for the later delete failure.folder_bad_nameuses real umlauts.DropdownMenu(see feat: AWS Control - account portal + S3-backed cloud drive #5517 above).drive_bad_name,folder_bad_name) say the same thing the same way.#6780 — rebuild the Drive around real contents
library_add_empty("nothing here yet"), distinct from a search that matched nothing; the one-way cost disclosure reads in body tone.Copy
backup_restored_note).Not in this PR
setUploadErrorper file). Separate follow-up.Tests
15 new frontend tests across
AwsControlPage.test.tsx,ConsoleView.test.tsx,DrivePage.test.tsx,AwsConsentGate.test.tsx,ArtifactsPage.stickyActions.test.ts:aria-busy+ dimmed only while pending; Escape mid-move dismisses the picker while the row stays busy and says "Moving…" in words (gone once the move lands); the Share dialog opened from a search hit returns focus to that hit's menu; the pinned header of the search-results table paints the page surface too; a refusal arriving after dismissal lands on the page strip, not in a re-opened picker; one move at a time — while a move runs the other rows are not draggable and their "Move to folder…" item is disabled, and the gate reopens when the move settlescosts-errornotice (the consent-missing reason stays inline, it is status not failure)folder_deletedrenders a real plural with no(s)and names the folder; the count attaches to the folder the delete happened in even when the reader navigated during the request, and leaving that folder retires it; a delete that fails while the Move picker is open lands on the page strip, not inside the picker; a stale drag refusal does not stand in for a later delete failure (two notices)library_add_empty, notlibrary_add_nonebg-bg, neverbg-card; the library table keeps its card surfaceLocally:
tsc -b --force, eslint on touched files,npm run i18n:check,src/i18nstyle tests, and every touched vitest file (47 files, 863 tests) green after rebasing onto6d1b51704.Manual verification
Captured on a Vite dev server with the API answered from fixtures (
website/scripts/capture-aws-control-ux-debt.mjs, same harness as #8320'scapture-aws-control-errors.mjs), once onmainand once on this branch. Walked: rail labels, totals strip, keys section header, Share links pane, row menu → Move to folder… → picker, row menu → Share dialog.Screenshots / video
Accounts pane — rail, totals strip, keys section (before → after: "Access"→"Share links", "Library"→"Artifact library", "3 keys · 2 healthy"→"2 of 3 keys healthy", "Connections"→"Keys for prod-main")
Files — row menu → Move to folder… (before: Download / Share / Delete only, and the pinned Actions column is a tinted stripe; after: the picker, on a table whose sticky cells paint the page surface)
Share links pane and Share dialog (before / after; the after-frame is the current head's, with the file name in the title)
States the UX lane asked to see on this branch (all "after"; captured by
website/scripts/capture-aws-control-ux-debt-more.mjs)Row menu with "Move to folder…" · a move in flight (source row dimmed and
aria-busy, picker says "Moving…") · the other row's menu while that move runs (Move disabled — one move at a time) · a refused move reported inside the pickerNo accounts yet (Add accounts auto-expanded; the orphan consent receipt also shows the credential source) · account switcher open ("Needs attention" in words) · Usage pane with the consent-missing cost reason inline and the compact S3 receipt's credential source · Add-from-Artifacts on an empty library · the Share dialog naming its file
Move picker opened two levels down ("Files (top level)" · "Up to contracts" · the sub-folder) · picker at a top level with no folder (
move_no_folders) · picker dismissed mid-move: the dimmed row says "Moving…" · a folder deleted ("Deleted 14 items.") · a backup restored ("Your current data was not replaced…" + staging path)Search results (pinned header on the page surface, same as the folder table) · a refusal arriving after the picker was dismissed, on the pane's strip · the Share dialog mid-mint ("Creating…", Close dimmed, Escape held)
Breadcrumb ancestor menu open (shared dropdown, two levels down) · grid view mid-move: the tile says "Moving…" and its neighbour's Move item is disabled
Related Issues
no linked issue: this closes out the UX Review Watch ledger on #5517, #6446, #6780, #7801, #7741, #7972 and #7983 directly; no issue was filed for those dispositions.
Pattern harvest
Rule candidate: review-prompt
Pattern: an advisory UX Watch item dispositioned "accepted-and-deferred" with no issue, owner or date is debt with no owner — the disposition contract now requires all three (
deferred-findinglabel, assignee,Due:), which is exactly the gap that let these seven PRs' items sit.Checklist
feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)docs/system-specs/features/aws-control.md