feat(archive): age-bucketed archived view with per-bucket bulk remove + 'Archived recently' quick restore#46
Merged
Merged
Conversation
miguelrisero
marked this pull request as ready for review
July 23, 2026 14:56
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Archive buckets: age-grouped archived view, per-bucket bulk remove, and "Archived recently" quick restore
Implements the two-part owner request:
How it works
archived_atcolumn onworkspaces(migration + one-time backfill; see the disclosure section below). Transition logic maintains it through both mutation paths (Workspace::updateandWorkspace::set_archived) including the auto-unarchive-on-activity flow, with unit tests pinning first-archive / redundant-write idempotency / unarchive-clears / re-archive-restamps through both functions.[0,1) [1,3) [3,7) [7,15) [15,30) [30,∞)days, floor-computed, future timestamps clamp to Today, and a NULL/invalidarchived_atdefensively lands in the oldest bucket (unknown age must never look fresh). Implemented once in Rust (crates/db/src/models/archive_bucket.rs) and mirrored in TS (packages/ui/src/lib/archiveBuckets.ts) with cross-referencing keep-in-sync comments — the same convention this repo already uses formodel_supports_effort. Exact-edge boundary tests on both sides (every boundary ±1ms).(workspace_id, archived_at)pairs. The server loops only over submitted targets — there is no bucket-wide lookup — and revalidates each one fresh immediately before deleting: still exists, still archived, andarchived_atequal (Option-to-Option, so a never-stamped NULL row is representable, deletable, and still protected). Any mismatch is skipped with a distinct reason (already deleted/no longer archived/archive state changed since review). This catches the resurrect-and-re-archive race (auto-unarchive on activity gives a fresh timestamp → mismatch → skip) while never false-skipping a workspace that merely aged across a bucket boundary while the dialog sat open.N removed, M skipped, K failed, each with its reason — always surfaced; one failed item never aborts the rest of the batch. The result never claims the bucket is empty (anything archived after the snapshot is untouched by design).ensure_container_exists.REPO_MUTATION_LOCKSaround branch deletion and worktree removal) — N concurrent detached cleanups against one shared repo was a bulk-only hazard single-delete never had.delete_workspace_corewas extracted and both routes call it; the single route returns the same 202/409 envelopes with the same message text. The shared DELETE is deliberately not conditional onarchived = TRUE(single-delete must remove active workspaces) — there's an inline comment so a future pass doesn't "simplify" that guard back in.Backfill accuracy:
archived_atis approximate for pre-existing rowsThis PR adds an
archived_atcolumn and backfills it for workspaces that were already archivedbefore the migration ran, using
updated_atas the proxy. For those rows the timestamp is anapproximation, and it is approximate in one direction only.
updated_atis the last time the row changed for any reason — a rename, a pin, a container_refupdate — not the moment it was archived. So for a workspace archived long ago but touched since,
the backfilled
archived_atis later than the true archive time.Why that direction is the safe one. All ten
updated_atwrite sites were audited; onlyupdate_container_refbinds a value and it uses an in-functionUtc::now(). Nothing anywherebackdates
updated_at. Thereforebackfilled archived_at >= true archive time, and a pre-existingrow can only ever appear younger than it really is. A younger-looking row sorts into a newer
bucket, so a sweep of an older bucket can only ever miss it — never capture something older
than the operator intended. The failure mode is under-deletion, which is recoverable by sweeping
again; over-deletion is not possible through this path.
Rows archived after this migration get an exact timestamp from the transition logic, in both
Workspace::update()andWorkspace::set_archived()— including the silent auto-unarchive onactivity (
crates/services/src/services/container.rs), which is covered by its own test.Decision: no
archived_at_is_approximateflag. A schema flag was considered and skipped. Thesafe direction above is proven rather than assumed, so the residual risk is bounded to "an old
workspace survives a sweep it arguably should have been caught by" — visible, harmless, and fixed
by sweeping again. Disclosure plus the proven guarantee is the right-sized response; new schema
would add surface area to an already-large change for a bounded, recoverable risk.
Distribution on the author's instance at time of writing
71 workspaces total — 59 archived, 12 active. Archived, bucketed by the
updated_atproxy thebackfill will use:
~80% (47 of 59) land in the two oldest buckets, which are the ones a first cleanup pass would
target. Those are also the rows where the
updated_atproxy is least likely to be misleading — anuntouched-for-a-month workspace has an
updated_atclose to its true archive time by construction.Scale of what a sweep actually touches (same instance, same reading)
Workspaces are not 1:1 with repos — 59 archived workspaces span 75 repos (45 workspaces
with 1 repo, 12 with 2, 2 with 3). Per bucket, and including how many worktrees the 1-hour archived
cleanup job has already reaped:
Two consequences the review acted on:
bucket a 1:1 assumption would report 28 branches where the real figure is 40 — understating by
43% on the most likely first use of the feature.
worktree_deleted = 1. Computing thebranch-comparison preview through a code path that recreates missing worktrees would perform ~21
worktree recreations just to render the confirmation dialog, before the operator confirms
anything. Preview is required to stay read-only; verified by asserting zero recreations.
Note on NULL
archived_atThe backfill sets
archived_at = updated_atfor every archived row, andupdated_atis NOT NULL,so this instance will have zero NULL
archived_atvalues immediately after migrating. TheNULL-handling path (treat as oldest bucket; representable as a bulk-delete target; deletable) is
therefore defensive rather than day-one load-bearing here — but it is still required, because a row
can reach an archived state without a stamped timestamp, and an unrepresentable target would be a
workspace the bulk tool can never remove.
Known limitations — read this first
This feature deletes branches and worktrees with no undo, so the limits of its consent model are listed up front rather than buried. Each was found by review, verified in code, and deliberately dispositioned; none is an unknown.
delete_workspace_coreperforms destructive teardown (CLI-session kill, dev-server stop, deletion-context capture) before the row delete, so a conditional DELETE matching zero rows would leave a half-torn-down but surviving workspace — a worse state than the residual it closes; (2) the statement is shared with single-delete, which must delete active workspaces. Closing this properly means a transactional re-validation refactor of a shared destructive path — real risk to buy out a sub-second, single-user window that thearchived_atsnapshot check already covers for every realistic case (the dialog-open-to-confirm window, minutes long, is fully closed). Inline comments guard this reasoning at both decision points.archived_at, so a repo attached while the confirmation dialog is open would be swept without ever having been shown or counted. Stated plainly because it is a real hole in the "delete exactly what you reviewed" contract, not hand-waved: the exposure requires a self-inflicted single-user race (attach a repo to an already-archived workspace, commit unique work to it, and confirm the bulk delete, all within the dialog window), and the attached branch is created at the target-branch tip — empty by construction absent that in-window commit. Kept out of this PR to keep the destructive surface frozen. Backlogged fix: a per-target repo-set snapshot in the request, validated likearchived_at; a cheaper partial guard also noted for whoever picks it up — carry each target's reviewedrepo_countand skip any target whose current count differs, which catches the attach-mid-window case without full set-snapshot semantics.deletedreports the DB-row deletion; branch/worktree cleanup runs as the same detached background task single-delete has always used, with failures logged, not itemized. Orphaned worktrees are reaped by the existing startup orphan-cleanup; orphaned branches survive (the recoverable direction).REPO_MUTATION_LOCKSnever evicts — growth is bounded by the number of distinct registered repo paths over process lifetime.tsconfigexcludes*.test.tsand the frontend CI job runs build/typecheck/lint/format only — so the target-sourcing, partial-failure, and count-sum assertions can regress while required checks stay green.pnpm testfrom the repo root is the verification affordance (details in the tooling section below); wiring it into CI is backlogged.Verification evidence
Automated
cargo test --workspace— green (includes: archived_at transition tests through both mutation paths; exact-edge bucket boundary tests; the backfill migration's actual SQL executed against seeded pre-existing rows viainclude_str!; and the bulk-delete integration test below).crates/server/tests/bulk_delete_archived_workspaces.rs) runs through the real HTTP router with real JSON serde (Router::oneshot) — deliberately, so any timestamp-precision loss across serde/JSON/SQLite would surface as a failure instead of silently skipping every target. Covers: unsubmitted same-bucket workspace untouched (doesn't even receive an outcome), running-process skip with reason, non-archived target skip, resurrect (unarchive+re-archive) →archive state changed since review, natural aging with unchanged timestamp → still deletes (verified across an actual bucket-boundary crossing), NULLarchived_atround-trips and deletes, already-deleted target itemized without aborting the batch, and a forced DB-level delete failure (SQLite trigger) →failedwith reason while the next target still processes.cargo clippy --workspace --all-targets --exclude vibe-kanban-tauri -- -D warnings) — green../scripts/check-i18n.shrun manually (it is CI-only — not part ofpnpm run lint/check): no new literal strings, no duplicate keys, keys consistent across all 7 locales.pnpm run generate-types,pnpm run prepare-db,pnpm run format— green;shared/types.tsregenerated, never hand-edited.pnpm install --frozen-lockfile— clean (validates the lockfile from the vitest-dependency commit).packages/ui/src/lib/bulkDeleteArchivedWorkspaces.test.ts(9 tests) —buildArchivedBucketState > builds targets from the full bucket despite search and pagination visibility;inspectArchivedWorkspaceTargets > preserves successful results when one workspace inspection fails;inspectArchivedWorkspaceTargets > does not inspect a workspace whose worktree is already removed;sumRepoCounts > sums mixed repository counts across the targeted workspaces;sumRepoCounts > returns null when any targeted workspace has an unknown repository count;resolveDialogTotals > uses inspection totals when initial totals are unavailable and inspection is complete;resolveDialogTotals > returns unknown fallback totals after an inspection failure;resolveDialogTotals > returns unknown fallback totals after a removed-worktree skip;resolveDialogTotals > prefers initial totals regardless of incomplete inspection state. (TheresolveDialogTotalsgate exists because two independent final-round reviewers converged on the same defect: with repo-count data unavailable and an incomplete inspection, the fallback totals could render an exact-looking but understated number; totals now render as unknown unless the inspection covered every target.)packages/web-core/src/shared/lib/archiveBuckets.test.ts— 20 tests pinning every bucket boundary ±1ms, future-timestamp clamping, NULL/invalid-timestamp defensive bucketing, and the 3-day "Archived recently" edge.sumRepoCountsfixture (1-repo / 2-repo / 3-repo, asserting 6) is first coverage for the repo-count-sum property — no prior test in either language attached repos to a fixture at all, so a uniform fixture would have been vacuously green under a correct sum and under a 1:1 bug alike.Live end-to-end (isolated scratch instances — production untouched)
BC_DATA_DIRunder/tmp, ports 4126-4128 and 4136/4137). The real instance on port 4111 (~/.local/share/vibe-kanban/) was verified untouched — held by its own separate process before, during, and after, confirmed via socket/process inspection, not self-report.git branch --list— and the running-process workspace skipped with the exact reason shown in the itemized results.archived=0, archived_at=NULLin the DB.worktree_deleted=1and their worktree directories physically absent, plus 2 normal ones, all in one bucket; opened the confirmation dialog via a real browser. Zero recreations — all 5 directories still absent after; the 2 normal worktrees byte-identical (same inodes). Network trace confirmed the mechanism:GET /git/statusfired only for the 2 present-worktree workspaces; the 5 reaped ones received no status call at all (so this isn't first-item luck — all 5 skipped in one dialog open). Dialog correctly showed "Worktrees removed 2" + 5 × "unmerged status unknown, worktree already removed." Evidence including screenshot retained at/tmp/gate4-verify/.Review gauntlet
Two full rounds (initial 13 commits, then the fix stack): 10-angle deep-review × 2, a 4-5 persona council × 2 (data-integrity, rollback, QA-strategy, UX lenses across Codex + Kimi K3 + Claude engines), and a final read-only Codex sweep. The first round caught a real defect — the confirm request originally carried only a bucket label, so the server could delete a set differing from what the user reviewed — which drove the
(workspace_id, archived_at)reviewed-snapshot rework above. The final round's verdicts: no blockers; remaining findings are the known-limitations below.Review gauntlet outcome note
The final-round sweep's remaining findings are all dispositioned in "Known limitations" above; its clean-areas report independently re-confirmed the worker pool, the target-array/display consistency, the lock ordering, and the absence of new panic paths — a fourth engine agreeing with the earlier passes.
Test tooling change (scope creep, disclosed deliberately)
vitest 3.2.4is now a pinned root devDependency with a roottestscript (pnpm testruns the whole frontend suite from the repo root, which is what makes cross-package discovery work — previously this was tribal knowledge and an ad-hocnpx vitest). This adds ~340 lockfile lines inside a feature PR — flagged so it doesn't hide in the diff. These tests still do not run in CI (tsconfigexcludes*.test.ts; the frontend CI job runs build/typecheck/lint/format only). Local evidence above is the verification; wiringpnpm testinto CI is a separate backlog item.Running the suite also (re)generates an untracked snapshot for a pre-existing, unrelated upstream test (
diffDataAdapter.test.tshas atoMatchSnapshot()with no committed baseline anywhere in the repo — it regenerates and passes unconditionally on every machine). That artifact is deliberately not committed here: a first-run snapshot enshrines an unreviewed baseline and this PR is not the place to set diff-viewer testing policy. Expect the dirty untracked file after running tests locally; recorded as its own backlog item.Deviations & caveats
pnpm run check/pnpm run lintpass every frontend/UI phase and the primary Rust workspace, then fail at thecrates/remotephase: its optionalbillingdependency lives in the privateBloopAI/vibe-kanban-privaterepo, unreachable from this fork (CI skips that job on forks for exactly this reason). Pre-existing and unrelated — this diff touches neithercrates/remote/,Cargo.toml, nor.cargo/(verified: empty diff on those paths)./deep-reviewskill'sReportFindingstool is only available inside the built-in/code-reviewcontext → its findings were processed as text through the same pipeline instead.