feat(accessories): track serialized accessories with type, compatibility, attachments, and sharing by unclesp1d3r · Pull Request #106 · unclesp1d3r/mag_stacker

unclesp1d3r · 2026-08-06T03:07:40Z

Summary

An owner can now treat a suppressor the way they actually own one: a serialized item that declares which of their hosts it fits, carries its own mounts and pistons, and can be lent to someone else on its own — even while it sits unmounted in the safe.

That last part reverses a decision #8 shipped deliberately. Under the old model an accessory had no shares of its own and was visible only through the firearm it happened to be attached to, so an unmounted can was invisible to everyone but its owner. Exactly backwards for the item most likely to be lent, co-owned through a trust, or shown to an armorer.

Issue #23 says the schema "has no home for them" and that grant.parent_type is in (firearm, magazine). Both were stale — #8 already shipped the accessory parent with serial_number and is_nfa, and ammo was already a grant type. Three of the four asks were genuinely new; the parent record was not. This evolves the shipped entity rather than adding a parallel suppressor one.

How visibility resolves now

An accessory is reachable three ways, and the requester holds the strongest permission any path grants:

Path	Added by	Notes
Owned	#8	Always wins
Granted directly	#23	Works while unmounted — the point of the change
Mounted on a visible firearm	#8	Retained, so nobody loses access they had

Keeping the third path is why this is additive rather than a swap, and there is an explicit no-regression test for it.

Design decisions

type and category both stay, with different jobs. type is the controlled, CHECK-backed discriminator that decides which subtype's rules apply and is what future per-type detail tables (optics) will key off. category stays free text and becomes optional, because #8 was right that the long tail — bipod, red dot mount — must not be forced into an enum. Collapsing them would have flattened real values to other.

"Fits" and "mounted on" are different facts. A can fits five hosts and is mounted on at most one. The existing current_firearm_id keeps its meaning (and the installed_date and range-session snapshots that depend on it); the new accessory_firearm join carries capability. Tests assert neither write path disturbs the other, in both directions.

Delete is gated more tightly than edit. Every other grantable type routes delete through an owner-only gate. #8 nonetheless shipped delete-by-firearm-edit-grantee for accessories, so both rules hold: the inherited path keeps what it had, and the grant path this PR adds does not confer deletion.

The compatibility core is shared, not copied. magazine_firearm and accessory_firearm express the same relation, and the duplicated block contained the visibility gate that refuses to link a firearm the actor cannot see. Two hand-maintained copies means two places an authorization fix must land. The magazine suite passes untouched, which is the evidence the extraction preserved behavior.

That shared core immediately earned its keep. Writes are now viewer-relative too, not just reads. Compatibility reads drop firearms outside the reader's visible set, so the list any editor submits was necessarily built from a filtered view — an editor holding a grant on the accessory but not on one of its hosts is handed a short list and has no way to know it. The original delete-all-then-reinsert read that unavoidable omission as a deletion and silently destroyed links the actor was never shown, with no error and nothing to tell the owner. The replace is now bounded by the actor's visible set: rows outside it survive untouched and keep their ordinals, and the replaced slice is appended after them. "Omission clears" still holds, but only over what the actor could actually see.

Because it landed in the core, magazines are fixed too — they had the same silent loss on a narrower trigger, when a magazine was linked to a cross-owner firearm whose grant was later revoked. This changes shipped magazine save behavior, but as a data-loss fix rather than an interface change, so it is not marked breaking.

Migration 0022 — read before deploying

The one statement here that is not ordinary DDL is a hand-written backfill that maps existing free-text category onto the new type. It runs once against real owner data and cannot be re-run.

Know what the accessory_type_valid CHECK does and does not protect: its value list is hand-copied from the same constant the backfill's WHERE clause is, so it validates set membership, not mapping correctness. A value that drifts out of the set aborts the whole migration — fail-safe. A value mapped to the wrong member of the set commits silently. Diff SELECT lower(trim(category)), count(*) FROM accessory GROUP BY 1 either side of the deploy to confirm.

Recovery is a forward fix, not a rollback: category is never rewritten, so the source data survives a bad mapping. That property has its own test.

Validation

just ci-check green: 1254 unit/integration tests, 51 Playwright e2e.
The viewer-relative write fix carries two regression tests, each proven to fail without it: one at the core level driving both the magazine and accessory bindings, one end-to-end through updateAccessory as an edit grantee saving an unrelated field.
The backfill is covered by a test that stands up its own Postgres, migrates through 0021 only, seeds real pre-migration rows, then applies the shipped 0022 — so the mapping runs against data, not fixtures-of-convenience.
An 11-reviewer pass produced 4 actionable findings (2 P1 authorization, 1 P1 + 1 P2 performance, 1 P3), no P0s. All fixed in this branch; none deferred. Two were real bugs: a type-only accessory rendered a blank link on the firearm detail page, and an accessory-only editor could detach an accessory from a firearm they cannot see.
The scripted browser-test step did not run — its sanctioned driver is not installed locally, and substituting another browser stack is disallowed. The Playwright suite covers the same routes end-to-end.
Two e2e type assertions were de-flaked while verifying the above. The submit button relabels itself to "Saving…" while the action is in flight, so waiting for "Save changes" to disappear went true during the save; the wait now keys on the Edit button returning, then reloads, which also makes the assertion about the persisted row rather than the optimistic UI.
just ci-check had been failing in a different subset of src/backup on each run while every one of those files passed in isolation. Root cause was bun's 5s default timeout against suites that start their own Postgres: a timeout is not contained, so afterAll ends the pool underneath the still-running body and the rest of the file cascades into "Cannot use a pool after calling end on the pool", naming the wrong tests. Fixed with a shared containerTest helper imported aliased so all 78 call sites stay byte-identical. This was noise I dismissed as flaky infrastructure twice before running it down.

Session-settled decisions carried from planning: evolve the shipped accessory with a type discriminator rather than adding a suppressor table (user-directed, over a dedicated table); accessories become independently shareable while retaining inherited visibility (user-approved, over inherit-only); category survives as optional free text alongside type (user-approved, over dropping it); all nine units land in one PR (user-approved, over splitting by layer).

Scope note: this branch also carries 6b76c9f, a Mermaid VS Code tooling doc unrelated to #23 (.github/copilot-instructions.md plus .github/instructions/mermaid.instructions.md, 92 added lines, no runtime effect). Since the repo squash-merges, it folds into this commit. Keeping it — it is additive editor tooling that nothing depends on, and rewriting the branch to excise 92 lines of docs costs more than it returns. Called out here so the squashed commit is honest about its contents rather than silently carrying them.

Deferred, with an issue rather than a residual: CodeRabbit flagged a CWE-367 TOCTOU window between the permission check and the write, then agreed it is not this PR's — the pattern predates #23 and spans every grant-gated mutation. Filed as #107 with the three options and a recommendation, so the finding outlives the review thread.

New concepts

Strongest-path permission resolution

What it is. When an item is reachable through more than one independent grant path, the resolver evaluates every path and takes the highest-ranked result, instead of returning the first path that matches.

Why here. The obvious alternative — check paths in a fixed order and return the first hit — silently downgrades. A user with a view grant on the accessory and edit on the firearm it is mounted to would land on view purely because that path was checked first. Ordering would become load-bearing, and the bug it produces is invisible until someone complains they cannot edit something they own access to.

// Rank so "strongest wins" is a comparison, not a chain of ifs whose
// order silently decides the answer.
const PERMISSION_RANK: Record<Permission, number> = { view: 1, edit: 2, owner: 3 };

const [direct, inherited] = await Promise.all([...]);
return strongest(direct, inherited);

When not to use it. Only when paths are genuinely independent grants of the same right. If one path is a restriction rather than a grant — a deny rule, a maintenance-mode freeze, a legal hold — the strongest-wins union is exactly wrong, and the restriction has to gate the whole resolution instead.

Fixes #23

Plan for issue #23. Corrects the issue's stale premise: #8 already shipped the `accessory` parent with serial_number/is_nfa, and grant.parent_type already included 'ammo' — so this evolves the shipped entity rather than adding a parallel `suppressor` one. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>

…I (U1, U5) Adds `accessory.type`, a controlled, CHECK-backed structural discriminator, and relaxes the free-text `category` from required to optional. The two classifications coexist deliberately (#23 KD4): `type` answers "which subtype's rules apply" and is the seam future per-type detail tables key off; `category` answers "what does the owner call it" and keeps #8's long tail (bipod, red dot mount) that an enum would reject. Migration 0022 is generated then hand-edited in two places SQL can express but drizzle cannot generate: the one-shot backfill that maps existing categories onto `type` case-insensitively, and the accessory grant-cleanup trigger. The backfill never rewrites `category`, so an unmapped value keeps its original string and simply lands on `other` — lossless, and safe to revisit later with a better mapping. 0022 also carries the `accessory_firearm` and `accessory_attachment` tables and the widened `grant_parent_type_valid` CHECK, so U2/U3/U4 add domain code against a schema that is already in place (KTD8: one migration). Covered by a migration test that stands up its own Postgres, migrates through 0021 only, seeds real pre-migration rows, and then applies the shipped 0022 — the mapping runs exactly once in production and cannot be re-run to fix a bad result. `accessoryDisplayName` gains a type-label fallback and the list link renders it: with `category` now optional, both would otherwise render empty, leaving a row link with no accessible name. Registers both new tables in the backup export order — the round-trip regression guard caught their absence, which would have silently dropped them from every backup. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>

Adds the accessory ↔ firearm compatibility relation: "which hosts does this fit", many-to-many and ordered, alongside the mount `current_firearm_id` already records. Both edges persist deliberately (#23 KD2) — a suppressor fits five hosts while mounted on at most one — and the R6 tests assert the independence in both directions so a later contributor cannot collapse one into the other without a red test. #23 asked for this to reuse the magazine implementation rather than duplicate it. Rather than porting by copy, the shared invariants now live in `src/domain/compatibility/relation.ts` and both parents bind to it. The deciding factor was not line count: the duplicated block contains the visibility gate that refuses to link a firearm the actor cannot see. Two hand-maintained copies means two places an authorization fix has to land, and the copy that gets missed is the one that leaks. Only the insert row shape differs per parent, since drizzle's `.values()` is keyed by model property name and cannot be derived from a column reference. The magazine suite passes untouched, which is the evidence that the extraction preserved behavior. The form's firearm picker is lifted to `components/inventory/compatible-firearms-field.tsx` and shared the same way — the plan left "can the picker be lifted cheaply?" open, and it is: the control was already a self-contained fieldset. Compatibility offers every VISIBLE firearm, deliberately wider than the mount picker's editable set. Declaring a fit is a statement about the accessory, not a write to the firearm; mounting stays narrower because it does write that relationship and carries a cross-tenant guard. The detail view lists what it fits for every viewer including a view-only grantee (R15/R17); it is editable only through the edit form, which a viewer is never offered, so no separate read-only picker is needed. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>

Adds `accessory_attachment` — the mounts, pistons, end caps and muzzle devices that make a serialized accessory fit a host — plus its service and the detail-view panel that manages it. Attachments are child records in the `firearm_photo` mold (#23 KTD7): no `owner_id`, no grants of their own. Every read and write authorizes through the parent accessory, which is what guarantees an attachment can never be reachable by someone who cannot reach the accessory — including once U4 makes accessories independently shareable, since the parent gate is the one place that decision has to land. The parent-permission check moves to `requireAccessoryEdit` / `requireAccessoryView` in the auth layer, shared with the accessory service rather than re-implemented here. The three-way outcome is the reason it is shared: a `view` holder is visible-but-forbidden (403) while anything outside the visible set must be indistinguishable from absent (404), so the response cannot be used to probe for accessories the requester should not know exist. It is also the single place `assertWritesAllowed` is enforced, so a new mutating path cannot forget the maintenance-mode gate. Recording an attachment deliberately does not touch compatibility. #23 notes a piston is what makes a can fit a host, but DERIVING compatibility from one is an explicit scope boundary, so nothing here writes to `accessory_firearm`. Every control is reachable by role and accessible name — no `data-testid` (R18). A viewer gets the list and no mutating affordance; that is presentation only, and the domain-layer tests assert the real gate rejects a viewer's writes with 403 and a stranger's with 404. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>

…, U8, U9) Reverses #8's deliberate decision that an accessory carries no grants of its own. Under that model an unmounted suppressor was invisible to everyone but its owner — exactly backwards for the item most likely to be lent, co-owned through a trust, or shown to an armorer. The reversal is additive, not a replacement. An accessory is now reachable by three paths: owned ∪ directly granted ∪ mounted on a firearm the requester can see and the effective permission is the strongest any path grants, with ownership always winning. The inherited path is RETAINED specifically so nobody who can see a mounted accessory today silently loses that access; AE3 is the no-regression test and is the one most worth keeping. `ParentType` gains its `accessory` arm in `visibility.ts`, and the mount-inheritance half stays in `accessory-visibility.ts` — pushing `current_firearm_id` semantics into the generic auth layer would make one parent type special (KTD6). Widening the union turned out to need no other edit: `parentTable` was the only exhaustive dispatch, so a clean typecheck really was the completeness signal KTD5 predicted. Written test-first per the unit's execution note: the sharing spec was red on 12 cases before `visibility.ts` was touched. Service-rule configuration is deliberately NOT opened up. #23 made accessories shareable for inventory visibility; `rules-service.ts` still requires direct ownership, and its comment now says why rather than citing the no-longer-true "accessories aren't a ParentType". The e2e journey surfaced a property worth pinning down: compatibility reads are viewer-relative, so a grantee sees only the hosts also shared with them. Sharing an accessory must not disclose the identity of firearms the grantee has no access to, so the spec now shares exactly one of three hosts and asserts the other two stay hidden. CONCEPTS.md gains Accessory Type, Accessory Compatibility, and Attachment — plus an Accessory entry, which #8 never added at all — and the Grant, Child record, and Compatibility entries are corrected where #23 made them stale. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>

…on and editing Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>

…ession found in review Eleven-reviewer pass over the branch. Four actionable findings, no P0s; all fixed in-branch rather than deferred. **A firearm's loadout could be changed by someone with no access to it.** `authorizeMount` only checked firearm permission when mounting ONTO a firearm. That was safe by construction until this branch: previously the only way to hold `edit` on an accessory was to inherit it from the firearm it was mounted to, so passing the accessory gate implied permission on that firearm. A direct accessory grant breaks the implication — an accessory-only editor could detach it from a gun they cannot even see. Now guarded on the detach side too, with tests proving owners and firearm edit-grantees are unaffected. **An edit grant conferred deletion.** Every other grantable parent type routes delete through an owner-only gate; sharing at `edit` has never meant "may destroy". #8 nonetheless shipped delete-by-firearm-edit-grantee for accessories, so the two rules are reconciled rather than one overriding the other: `requireAccessoryDelete` keeps #8's inherited path exactly as shipped and applies the repo-wide owner-only rule to the grant path this branch adds. (The reviewer reported this as newly introduced; `main` shows the inherited half predates it. Only the direct-grant half is new.) That fix then made the UI lie — the detail view still offered Delete to any editor. An action refused after the click is worse than one never offered, so `canDelete` is now resolved server-side from data already in hand. My own new e2e test caught it. **Type-only accessories rendered a blank link on the firearm detail page.** A third hand-written copy of the label fallback still ended at `category`, which this branch made optional. Deleted the copy rather than patching it: all three surfaces now share `accessoryDisplayName`, which finally has a test. **Performance.** "Owned union granted firearms" was resolved three times per accessories page load. It is now derived once from the permission map already being fetched — its keys are that set — and threaded through. Also corrected two comments that claimed more than the code delivers: the migration's CHECK validates set MEMBERSHIP, not mapping CORRECTNESS (its value list is hand-copied from the same constant the backfill is, so it cannot independently catch a wrong-but-valid mapping), and the parallel permission lookups only genuinely overlap on the pool, not on a transaction. New coverage for the gaps reviewers named: the label fallback, the delete policy, update-rollback on a rejected compatibility id, the documented compatibility-clearing footgun, service-rule config staying owner-only under a direct grant, magazine/accessory parity through the shared compatibility core, and e2e for the edit paths and edit-grant sharing. One timezone-fragile date comparison fixed per docs/solutions. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>

coderabbitai · 2026-08-06T03:08:15Z

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

Added serialized accessory tracking with controlled types, optional categories, firearm compatibility, attachment CRUD, independent sharing, and visibility-aware Next.js App Router UI.
Added Drizzle/Postgres migration 0022_confused_lockheed with category backfill, constraints, grant support, cascade cleanup, and backup ordering. Existing records require this migration.
Extended authorization with strongest-permission resolution across direct grants and mounted-firearm visibility. Shared compatibility logic now supports accessories and magazines while preserving hidden links during updates.
Added Bun unit/integration, migration, Testing Library component, and Playwright E2E coverage. Reports show 1,254 unit/integration tests and 51 E2E tests passing. The scripted browser-test step was not run because its sanctioned driver was unavailable locally.
NFA-specific fields and document handling remain out of scope. Mermaid VS Code documentation and a shared timeout helper for container-backed tests were also added.

Walkthrough

This change adds typed serialized accessories with many-to-many firearm compatibility, child attachments, independent sharing, viewer-relative authorization, migration support, reusable compatibility UI, and end-to-end coverage.

Changes

Serialized accessories

Layer / File(s)	Summary
Schema and domain contracts `CONCEPTS.md`, `src/domain/accessories/`, `src/db/`, `src/backup/table-order.ts`, `src/demo/inventory.ts`	Accessories now require controlled types. Categories remain optional. The schema adds compatibility and attachment tables, grant support, cascade cleanup, and migration backfill.
Compatibility relations and shared picker `src/domain/compatibility/`, `src/domain/accessories/compatibility.ts`, `src/domain/magazines/compatibility.ts`, `components/inventory/compatible-firearms-field.tsx`, `src/domain/firearms/`	Compatibility replacement, ordering, visibility filtering, and batch loading use shared relation helpers. Accessories and magazines use the shared picker.
Authorization and domain services `src/auth/`, `src/domain/accessories/service.ts`, `src/domain/accessories/attachments.ts`, `app/(app)/grants/`	Accessory visibility combines ownership, direct grants, and mounted-firearm access. Edit, view, delete, mount, and attachment authorization use separate gates.
Accessory pages and attachment controls `app/(app)/accessories/*`, `app/(app)/firearms/mounted-accessories.tsx`	List and detail views show types, compatible firearms, attachments, sharing controls, and permission-specific actions.
Migration and domain validation coverage `src/db/__tests__/`, `src/domain/accessories/__tests__/`, `src/domain/compatibility/__tests__/`, `src/auth/__tests__/`	Tests cover migration backfill, constraints, validation, compatibility transactions, attachment CRUD, display labels, visibility, sharing, and deletion rules.
Integration fixtures and repository guidance `e2e/`, `scripts/seed-demo.ts`, `docs/plans/`, `.github/`, `src/test-support/`	Seed data and existing E2E flows provide accessory types. A serialized-accessory journey covers compatibility, attachments, sharing, viewer filtering, editing, and deletion cascades. Mermaid workflow guidance and container-test support are documented.

Sequence Diagram(s)

sequenceDiagram
  participant Owner
  participant AccessoryPage
  participant AccessoryService
  participant Compatibility
  participant Attachments
  participant GrantService
  Owner->>AccessoryPage: Create or edit accessory
  AccessoryPage->>AccessoryService: Submit typed accessory and compatible firearm IDs
  AccessoryService->>Compatibility: Replace compatibility links
  AccessoryPage->>Attachments: Create or update attachment
  Owner->>GrantService: Share accessory
  GrantService-->>AccessoryPage: Revalidate accessory views
  AccessoryPage-->>Owner: Render accessory, compatibility, attachments, and permissions

Possibly related PRs

unclesp1d3r/mag_stacker#54: Extends shared parent-type and grant handling for another inventory entity.
unclesp1d3r/mag_stacker#59: Introduces the accessory tracker extended here with types, compatibility, attachments, and independent sharing.
unclesp1d3r/mag_stacker#88: Shares the Testcontainers-based test support updated in this change.

Suggested labels: enhancement, backend, frontend, shared, testing, security, documentation, priority:medium

🚥 Pre-merge checks | ✅ 2 | ❌ 3

❌ Failed checks (3 warnings)

Check name	Status	Explanation	Resolution
Title check	⚠️ Warning	The title follows Conventional Commits, but the schema migration and exported API changes require `!`; the body also lacks the required exact BREAKING CHANGE footer.	Add `!` after the scope and include a `BREAKING CHANGE: <what breaks>` footer in the PR body.
Out of Scope Changes check	⚠️ Warning	The PR includes unrelated Mermaid tooling documentation and backup test timeout infrastructure that are outside issue `#23`.	Remove the unrelated Mermaid documentation and backup timeout changes, or split them into separate pull requests.
Docstring Coverage	⚠️ Warning	Docstring coverage is 63.10% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description check	✅ Passed	The description provides detailed scope, related issue, implementation changes, and validation results, but omits the explicit AI disclosure and checklist sections.
Linked Issues check	✅ Passed	The implementation satisfies issue `#23` with serialized accessories, compatibility, attachments, independent sharing, authorization, cascade behavior, UI support, and comprehensive tests.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

Create stacked PR
Commit on current branch

✨ Simplify code

Create PR with simplified code
Commit simplified code in branch 23-track-serialized-accessories-suppressors-serial-numbers-firearm-compatibility-and-attachments

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.}

…serial-numbers-firearm-compatibility-and-attachments

Copilot

Pull request overview

Adds first-class serialized accessories to MagStacker by evolving the existing accessory parent into a grantable, typed, compatibility-aware item with attachment child records, plus UI + migration + test coverage to support the new model (Fixes #23).

Changes:

Make accessory a grantable ParentType, resolving effective permission via strongest-path (owned ∪ direct grant ∪ mounted-on-visible-firearm).
Add controlled accessory.type, accessory_firearm compatibility (many-to-many, ordered), and accessory_attachment child records, with migration 0022 including a one-shot category→type backfill.
Share magazine/accessory compatibility invariants via a common relation module and add broad unit + migration + Playwright coverage (including a new serialized-accessories e2e journey).

Reviewed changes

Copilot reviewed 52 out of 52 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/test-support/factories.ts	Update service-rule parent-type commentary after accessory becomes grantable.
src/domain/validation-messages.ts	Replace category-required message with accessory/attachment type validation messages.
src/domain/service-intervals/rules-service.ts	Clarify that accessory service rules remain owner-only despite new sharing.
src/domain/range-sessions/tests/service.test.ts	Update fixtures to supply required accessory `type`.
src/domain/magazines/compatibility.ts	Rebind magazine compatibility to shared compatibility relation core.
src/domain/firearms/service.ts	Allow passing precomputed visible firearm ids to avoid duplicate visibility queries.
src/domain/firearms/mount-options.ts	Extend mount context with `visibleFirearms` options for compatibility picker.
src/domain/compatibility/relation.ts	New shared compatibility relation implementation (dedupe/visibility/atomic replace/viewer-relative read).
src/domain/compatibility/tests/relation.test.ts	New tests asserting magazine/accessory compatibility behavior stays identical.
src/domain/accessories/validate.ts	Make `type` required/validated; make `category` optional free text.
src/domain/accessories/service.ts	Add compatibility to accessory CRUD; tighten edit/delete gates; thread visible-firearm set for perf.
src/domain/accessories/display.ts	Ensure accessory display name falls back to type label when category is blank.
src/domain/accessories/constants.ts	Introduce controlled `ACCESSORY_TYPES` / `ATTACHMENT_TYPES` + label helpers.
src/domain/accessories/compatibility.ts	New accessory↔firearm compatibility binding using shared relation core.
src/domain/accessories/attachments.ts	New attachment CRUD/validation authorized through parent accessory.
src/domain/accessories/tests/validate.test.ts	Update validation tests for required `type` and optional `category`.
src/domain/accessories/tests/service.test.ts	Update service tests for new type requirement + sharing/delete rules.
src/domain/accessories/tests/display.test.ts	New tests pinning accessory display-name fallback chain.
src/domain/accessories/tests/compatibility.test.ts	New tests for accessory compatibility invariants + mount independence.
src/domain/accessories/tests/attachments.test.ts	New tests for attachment CRUD + auth-through-parent behavior.
src/demo/inventory.ts	Add `type` to demo accessory seed shape and fixtures.
src/db/migrations/meta/_journal.json	Register migration 0022.
src/db/migrations/0022_confused_lockheed.sql	New migration adding attachments/compatibility/type, grant parent type, backfill, and cleanup trigger.
src/db/inventory-schema.ts	Add accessory `type` + new tables + checks + grant parent_type expansion.
src/db/tests/migration-0022-accessory-type-backfill.test.ts	New integration test exercising 0022 backfill against real pre-migration rows.
src/backup/table-order.ts	Include new accessory child tables in backup export order.
src/auth/visibility.ts	Add `accessory` to `ParentType`; split `resolveGrantPermission` from ownership-aware `resolvePermission`.
src/auth/accessory-visibility.ts	Implement strongest-path accessory permission + new edit/delete/view gates + mount authorization fix.
src/auth/tests/accessory-sharing.test.ts	New tests covering direct grants, inherited path retention, strongest-permission, and delete policy.
scripts/seed-demo.ts	Pass accessory `type` when seeding demo data.
e2e/service-intervals-sharing.spec.ts	Update accessory creation flow to select Type.
e2e/fixtures/user-pool.ts	Add new e2e users for serialized accessories scenario.
e2e/fixtures/demo-seed.ts	Update e2e demo seed accessory creation to select Type.
e2e/accessories.spec.ts	Update accessories e2e flows to select Type.
e2e/accessories-serialized.spec.ts	New e2e journey covering serialized accessories: type, compatibility, attachments, sharing, cascade delete.
CONCEPTS.md	Document Accessory, Attachment, and Accessory Compatibility concepts + grant model update.
components/inventory/compatible-firearms-field.tsx	New shared checkbox picker + toggle helper for compatibility selection.
app/(app)/magazines/magazine-form.tsx	Reuse shared compatible-firearms component and toggle helper.
app/(app)/grants/share-control.tsx	Allow edit grants for accessories (still view-only for magazines).
app/(app)/grants/actions.ts	Revalidate `/accessories` on share/revoke.
app/(app)/firearms/mounted-accessories.tsx	Use `accessoryDisplayName` to avoid empty/unnamed accessory links.
app/(app)/accessories/page.tsx	Thread visible-firearm id set through page queries; pass type + visibleFirearms into view.
app/(app)/accessories/attachments-section.tsx	New client UI panel for attachment list + inline add/edit/delete.
app/(app)/accessories/actions.ts	Add server actions for attachment CRUD + path revalidation.
app/(app)/accessories/accessory-form.tsx	Add required Type select + compatibility picker integration.
app/(app)/accessories/accessory-detail-view.tsx	Add ShareControl, compatibility readout, attachments panel, and canDelete gating.
app/(app)/accessories/accessories-view.tsx	Add Type column + ShareControl for accessories; adjust row labels and a11y disambiguation.
app/(app)/accessories/[id]/page.tsx	Fetch attachments, compute canDelete, and thread visible-firearm ids through queries.
.github/instructions/mermaid.instructions.md	Add Mermaid diagram assistance instructions (tooling doc).
.github/copilot-instructions.md	Link Mermaid instructions into Copilot instructions.

Suppressed comments (1)

app/(app)/accessories/actions.ts:105

deleteAttachmentAction also trusts the caller-provided accessoryId for revalidatePath. Since deletion is keyed by attachmentId, a mismatched {attachmentId, accessoryId} can delete successfully but revalidate the wrong page, leaving the real accessory detail view stale.

Consider returning the parent accessoryId from deleteAttachment(...) (or returning it from the SQL DELETE ... RETURNING) so the action can revalidate the correct route without trusting client input.

export async function deleteAttachmentAction(
  attachmentId: string,
  accessoryId: string,
): Promise<ActionResult> {
  return withActionContext("accessories", async (userId) => {
    await deleteAttachment(userId, attachmentId);
    revalidatePath(`/accessories/${accessoryId}`);
    return { ok: true };

coderabbitai

Actionable comments posted: 8

🧹 Nitpick comments (4)

src/db/__tests__/migration-0022-accessory-type-backfill.test.ts (1)
118-118: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Rename byCategory to match its key.

The map is keyed by serial_number, not by category. Every lookup uses a serial (byCategory.get("fx-longtail")). The name misleads a reader of the assertions.
♻️ Rename to `bySerial`
-  let byCategory: Map<string, AccessoryRow>;
+  let bySerial: Map<string, AccessoryRow>;
Update the assignment and all byCategory.get(...) / byCategory.size reads accordingly.
🤖 Prompt for 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.

In `@src/db/__tests__/migration-0022-accessory-type-backfill.test.ts` at line 118,
Rename the map variable byCategory to bySerial in the migration test, updating
its declaration, assignment, and every byCategory.get(...) and byCategory.size
reference to match its serial_number key.
src/domain/accessories/__tests__/display.test.ts (1)
57-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Drive the exhaustive loop from ACCESSORY_TYPES.

The literal array omits "muzzle device" and will not pick up any type added later. The sibling suite src/domain/accessories/__tests__/validate.test.ts already loops over ACCESSORY_TYPES at Line 18. Use the same source so this "never empty" guarantee stays exhaustive.
♻️ Loop the controlled set
   test("never returns an empty string for any valid accessory shape", () => {
-    for (const type of ["suppressor", "optic", "light", "laser", "other"]) {
+    for (const type of ACCESSORY_TYPES) {
       expect(accessoryDisplayName({ ...base, type }).trim()).not.toBe("");
     }
   });
Add the import:
 import { describe, expect, test } from "bun:test";
+import { ACCESSORY_TYPES } from "../constants";
 import {
🤖 Prompt for 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.

In `@src/domain/accessories/__tests__/display.test.ts` around lines 57 - 61,
Update the exhaustive test in “never returns an empty string for any valid
accessory shape” to import and iterate over ACCESSORY_TYPES instead of the
hard-coded type array, matching the approach in the sibling validation suite and
covering all current and future accessory types.
src/domain/accessories/__tests__/compatibility.test.ts (1)
154-161: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Add the positive shared-reader case for viewer-relative compatibility reads.

This test pins only the negative direction: an outsider sees nothing. It passes even if the filter were owner-only rather than visibility-relative. Add a grantee who holds a view grant on firearmA and assert loadAccessoryCompatibility returns [firearmA] for that grantee while still dropping firearmB.

That asymmetric expectation is what proves the filter follows the visible set instead of ownership.

As per coding guidelines: "Test inventory parity behavior against the pinned parity specification, including two-user sharing and authorization tests."
🤖 Prompt for 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.

In `@src/domain/accessories/__tests__/compatibility.test.ts` around lines 154 -
161, Add a positive shared-reader case to the compatibility read tests: grant a
grantee “view” access to firearmA but not firearmB, then assert
loadAccessoryCompatibility returns [firearmA] for that grantee. Retain the
existing outsider assertion and use the established grant/setup helpers so the
test verifies visibility-relative filtering rather than owner-only behavior.
Source: Coding guidelines
e2e/accessories-serialized.spec.ts (1)
52-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use accessible locators instead of generic form selectors.

These lines use locator("form") or a fallback to it. Target the existing labeled controls directly, or give the form an accessible name and use getByRole("form", { name }). This keeps the test aligned with the required selector policy.

Also applies to: 67-79, 116-119, 138-140, 149-157, 167-173
🤖 Prompt for 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.

In `@e2e/accessories-serialized.spec.ts` around lines 52 - 57, Replace the generic
form fallback selectors in the test cases around the existing labeled controls
with accessible locators only. Update the affected sections, including the flows
using Name, Caliber, Type, and Action, to target controls directly or use
getByRole("form", { name }) after assigning an accessible form name; remove
locator("form") usage while preserving the current interactions.
Source: Coding guidelines

🤖 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)/accessories/accessories-view.tsx:
- Around line 126-135: Update the linkLabel callback to capture the current
labelCounts directly instead of reading from the mutable labelCountsRef during
render. Add labelCounts to the useCallback dependency array and remove the
render-time labelCountsRef mutation and associated access.

In `@app/`(app)/accessories/attachments-section.tsx:
- Around line 148-163: Update confirmDelete to clear the existing serverError
before starting the delete retry, ensuring a subsequent successful delete does
not leave the prior failure message visible.

In `@CONCEPTS.md`:
- Around line 26-32: Update the owned-parent taxonomy in the Relationships
section of CONCEPTS.md to include Accessory alongside Firearms, Magazines, and
Ammo, and change the stated count from three to four. Use the canonical
“Accessory” terminology already defined in the Accessory concept.

In `@e2e/accessories-serialized.spec.ts`:
- Around line 242-251: Extend the read-only assertions in the R17 test to verify
zero attachment edit controls by querying buttons with name /^Edit .*
attachment$/. Keep the existing exact-name Edit assertion and other
mutation-affordance checks unchanged.
- Around line 147-175: Extend the edit flow in the test step around the first
“Save changes” to reopen the accessory editor and assert the “Type” control has
value “muzzle device” before restoring it to “suppressor.” Keep the existing
compatibility assertions and final restoration behavior unchanged.

In `@src/auth/__tests__/accessory-sharing.test.ts`:
- Around line 334-349: Rename the test describing the unmounted accessory
scenario to reflect that the direct accessory editor is refused when they lack
permission to edit the target firearm, while preserving the existing setup and
NotFoundError assertion.

In `@src/domain/accessories/display.ts`:
- Line 28: Update the category selection expression in the accessory label logic
to trim the category once, use that trimmed value for the non-empty check, and
return the trimmed value; retain accessoryTypeLabel(a.type) as the fallback for
blank categories.

In `@src/domain/compatibility/__tests__/relation.test.ts`:
- Around line 115-144: Add a positive two-user sharing test near the existing
visibility-gate tests: grant owner access to the second user’s firearm, then
verify replaceMagazineCompatibility and replaceAccessoryCompatibility succeed
and loadMagazineCompatibility and loadAccessoryCompatibility retain that firearm
ID. Reuse the existing sharing and fixture helpers, and assert parity for both
compatibility types.

---

Nitpick comments:
In `@e2e/accessories-serialized.spec.ts`:
- Around line 52-57: Replace the generic form fallback selectors in the test
cases around the existing labeled controls with accessible locators only. Update
the affected sections, including the flows using Name, Caliber, Type, and
Action, to target controls directly or use getByRole("form", { name }) after
assigning an accessible form name; remove locator("form") usage while preserving
the current interactions.

In `@src/db/__tests__/migration-0022-accessory-type-backfill.test.ts`:
- Line 118: Rename the map variable byCategory to bySerial in the migration
test, updating its declaration, assignment, and every byCategory.get(...) and
byCategory.size reference to match its serial_number key.

In `@src/domain/accessories/__tests__/compatibility.test.ts`:
- Around line 154-161: Add a positive shared-reader case to the compatibility
read tests: grant a grantee “view” access to firearmA but not firearmB, then
assert loadAccessoryCompatibility returns [firearmA] for that grantee. Retain
the existing outsider assertion and use the established grant/setup helpers so
the test verifies visibility-relative filtering rather than owner-only behavior.

In `@src/domain/accessories/__tests__/display.test.ts`:
- Around line 57-61: Update the exhaustive test in “never returns an empty
string for any valid accessory shape” to import and iterate over ACCESSORY_TYPES
instead of the hard-coded type array, matching the approach in the sibling
validation suite and covering all current and future accessory types.

🪄 Autofix

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: 2161e7eb-f605-4209-838e-fcaab8653ba7

📥 Commits

Reviewing files that changed from the base of the PR and between fe0caa0 and c413038.

📒 Files selected for processing (52)

.github/copilot-instructions.md
.github/instructions/mermaid.instructions.md
CONCEPTS.md
app/(app)/accessories/[id]/page.tsx
app/(app)/accessories/accessories-view.tsx
app/(app)/accessories/accessory-detail-view.tsx
app/(app)/accessories/accessory-form.tsx
app/(app)/accessories/actions.ts
app/(app)/accessories/attachments-section.tsx
app/(app)/accessories/page.tsx
app/(app)/firearms/mounted-accessories.tsx
app/(app)/grants/actions.ts
app/(app)/grants/share-control.tsx
app/(app)/magazines/magazine-form.tsx
components/inventory/compatible-firearms-field.tsx
docs/plans/2026-08-04-001-feat-serialized-accessories-plan.md
e2e/accessories-serialized.spec.ts
e2e/accessories.spec.ts
e2e/fixtures/demo-seed.ts
e2e/fixtures/user-pool.ts
e2e/service-intervals-sharing.spec.ts
scripts/seed-demo.ts
src/auth/__tests__/accessory-sharing.test.ts
src/auth/accessory-visibility.ts
src/auth/visibility.ts
src/backup/table-order.ts
src/db/__tests__/migration-0022-accessory-type-backfill.test.ts
src/db/inventory-schema.ts
src/db/migrations/0022_confused_lockheed.sql
src/db/migrations/meta/0022_snapshot.json
src/db/migrations/meta/_journal.json
src/demo/inventory.ts
src/domain/accessories/__tests__/attachments.test.ts
src/domain/accessories/__tests__/compatibility.test.ts
src/domain/accessories/__tests__/display.test.ts
src/domain/accessories/__tests__/service.test.ts
src/domain/accessories/__tests__/validate.test.ts
src/domain/accessories/attachments.ts
src/domain/accessories/compatibility.ts
src/domain/accessories/constants.ts
src/domain/accessories/display.ts
src/domain/accessories/service.ts
src/domain/accessories/validate.ts
src/domain/compatibility/__tests__/relation.test.ts
src/domain/compatibility/relation.ts
src/domain/firearms/mount-options.ts
src/domain/firearms/service.ts
src/domain/magazines/compatibility.ts
src/domain/range-sessions/__tests__/service.test.ts
src/domain/service-intervals/rules-service.ts
src/domain/validation-messages.ts
src/test-support/factories.ts

Revalidate attachment pages from the updated row, not the caller. Both attachment actions keyed on `attachmentId` alone while invalidating the cache path built from a separately-supplied `accessoryId`, so a mismatched pair would update the right attachment and refresh some other accessory's page — leaving the page that actually changed stale. The parent id now comes from the write itself, which also lets the redundant parameter go. Return the trimmed category from `accessoryDisplayName`. The branch tested `category.trim()` but returned the raw value, and migration 0022 preserves pre-existing categories verbatim — the backfill test pins `" light "` as a surviving row — so the padding was reaching a link's accessible name. Clear a stale delete error before retrying an attachment delete; a successful retry was leaving the previous failure banner on screen. CONCEPTS.md still described three owned parents after Accessory became the fourth. Test fixes, all of which were passing for the wrong reason: the e2e type-edit step changed and restored `type` without asserting either save (a write path dropping `type` would have passed on the compatibility assertions alone); the read-only viewer assertion matched the exact name "Edit", which never matches "Edit Piston attachment", so it would have passed with every attachment control rendered; a sharing test's name claimed an accessory was "freely mountable" while asserting NotFoundError; the shared-core parity suite proved only that an unseeable firearm is rejected, never that a firearm *shared* to the actor is linkable — the gate is visibility, not ownership, and only the negative was covered. Declined one finding: moving `labelCounts` into `linkLabel`'s dependencies. The ref indirection is deliberate and documented on `main` — adding that dependency rebuilds `columns` and remounts the actions cell, which closes the ShareControl dialog mid-flow. This branch put a ShareControl in that cell, so the suggested change would reintroduce exactly the regression the pattern exists to prevent. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>

unclesp1d3r · 2026-08-06T03:33:40Z

Rename byCategory to match its key. … Drive the exhaustive loop from ACCESSORY_TYPES.

Both nitpicks fixed in 7e7fcc0.

The map is renamed to bySerial, which is what it is keyed by.

The exhaustive display-name loop now iterates ACCESSORY_TYPES instead of a literal list. That one was worth more than a rename: the literal had already drifted — it omitted "muzzle device" — so a test whose whole purpose is "never returns an empty string for any valid type" was silently skipping one.

coderabbitai

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

CONCEPTS.md (1)
56-56: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Correct the Accessory Compatibility wording.

Change “this can fits these five hosts” to “this can fit these five hosts.”
🤖 Prompt for 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.

In `@CONCEPTS.md` at line 56, In the Accessory compatibility description, correct
the grammar by changing “this can fits these five hosts” to “this can fit these
five hosts.”

🤖 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 `@CONCEPTS.md`:
- Around line 7-9: Update the Firearm definition in CONCEPTS.md to describe
Firearm as one of the four owned parents, keeping the surrounding ownership and
child-record documentation unchanged.

In `@src/domain/accessories/attachments.ts`:
- Around line 143-151: Serialize authorization with attachment mutations so
revokeGrant cannot commit between permission validation and deleteAttachment or
updateAttachment. Lock the shared authorization rows consistently in the
transaction, or enforce active permission directly in each mutation predicate,
ensuring revoked editors cannot mutate attachments. Add a concurrency test
covering grant revocation racing with attachment deletion and update.

---

Outside diff comments:
In `@CONCEPTS.md`:
- Line 56: In the Accessory compatibility description, correct the grammar by
changing “this can fits these five hosts” to “this can fit these five hosts.”

🪄 Autofix

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: d3293605-1f7d-48d9-a8e3-e36e59910415

📥 Commits

Reviewing files that changed from the base of the PR and between 6c61005 and 7e7fcc0.

📒 Files selected for processing (10)

CONCEPTS.md
app/(app)/accessories/actions.ts
app/(app)/accessories/attachments-section.tsx
e2e/accessories-serialized.spec.ts
src/auth/__tests__/accessory-sharing.test.ts
src/db/__tests__/migration-0022-accessory-type-backfill.test.ts
src/domain/accessories/__tests__/display.test.ts
src/domain/accessories/attachments.ts
src/domain/accessories/display.ts
src/domain/compatibility/__tests__/relation.test.ts

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

src/domain/accessories/display.ts
src/db/tests/migration-0022-accessory-type-backfill.test.ts
e2e/accessories-serialized.spec.ts
src/auth/tests/accessory-sharing.test.ts
src/domain/accessories/tests/display.test.ts
app/(app)/accessories/attachments-section.tsx

The Relationships section was updated to four owned parents when Accessory joined them; the Firearm definition still said three. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>

…e guards Findings from a five-lens review pass (comments, types, silent failures, test coverage, general). No criticals; all fixed here. `parentTable` could route a future parent type at the WRONG TABLE with no build error. It ended in `return accessory` rather than an exhaustive check, so adding a fifth `ParentType` arm and forgetting this dispatch would compile cleanly and silently read accessories. Now a switch with a `never` default. That also falsifies a claim this file made about itself, repeated in an earlier commit message: that widening `ParentType` is safe because `tsc` surfaces every arm needing an update. It doesn't — the fallback absorbed anything. The docblock now says what's true and points at the deliberate allowlist in share-control.tsx, which existed precisely BECAUSE the union is not self-enforcing. Two comments in this branch contradicted each other on that point. Removed the post-commit fallback in create/updateAccessory. It was unreachable — `attachCompatibility` maps over the array it is handed, so a one-element input always yields one element — and the comment justifying it described a concurrent-delete race that cannot occur. It was added while over-applying an earlier advisory. The sibling magazine service has no such fallback, which is the shape this should have matched. Left in place it would have become a landmine: a later change that dropped a row would return fabricated `compatibleFirearmIds: []` as though it were verified state. `listAttachments` re-authorizes through the parent and can throw its own NotFoundError, but sat outside the page's notFound() catch — a grant revoked between the two calls surfaced as an unhandled error rather than a 404, and there is no error boundary under app/. `CompatibilityRelation` is now generic over its table, so `buildRow` is checked against that table's real insert model. Verified by deliberately mis-wiring the accessory binding to emit `magazineId`: it now fails typecheck, where before it compiled and would only have failed at runtime. `type` — this branch's headline field — was never round-tripped through `updateAccessory`. Every test passed the same value on create and update, so none would have noticed if the update path stopped writing it. Added the round-trip, and the e2e edit step now asserts the saved value in both directions instead of checking a serial number that was already on screen. Also: `.$type<>()` on both `type` columns so the controlled set survives the read boundary; `exact: true` on the owner Edit locator, since Playwright matches accessible names by substring and the bare name also matched "Edit Piston attachment", making `.first()` resolve by DOM order; and the dead `Link` import left by the CompatibleFirearmsField extraction. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>

coderabbitai

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)

src/domain/accessories/service.ts (3)
235-240: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Preserve compatibility links that are outside the editor’s visible firearm set.

An edit grantee receives compatibleFirearmIds filtered by visible firearms, but AccessoryForm submits that filtered list to updateAccessory. The wholesale replacement can delete existing links to firearms the editor cannot see when the editor saves another field.

Keep the documented omission-clears behavior. When an editor submits compatibility IDs, preserve existing links outside the editor’s visible firearm set and replace only the visible portion. Add a regression test for an edit grantee with a hidden compatibility link.
🤖 Prompt for 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.

In `@src/domain/accessories/service.ts` around lines 235 - 240, Update the
compatibility replacement flow around replaceAccessoryCompatibility so submitted
IDs continue to clear all links when compatibility is omitted, but when an edit
grantee submits filtered compatibility IDs, only replace links within the
editor’s visible firearm set and preserve hidden links. Pass the appropriate
visible-firearm scope from the surrounding update logic, and add a regression
test covering an edit grantee saving with a hidden compatibility link.
Sources: Coding guidelines, Path instructions

172-188: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift

Prevent authorization revocation races before merging.

resolveCreateOwner, authorizeCreateMount, requireAccessoryEdit, authorizeMount, and requireAccessoryDelete read permissions, then write without locking authorization rows or rechecking permission. The transaction alone does not prevent a concurrent revokeGrant from deleting a grant between these operations. The accessory mutation can then commit for a revoked actor. Apply a consistent serialization strategy to these paths and other mutation services using the same gates. Add a concurrency regression test for revocation during authorization.
🤖 Prompt for 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.

In `@src/domain/accessories/service.ts` around lines 172 - 188, Serialize
authorization checks with revocation by locking the relevant authorization rows
or otherwise rechecking permissions immediately before each accessory mutation
commits. Apply this consistently across resolveCreateOwner,
authorizeCreateMount, requireAccessoryEdit, authorizeMount, and
requireAccessoryDelete, plus other mutation services using these gates, and add
a concurrency regression test covering revokeGrant racing with authorization.
Sources: Coding guidelines, Learnings, MCP tools

219-228: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Lock the accessory row before deriving installedDate.

Under PostgreSQL READ COMMITTED, this SELECT does not lock the row. A concurrent mountAccessory can commit before the update, causing the stale snapshot to overwrite the new installedDate with null. A concurrent unmount can also make the update fail the mount constraint when the input contains an installed date. Add .for("update") to this query, or derive the invariant atomically, and add a concurrency regression test.
🤖 Prompt for 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.

In `@src/domain/accessories/service.ts` around lines 219 - 228, Update the
accessory lookup in the transaction before persistableFields is called to lock
the selected row for update using the query builder’s row-locking mechanism,
preserving the existing not-found behavior and ensuring installedDate is derived
from the locked currentFirearmId. Add a concurrency regression test covering
concurrent mount and unmount updates.
Sources: Coding guidelines, MCP tools

🤖 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.

Outside diff comments:
In `@src/domain/accessories/service.ts`:
- Around line 235-240: Update the compatibility replacement flow around
replaceAccessoryCompatibility so submitted IDs continue to clear all links when
compatibility is omitted, but when an edit grantee submits filtered
compatibility IDs, only replace links within the editor’s visible firearm set
and preserve hidden links. Pass the appropriate visible-firearm scope from the
surrounding update logic, and add a regression test covering an edit grantee
saving with a hidden compatibility link.
- Around line 172-188: Serialize authorization checks with revocation by locking
the relevant authorization rows or otherwise rechecking permissions immediately
before each accessory mutation commits. Apply this consistently across
resolveCreateOwner, authorizeCreateMount, requireAccessoryEdit, authorizeMount,
and requireAccessoryDelete, plus other mutation services using these gates, and
add a concurrency regression test covering revokeGrant racing with
authorization.
- Around line 219-228: Update the accessory lookup in the transaction before
persistableFields is called to lock the selected row for update using the query
builder’s row-locking mechanism, preserving the existing not-found behavior and
ensuring installedDate is derived from the locked currentFirearmId. Add a
concurrency regression test covering concurrent mount and unmount updates.

ℹ️ Review info

⚙️ Run configuration

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

Review profile: CHILL

Plan: Pro

Run ID: ee07141f-5049-41ca-bdbc-751b83e28591

📥 Commits

Reviewing files that changed from the base of the PR and between 37c15b4 and 8f91c97.

📒 Files selected for processing (12)

app/(app)/accessories/[id]/page.tsx
app/(app)/magazines/magazine-form.tsx
e2e/accessories-serialized.spec.ts
src/auth/visibility.ts
src/db/inventory-schema.ts
src/domain/accessories/__tests__/attachments.test.ts
src/domain/accessories/__tests__/service.test.ts
src/domain/accessories/attachments.ts
src/domain/accessories/compatibility.ts
src/domain/accessories/service.ts
src/domain/compatibility/relation.ts
src/domain/magazines/compatibility.ts

💤 Files with no reviewable changes (1)

app/(app)/magazines/magazine-form.tsx

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

src/domain/magazines/compatibility.ts
src/domain/accessories/compatibility.ts
src/domain/accessories/tests/attachments.test.ts
src/domain/accessories/attachments.ts
app/(app)/accessories/[id]/page.tsx
src/domain/compatibility/relation.ts
src/auth/visibility.ts
src/db/inventory-schema.ts
src/domain/accessories/tests/service.test.ts

Corrects the record: 8f91c97's message states "the e2e edit step now asserts the saved value in both directions instead of checking a serial number that was already on screen." That change was never applied. The edit that was meant to add it silently matched nothing — the surrounding block had already been rewritten in an earlier round — and the result was not checked. The e2e run afterwards passed, which proved only that an unchanged test still passes. So for two rounds the one finding flagged as most important was recorded as fixed while the assertions did not exist. The step still ended by asserting a serial number that was on screen before it ran. The assertions exist now, and are verified rather than asserted: with the update path patched to drop `type`, the new unit round-trip fails by name and the e2e fails at the exact new `toHaveValue`. Restored, both pass. They were also racy on first write — reopening the edit form immediately after saving reads the previous form's value, which passed in isolation and failed under full-suite load. The step now waits for the form to close, which is the signal the write landed and the view refreshed. Also from this review round: - `CompatibilityRelation`'s type parameter had a `= PgTable` default, so a binding could omit the argument and silently fall back to the unchecked shape the generic was added to prevent. Default dropped; omission is now TS2314. The read paths take a narrower `CompatibilityColumns` view, since they never build a row and drizzle's `.from()` rejects a generic table. Still open, and now stated in the file: the three column fields are bare `PgColumn` and are not tied to the table, so pointing one at another table's column still compiles. That is also why the `as string` casts at the read sites are load-bearing rather than removable. - `listAttachments`/`getAccessory` now use the shared `asNotFound` helper instead of a hand-rolled catch. The helper was already imported in that file and used three times, and its own docstring describes this exact scenario. Removing the duplication orphaned the `NotFoundError` import. - `exact: true` on the remaining accessory-level Edit locators. The viewer's absence check deliberately keeps the bare name, since substring matching is what makes one assertion cover the attachment buttons too. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>

…s work Both are cases where a check appeared to prove something it did not. logic-errors/non-exhaustive-union-dispatch-silently-routes-to-wrong-table — `parentTable` mapped a `ParentType` to its table with an if/else chain ending in an unconditional `return`, so widening the union resolved a new member to whatever that fallback named, with a clean typecheck. The near-miss is verifiable: on `main` the fallback was `return ammo`, so adding `"accessory"` without touching the function would have run every accessory permission check against the ammo table. The rule the doc lands on is to treat "the typecheck passed" as evidence only when you can name the construct that would have failed. test-failures/playwright-accessible-name-matches-by-substring — accessible names match by substring unless `exact: true` is passed, which cuts both ways and never goes red. A bare "Edit" also matches "Edit Piston attachment", so `.first()` was selecting by DOM order rather than by name; separately, the belief that a bare name could NOT match those buttons produced a confident wrong account of a passing assertion. The doc deliberately does not conclude "always use exact" — for an absence assertion the substring default makes the check stronger, and one spec now carries both forms on purpose. Written in lightweight mode (session near its context limit), so neither had a semantic overlap search or the semantic grounding validator. Cited paths and frontmatter were validated mechanically; the claims about those lines were checked by hand against the tree. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>

"this can fits these five hosts" parses correctly with "can" as the noun, but two independent reviewers read it as a modal verb. A glossary line readers trip over is defective regardless of whether the grammar is defensible. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>

Compatibility reads are viewer-relative, so the list a caller submits is always built from a FILTERED view. The write path replaced wholesale, so an editor holding a grant on the item but not on one of its hosts silently deleted the links they were never shown — no error, and nothing to tell the owner. The replace is now bounded by the actor's visible set: rows outside it survive untouched and keep their ordinals, and the replaced slice is appended after them. "Omission clears" still holds, but only over what the actor could actually see. Fixed in the shared relation core rather than at the accessory call site, so magazines get it too — a magazine linked to a firearm whose grant was later revoked had the same silent loss on the owner's next save. Also de-flakes the accessory e2e type assertions, which failed while verifying this. The submit button relabels itself to "Saving…" while the action is in flight, so waiting for "Save changes" to disappear went true DURING the save. The wait now keys on the Edit button coming back — the one signal that means the write finished — and then reloads, so the reopened form is read from the persisted row rather than a cached render. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>

…eout Every test in this file drives a real Postgres container through key derivation, tar packing, a full-database wipe, and — twice — advisory-lock serialization of two concurrent restores. Several legitimately take 5–8 seconds against bun's 5s default, so the default was acting as a load tripwire rather than a signal about the code. Tripping it was not a contained failure. Bun marks the test failed but the body keeps running, so `afterAll` ended the pool underneath it and every remaining test in the file cascaded into "Cannot use a pool after calling end on the pool" — failures naming the wrong test and pointing at nothing real. That is why `just ci-check` failed in a different, larger subset on each run while this file passed 14/14 in isolation, and why the noise read as "flaky infrastructure" for long enough to get waved off twice. 60s is ~7x the slowest observed run, so a genuine hang still fails, just later. Scoped to this file rather than raised suite-wide, which would hide slowness in the ~1250 tests that have no business taking seconds. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>

… suites Supersedes the per-file version in 8254b11, which was both too narrow and too noisy. Too narrow: scoping it to the restore suite was a judgment call made on the evidence available, and the very next gate run disproved it by failing in routes.test.ts the same way. Every suite in src/backup starts its own Postgres and does real bundle work per test, so they all sit against the same 5s tripwire. Too noisy: renaming the call sites to `containerTest(` reflowed the formatter's wrapping through every test body — a 930-line diff for a one-line change. Importing the helper aliased (`containerTest as test`) leaves all 78 call sites byte-identical, so the whole change is one import line per file. The failure mode is what justifies a shared helper over a per-test number: a timeout is not contained. Bun marks the test failed but the body keeps running, so `afterAll` ends the pool underneath it and the rest of the file cascades into "Cannot use a pool after calling end on the pool" — failures naming the wrong test. That is why ci-check failed in a different subset each run while each file passed in isolation. Not applied to the src/db migration suites: their per-test bodies are assertions over data prepared in `beforeAll`, which already carries its own 120s timeout, so the tight default is correct there. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>

…cade Two learnings from this branch that generalize past #23. The first is the pairing that caused the data loss: a viewer-relative read feeding a replace-all write. Each half reviews as correct, which is why it survived eleven reviewers and a green suite — the defect exists only in their composition, and only a two-user fixture can observe it. Any future grant-shareable many-to-many inherits the trap. The second is why `just ci-check` looked flaky: a bun timeout is not contained. The body keeps running past the failure, `afterAll` ends the pool underneath it, and the rest of the file fails for reasons that name the wrong tests. Written down mostly because "passes in isolation" reads as exoneration and is worthless signal for load-sensitive failures — it is what let this get dismissed twice. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>

…from Addresses PR review feedback on #106 that was reported outside the diff range and so never became a resolvable thread. `updateAccessory` read `currentFirearmId` and derived `installedDate` from it (R6) without locking. Under READ COMMITTED that is a lost update, and the repo already had the answer: `updateMagazine` takes `FOR UPDATE` on its label read for precisely this reason. Accessories just never got it. Both directions are real, and both now have a regression test that drives the interleaving deterministically — a second connection holds the row under FOR UPDATE, the update parks on the lock, and only then does the holder commit: - a mount committing mid-update loses its `installedDate`, because the update still believes the accessory is unmounted and writes null over a date set moments earlier; - an unmount committing mid-update leaves the update writing a date onto a now-unmounted row, violating `accessory_installed_date_requires_mount` and surfacing as a 500 on an edit that never touched the mount. Verified both fail without the lock: the first as a silent revert, the second as the CHECK violation. Also replaces every generic `locator("form").last()` in the accessories e2e journey with `getByRole("form", { name })`, and gives the accessory, attachment, and firearm forms accessible names so the role exists at all. `.last()` resolves by DOM order rather than by name, which is the same class of ambiguity that already sent two assertions in this branch at the wrong element. It is also an accessibility gain: each form is now a named landmark rather than an anonymous one. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>

unclesp1d3r · 2026-08-08T14:50:35Z

Working the four findings that were posted in review bodies rather than as inline threads — the "outside diff range" and nitpick sections have no resolve mechanism, so they don't show up as unresolved threads and are easy to lose.

Lock the accessory row before deriving installedDate. Under PostgreSQL READ COMMITTED, this SELECT does not lock the row.

Fixed in ccb6943. This was the real one, and the repo already had the answer sitting next to it: updateMagazine takes FOR UPDATE on its label read with a comment explaining the lost update, and updateAccessory simply never got the same treatment.

Both directions you described are real and both now have a regression test. The tests drive the interleaving deterministically rather than hoping for it — a second connection holds the row under FOR UPDATE, the update parks on the lock, and only then does the holder commit:

a mount committing mid-update loses its installedDate: the update still believes the accessory is unmounted, so R6 force-nulls a date set moments earlier and the row ends up mounted with no install date;
an unmount committing mid-update leaves the update writing a date onto a now-unmounted row, violating accessory_installed_date_requires_mount — a 500 on an edit that never touched the mount.

Verified both fail with the lock stashed: the first as a silent revert, the second as the CHECK violation.

Use accessible locators instead of generic form selectors.

Fixed in ccb6943. All eight uses in the accessories journey are now getByRole("form", { name }), and the accessory, attachment, and firearm forms carry aria-label so the role exists at all — a bare <form> exposes no form role without an accessible name, which is why line 52 needed the .or(page.locator("form")) fallback.

Worth recording that this was not cosmetic. .last() resolves by DOM order rather than by name, and that exact ambiguity sent two assertions in this branch at the wrong element earlier in the work. The spec also got measurably faster, since the locators now resolve directly.

Add the positive shared-reader case for viewer-relative compatibility reads. This test pins only the negative direction: an outsider sees nothing. It passes even if the filter were owner-only rather than visibility-relative.

Covered, though not where you suggested. The regression tests added for the compatibility data-loss fix build exactly that fixture in src/domain/compatibility/__tests__/relation.test.ts — an editor holding a view grant on one host and nothing on the other, asserting the read returns [visibleHost] while dropping the hidden one, against both the magazine and accessory bindings.

That is the asymmetric expectation you asked for: a non-owner getting a non-empty result fails immediately if the filter is narrowed to ownership.

Drive the exhaustive loop from ACCESSORY_TYPES.

Already addressed — display.test.ts loops ACCESSORY_TYPES rather than a literal array. Same for the byCategory → bySerial rename in the 0022 migration test.

One process note: reviews on this branch are currently auto-paused ("It looks like this branch is under active development"). I previously read a green CodeRabbit check as a clean re-review of the newer commits, which was wrong — a paused review reports success without having looked. Triggering one below so the last several commits actually get read.

unclesp1d3r · 2026-08-08T14:50:50Z

@coderabbitai review

coderabbitai · 2026-08-08T14:50:58Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

coderabbitai

Actionable comments posted: 3

🧹 Nitpick comments (4)

src/domain/compatibility/relation.ts (1)
72-88: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

CompatibilityRelation redeclares three members it already inherits.

parentIdColumn, firearmIdColumn, and ordinalColumn are declared identically in CompatibilityColumns at lines 65-69. The redeclarations narrow nothing. Only table: TTable and buildRow are real additions. Two copies of the same declaration can drift when one side changes.
♻️ Keep only the members that refine the base
 export interface CompatibilityRelation<TTable extends PgTable>
   extends CompatibilityColumns {
-  /** The join table itself. */
+  /** The join table itself, narrowed so `buildRow` is checked against it. */
   table: TTable;
-  /** The parent-id column (`magazine_id`, `accessory_id`, ...). */
-  parentIdColumn: PgColumn;
-  /** The `firearm_id` column. */
-  firearmIdColumn: PgColumn;
-  /** The `ordinal` column the stable read order is defined by. */
-  ordinalColumn: PgColumn;
   /** Build one insertable row; keyed by the table's model property names. */
   buildRow: (
     parentId: string,
     firearmId: string,
     ordinal: number,
   ) => InferInsertModel<TTable>;
 }
🤖 Prompt for 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.

In `@src/domain/compatibility/relation.ts` around lines 72 - 88, Remove the
redundant parentIdColumn, firearmIdColumn, and ordinalColumn declarations from
CompatibilityRelation, leaving the inherited CompatibilityColumns members
intact. Keep only the TTable-specific table property and buildRow method in the
interface.
src/domain/accessories/__tests__/service.test.ts (2)
446-459: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value

done is an eagerly started promise with no rejection handler until the test awaits it.

db.transaction(...) starts immediately at line 446. If the transaction rejects before the test reaches await holder.done, Bun reports an unhandled rejection and can fail an unrelated test in the same process. Attach a no-op catch at creation and keep the original promise for the assertion.
♻️ Suppress the interim unhandled rejection
     const done = db.transaction(async (tx) => {
@@
     });
+    // Started eagerly; keep the process from seeing an unhandled rejection
+    // in the window before the test awaits `done`.
+    done.catch(() => {});
 
     return { locked, release, done };
🤖 Prompt for 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.

In `@src/domain/accessories/__tests__/service.test.ts` around lines 446 - 459,
Update the transaction promise assigned to done so it immediately has a no-op
rejection handler while preserving the original promise for the later assertion.
Apply this in the db.transaction flow around signalLocked and mayFinish without
changing the transaction operations or error assertion behavior.
476-486: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

The interleaving depends on a fixed 250 ms sleep.

setTimeout(250) assumes updateAccessory reached its FOR UPDATE read and parked on the lock. Under a loaded CI runner the holder can commit first. The test then passes without exercising the race, so a regression in the lock would not be caught. The suite header states the interleaving is deterministic; the sleep makes it probabilistic.

Poll pg_stat_activity for the blocked backend instead of sleeping.
♻️ Wait for the lock wait instead of sleeping
+  /** Resolves once a backend is blocked waiting on a lock. */
+  async function waitForBlockedBackend(): Promise<void> {
+    for (let attempt = 0; attempt < 100; attempt++) {
+      const [row] = await db.execute<{ blocked: number }>(
+        sql`select count(*)::int as blocked from pg_stat_activity
+            where wait_event_type = 'Lock' and state = 'active'`,
+      );
+      if (Number(row?.blocked ?? 0) > 0) return;
+      await new Promise((resolve) => setTimeout(resolve, 25));
+    }
+    throw new Error("update never parked on the row lock");
+  }
Then replace both sleeps:
-    await new Promise((resolve) => setTimeout(resolve, 250));
+    await waitForBlockedBackend();
     holder.release();
Also applies to: 508-516
🤖 Prompt for 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.

In `@src/domain/accessories/__tests__/service.test.ts` around lines 476 - 486,
Replace the fixed 250 ms sleeps in the interleaving test around updateAccessory
with polling of pg_stat_activity until the update backend is confirmed blocked
on the row lock. Apply the same synchronization to the second occurrence, then
release the holder only after that condition is observed so the test
deterministically exercises the lock race.
src/test-support/container-test.ts (1)
34-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Replace void with undefined in the callback return type.

Use body: () => undefined | Promise<unknown> to satisfy lint/suspicious/noConfusingVoidType while preserving Bun test callback compatibility.
🤖 Prompt for 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.

In `@src/test-support/container-test.ts` around lines 34 - 38, Update the callback
return type in containerTest from void to undefined, using () => undefined |
Promise<unknown>, while preserving the existing Bun test invocation and callback
compatibility.
Sources: Coding guidelines, Linters/SAST tools

🤖 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
`@docs/solutions/logic-errors/non-exhaustive-union-dispatch-silently-routes-to-wrong-table.md`:
- Around line 77-79: Update the compiler-error fenced block in the documentation
to specify the text language, changing its opening fence to use text while
preserving the existing error output.

In
`@docs/solutions/test-failures/playwright-accessible-name-matches-by-substring.md`:
- Line 67: Update the inline Markdown code span in the documented
accessible-name expression to use double backticks around the complete
JavaScript template literal, preventing nested backticks from breaking Markdown
parsing and resolving MD038.

In `@src/domain/accessories/__tests__/service.test.ts`:
- Around line 418-427: Guard all three database-backed describe suites in
service.test.ts with the existing DATABASE_URL check, replacing each bare
describe declaration while preserving their database setup and test bodies.
Ensure every suite that invokes database helpers in beforeAll is skipped when
DATABASE_URL is unset.

---

Nitpick comments:
In `@src/domain/accessories/__tests__/service.test.ts`:
- Around line 446-459: Update the transaction promise assigned to done so it
immediately has a no-op rejection handler while preserving the original promise
for the later assertion. Apply this in the db.transaction flow around
signalLocked and mayFinish without changing the transaction operations or error
assertion behavior.
- Around line 476-486: Replace the fixed 250 ms sleeps in the interleaving test
around updateAccessory with polling of pg_stat_activity until the update backend
is confirmed blocked on the row lock. Apply the same synchronization to the
second occurrence, then release the holder only after that condition is observed
so the test deterministically exercises the lock race.

In `@src/domain/compatibility/relation.ts`:
- Around line 72-88: Remove the redundant parentIdColumn, firearmIdColumn, and
ordinalColumn declarations from CompatibilityRelation, leaving the inherited
CompatibilityColumns members intact. Keep only the TTable-specific table
property and buildRow method in the interface.

In `@src/test-support/container-test.ts`:
- Around line 34-38: Update the callback return type in containerTest from void
to undefined, using () => undefined | Promise<unknown>, while preserving the
existing Bun test invocation and callback compatibility.

🪄 Autofix

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: 46af5fac-be09-4a78-96c6-90d1ccd8f790

📥 Commits

Reviewing files that changed from the base of the PR and between 37c15b4 and ccb6943.

📒 Files selected for processing (29)

CONCEPTS.md
app/(app)/accessories/[id]/page.tsx
app/(app)/accessories/accessory-form.tsx
app/(app)/accessories/attachments-section.tsx
app/(app)/firearms/firearm-form.tsx
app/(app)/magazines/magazine-form.tsx
docs/solutions/logic-errors/a-viewer-relative-read-feeding-a-replace-all-write-destroys-hidden-rows.md
docs/solutions/logic-errors/non-exhaustive-union-dispatch-silently-routes-to-wrong-table.md
docs/solutions/test-failures/a-test-timeout-ends-the-pool-and-cascades-into-unrelated-failures.md
docs/solutions/test-failures/playwright-accessible-name-matches-by-substring.md
e2e/accessories-serialized.spec.ts
src/auth/__tests__/accessory-sharing.test.ts
src/auth/visibility.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/inventory-schema.ts
src/domain/accessories/__tests__/attachments.test.ts
src/domain/accessories/__tests__/service.test.ts
src/domain/accessories/attachments.ts
src/domain/accessories/compatibility.ts
src/domain/accessories/service.ts
src/domain/compatibility/__tests__/relation.test.ts
src/domain/compatibility/relation.ts
src/domain/magazines/compatibility.ts
src/test-support/container-test.ts

💤 Files with no reviewable changes (1)

app/(app)/magazines/magazine-form.tsx

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

src/domain/magazines/compatibility.ts
src/auth/tests/accessory-sharing.test.ts
src/domain/accessories/compatibility.ts
src/domain/accessories/tests/attachments.test.ts
app/(app)/accessories/[id]/page.tsx
src/domain/accessories/attachments.ts
src/auth/visibility.ts
src/db/inventory-schema.ts
e2e/accessories-serialized.spec.ts
app/(app)/accessories/attachments-section.tsx
app/(app)/accessories/accessory-form.tsx

Both from PR review on #106. MD040: the compiler-error block in the non-exhaustive-union write-up opened a fence with no language; it is `text`. MD038: two spans in the Playwright write-up wrapped a JS template literal in single backticks and escaped the inner ones, which Markdown parses wrong and renders with visible backslashes. Double backticks let the inner backticks stand as content. Swept both files this branch added today for the same two classes rather than waiting for another round; they were already clean. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>

unclesp1d3r · 2026-08-08T15:07:13Z

@coderabbitai review

coderabbitai · 2026-08-08T15:07:20Z

⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

A green review-bot check and a clean unresolved-thread count are not evidence that a PR was reviewed or that its findings were surfaced. Both signals misled me on #106, in four distinct ways: - CodeRabbit's check reported "pass — Review completed" while its reviews were auto-paused for rapid commits. It had read nothing. Triggering one explicitly then produced three findings, one a real data-integrity bug. - A later trigger was rate-limited, so the newest commit went unreviewed. - A reviews-API row stamped with the head SHA was cited as proof the head had been reviewed. It had an empty body — a thread reply, not a review pass. GitHub stamps replies with whatever the head is at reply time. - CodeRabbit files some findings in review *bodies* ("outside diff range", "nitpick"). Those never become resolvable threads, so a 0-unresolved count concealed a silent data-loss bug and later a missing row lock. The doc gives the commands that distinguish the cases — check bodylen before trusting a reviews row, read bodies as well as threads — and the general question worth asking of any automated reviewer: what does its success signal report when it did nothing? Also refines two CONCEPTS.md entries. Accessory Compatibility said reads are viewer-relative; this branch made writes viewer-relative too, so the glossary understated the rule. Magazine Compatibility gets the same correction — one shared core, one rule, so they cannot drift. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>

unclesp1d3r added 7 commits August 5, 2026 19:24

feat(mermaid): add Mermaid AI skills documentation for diagram creati…

6b76c9f

…on and editing Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>

Copilot AI lite review requested due to automatic review settings August 6, 2026 03:07

unclesp1d3r linked an issue Aug 6, 2026 that may be closed by this pull request

Track serialized accessories (suppressors): serial numbers, firearm compatibility, and attachments #23

Closed

4 tasks

Copilot started reviewing on behalf of unclesp1d3r August 6, 2026 03:08 View session

Merge branch 'main' into 23-track-serialized-accessories-suppressors-…

c413038

…serial-numbers-firearm-compatibility-and-attachments

coderabbitai Bot added backend documentation Improvements or additions to documentation enhancement New feature or request frontend priority:medium security shared testing labels Aug 6, 2026

Copilot AI reviewed Aug 6, 2026

View reviewed changes

Comment thread app/(app)/accessories/actions.ts

docs: Dosu updates for PR #106

6c61005

coderabbitai Bot reviewed Aug 6, 2026

View reviewed changes

Comment thread CONCEPTS.md

Comment thread src/domain/accessories/attachments.ts

docs(concepts): correct the owned-parent count in the Firearm entry

37c15b4

The Relationships section was updated to four owned parents when Accessory joined them; the Firearm definition still said three. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>

unclesp1d3r self-assigned this Aug 6, 2026

coderabbitai Bot reviewed Aug 7, 2026

View reviewed changes

unclesp1d3r added 6 commits August 6, 2026 21:36

unclesp1d3r mentioned this pull request Aug 8, 2026

Grant-gated mutations have a TOCTOU window between the permission check and the write #107

Open

unclesp1d3r added 2 commits August 8, 2026 01:44

coderabbitai Bot reviewed Aug 8, 2026

View reviewed changes

Comment thread docs/solutions/logic-errors/non-exhaustive-union-dispatch-silently-routes-to-wrong-table.md Outdated

Comment thread docs/solutions/test-failures/playwright-accessible-name-matches-by-substring.md Outdated

Comment thread src/domain/accessories/__tests__/service.test.ts

unclesp1d3r merged commit 4a85247 into main Aug 8, 2026
7 checks passed

unclesp1d3r deleted the 23-track-serialized-accessories-suppressors-serial-numbers-firearm-compatibility-and-attachments branch August 8, 2026 21:27

coderabbitai Bot mentioned this pull request Aug 9, 2026

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

Merged

unclesp1d3r mentioned this pull request Aug 11, 2026

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

Open

Uh oh!

Conversation

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

Uh oh!

Summary

How visibility resolves now

Design decisions

Migration 0022 — read before deploying

Validation

New concepts

Strongest-path permission resolution

Uh oh!

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

Uh oh!

Reviews paused

Walkthrough

Changes

Sequence Diagram(s)

❌ Failed checks (3 warnings)

Review ran into problems

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

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!

Uh oh!

Uh oh!

Uh oh!

unclesp1d3r commented Aug 6, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

unclesp1d3r commented Aug 8, 2026

Uh oh!

unclesp1d3r commented Aug 8, 2026

Uh oh!

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

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

unclesp1d3r commented Aug 8, 2026

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

unclesp1d3r commented Aug 6, 2026 •

edited

Loading

coderabbitai Bot commented Aug 6, 2026 •

edited

Loading

coderabbitai Bot commented Aug 8, 2026 •

edited

Loading

coderabbitai Bot commented Aug 8, 2026 •

edited

Loading