fix(mobile): stop sr-only labels widening every route; move integration tests to Testcontainers by unclesp1d3r · Pull Request #88 · unclesp1d3r/mag_stacker

unclesp1d3r · 2026-07-29T12:47:14Z

Started as a mobile overflow fix; the investigation surfaced two deeper problems worth fixing in the same pass.

1. Mobile: the app was unusable at phone width

Every route measured 976px wide at a 393px viewport. The cause was not the tables or the forms — those scroll correctly.

Header cells render their "Actions" label as a .sr-only span, which is position: absolute. The DataTable's scroll container was static, so those spans resolved their containing block past it to the initial containing block. overflow-x-auto therefore never clipped them, and each sat at the table's full width in document space. On every route, document.scrollWidth equalled that span's right edge exactly (541 / 621 / 615 / 629 / 603).

Fix: relative on the scroller. The primary nav rail had the same latent shape and is hardened too, plus min-w-0 + overflow-x-auto so its 6–8 links scroll in place instead of forcing the document wider.

Verified: 32/32 route × width combos (320/390/768/1440) show no horizontal document scroll, asserting the pages actually rendered — a table route reporting zero tables counts as a failure, not a pass.

⚠️ Worth knowing: Playwright's isMobile: true masks this entire class of bug. It widens the layout viewport instead of scrolling, so maxScrollX reads 0 and everything looks fine.

2. Tests: the integration suite was silently skipping

const live = process.env.DATABASE_URL ? describe : describe.skip gated 34 files against a hand-started dev database that isn't described anywhere in this repo — the justfile states outright there is no compose stack. Three failure modes, all hit:

No database running → 100+ connection failures, not skips. The gate only checks the variable is set, never that the server is reachable.
Database running but seeded (e.g. by the new just db-seed) → tests that count or list rows fail against someone else's data.
Variable never set → the suite reports green while asserting nothing. This is how it always ran in CI.

bunfig.toml now preloads src/test-support/preload.ts, which starts a migrated ephemeral Postgres and exports DATABASE_URL before any test module is imported. (Top-level await is required — module-scope code reads it, so a beforeAll hook runs too late.) Teardown is Ryuk's, matching the e2e launcher.

	Before	After
Tests executed	554	773 (+219 previously skipped)
Result	448 pass / 106 fail / 11 errors	773 pass / 0 fail
Run time	456s	67s
CI test step	none	runs the suite

It also exposed a real bug the skips were hiding: the firearms ordering test compared Postgres's ORDER BY against JS localeCompare, over rows a different test left behind (" Spacey "). Those collations genuinely disagree about leading whitespace — Postgres sorts spaces first, ICU ignores them at primary strength. The test now owns its fixture rather than asserting over whatever ran before it.

The pinned Postgres digest previously lived in 9 places with a comment warning it had to be bumped in lockstep. It now lives in src/test-support/postgres-image.ts, which a composite action reads directly, so CI cannot pull a different runtime than the suites use.

3. Design system: the rules now enforce themselves

DESIGN.md names a Tabular Rule and a Mono-Label Rule that were being broken the same way — by hand-writing an approximation at each call site. <Data> and <Kicker> replace ~20 className="tabular" sites missing font-mono and 6 tracked-sans kickers. DetailRow collapses 4 byte-identical definitions to 1; orDash 6 to 1.

Also: Revoke was the only destructive action firing straight off the click — now confirms. And the accent stripes on PageHeader/Stat are gone; the One Accent Rule says the anodized orange is "never to decorate a heading, a border stripe, or a background panel," and a stripe on every page spent the rarity that makes "lit" read as a signal.

4. `just db-seed`

Seeds a realistic local collection (3 firearms, 33 labelled magazines across 4 calibers with compatibility links, 2 ammo lots, 5 accessories) so the dense table — the app's signature surface — can be worked on with real data. Writes through the domain services, not raw inserts, so seeded rows pass the same validation the UI does. Idempotent by default; --reset is opt-in.

6. Interrupted-restore recovery could destroy the wrong blobs

Turning the suite on in CI immediately paid for itself. recoverInterruptedRestore picks the newest of several moved-aside pre-restore blob directories and deletes the rest — ordering them by mtime, which cannot answer that question:

rename does not update a directory's own mtime, so a moved-aside directory keeps whatever mtime uploadDir had — when blobs were last written, not when the swap happened. Two directories can be ordered backwards outright.
The names carried only randomUUID(), so equal mtimes fell through to readdir order, which is filesystem-dependent.

Picking wrong means recovery restores stale blobs and destroys the real pre-restore ones. beginBlobSwap now stamps the creation time into the name (.pre-restore-<epochMs>-<uuid>) and recovery orders on that, falling back to mtime only for directories left by an older build.

The test had been skipping for want of a database; once it ran it failed on a fast runner where two mkdir calls land in the same tick. It no longer races the clock — it sets the stamps explicitly and forces the stale directory to hold the newest mtime, so mtime ordering would now pick the wrong one and only the stamp gets it right.

Review findings addressed

Reviewed by CodeRabbit plus five focused passes (code quality, test coverage, silent failures, comment accuracy, type design). Every valid finding is fixed in-branch — nothing deferred.

Bugs found that this PR introduced:

Every generated magazine label exceeded MAX_LABEL_LENGTH (4), so just db-seed threw magpulLabelTooLong part-way through for any Magpul-mode owner. Invisible locally because the seeded admin defaults to magpulMode: false.
resetInventory ran five auto-committing deletes; a mid-sequence failure left a half-wiped account, and because hasInventory probed only firearm, the next run reported "nothing to do" over data the failed run had destroyed.
The revoke confirmation dismissed itself before the async revoke was even sent, making its own pending/"Revoking…" state unreachable and a failure look like success. Escape also closed the parent share modal.
Registering SIGINT/SIGTERM handlers replaced the default terminate-on-signal behavior without exiting, so Ctrl-C hung the run — and then exited 0, reporting an interrupted run as passing.
process.on("exit", …) could never await an async container stop; it only read as cleanup.

Bugs found in pre-existing code this PR touched:

Interrupted-restore recovery deleted the live upload directory before confirming a replacement existed, and swallowed every fs error rather than just ENOENT — leaving the database rolled back while blobs held the half-promoted state.
Worse, the first fix for that logged "reconcile by hand" and then dropped the snapshot schema and cleared maintenance mode, destroying the thing an operator would reconcile from. Blob failure now escalates like a failed DB rollback: flag stays active, snapshot preserved.
The stamp regex searched the whole path, so a .pre-restore-<digits>- segment in an ancestor directory would give every candidate the same rank.
Date.now() is not collision-safe for two swaps in the same millisecond; equal stamps tie and fall back to readdir order.

Two more instances of the test-isolation bug class this PR already fixed once: csv/build.test.ts and csv/ammo-build.test.ts asserted "empty inventory" over a shared viewer that a later test in the same file grants access to.

New coverage: e2e/responsive-overflow.spec.ts (the mobile P0 had no automated guard at all — asserts against a populated table, on a plain viewport, and that pages actually rendered), demo-dataset label tests, and maintenance tests for the legacy-unstamped path, the decoy-parent path, and same-millisecond stamps.

Test plan

just ci-check green (lockfile, lint, format, typecheck, pre-commit, unit/integration, e2e)
773 pass / 0 fail with no database on :5544 — the suite provisions its own
e2e 36 passed, 4 skipped (DEMO-gated)
32/32 route × width overflow verification against populated tables
Table and nav still scroll in place (table 356→757px; nav 96→699px, all 8 links reachable)
Visual pass at 1440 + 390, both themes
just ci-check green after every round — now 785 pass / 0 fail, e2e 39 passed
CI's new Test step passes on a runner — ci and e2e both green

5. "Add" CTA placement standardized

Four near-identical CRUD surfaces placed their primary action two different ways: firearms-view used PageHeader.actions, while magazines/ammo/accessories each floated it in a flex justify-end row below the header rule — reading, at phone width, as an unanchored control stranded between the title and the filter panel.

Structural, not cosmetic: those three declared PageHeader in page.tsx while the button needs the view's client state, so the header moved into the view (the arrangement firearms already used). Export and Add now sit together as page actions, Add carrying primary emphasis.

Verified across all four surfaces at 1440 and 390: every Add button is inside the page header, and no route scrolls horizontally.

The DataTable scroll container was `static`, so the `.sr-only` "Actions" spans in header cells — which are `position: absolute` — resolved their containing block past it to the initial containing block. `overflow-x-auto` therefore never clipped them, and each sat at the table's full width in document space, widening every table route to 541-629px at a 393px viewport even though the table itself scrolled correctly. On each route the document's scrollWidth equalled that span's right edge exactly. Add `relative` to the scroller, and to the primary nav rail which had the same latent shape. The nav also gains min-w-0 + overflow-x-auto so its six-to-eight links scroll in place rather than forcing the document wider, and the toast rail is capped at 100vw so it cannot inherit a wider document. Verified 32/32 route x width combos (320/390/768/1440) show no horizontal document scroll, asserting the pages actually rendered. Note that Playwright's `isMobile: true` masks this class of bug entirely: it widens the layout viewport instead of scrolling, so maxScrollX reads 0. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>

Seeds a realistic local collection (3 firearms, 33 labelled magazines across 4 calibers with compatibility links, 2 ammo lots, 5 accessories) so the dense table — the app's signature surface — can be worked on with real data instead of an empty state. Writes through the domain services rather than raw inserts, so seeded rows pass the same validation, owner resolution and label normalization the UI does: a seed the app itself would reject is worse than no seed. Idempotent by default; --reset (or SEED_RESET=1) is opt-in because the script writes to whatever DATABASE_URL points at. The datasets move to src/demo/inventory.ts, which stays dependency-free so both the plain Bun script and the Playwright demo fixture can share one definition — the fixture previously held its own copy. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>

DESIGN.md names two rules that were being broken the same way — by hand-writing an approximation of them at each call site. Four detail views each declared a byte-identical DetailRow whose label used tracked sans (the Mono-Label Rule says mono), and ~20 sites wrote className="tabular" without font-mono (the Tabular Rule says both). Neither is catchable by review at a glance, so name the rules and let the primitives carry the classes. DetailRow collapses 4 definitions to 1 and orDash 6 to 1. Stat's label and value now go through the primitives too, which is what put its big numbers in mono. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>

Revoke was the only destructive action in the app that fired straight off the click, with no confirm step. Route it through ConfirmDialog. The One Accent Rule reserves the anodized orange for the live control, the current selection, the lit row — "never to decorate a heading, a border stripe, or a background panel." PageHeader and Stat each painted an accent stripe on every page, which is exactly the decorative use that spends the rarity making "lit" read as a signal. The hairline rule and tonal layering carry it instead. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>

The integration suite gated on an ambient DATABASE_URL — `const live = process.env.DATABASE_URL ? describe : describe.skip` — pointing at a hand-started dev database that is not described anywhere in this repo (the justfile states outright that there is no compose stack). Three failure modes, all of which we hit: 1. No database running -> 100+ connection failures, not skips: the gate only checks the variable is set, never that the server is reachable. 2. Database running but seeded (e.g. by `just db-seed`) -> tests that count or list rows fail against someone else's data. 3. Variable simply never set -> the suite reports green while asserting nothing. This is how it always ran in CI. bunfig.toml now preloads src/test-support/preload.ts, which starts a migrated ephemeral Postgres and exports DATABASE_URL before any test module is imported (top-level await is required: module-scope code reads it, so a beforeAll hook would run too late). Teardown is Ryuk's, matching the e2e launcher. Consequences: - 219 previously-skipped tests now execute (554 -> 773), and run time drops from 456s to 67s. - The ci job had no test step at all; it now runs the suite. - Removed the skip-gate from 34 files. - Fixed a real bug those skips were hiding: the firearms ordering test compared Postgres's ORDER BY against JS localeCompare over rows a *different* test left behind (" Spacey "). The two collations genuinely disagree about leading whitespace. The test now owns its fixture instead of asserting over whatever ran before it. - The pinned image digest lived in 9 places with a comment warning it had to be bumped in lockstep; it now lives in src/test-support/postgres-image.ts, which a composite action reads directly so CI cannot pull a different runtime than the suites use. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>

coderabbitai · 2026-07-29T12:47:47Z

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

Fixed mobile horizontal overflow by constraining data-table and primary-navigation scrollers, adding scrollbar utilities, and capping toast width.
Centralized UI primitives and patterns with reusable Data, Kicker, DetailRow, and orDash helpers; standardized PageHeader.actions, confirmation-based revoke flows, and removed decorative accent stripes.
Migrated all integration tests to preload-managed, migrated Testcontainers Postgres with a pinned image digest; improved fixture isolation, seed transactions/validation, and blob-restore recovery.
Added idempotent demo inventory seeding and expanded coverage with Bun tests for seed integrity, maintenance recovery tests, and Playwright responsive-overflow scenarios. Reported 773 unit/integration, 36 E2E, and 32/32 route-width checks passing.
No migration steps or known breaking changes beyond the async GrantsList.onRevoke API; tests now require Docker/Testcontainers rather than an externally configured DATABASE_URL.

Walkthrough

The PR centralizes pinned Postgres setup, enables always-on database tests, adds shared demo inventory seeding, standardizes UI primitives and responsive layouts, adds grant-revocation confirmation, and makes blob recovery ordering timestamp-based.

Changes

Infrastructure and test setup

Layer / File(s)	Summary
Centralized Postgres test setup `.github/actions/...`, `.github/workflows/ci.yml`, `src/test-support/*`, `bunfig.toml`, `AGENTS.md`	CI and E2E reuse a composite image-pull action; Bun preload starts and migrates an ephemeral Postgres container before tests.
Database test execution `src/*/__tests__/`	Database-backed suites no longer use `DATABASE_URL`-based skipping, and fixtures add isolated users, shared image imports, and cleanup.
Demo inventory seeding `src/demo/inventory.ts`, `scripts/seed-demo.ts`, `e2e/fixtures/demo-seed.ts`, `e2e/responsive-overflow.spec.ts`, `package.json`, `justfile`	Shared datasets support E2E form seeding and a reset-aware database seed command with label and mount validation.

UI consistency and interactions

Layer / File(s)	Summary
Shared UI primitives and layout `components/ui/*`, `app/globals.css`, `app/(app)/app-shell.tsx`, `app/(admin)/users/admin-users.tsx`, `DESIGN.md`	Adds `Kicker`, `Data`, `DetailRow`, and `orDash`; updates table scrolling, navigation, surfaces, toasts, admin layout, and design guidance.
UI adoption and page actions `app/(app)/*/.tsx`	Inventory values and detail rows use shared typography, and accessory, ammo, and magazine actions move into `PageHeader`.
Grant revocation confirmation `app/(app)/grants/grants-list.tsx`, `app/(app)/grants/share-control.tsx`	Revocation opens a confirmation dialog before calling the asynchronous revoke handler; modal headings use `Kicker`.

Backup recovery

Layer / File(s)	Summary
Timestamped pre-restore directories `src/backup/restore-service.ts`, `src/backup/maintenance.ts`, `src/backup/__tests__/maintenance.test.ts`	Pre-restore directory names include epoch timestamps, and recovery sorts stamped directories before using modification times as fallback.

Possibly related PRs

unclesp1d3r/mag_stacker#13: Related grants revocation and share-control flow.
unclesp1d3r/mag_stacker#25: Related CI and E2E Postgres image pre-pull workflow.
unclesp1d3r/mag_stacker#69: Related backup recovery changes in the same maintenance and restore modules.

Suggested labels: bug, frontend, testing, infrastructure, priority:medium

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 41.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	Uses a valid conventional-commits format and accurately summarizes the mobile overflow fix plus Testcontainers test setup.
Description check	✅ Passed	The description is detailed and covers summary, changes, and test plan, but it doesn't follow the repo's template headings.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

Create stacked PR
Commit on current branch

✨ Simplify code

Create PR with simplified code
Commit simplified code in branch fix/mobile-nav-overflow

Warning

Review ran into problems

🔥 Problems

These MCP integrations need to be re-authenticated in the Integrations settings: Notion

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

Actionable comments posted: 6

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@app/`(app)/grants/grants-list.tsx:
- Around line 65-81: Update the confirmation flow around ConfirmDialog and
ShareControl so cancelling revocation with Escape does not close the parent
sharing modal. Coordinate the confirming state with ShareControl and suppress
its window Escape handler while confirming is non-null, allowing Escape to only
call setConfirming(null); preserve the existing behavior for other modal states.

In `@scripts/seed-demo.ts`:
- Around line 55-62: Update hasInventory to check every seeded inventory table,
including firearms, ammo, magazines, and unmounted accessories, rather than
querying only firearm. Return true when any table contains a row for ownerId, so
normal seeding skips existing inventory and --reset clears all inventory types.
- Around line 127-130: Update the seed-demo status messages and thrown errors
around the seeding flow, including the code covering the final summary and lines
133-171, to remove target account email values. Replace them with
account-neutral structured JSON logging/events while preserving the existing
success and failure information and seeding behavior.

In `@src/demo/inventory.ts`:
- Around line 203-205: Update the generated label construction in the inventory
seed mapping around label and labelPrefix so labels remain valid for Magpul-mode
owners: use a Magpul-compliant format that preserves the prefix and sequence
without truncating or stripping characters. Ensure the resulting labels are
accepted by createMagazine() for every generated prefix.

In `@src/domain/reference/__tests__/reference.test.ts`:
- Line 128: Update the documentation headings associated with the
distinctCalibers and other unconditional test suites to describe the
preload-managed Testcontainers database instead of claiming tests are skipped
without DATABASE_URL. Remove the stale DATABASE_URL gate references at all
indicated locations while leaving test behavior unchanged.

In `@src/test-support/preload.ts`:
- Around line 58-70: Update the SIGINT and SIGTERM handlers around stop to
explicitly terminate the process after initiating the best-effort container
cleanup, using an appropriate exit status. Keep the process.on("exit", stop)
handler and its synchronous best-effort semantics unchanged, while ensuring
repeated signals remain guarded by stopped.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: ff2d12c3-750d-498c-8326-2deace45f9ce

📥 Commits

Reviewing files that changed from the base of the PR and between 0402d32 and 6239b4d.

📒 Files selected for processing (76)

.github/actions/pre-pull-postgres/action.yml
.github/workflows/ci.yml
AGENTS.md
app/(admin)/users/admin-users.tsx
app/(app)/accessories/accessories-view.tsx
app/(app)/accessories/accessory-detail-view.tsx
app/(app)/ammo/ammo-detail-view.tsx
app/(app)/ammo/ammo-view.tsx
app/(app)/app-shell.tsx
app/(app)/firearms/[id]/firearm-documents.tsx
app/(app)/firearms/[id]/firearm-photos.tsx
app/(app)/firearms/firearm-detail-view.tsx
app/(app)/firearms/firearms-view.tsx
app/(app)/firearms/mounted-accessories.tsx
app/(app)/firearms/range-session-history.tsx
app/(app)/grants/grants-list.tsx
app/(app)/grants/share-control.tsx
app/(app)/inventory-log/inventory-log-history.tsx
app/(app)/magazines/magazine-detail-view.tsx
app/(app)/magazines/magazines-view.tsx
app/(app)/summary/summary-tables.tsx
app/globals.css
bunfig.toml
components/ui/data-table/data-table.tsx
components/ui/detail-row.tsx
components/ui/surface.tsx
components/ui/toast.tsx
components/ui/typography.tsx
e2e/fixtures/demo-seed.ts
e2e/start-test-server.ts
justfile
package.json
scripts/seed-demo.ts
src/auth/__tests__/accessory-visibility.test.ts
src/auth/__tests__/authorize.test.ts
src/auth/__tests__/gating.test.ts
src/auth/__tests__/grants.test.ts
src/auth/__tests__/visibility-ammo.test.ts
src/auth/__tests__/visibility.test.ts
src/backup/__tests__/db-roundtrip.test.ts
src/backup/__tests__/export-service.test.ts
src/backup/__tests__/maintenance.test.ts
src/backup/__tests__/restore-service.test.ts
src/backup/__tests__/routes.test.ts
src/backup/__tests__/write-path-maintenance-guard.test.ts
src/db/__tests__/client.test.ts
src/db/__tests__/firearm-document.test.ts
src/db/__tests__/health.test.ts
src/db/__tests__/idempotency.test.ts
src/db/__tests__/migrate-exit-code.test.ts
src/db/__tests__/schema.test.ts
src/demo/inventory.ts
src/domain/accessories/__tests__/service.test.ts
src/domain/ammo/__tests__/service.test.ts
src/domain/bulkadd/__tests__/service.test.ts
src/domain/csv/__tests__/ammo-build.test.ts
src/domain/csv/__tests__/build.test.ts
src/domain/firearm-documents/__tests__/service.test.ts
src/domain/firearm-documents/__tests__/serving.test.ts
src/domain/firearm-photos/__tests__/service.test.ts
src/domain/firearm-photos/__tests__/serving.test.ts
src/domain/firearms/__tests__/service.test.ts
src/domain/inventory-log/__tests__/last-inventoried.test.ts
src/domain/inventory-log/__tests__/service.test.ts
src/domain/magazines/__tests__/authorize-owner-only.test.ts
src/domain/magazines/__tests__/compatibility.test.ts
src/domain/magazines/__tests__/filter.test.ts
src/domain/magazines/__tests__/prefixes.test.ts
src/domain/magazines/__tests__/service.test.ts
src/domain/range-sessions/__tests__/service.test.ts
src/domain/reference/__tests__/reference.test.ts
src/domain/summary/__tests__/summary.test.ts
src/storage/__tests__/document-blobs.test.ts
src/storage/__tests__/orphan-sweep.test.ts
src/test-support/postgres-image.ts
src/test-support/preload.ts

Four near-identical CRUD surfaces placed their primary action two different ways: firearms put it in PageHeader.actions, while magazines, ammo and accessories each floated it in a `flex justify-end` row below the header rule. At phone width that reads as an unanchored control stranded between the page title and the filter panel. The fix is structural rather than cosmetic. Those three declared PageHeader in their page.tsx while the button needs the view's client state, so the header moves into the view — the arrangement firearms already used. Export and Add now sit together as page actions, with Add carrying the primary emphasis. Also drops the `ReactNode` imports the four detail views stopped needing when DetailRow and orDash moved to components/ui/detail-row.tsx. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>

The Dosu sync updated the Stat component entry but left two prose references to the anodized tick-mark in the north-star and Don't sections. The mark no longer exists — it was a decorative accent on a background panel, which the One Accent Rule forbids — so the identity prose now leans on what actually carries it: the lit state, the tabular figures, and the mono voice. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>

Interrupted-restore recovery picks the newest of several moved-aside pre-restore blob directories and deletes the rest. It ordered them by mtime, which cannot answer that question: - `rename` does not update a directory's own mtime, so a moved-aside directory keeps whatever mtime `uploadDir` had — when blobs were last written, not when the swap happened. Two directories can therefore be ordered backwards outright. - The names carried only `randomUUID()`, so equal mtimes fell through to readdir order, which is filesystem-dependent. Picking wrong means recovery restores stale blobs and destroys the real pre-restore ones. `beginBlobSwap` now stamps the creation time into the name (`.pre-restore-<epochMs>-<uuid>`) and recovery orders on that, falling back to mtime only for directories left by an older build. Surfaced by CI: this test had been skipping (it needs a database), and once it ran it failed on a fast runner where two mkdir calls land in the same tick. The test no longer races the clock — it sets the stamps explicitly and forces the STALE directory to have the NEWEST mtime, so ordering by mtime would now pick the wrong one and only the stamp gets it right. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/backup/restore-service.ts`:
- Line 459: Make pre-restore swap ordering collision-safe: in
src/backup/restore-service.ts lines 459-459, generate a unique token that
preserves creation order for swaps occurring within the same millisecond; in
src/backup/maintenance.ts lines 447-452, parse and compare that token so
recovery candidates are ordered deterministically without relying on filesystem
enumeration. Add a regression test covering two directories with the same
epoch-millisecond timestamp.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: 2d2fe328-ad59-4a4d-adce-e8d461d18e7a

📥 Commits

Reviewing files that changed from the base of the PR and between b1e3cd4 and 8d2599e.

📒 Files selected for processing (3)

src/backup/__tests__/maintenance.test.ts
src/backup/maintenance.ts
src/backup/restore-service.ts

Bugs, highest severity first: - `src/demo/inventory.ts`: every generated magazine label (`AR-01`, `P320-01`) exceeded MAX_LABEL_LENGTH (4), so `just db-seed` threw magpulLabelTooLong for any owner with Magpul mode on, part-way through seeding. It only looked fine because the seeded admin defaults to magpulMode: false. Two-character prefixes plus two digits now spend the budget exactly, and `bulkMagazines()` throws at authoring time if a line ever exceeds it. - `scripts/seed-demo.ts`: `resetInventory` ran five auto-committing deletes, so a mid-sequence failure left a half-wiped account — and because `hasInventory` probed only the `firearm` table, the next run reported "already has inventory; nothing to do" over data the failed run had destroyed. Now one transaction, and the probe covers every owned table. - `src/backup/maintenance.ts`: pre-restore blob recovery deleted the live upload directory before confirming the replacement existed, and swallowed every fs error rather than just ENOENT — leaving the database rolled back while the blob store held the half-promoted state, logged as a routine step. Existence is now checked first, only ENOENT is tolerated, and that divergence gets the same MANUAL INTERVENTION treatment as a failed DB rollback. (Pre-existing on main; fixed here because this PR reworked the same function.) - `app/(app)/grants/grants-list.tsx`: the revoke confirmation dismissed itself synchronously, before the async revoke had even been sent — so its own pending/"Revoking…" state was unreachable and a failure surfaced later as a banner with nothing tying it to the action. `onRevoke` now reports its outcome and the dialog stays open on failure. Escape no longer closes the parent share modal while the confirmation is open. - `src/test-support/preload.ts`: registering SIGINT/SIGTERM handlers replaces the default terminate-on-signal behavior, so Ctrl-C left the run hanging; the handler now exits. Dropped the `process.on("exit")` handler, which could never await an async container stop and only read as cleanup. Container-start and migration failures now say what went wrong. Test isolation — two more instances of the bug class this PR already fixed once: `csv/build.test.ts` and `csv/ammo-build.test.ts` asserted "empty inventory" over a shared viewer that a later test in the same file grants access to. Each now owns its viewer. New coverage: - `e2e/responsive-overflow.spec.ts` — the mobile P0 had no automated guard at all. Asserts no route scrolls horizontally at 320/390 against a POPULATED table, that the pages actually rendered, and that the table and nav still scroll in place. Uses a plain viewport deliberately: isMobile:true masks this entire bug class. - `src/demo/__tests__/inventory.test.ts` — holds the demo labels to the same Magpul rule the domain enforces; would have caught the bug above outright. - A maintenance test for the legacy unstamped pre-restore directory path. Types: firearm `type`/`action` use the FirearmType/FirearmAction unions and accessory `mount` is keyed to the declared firearm names, so a typo in this fixture data fails typecheck instead of mid-seed. Named DetailRowProps; extracted `pruneRank` so the directory comparator is two branches instead of four; bounded the stamp regex so an over-long digit run cannot silently lose precision. Comments: removed a stale skip-gate comment that survived the degating, corrected the image-bump instructions (CI reads the constant now, so there is nothing to keep in lockstep), dropped an orphaned comment in the e2e launcher, and fixed stale DATABASE_URL prose in four test files. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>

coderabbitai

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@scripts/seed-demo.ts`:
- Around line 43-99: The seed-demo logging paths for account-not-found, existing
inventory, and clearing inventory must not interpolate the target email. Update
those messages to use a redacted identifier or generic account wording, while
preserving the existing behavior and context of the logs.

In `@src/backup/maintenance.ts`:
- Around line 624-632: Update the blob-recovery catch around the existing
log.error call to set a recovery-failed flag, then return before snapshot/schema
cleanup, maintenance-mode exit, and housekeeping sweeps. Preserve the recovery
snapshot and maintenance mode whenever restoring pre-restore blobs fails, while
keeping normal cleanup for successful recovery.
- Around line 409-417: Update the stamp extraction logic in the
candidate-directory handling to parse the suffix from entry.name immediately
after the known prefix, rather than matching the full path. Preserve the
safe-integer validation and undefined behavior for non-matches, and add a
regression case covering a pre-restore-like parent path so unrelated path
segments cannot determine the candidate rank.

In `@src/test-support/preload.ts`:
- Around line 76-97: Update the stopAndExit signal handling to preserve a
non-zero interrupt status instead of always calling process.exit(0). Track the
received signal from the SIGINT and SIGTERM listeners and exit with the
conventional 128 + signal code after container.stop() settles, while retaining
the stopping guard and cleanup behavior.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: 7b5fb977-93c6-4f14-95f6-421aab905f91

📥 Commits

Reviewing files that changed from the base of the PR and between 8d2599e and 862e0da.

📒 Files selected for processing (19)

app/(app)/grants/grants-list.tsx
app/(app)/grants/share-control.tsx
components/ui/detail-row.tsx
components/ui/surface.tsx
e2e/fixtures/user-pool.ts
e2e/responsive-overflow.spec.ts
e2e/start-test-server.ts
scripts/seed-demo.ts
src/auth/__tests__/gating.test.ts
src/backup/__tests__/maintenance.test.ts
src/backup/maintenance.ts
src/db/__tests__/migrate-exit-code.test.ts
src/demo/__tests__/inventory.test.ts
src/demo/inventory.ts
src/domain/csv/__tests__/ammo-build.test.ts
src/domain/csv/__tests__/build.test.ts
src/domain/reference/__tests__/reference.test.ts
src/test-support/postgres-image.ts
src/test-support/preload.ts

💤 Files with no reviewable changes (1)

e2e/start-test-server.ts

🚧 Files skipped from review as they are similar to previous changes (4)

components/ui/detail-row.tsx
src/db/tests/migrate-exit-code.test.ts
src/auth/tests/gating.test.ts
components/ui/surface.tsx

Second round of PR review findings. - Blob-restore failure now escalates like a failed DB rollback: it rethrows into the handler that keeps the maintenance flag ACTIVE and preserves the snapshot schema. Logging loudly and then dropping the snapshot and clearing the flag — which is what the previous fix did — told the operator to reconcile by hand while deleting the thing they would reconcile from, and readmitted ordinary writes on top of a database and blob store that describe different states. - `preRestoreStamp` parses the stamp from the candidate directory's own name, anchored after the known prefix. Searching the whole path could match a `.pre-restore-<digits>-` segment in an ancestor directory instead, giving every candidate the same rank and handing the choice back to readdir order — the exact failure the stamp exists to prevent. Covered by a new test whose upload directory sits under a deliberately pre-restore-looking parent. - `src/test-support/preload.ts`: interrupts exit `128 + signal` rather than 0. bun test propagates a preload's exit code as the whole run's status, so Ctrl-C was reporting an interrupted run as a passing one. - Seed scripts no longer interpolate the target account's email into their output. Applied to `seed-admin.ts` as well, which established the pattern — otherwise one script would redact and its sibling would not. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>

`Date.now()` can return the same value for two consecutive swaps. Equal stamps compare equal, so `pruneRank` gave both candidates the same key and the ordering fell back to readdir order — the exact failure the stamp was added to prevent. `nextPreRestoreStamp()` clamps to `previous + 1`, making the sequence strictly increasing within a process regardless of clock resolution, without changing the name format or growing past 13 digits. It lives in maintenance.ts beside the parser so the format has a single owner, and is unit-tested directly: 50 back-to-back calls must be unique, ordered, and still epoch-ms width. Across restarts ordering still relies on the wall clock advancing, which holds here because a restore holds an app-wide advisory lock for its whole body, so swaps never interleave between processes. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>

unclesp1d3r added 5 commits July 29, 2026 08:43

coderabbitai Bot added bug Something isn't working frontend infrastructure priority:medium testing labels Jul 29, 2026

docs: Dosu updates for PR #88

b67e2b0

coderabbitai Bot reviewed Jul 29, 2026

View reviewed changes

Comment thread app/(app)/grants/grants-list.tsx

Comment thread scripts/seed-demo.ts

Comment thread scripts/seed-demo.ts

Comment thread src/demo/inventory.ts Outdated

Comment thread src/domain/reference/__tests__/reference.test.ts

Comment thread src/test-support/preload.ts Outdated

coderabbitai Bot added the documentation Improvements or additions to documentation label Jul 29, 2026

unclesp1d3r added 2 commits July 29, 2026 09:05

coderabbitai Bot added the shared label Jul 29, 2026

coderabbitai Bot removed documentation Improvements or additions to documentation shared labels Jul 29, 2026

coderabbitai Bot reviewed Jul 29, 2026

View reviewed changes

Comment thread src/backup/restore-service.ts Outdated

coderabbitai Bot reviewed Jul 30, 2026

View reviewed changes

Comment thread scripts/seed-demo.ts

Comment thread src/backup/maintenance.ts Outdated

Comment thread src/backup/maintenance.ts Outdated

Comment thread src/test-support/preload.ts Outdated

unclesp1d3r added 2 commits July 29, 2026 21:33

unclesp1d3r merged commit 7de22ab into main Jul 31, 2026
7 checks passed

unclesp1d3r deleted the fix/mobile-nav-overflow branch July 31, 2026 01:41

This was referenced Aug 2, 2026

feat(magazines): render magazine labels as Magpul dot-matrix glyphs #90

Merged

feat(accessories): track serialized accessories with type, compatibility, attachments, and sharing #106

Merged

feat(firearms): mark a firearm as not magazine-fed #108

Merged

coderabbitai Bot mentioned this pull request Aug 11, 2026

Flaky e2e: accessories-serialized spec fails under full parallel load, passes in isolation #113

Open

Navigation Menu

Uh oh!

fix(mobile): stop sr-only labels widening every route; move integration tests to Testcontainers - #88

fix(mobile): stop sr-only labels widening every route; move integration tests to Testcontainers#88
unclesp1d3r merged 12 commits into
mainfrom
fix/mobile-nav-overflow

unclesp1d3r commented Jul 29, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Jul 29, 2026 •

edited

Loading

Reviews paused

❌ Failed checks (1 warning)

Review ran into problems

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

unclesp1d3r commented Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1. Mobile: the app was unusable at phone width

2. Tests: the integration suite was silently skipping

3. Design system: the rules now enforce themselves

4. just db-seed

6. Interrupted-restore recovery could destroy the wrong blobs

Review findings addressed

Test plan

5. "Add" CTA placement standardized

Uh oh!

coderabbitai Bot commented Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews paused

Walkthrough

Changes

❌ Failed checks (1 warning)

Review ran into problems

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

unclesp1d3r commented Jul 29, 2026 •

edited

Loading

4. `just db-seed`

coderabbitai Bot commented Jul 29, 2026 •

edited

Loading