Skip to content

feat(aws-control): move the library removal onto the cloud listing - #7810

Merged
iamwhatever merged 1 commit into
mainfrom
feat/aws-control-remove-on-cloud-listing
Sep 2, 2026
Merged

feat(aws-control): move the library removal onto the cloud listing#7810
iamwhatever merged 1 commit into
mainfrom
feat/aws-control-remove-on-cloud-listing

Conversation

@chenmingwei23

@chenmingwei23 chenmingwei23 commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Problem / Motivation

What is on main today. The picker-based removal shipped with #7026 (90f708efb). Its only entry point is a per-card control inside a dialog labelled "Add from Artifacts", so a user standing in the Library folder -- looking at the very copies they are paying to store -- has no affordance to remove one. Raymond reviewed that placement and directed the move onto the folder's own listing, behind an overflow menu.

That is not a change of mind about a working design. The UX lane on #7026 reached the same conclusion independently, on the merged head, and stated it as its verdict:

The only way to remove a cloud copy is hidden inside a dialog named "Add from Artifacts" -- the opposite of the user's intent.

  • Removal is undiscoverable from where the copies live. The Library folder's cards and rows (LibraryCloudCard, library-list-row) offer no remove, the old library_remove_hint is deleted, and the sole entry point is the button/dialog labeled "Add from Artifacts".

#7026 did not dispute the mechanism. It dispositioned the finding as disproportionate for its own scope and named this exact change as the deferred fix: "Smallest fix is the PR's own deferred item: Remove on LibraryCloudCard." This PR is that deferred item.

Why the placement is not merely undiscoverable but unsound. The picker cannot host this control at all. Its rows are local artifacts joined to a ledger keyed account -> slug. ArtifactStore.delete (src/kiro_crew/artifacts.py) does not prune that ledger, and a newly created artifact starts at version = 1. So:

  1. push artifact A, and the ledger records slug -> {version: 1, pushedAt}
  2. delete A locally, and the ledger entry survives
  3. create B, which takes A's slug, and list_pushable joins B to A's record

B's tile now renders as synced, Remove appears on it, and clicking it empties artifacts/<slug>/ -- A's cloud copy, under B's name. No predicate available on that card separates the two: synced is the inherited record, and pushedVersion === version is satisfied by a never-pushed B at v1 against A's pushed v1. #7026 mitigated this by naming the bucket folder in the confirm, which narrows the blast radius but cannot fix it: the reader is still asked to vouch for an identity this machine cannot establish.

The inversion is the cleanest statement of why the placement is wrong: a copy pushed from another machine could not be removed at all, while a locally reused slug could remove the wrong one.

Why it matters

library_remove's own docstring already expects the cloud listing to be its caller:

the objects then surface as remoteOnly on the next render and stay removable.

That is the backend describing a UI that walks the bucket, not the local store. The spec says the same thing about remoteOnly: a copy pushed from another machine "would otherwise be unreachable from the console that must be able to remove it". Today's placement gets that exactly backwards. This PR is not a redesign; it finishes the wiring the backend was written for.

What changed (motivation -> approach -> change)

Removal moves to LibrarySection's listing. Its rows come from driveList, so a row is a cloud folder and removing its slug empties the object on screen. Offered on every listed copy, gated on nothing, including one no local artifact backs.

It is hidden behind the same overflow menu the Files folder uses. Same MoreHorizontal trigger, same DropdownMenu / DropdownMenuTrigger / DropdownMenuContent align="end" primitives, same TileConfirm follow-up, on cards and on list rows. A visible danger button on every card of a browse surface was the rejected design; a reader moving between the two folders met two grammars for "act on this item".

The trigger sits outside the <Link>. The card shell became a <div> with the link inside it. A <button> inside an <a> is invalid content, and a trigger that had to preventDefault out of the surrounding navigation is the nested-control hijack this page avoids elsewhere. cmd-click and middle-click still open the artifact.

Confirm state lives on the section, not the card: one confirmSlug plus one useMutation, with removeMut.reset() on open. This removes the state two review findings lived in rather than resetting it. There is no synced flag to fall out of (a card exists exactly while its object is listed, and success invalidates the listing so card and strip unmount together), and one card's failure cannot render inside another's confirm. The drive's own confirms carry that second wart; this deliberately does not copy it.

Deliberate scope extension, on Raymond's instruction: Drive's Download moves into the same overflow in both views. It was a bare <button> beside the trigger while Share and Delete were inside it, the same inconsistency one component over, and shipping a new Library menu while leaving it would recreate what was just rejected. The non-obvious part is popup survival: download() calls window.open('', '_blank') and must stay inside the user gesture or a blocker eats it. Radix dispatches onSelect synchronously from the item's own click handler, so it does; the assertion that proves it is unchanged.

Four new i18n keys -- library_actions, library_remove_action, library_removing, library_remove_failed -- across en.json plus 11 translated catalogs, en-XA regenerated. Four, not seven: main already carries library_remove, library_remove_confirm and library_remove_confirm_slug from #7026. library_remove_failed replaces delete_failed, whose "Delete failed - the file is unchanged" is the wrong verb and the wrong noun for removing an artifact's cloud copy.

Kept from #7026 rather than reverted: the confirm stays a single <Trans> with a <folder> tag. That is better than a lead-in string plus a sibling <code>, because a translator can reorder around the monospace chip. Only its contrast changed, to text-text from text-muted, per UX below.

Scope limit -- what this does NOT fix

Relocating fixes the delete target: the object deleted is the object the bucket listing enumerated, not one inferred from the ledger. It does not fix the borrowed title. bySlug is built from the same slug-keyed ledger (if (a.pushedAt !== null) bySlug.set(a.slug, a)), so a reused slug can still lend a card the wrong artifact's name, kind and preview. That is #6987's cause-level fix -- read the pushed meta.json sidecar instead of inferring from local state -- and it is unchanged here.

Being exact about the residual, because an earlier draft of this section was not. I previously argued that naming artifacts/<slug>/ in the confirm reduced this to a display defect. It does not. The prefix is derived from the slug, and the slug is precisely what the second artifact reuses, so artifacts/S/ reads identically whether the bytes are A's or B's. The reader is given nothing they can check. So the honest statement is: the delete target is correct and never a ledger inference, and the displayed identity can belong to a different artifact, on an irreversible action. Both halves are true at once.

Measured against base, this is zero-delta. On origin/main today, data-testid="library-remove" sits at DrivePage.tsx:1034 inside PickerCard (defined at :927) and calls the same libraryRemove(account, slug) at :749. So in the reuse case on the current release a reader opens "Add from Artifacts", sees row B, clicks Remove, and artifacts/S/ -- A's bytes -- is emptied under B's name. After this change the same reader sees a card labelled B in the Library folder and the same call empties the same prefix. Same slug, same request, same bytes, same false belief. Main's own comment at :1045 already describes this mechanism and concludes it "cannot prove the copy is this artifact's without reading the pushed meta.json sidecar". The relocation moves where the control lives and does not move this harm in either direction, so it is a pre-existing hole owned by #6987 -- not applicable to this PR, rather than a false positive. The finding is real.

Why the proposed remedy is refused. Rendering Remove only when local is undefined would hide it in the normal case -- you pushed a copy and still hold the original. Combined with this change removing the picker's Remove, the product would then have no path at all to remove a cloud copy in that case, which is worse than main, where the picker at least offered one. It leaves the harm intact and deletes the operation.

The next step, deliberately not in this diff. A per-slug rollup -- object count, bytes, and newest last-modified, grouped by the first path segment after artifacts/ -- would let the confirm show something a reader can actually check ("2 files, 1.4 MB, last modified 12 Aug" is visibly not an artifact created minutes ago). The whole-bucket undelimited listing usage() already performs is the natural source; it projects Contents[].{Key, Size} today and would need LastModified added. That is a new capability across storage.py, routes.py and types.ts, so it belongs in its own change rather than bolted onto a converged relocation.

Review advisories from #7026, and what answers each

  • UX, "the only way to remove a cloud copy is hidden inside a dialog named Add from Artifacts": the move itself. Removal is now on the folder that lists the copies.
  • UX, delete_failed vocabulary: the new library_remove_failed key.
  • UX, un-mute the consequence line: taken. text-text on the sentence and its path chip. It names the bucket folder the delete will empty, which is the identity check the whole rationale rests on; styling it as an aside undercut the argument.
  • First Principles, "a third spelling of the confirm, use TileConfirm": satisfied structurally. The rebuild reuses TileConfirm, widened only for a ReactNode label and a test-id base.
  • GPT 5.6 round four, the two listing Remove items: the premise is right and the conclusion is rebutted, with the render path named. Both items render in the Library folder's cloud listing -- one in LibraryCloudCard, one in the list row -- and local can indeed be defined at both, so that half of the finding is accurate. It is not what makes identity provable. The listing enumerates the bucket (driveList(account, 'library') -> pg.folders), so a row exists if and only if artifacts/<slug>/ is in the cloud, and the removal empties exactly the prefix the row was listed from: the target is the object that was observed. local supplies the card's label and thumbnail only. On the picker the row IS a local artifact and no cloud object has been observed at all, so there the target would be inferred from a slug-keyed ledger -- which is why that surface has no removal, pinned by its own test. Under slug reuse the residual is the LABEL, not the target, and the confirm's operative sentence already names the folder it empties rather than the label. GPT's remedy -- render only when local is undefined -- is declined: most cloud copies still have their local original, so it would remove the affordance from nearly every card and restore the gap this PR exists to close. The rebuttal is now enforced rather than argued: a new test pins the boundary in both views, and applying GPT's remedy to either site reds it.
  • GPT 5.6 on this PR, the shared per-card state: real and reachable across three rounds, all three fixed, and the third one closed the class rather than the instance. Round one: onSuccess cleared confirmSlug unconditionally. Round two: removeMut.reset() discarded an in-flight removal's state, so a failure had nowhere to report. Round three: the slug-keyed failure was itself one slot, so of two overlapping failures the later hid the earlier -- GPT's Set is the right answer and is taken, extended to the pending state as well, which had the identical defect one round ahead of being reported. Its prescription in the first two rounds -- return early from askRemove while isPending -- remains declined: it makes the Remove item a dead click with no feedback whenever any sibling removal is in flight, trading these defects for a worse one. See the class note under Tests.

Tests

vitest run src/apps/aws-control/DrivePage.test.tsx src/i18n/ gives 730 pass across 43 files. tsc -b and eslint clean. Counts are not the argument, though -- what follows is.

Mutation-verified: what each guard is worth

Each row below was checked by breaking the code and confirming a named test goes red. No mutant survived, across eleven rows. Earlier drafts of this table did have survivors, and each one was the useful signal: once it showed a guard was genuinely redundant (it was deleted), once it showed a test was missing (it was written), and twice it showed my own mutant was broken and never reached the code -- which is now checked before any conclusion is drawn from a survival.

mutant result what the redness means to a user
failedSlugs collapsed to a single slug RED -- keeps both failures when two removals fail Two removals failed and the app admits to one, so you retry a card it never warned you about -- or believe a copy is gone that is still there.
pendingSlugs collapsed to isPending RED -- 3 tests The app says it is working on a card it is not touching, and disables the Cancel of a removal that has not started.
onSettled never clears pending RED -- reports a failed removal on its card and retires it on cancel A failed removal goes on claiming to be in progress, so the card offers no way to retry.
onSuccess clears the confirm unconditionally RED -- does not close one card's confirm when a different removal completes A removal you were about to confirm silently vanishes because an unrelated one finished.
the failure is not recorded at all RED -- 4 tests A removal that failed reports nowhere; the copy is still in the bucket and nothing says so.
opening any confirm clears every failure RED -- does not erase an existing failure when another confirm opens The app told you a removal failed, then quietly took the message back because you looked at another card.
Cancel clears every failure, not its own RED -- same test Backing out of one card retires a different card's unresolved failure.
the in-flight label read unscoped across rows RED -- shows the in-flight label only on the card being removed A sibling's in-flight removal relabels your card and disables your Cancel, so you cannot close the confirm you opened.
confirm names library/<slug>/ instead of artifacts/<slug>/ RED -- removes from the cloud listing, naming the artifacts/ prefix it empties The one identity a reader can cross-check in the S3 console names a folder the bucket does not contain.
a Remove control restored on the picker card RED -- does not offer removal from the picker, where identity is unprovable The wrong-target delete is back on display inside a dialog labelled "Add".
Remove hidden wherever a listed copy has a local twin, grid card RED -- offers removal on a listed copy with a local twin, and targets the listed prefix Most cloud copies still have their local original, so removal vanishes from nearly every card and the folder again lists copies it cannot remove.
Remove hidden wherever a listed copy has a local twin, list row RED -- same test The same regression, reachable only in list view -- and the view choice persists per section, so a reader who switched once would never see the control again.
both listing Remove items deleted outright RED -- 9 tests The feature is gone.
Download no longer inside the grid overflow RED -- grid mode carries the same actions the table rows carry A bare Download button sits outside the one home for per-item actions.

One class, three layers, and why the third fix changed shape

Three review rounds each found a real, reachable defect here, and the first two fixes were each one layer deep. Recording all three, because the pattern is the point:

  1. confirmSlug was cleared unconditionally, so a completing removal closed a different card's confirm.
  2. removeMut.reset() on open discarded an in-flight removal's state, so a failure that arrived afterwards had nowhere to report.
  3. the slug-keyed replacement was itself one slug, so of two overlapping failures the later hid the earlier -- and the observer's pending flag is likewise one boolean whose variables holds only the most recent slug, so an in-flight label could appear on a card nothing was happening to.

All three are one defect: per-card state held in a value shared across cards. Patching them individually was chasing shapes of the same mistake, and layer four was already implied by the code before it was reported. So the third fix changes the shape instead: N cards get N slots. pendingSlugs and failedSlugs are sets, every per-card indicator is derived from membership, and nothing per-card consults a shared value. The check is mechanical rather than a promise -- the only member this section reads off the mutation observer is .mutate.

confirmSlug deliberately stays a scalar: only one confirm strip is open at a time, which is a constraint on the surface, not a shared slot standing in for N.

This shape was not invented here. The picker 600 lines down has always held its per-card push state as addingSlugs / failedSlugs; the regression was mine, in moving the affordance across without carrying that lesson with it. The copy-on-write helpers are now hoisted to module scope and used by both, so there is one spelling of the idea rather than two.

What each test would let through if deleted

  • removes from the cloud listing, naming the artifacts/ prefix it empties also asserts not.toHaveTextContent('library/notes/'), and drives the control through the menu. Delete it and both a visible danger button on every browse card and a removal keyed to local state pass again. The fixture is deliberately the reused-slug shape: the bucket holds notes while the local artifact of that slug reports pushedVersion: null.
  • removes a cloud copy that has no local artifact behind it: the local library is empty and the removal still fires. Delete it and gating removal on synced passes again, which strands every copy pushed from a second machine.
  • does not offer removal from the picker, where identity is unprovable: both fixture cards are synced, the state that used to reveal the control, and both library-remove and library-more are asserted absent inside the dialog, so the unsound placement cannot return as a button or as a menu.
  • offers removal on a listed copy with a local twin, and targets the listed prefix: the complement of the picker guard -- lists a cloud object whose local twin is a different artifact wearing its slug, and asserts the control stays and targets the listed prefix, in grid AND list view.
  • keeps both failures when two removals fail: starts two removals, fails the earlier one first, and asserts both cards carry their own error -- a single slot would end up holding only the later.
  • shows the in-flight label only on the card being removed: holds A in flight, opens B, and asserts B's strip offers its action and a usable Cancel rather than A's progress.
  • does not close one card's confirm when a different removal completes: holds A's removal open, opens B while it is in flight, asserts B's own controls are not wearing A's pending state, then resolves A and asserts B's strip survives.
  • keeps a failure on the card that owns it when another confirm is opened: A fails while B's strip is the open one, and the failure reports on A's card, not on B and not nowhere.
  • does not erase an existing failure when another confirm opens: the reverse order -- A's failure is already on screen, then B's confirm opens and B is cancelled, and A's failure survives both.
  • reports a failed removal on its card and retires it on cancel: the strip stays open so a retry is one click, and Cancel retires that card's failure with the attempt it describes.
  • carries the removal in list view too: the view choice persists per section, so a control the grid has and rows do not is one a reader can never reach again.

The screenshot harness carries 13 assertions of its own. The load-bearing one is library-remove count 0 at rest: the item is portaled, so its absence from the document is the proof that the destructive path is not on display.

Manual verification

Captured against the real built website/dist with the API layer answered from fixtures, full app window at 2360x1640 (deviceScaleFactor 2), no crop. A pod cannot show this feature: with no AWS profile, no S3 consent and no bucket, the Library folder renders empty and the control has nothing to attach to.

Screenshots / video

The listing at rest, one card per object in the bucket, each with the overflow trigger, no destructive control on display. The middle card, from-another-machine, has no local artifact behind it and is the case the old placement could not reach:

Library folder at rest

The menu open. A deliberately hidden control needs one frame that shows it:

The overflow menu open, showing Remove

The confirm, naming both the item and the prefix it will empty, at full contrast:

The confirm naming the artifacts prefix

Related Issues

Follows up #7026. Leaves #6987's cause-level sidecar read open; see the scope limit above.

Pattern harvest

Two findings worth carrying beyond this PR.

A screenshot harness without assertions cannot tell a stale bundle from a broken page. capture-aws-control-library-remove.mjs's DRIVE fixture originally omitted usage.sections, which DriveUsage requires, so StorageMeter dereferenced usage.sections.drive on undefined and the harness photographed the page's error boundary while exiting 0. It surfaced only because this script's expectCount assertions fail the run. main has since fixed this file's fixture, and #7801 fixes the sibling capture-aws-control.mjs the same way, so nothing is changed here for it. The class is still worth naming: an evidence harness needs assertions or it will publish "Something went wrong" as a feature demo.

Reading headRefOid without state makes a merged PR look open. gh pr view populates headRefOid on merged PRs, so a check keyed to it reported #7026 as open at its squashed pre-merge head and nearly produced a force-push onto a deleted ref, from a base 440 files behind main.

Any other suggestions

The library_remove_confirm_slug key resolves from en.manual.json rather than en.json. That is legitimate and the key-reference gate passes correctly, but it means a grep of en.json alone reports the key missing, which is worth knowing before anyone concludes the confirm is broken.

@chenmingwei23
chenmingwei23 requested a review from a team September 2, 2026 05:48
@chenmingwei23
chenmingwei23 requested a review from a team as a code owner September 2, 2026 05:48
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Sep 2, 2026
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

Removal now targets the object the bucket listing observed instead of a ledger inference — root-cause placement, residual label defect honestly scoped to #6987.

[DESIGN-REVIEWED] f908b2f

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — 🟡 CONCERNS

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

UX-Verdict: CONCERNS

Solid, well-evidenced move of a destructive control; two copy/friction trades deserve a human eye before merge.

Watch

  • "Remove failed — the cloud copy is unchanged. Try again." asserts a state the client cannot know: the diff's own comment says the removal is "a delete_prefix sweep over several S3 objects", so a mid-sweep failure leaves the copy partially emptied while the message tells the user nothing was touched — misinformation on an irreversible action. Low frequency × high impact (user skips re-verifying) × persists on every partial failure. Fix: drop the claim (Remove failed. Try again.) unless the backend reports zero deletions.
  • Download in Drive drops from a one-click visible button to an overflow item in both grid and list views ("Download used to sit outside the menu as a bare button"). Every download — plausibly the most frequent Files action on this daily surface — now costs two clicks, and habituated users lose the affordance. The consistency rationale is stated and sound; flagging so a human ratifies the trade rather than inherits it.

Suggestions

  • TileConfirm's comment justifying disabled={pending} on Cancel ("this strip is the ONLY place the outcome can render") is untrue for the library caller, whose failure renders on the card via failedSlugs — consider re-enabling Cancel there or correcting the rationale so the next editor doesn't propagate it.

[UX-REVIEWED] f908b2f

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of f908b2fbad46ecc786956cd3a57f482b705d4167 — 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 counts are run and the repository verification is complete. Here is the review.

First-Principles-Verdict: CONCERNS

The removal relocation is a derived, well-aimed fix; the Drive Download move and the shared confirm's Cancel-disable ride along on consistency-only and undeclared grounds.

What this change ships

Intent: let a user remove a cloud copy from the folder that lists it — a FIX (the deferred item #7026 named).

  1. Remove moved off the "Add from Artifacts" picker onto the Library folder's cards — justified
  2. Remove offered on list-view rows too — justified (view choice persists per section)
  3. A copy pushed from another machine is now removable — justified (spec's remoteOnly promise)
  4. The picker's Remove control and confirm strip deleted — justified subtraction
  5. Remove hidden behind a per-card menu — declared; recorded review decision, not relitigated
  6. Drive's Download moved into the overflow, grid and list — rides along, unjustified move
  7. Confirm Cancel now disabled while a request runs, on Files-folder deletes too — rides along, undeclared
  8. Failure text and 4 new i18n keys × 13 catalogs — justified, all consumed (grepped, 6 uses)
  9. Per-slug pending/failed sets, one confirm per section — justified mechanism fix
  10. Spec update, capture-script rewrite, 3 committed screenshots — mandated conventions

Watch

  • The Download move's only stated harm is "two grammars for act on this item" — consistency, which is analogy; no user is named who misclicked or failed to find the bare button, and every existing user now pays a second click for the most common file action. Deferring it would not dent the fix: the Remove items alone close the reported defect.
  • TileConfirm's disabled={pending} on Cancel reaches all 4 call sites (grepped TileConfirm, DrivePage.tsx:1706, 1774 are Files-folder deletes), changing behavior on a surface this PR is not about; the visible description never mentions the Files folder's confirms.

Subtractions

  • Defer the Download relocation (DrivePage.tsx grid drive-grid-download, list drive-download) to its own change with a named failing user; this PR's fix stands without it.

[FIRST-PRINCIPLES-REVIEWED] f908b2f

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ human override accepted

Human judgment by @chenmingwei23 overrides the GPT 5.6 finding for f908b2fbad46ecc786956cd3a57f482b705d4167; the recorded reason is authoritative for this commit.

This comment is updated in place on each push.

The model was not re-run because an authorized human decision supersedes it.

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

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 2, 2026
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No findings.

The sole candidate — disabled={pending} newly added to the shared TileConfirm Cancel button (DrivePage.tsx:1273) — does not survive falsification. The mutations feeding pending (deleteMut.isPending, folderDeleteMut.isPending) always resolve, re-enabling Cancel; Cancel only closes the inline strip and never aborts the request, so disabling it mid-flight discards no user action. The same file already disables sibling Cancel buttons while their mutation is pending (drive-folder-cancel at line 1543 uses disabled={folderCreateMut.isPending}), so the change follows the established convention rather than breaking one. No crash, data loss, corruption, or removed guard results, and the "stuck forever" case requires a request that never resolves — a hang, not a defect this line introduces. Confidence it is a real defect is well below the bar.

[OPUS-REVIEWED] f908b2f

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

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

@chenmingwei23
chenmingwei23 force-pushed the feat/aws-control-remove-on-cloud-listing branch from 04abc05 to 14940f7 Compare September 2, 2026 06:12
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 2, 2026
@chenmingwei23
chenmingwei23 force-pushed the feat/aws-control-remove-on-cloud-listing branch from 14940f7 to 2dfb3b9 Compare September 2, 2026 06:41
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 2, 2026
@chenmingwei23
chenmingwei23 force-pushed the feat/aws-control-remove-on-cloud-listing branch from 2dfb3b9 to 72ad64b Compare September 2, 2026 06:43
@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision merge conflict Branch has merge conflicts with its base — author must resolve before merge and removed readiness: checking Automated validation is still running labels Sep 2, 2026
@chenmingwei23
chenmingwei23 force-pushed the feat/aws-control-remove-on-cloud-listing branch from 72ad64b to e3d51d8 Compare September 2, 2026 07:57
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: passed Eligible automated validation passed for the current revision readiness: checking Automated validation is still running merge conflict Branch has merge conflicts with its base — author must resolve before merge labels Sep 2, 2026
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 2, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

On the two Remove items at DrivePage.tsx (round four): the render path, one concession, and why the remedy is declined.

The render path. Both items are in the Library folder's cloud listing, not the picker: one in LibraryCloudCard (grid), one in the list row inside LibrarySection. local can be defined at both -- it is bySlug.get(slug) -- so that part of the finding is correct.

What the delete targets. The listing is a delimited read of the bucket: list_section(section="library", subpath="") runs list-objects-v2 --prefix artifacts/ --delimiter /, and folders is built from the returned CommonPrefixes. A row therefore exists if and only if artifacts/<slug>/ is in the bucket, and libraryRemove(slug) empties that same enumerated prefix. The target is the object that was observed; it is never inferred from the ledger. That is the difference from the picker, where a row IS a local artifact, nothing on the surface observes the bucket, and the target would come from a slug-keyed ledger -- which is why that surface has no removal, pinned by its own test.

The concession, and it is the part of my earlier reasoning that does not hold. I previously argued the confirm naming artifacts/<slug>/ covers the slug-reuse case. It does not. The slug is IDENTICAL for the old and new artifact -- that is the premise of the reuse case -- so naming the folder gives the reader nothing they can check: the card says "B", the confirm says artifacts/S/, and both readings are equally consistent with the object being A's. So the residual is not merely a stale label next to a correct disclosure. The delete target is right, and the DISPLAYED IDENTITY can belong to a different artifact, on an irreversible action. That residual is #6987 and it is not closed by this PR.

Nothing ledger-derived can close it either. pushedVersion and pushedAt both come from list_pushable reading a ledger keyed account -> slug, so a later artifact taking the slug inherits the stamp exactly as it inherits the version.

Why the remedy is declined. Rendering Remove only when local is undefined hides the control in the NORMAL case -- you pushed a copy and you still have the original -- leaving it only on copies whose local twin has since been deleted. That is a withdrawal of the affordance from most rows on the surface this change exists to put it on, and it re-opens the UX finding that motivated the change. A test now pins the boundary in both grid and list view; applying this remedy to either site reds it.

What would actually close it is identity read from the bucket rather than the ledger. I have checked whether that data is already available: it is not. The Library listing is delimited, so each slug arrives as a CommonPrefix, which carries no size or last-modified; folders is typed string[]. The only other per-slug bucket read, list_library_folders, also returns names only, and the usage report is per-section rather than per-slug. Surfacing per-slug object count, bytes and newest last-modified needs a backend change -- a modest one, since usage() already performs a whole-bucket undelimited listing and projects Contents[].{Key, Size} -- and that is a maintainer decision, not something to fold into this round.

@chenmingwei23
chenmingwei23 force-pushed the feat/aws-control-remove-on-cloud-listing branch from 4e03a66 to 2e596a4 Compare September 2, 2026 12:40
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 2, 2026
A removal offered on the "Add from Artifacts" picker cannot know which cloud
copy it is deleting. The picker's rows are LOCAL artifacts joined to a ledger
keyed `account -> slug`; `ArtifactStore.delete` does not prune that ledger and a
new artifact starts at version 1, so pushing A, deleting A locally and creating a
B that takes A's slug leaves B's card wearing A's push record. Removing from B
empties `artifacts/<slug>/`, which is A's copy, under B's name. No predicate
available on that card separates the two.

The Library folder beside it lists the real bucket prefix, so a row there IS a
cloud folder and removing it empties the object the reader was shown. The control
moves there, into the same overflow menu the Files folder's own cards and rows
use, and is offered on every listed copy including one no local artifact backs --
which is what makes a copy pushed from another machine removable rather than
stranded, the promise `remoteOnly` already carries.

Drive's Download moves into that same menu, in both views, so a folder offers one
grammar for "act on this item" rather than a bare button beside an overflow.
@chenmingwei23
chenmingwei23 force-pushed the feat/aws-control-remove-on-cloud-listing branch from 2e596a4 to f908b2f Compare September 2, 2026 12:51
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 2, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

/ai-review override gpt f908b2f: Zero-delta vs base: main's PickerCard:1034 calls the same libraryRemove(slug) and empties the same artifacts/S/ bytes under the replacement's name; main's own comment at :1045 concludes it cannot prove ownership without the pushed meta.json. This PR relocates the control and does not move that harm -- #6987 owns the cause. Gating on !local would hide Remove whenever a local copy exists, which with the picker's Remove gone leaves no path to delete a cloud copy. Residual documented in the body.

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Human judgment recorded

@chenmingwei23 marked the gpt AI finding as false positive, not applicable, or explicitly accepted for f908b2fbad46ecc786956cd3a57f482b705d4167.

Zero-delta vs base: main's PickerCard:1034 calls the same libraryRemove(slug) and empties the same artifacts/S/ bytes under the replacement's name; main's own comment at :1045 concludes it cannot prove ownership without the pushed meta.json. This PR relocates the control and does not move that harm -- #6987 owns the cause. Gating on !local would hide Remove whenever a local copy exists, which with the picker's Remove gone leaves no path to delete a cloud copy. Residual documented in the body.

This decision applies only to this commit. A new push requires a new judgment.

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 2, 2026

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

Verdict: 0 blocking, 3 non-blocking. Reviewed at head f908b2fbad46ecc786956cd3a57f482b705d4167.

What I verified

  • Delete target is the row, not ambient state. removeMut calls awsControlApi.libraryRemove(account, slug) where slug is the folder enumerated by the bucket listing (driveList -> pg.folders), threaded per-card/per-row (onSelect={() => askRemove(slug)} / onAskRemove). The local = bySlug.get(slug) join supplies only the card's label/thumbnail. Verified there is exactly one libraryRemove caller on HEAD (DrivePage.tsx:336); the picker's old caller is gone, no orphaned callers.
  • Old removal path fully migrated. The picker's Remove control and confirm strip are deleted; the test does not offer removal from the picker asserts both library-remove and library-more are absent inside the "Add from Artifacts" dialog (DrivePage.test.tsx, queryByTestId('library-more') null). No dead entry point left behind.
  • Server-side authorization unchanged. libraryRemove is a pre-existing backend endpoint; this PR relocates the UI caller only and does not weaken any server guard.
  • AUTOSDE blocking rules (website/AUTOSDE.yaml). frontend-security: no innerHTML/dangerouslySetInnerHTML/template-literal HTML added. accessible-interactive-elements + icon-buttons-need-labels: both new library-more triggers carry aria-label={i18nT('apps.awsControl.console.library_actions')} (DrivePage.tsx:697,913), and a test asserts the label + that the trigger sits OUTSIDE the <Link> (no <button> in <a>). use-lucide-icons/no-emoji-as-icons: uses lucide MoreHorizontal, no emoji. page-layout-pattern regex applies to src/pages/*.tsx only — N/A here. Remove is a DropdownMenuItem behind the overflow, not a visible danger button.
  • i18n. 4 new keys (library_actions, library_remove_action, library_removing, library_remove_failed) added to en.json + 11 catalogs + en-XA regenerated + en.context.json; all user-facing strings route through the catalog.
  • Spec is in-commit. docs/system-specs/features/aws-control.md modified in the same commit (AGENTS.md maps this surface to that spec).
  • Reuse-slug residual (#6987) is pre-existing, not introduced. The delete target is the enumerated bucket prefix artifacts/<slug>/ (a test pins it names artifacts/, not library/); the residual is the borrowed label, identical in bytes/request/target to main's picker path. The GPT 5.6 finding on this was human-overridden with a sound zero-delta rationale. Correctly scoped out.
  • Tests pin behavior. New tests drive through the real menu (chooseFromMenu), assert library-remove count 0 at rest, cover the no-local-twin copy, the reuse-slug boundary in grid AND list, and the per-slug pending/failed sets. I did not re-run the suite; CI (Frontend Tests/Lint/Type Check, Coverage Gate) is green at this SHA.

Findings

  1. website/src/i18n/locales/en.json:3709library_remove_failed overclaims on partial failure (non-blocking). Text is "Remove failed — the cloud copy is unchanged. Try again." libraryRemove is a delete_prefix sweep over multiple S3 objects, so a mid-sweep failure can leave the prefix partially emptied while this asserts nothing changed — misinformation on an irreversible action. Consequence: a user skips re-verifying and believes a partially-deleted copy is intact. This is inherited framing from the old delete_failed and is recoverable (retry stays available), so non-blocking. Suggestion: drop the state claim ("Remove failed. Try again.") unless the backend confirms zero deletions. (Matches the UX lane's CONCERNS.)
  2. DrivePage.tsx Drive Download relocation (drive-grid-download / drive-download) — rides along on consistency grounds (non-blocking). Download moves from a one-click bare button to an overflow item in both views; the core Remove-relocation fix stands without it, and no failing user is named for the Download change. Consequence: the most frequent Files action costs a second click for habituated users. Suggestion: a human ratifies (or defers) this scope extension rather than inheriting it. (Matches First Principles + UX advisories.)
  3. DrivePage.tsx:~1706,1774TileConfirm disabled={pending} on Cancel now reaches the two Files-folder deletes (non-blocking). A behavior change on a surface outside this PR's stated scope. Opus falsified it as a defect (the feeding mutations always resolve and re-enable Cancel; Cancel only closes the strip and never aborts the request), so it is safe. The in-code rationale comment ("this strip is the ONLY place the outcome can render") is inaccurate for the library caller, whose failure also renders on the card via failedSlugs. Suggestion: correct or scope that comment so a later editor does not propagate it.

What I could not verify

  • I did not execute the 730-test suite or the screenshot harness locally; I relied on green CI at this SHA and read the test assertions statically.
  • The backend delete_prefix partial-failure behavior (whether it reports objects-deleted on error) I inferred from the diff comment ("a delete_prefix sweep over several S3 objects"); I did not open storage.py/routes.py, so finding #1's exact partial-failure surface is described from the PR's own text plus the client-side message.
  • Runtime rendering of the overflow menu (portal/z-index/focus behavior) was checked via tests and static reading only, not in a live browser.

@bolichen97
bolichen97 enabled auto-merge September 2, 2026 18:20
@bolichen97

Copy link
Copy Markdown
Collaborator

Audit note — part of this has already landed; the rest has not

This PR is not a duplicate and is not finished by anything on main. The audit checked it part by part against main, and some of what it does is already there. Flagging it so a reviewer does not have to rediscover the overlap, and so the PR is not mistaken for fully-covered work.

Already landed

#7026 CONFIRMED MERGED: the issue/PR reference check row 7026 PR closed 2026-09-02T03:56:56Z, and the landed-commit index for main line 25 90f708efbdd5a633bee1c4f26c89a8cd78c6be8e 2026-09-01 feat(aws-control): let the picker remove a library cloud copy (#7026); git merge-base --is-ancestor 90f708efb origin/main passes. This is the only ref that covers anything. #7801 CONFIRMED MERGED (the issue/PR reference check merged_at 2026-09-02T07:42:03Z; on main as be2ee94, ancestry-verified) but its coverage of this PR is NONE — it is a drive layout/rail refactor; the relation is rebase-conflict only. #7778 is an OPEN PR (the issue/PR reference check 7778 PR open -) — the initial scan already labelled it OPEN_PR / coverage NONE, so no coverage claim rests on it. #6987 is an ISSUE (6987 ISSUE closed -), no code, and the initial scan correctly labelled it an issue. No the initial scan "merged" claim turned out to be an open PR here.

Which parts main already has

Only the prerequisite plumbing and the bare existence of a dashboard library-removal, all from #7026 (90f708e), and this PR deliberately does not re-add any of it: (1) the libraryRemove API binding — origin/main:website/src/apps/aws-control/api.ts:223 libraryRemove(account: string, slug: string): Promise<{ removed: true }>; api.ts is not in this PR's files.tsv. (2) 3 of the 7 i18n keys the feature needs — origin/main:website/src/i18n/locales/en.json:3704 library_remove, :3705 library_remove_confirm, and origin/main:website/src/i18n/locales/en.manual.json:81 library_remove_confirm_slug; this PR adds only 4 new keys. (3) The destructiveConfirm gate entries — origin/main:website/src/i18n/destructiveConfirm.test.ts:175 and :253. (4) The design of the confirm itself: an inline Cancel-plus-danger strip that stays open until the request resolves and names the artifacts/<slug>/ bucket prefix — this PR keeps that verbatim (its own confirmFor reuses the same <Trans i18nKey="...library_remove_confirm_slug"> with folder: artifacts/${slug}/).

What is still genuinely yours

Everything this PR is actually about. 13 distinct parts, each probed against origin/main and absent:

  1. RELOCATION. main:website/src/apps/aws-control/DrivePage.tsx:1034 still has <Btn onClick={() => setConfirmRemove(true)} data-testid="library-remove"> inside PickerCard (function PickerCard( at :927), and function LibraryCloudCard({ slug, local, localAnswered } at :576 has no removal props at all. git grep -c 'library-more' origin/main -- website/src = 0 hits.
  2. LIST-VIEW removal. main:DrivePage.tsx:510/:515 data-testid="library-list-row" carries no menu; library-more absent.
  3. PICKER removal DELETED. main still has removingSlugs (2 hits), removeFailedSlugs (2 hits) and confirmRemove (4 hits) in DrivePage.tsx, plus the useEffect-on-synced reset this PR removes.
  4. SECTION-LEVEL STATE. confirmSlug, errorFor, pendingFor, clearIf, confirmFor, askRemove, and the slug-keyed onMutate/onSuccess/onError — 0 hits each on origin/main across website/src, docs, website/scripts.
  5. TileConfirm WIDENING. main:DrivePage.tsx:551 is still function TileConfirm({ label, error, pending, onCancel, onConfirm, action }: { label: string ... }) with hardcoded drive-grid-confirm* ids and an UNDISABLED Cancel (<Btn onClick={onCancel} data-testid="drive-grid-confirm-cancel">). The PR's label: React.ReactNode, testId base, and disabled={pending} on Cancel are all unlanded; library-remove-confirm-cancel / -action = 0 hits.
  6. CARD SHELL RESTRUCTURE. main puts data-testid="library-card" on the <Link> itself (:659) with SHELL = '... block ...'; the PR's <div> shell + relative + inner <Link> (so the trigger can be a real <button>) is unlanded.
  7. UNGATED removal (a cloud copy with no local artifact is removable) — main gates the control on synced, so a copy pushed from another machine is unremovable.
  8. drive-grid-download folded into drive-grid-more — main:DrivePage.tsx still has it as a bare <button ... data-testid="drive-grid-download"> (1 hit).
  9. drive-download folded into drive-more — main:DrivePage.tsx still has the bare button (2 hits), and main's test at DrivePage.test.tsx does fireEvent.click(await screen.findByTestId('drive-download')).
  10. 4 NEW i18n KEYS. library_actions, library_remove_action, library_removing, library_remove_failed — 0 hits on origin/main across all 13 catalogs, en-XA, and en.context.json.
  11. SPEC. origin/main:docs/system-specs/features/aws-control.md:237 still reads "is gated the way folder deletion is: the picker's per-card Remove control reveals"; the PR's "is offered on the Library folder's own listing — one overflow menu per listed cloud copy ... and never on the "Add from Artifacts" picker" is absent.
  12. TESTS. chooseFromMenu = 0 hits. main still carries the three picker-era tests this PR replaces — offers Remove on synced cards only, behind an inline confirm (:792), keeps the confirm open and says so when the removal fails (:846), does not re-arm the remove confirm when a removed card is pushed again (:885) — and none of the PR's 8 new tests (removes from the cloud listing, naming the artifacts/ prefix it empties, removes a cloud copy that has no local artifact behind it, carries the removal in list view too, does not offer removal from the picker, where identity is unprovable, does not close one card's confirm when a different removal completes, keeps a failure on the card that owns it when another confirm is opened, does not erase an existing failure when another confirm opens, reports a failed removal on its card and retires it on cancel) exist on main.
  13. HARNESS + SCREENSHOTS. origin/main:website/scripts/capture-aws-control-library-remove.mjs still drives library-add-open (:168) and asserts library-tile 3 / library-remove 2 / library-remove-cancel / library-remove-action. main's screenshots are temp-screenshots/aws-control-library-remove/{library-remove-at-rest,library-remove-confirm}.png; the PR's new temp-screenshots/aws-control-remove-on-cloud-listing/ dir with 3 frames including library-remove-menu.png does not exist on main.

Suggested action: REBASE — the remainder is real work; rebase onto the landed part rather than closing.


From a repository-wide duplicate/overlap audit of every pull request open against main (2026-09-02, 330 PRs, one reviewer per PR). Each PR was read as its full merge-base diff plus its description and every comment and review, then compared against each candidate PR's own diff and against origin/main at 1a765b88ceb7. This PR is not being closed — the note is informational. If the reading is wrong, please correct the reasoning rather than just the conclusion.

@iamwhatever
iamwhatever disabled auto-merge September 2, 2026 20:56
@iamwhatever
iamwhatever merged commit eee3f22 into main Sep 2, 2026
69 of 70 checks passed
@iamwhatever
iamwhatever deleted the feat/aws-control-remove-on-cloud-listing branch September 2, 2026 20:56
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Sep 2, 2026
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.

4 participants