feat(service-intervals): track service intervals and maintenance reminders - #99
Merged
Conversation
Requirements from issue #10 enriched to implementation-ready: ten implementation units, verification contract, and definition of done. Records the ten product decisions and ten technical decisions settled during brainstorming and planning. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
Adds service_rule_default, service_rule, and service_event, plus a nullable acquired_date on firearm. Service rows attach to a firearm or an accessory through two nullable foreign keys with an exactly-one CHECK rather than the parent_type shape used by grant and inventory_log, so native ON DELETE CASCADE replaces a hand-written cleanup trigger. Rules are keyed by name so a default and an item override are the same rule, and events name their rule by name rather than referencing a rule row. Registers all three tables in EXPORT_TABLE_ORDER and adds factories. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
resolveEffectiveRules folds category defaults and item rules into one effective rule set, marking each inherited, overridden, or item-only; a suppressed item rule removes its matching default. elapsedCounts folds days, sessions, and rounds from a measure-from date, counting only sessions strictly after it. isDue reports met-or-exceeded on any set threshold and names the axis that tripped. No database access here — every surface reads this layer's output, so it stays exhaustively testable without Docker. All calendar work is in the local frame with date fixtures built as new Date(y, month, d); tests pass under UTC, Asia/Tokyo, and America/New_York. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
Owner-scoped CRUD for category defaults, plus per-item overrides, suppressions, and item-only rules. Authorization splits by family and by operation: firearm rule writes are owner-only, firearm rule reads follow the firearm's own visibility so a view-grantee sees the owner's resolved rules, and accessory reads and writes check accessory ownership directly rather than inheriting permission from a mounted firearm. Unseen items raise NotFoundError so existence is never revealed. Renaming an item rule re-points that item's service events in the same transaction, and a rename onto a name the item already carries is rejected before the write rather than surfacing as a constraint violation. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
Logs service against a rule on one or many items, reads history back newest-first, and resolves due state across a whole visible collection in a bounded number of queries. Bulk mark-serviced authorizes each item with the same per-family rules as single logging, so it stays a convenience over the single path rather than a higher-privilege one, and rejects the whole batch if any item fails. Accessory session attribution restricts to firearms in the requester's visible set so a remounted accessory cannot leak rounds from a firearm the actor cannot see, and collection-wide reads scope accessories to the owner because accessory service is owner-only. Loaders only load; every due decision comes from the pure layer. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
BREAKING CHANGE: inventoried is now the only firearm inventory-log event type. Every existing cleaned entry becomes a Cleaning service event and every lubed entry a Lubrication service event, preserving actor, notes, and insertion time. The conversion is irreversible and has no down-migration, so it reconciles counts inside a PL/pgSQL block: rows read must equal rows inserted and rows deleted, and any mismatch raises, rolling the whole migration transaction back rather than silently losing history. The CHECK replacement follows the delete, since narrowing it first would reject the table's own historical rows. No service rule is seeded — a converted event is simply a point any later rule of that name measures from. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
Surfaces the nullable acquired date through the firearm form, detail view, validator, and service, matching how magazines and ammo already carry theirs. The field is clearable — blanking it persists as null rather than a zero date. Service-interval day counting measures from this date when it is set and falls back to the record's creation date when it is not, so an owner who enters a rifle owned for years is not told it was acquired today. Format is validated here (magazines and ammo persist theirs unvalidated) following the accessory installed-date precedent, since a malformed date would silently corrupt the day axis. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
An owner arms the whole feature from /settings/service without visiting a single item: firearm categories come from the existing type list, accessory categories from the owner's own distinct categories with free entry still allowed so a category can be armed before any accessory uses it. Editing a default states how many items it reaches, so the live inheritance consequence is visible before saving. Server actions carry the owner-only gate and never trust a client-supplied owner. The settings page links through to it — the screen that arms the feature is not reachable by URL alone. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
One panel serves both firearms and accessories, since the derivation already produces a single shape for both. Each rule shows its thresholds, elapsed counts on all three axes, its inheritance state, and whether it is due — as raw counts with no severity tier, and as a marker rather than a modal. This is advisory; nothing blocks. Rule actions are override, reset-to-inherited, suppress, restore, and add-item-only; override and add-item-only share one named rule form with the defaults screen's fields and messages. Log-service shows for an edit-grantee on a firearm and the owner only on an accessory, while rule actions stay owner-only on both. An item with no rules points at the defaults screen instead of rendering blank. Extracts the event validator into a database-free module so the client-side form can import it without pulling pg into the browser bundle. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
/summary gains a service line counting both items due and rules due, so an owner sees breadth and volume together. It folds into the existing Summary shape rather than a parallel one, since the page already loads the visible inventory once and the service loaders take the same visible set — the query count stays flat as the collection grows. Firearm and accessory rows carry a Service due badge with visible text, never color alone. An item due only because of an accessory mounted to it is not itself marked; the accessory is. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…(U10) The demo inventory now carries acquired dates, category defaults, a couple of overrides, and some service history, seeded relative to now so the walkthrough keeps showing a mix of due and not-due rules instead of going stale. Two end-to-end specs close the loop: the full flow proves a default set from settings marks items due across the collection without visiting one, that logging service clears that rule everywhere, and that an override survives a later default change. The sharing spec proves the permission split — an edit-grantee logs service but gets no rule actions, a view-grantee gets neither, and accessory service configuration never reaches a firearm's grantee. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
The due-resolution pipeline ran twice on every firearms page load — inventorySummary resolved it internally and the page resolved it again for the row markers. Both now share one result, and the visible firearm rows are threaded through instead of being fetched three times. The defaults settings page replaces two queries per category with one grouped query per scope. Consolidates the two near-identical rule-editing forms into one component, extracts the calendar-date helpers that this branch had grown to four copies, and drops getEffectiveRules, whose only callers were its own tests — those now exercise the live read path instead. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
The bulk path existed in the domain layer but nothing called it, so an owner with a large collection still had to open every item — the exact backlog that counting from day one creates. /summary now carries a checklist of every due item and rule with one date for the whole batch. Nothing is preselected, since marking service has no undo, and the checklist itself shows precisely what will be written rather than interrupting with a confirmation. Bulk authorization now resolves in two queries instead of one per item, with the outcome unchanged: every item is still checked and the whole batch still rolls back if any item fails. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
Logging service no longer rejects a submitter's own local date. The server compared against its own clock, so anyone east of it had today refused for part of every day — exactly the date the form pre-fills. Acquired dates get the same tolerance, and a future one no longer freezes day counting at zero. The bulk checklist now offers only items the actor can actually write to. A view-only grantee was shown the owner's due rules as selectable rows, and checking one rolled back every other item in the batch. Roll-up counts are unchanged — seeing a shared item's due state is still information, just not an action. Service events now verify their rule still exists, so a rename between render and submit fails loudly instead of writing history under a dead name that nothing measures from. Renames lock the row before repointing, so two of them cannot strand history on an intermediate name. Bulk batches are capped, future-dated sessions no longer count toward elapsed totals, and suppressing a rule with thresholds is rejected rather than silently dropping them. An item-only rule now offers Remove instead of Suppress. Suppression masks an inherited default; an item-only rule has nothing underneath it, so suppressing destroyed the thresholds and Restore could not bring them back despite saying so. Adds the U5 conversion tests the plan required and this branch had missed: the migration now runs against seeded pre-migration rows in its own container, proving the mapping, field fidelity, untouched rows, and that a reconciliation mismatch rolls the whole batch back. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
A mis-logged service event could not be fixed. Because due state is the latest event per rule, one wrong entry skewed that rule forever with no way back. Owners — and edit-grantees on a shared firearm — can now correct a date or notes, or delete the event outright. The rule and the parent item are deliberately not editable; renaming and re-logging cover that, and letting an event move between rules would rewrite two rules' due state at once. Deleting needs no special handling: due state derives on read, so removing the newest event falls back to the previous one, or to the item's origin date when it was the only one. Accessories gain an acquired date, closing the same cold start firearms already had — one owned for years but entered today no longer reads as freshly acquired. The accessory form now also suggests the owner's own existing categories. Category stays free text with exact matching per KD10; suggestions only reduce the typo that silently costs an owner a default set. Also fixes the migration-conversion test's folder truncation, which stripped one migration by name rather than everything past it — drizzle tracks a high-water mark, so a newer migration made the older one look applied and silently skipped it. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
Closed
9 tasks
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughService interval tracking adds configurable rules, service events, acquired dates, due-state calculation, service history, bulk servicing, settings, migrations, and permission-aware firearm and accessory workflows. Retired ChangesService interval tracking
Sequence Diagram(s)sequenceDiagram
participant User
participant SummaryPage
participant ServiceBacklogControl
participant markServicedBulkAction
participant EventsService
User->>SummaryPage: view due-service rollup
SummaryPage->>ServiceBacklogControl: render actionable backlog
User->>ServiceBacklogControl: select items and submit date
ServiceBacklogControl->>markServicedBulkAction: submit selected service items
markServicedBulkAction->>EventsService: create service events
EventsService-->>markServicedBulkAction: return created events
markServicedBulkAction-->>ServiceBacklogControl: return result
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
✨ Simplify code
Warning Review ran into problems🔥 ProblemsThese MCP integrations need to be re-authenticated in the Integrations settings: Notion Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/(app)/summary/page.tsx (1)
58-64: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe empty-state gate hides the new Service section for accessory-only owners.
The gate checks magazines, firearm counts, and ammo lots only. An owner who has accessories but no firearms, magazines, or ammo passes all three checks.
SummaryTablesnever renders, so the due roll-up and the bulk mark-serviced control stay hidden even whensummary.itemsDue > 0. Accessory due entries are reachable in that state:listDueForVisibleCollectionbuilds accessory entries independently of the firearm set.Include the service roll-up in the gate.
🐛 Include the service roll-up in the empty-state gate
{summary.totalMagazines === 0 && summary.firearmCounts.length === 0 && - summary.totalAmmoLots === 0 ? ( + summary.totalAmmoLots === 0 && + summary.itemsDue === 0 ? (🤖 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 `@app/`(app)/summary/page.tsx around lines 58 - 64, Update the empty-state condition in the summary page to also require no service items due, using summary.itemsDue alongside the existing magazine, firearm, and ammo checks. This must allow SummaryTables to render for accessory-only owners when due entries exist, preserving access to the service roll-up and bulk mark-serviced control.app/(app)/firearms/firearm-form.tsx (1)
153-159: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
acquiredDateInFutureblocks submit with no visible error.
validateFirearmreturnsacquiredDateInFuturefor a date beyondFUTURE_DATE_TOLERANCE_DAYS(src/domain/firearms/validate.ts:63-86). This form handles onlyinvalidAcquiredDate:focusOrderomits the future code, and theFielderror readsfirstMessage(codes, ["invalidAcquiredDate"]). On a future date,found.length > 0returns early, no field error renders, and focus does not move. The submit button appears dead.🐛 Proposed fix
+const ACQUIRED_DATE_CODES: FirearmValidationCode[] = [ + "invalidAcquiredDate", + "acquiredDateInFuture", +]; +- { codes: ["invalidAcquiredDate"], id: dateId }, + { codes: ACQUIRED_DATE_CODES, id: dateId }, ];<Field label="Acquired date" controlId={dateId} hint="Optional" - error={firstMessage(codes, ["invalidAcquiredDate"])} + error={firstMessage(codes, ACQUIRED_DATE_CODES)} > <Input id={dateId} type="date" value={values.acquiredDate} onChange={(e) => set("acquiredDate", e.target.value)} - aria-invalid={codes.includes("invalidAcquiredDate")} + aria-invalid={ACQUIRED_DATE_CODES.some((c) => codes.includes(c))} /> </Field>Also applies to: 310-324
🤖 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 `@app/`(app)/firearms/firearm-form.tsx around lines 153 - 159, Update the firearm form’s validation handling to include acquiredDateInFuture alongside invalidAcquiredDate: add it to focusOrder for dateId and include it in the date Field error codes used by firstMessage. Preserve the existing submit-blocking behavior while ensuring future dates display an error and move focus to the date field.
🧹 Nitpick comments (21)
src/test-support/factories.ts (2)
217-233: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winProtect
ownerIdthe same way the parent keys are protected.
overridesisPartial<typeof serviceRuleDefault.$inferInsert>, which includesownerId, and it spreads after the explicitownerId. A caller that passesownerIdinoverridessilently overrides the first argument.makeServiceRuleandmakeServiceEventguard against exactly this for the parent keys by omitting them from the override type. Apply the same shape here — these fixtures back owner-scoped authorization tests, where a mis-owned row makes a test pass for the wrong reason.♻️ Proposed fix
export async function makeServiceRuleDefault( ownerId: string, - overrides: Partial<typeof serviceRuleDefault.$inferInsert> = {}, + overrides: Partial< + Omit<typeof serviceRuleDefault.$inferInsert, "ownerId"> + > = {}, ): Promise<typeof serviceRuleDefault.$inferSelect> { const [row] = await db .insert(serviceRuleDefault) .values({ - ownerId, scope: "firearm", category: "rifle", name: "Cleaning", intervalRounds: 500, ...overrides, + ownerId, }) .returning(); return row; }🤖 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/factories.ts` around lines 217 - 233, Update makeServiceRuleDefault so its overrides type omits ownerId from serviceRuleDefault.$inferInsert, matching the parent-key protection used by makeServiceRule and makeServiceEvent. Keep the explicit ownerId argument authoritative while preserving all other override fields.
242-258: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
{ suppressed: true }alone fails the DB CHECK.The default sets
intervalRounds: 500, andoverridesspreads over it. A caller writingmakeServiceRule(parent, { suppressed: true })keepsintervalRounds: 500and violatesservice_rule_suppressed_thresholds_consistentinsrc/db/inventory-schema.ts. The test then fails with an opaque Postgres CHECK error instead of a clear signal, and the caller must remember to null all three thresholds.Force the thresholds to null when
suppressedis true, so the factory can only produce rows the schema accepts.♻️ Proposed fix
): Promise<typeof serviceRule.$inferSelect> { + const suppressed = overrides.suppressed ?? false; const [row] = await db .insert(serviceRule) .values({ name: "Cleaning", intervalRounds: 500, ...overrides, + ...(suppressed + ? { + intervalDays: null, + intervalSessions: null, + intervalRounds: null, + } + : {}), ...parent, }) .returning(); return row; }🤖 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/factories.ts` around lines 242 - 258, Update makeServiceRule so that when overrides.suppressed is true, intervalRounds and the other service-rule threshold fields are explicitly set to null after applying overrides and parent values. Preserve the existing defaults for non-suppressed rules, ensuring suppressed factory rows satisfy the schema CHECK without requiring callers to null thresholds manually.src/domain/service-intervals/rules-service.ts (2)
536-546: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the stale doc block.
This comment documents a function that no longer exists in the file ("this function was removed"). It now sits directly above
listItemRulesand describes different behavior. Delete it, or fold the useful part (defaults load against the item's owner, not the viewer) intogetItemDueState's doc indue-service.ts.🤖 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/service-intervals/rules-service.ts` around lines 536 - 546, Remove the stale documentation block immediately above listItemRules in rules-service.ts, since it describes the deleted function rather than listItemRules. Do not alter listItemRules behavior; only retain the owner-based defaults detail if it is incorporated into the existing getItemDueState documentation in due-service.ts.
161-169: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMap the unique-constraint violation back to
duplicateName.
assertNameAvailablereads siblings and then writes in the same transaction. Under READ COMMITTED, two concurrent creates (a double submit) both pass this check and both reach the insert. The DB unique constraints (service_rule_default_owner_scope_category_name_unique,service_rule_firearm_name_unique,service_rule_accessory_name_unique) then reject the second one with a raw driver error, not theValidationError(["duplicateName"])the form knows how to render. That contradicts the claim on Line 158-159 and Line 271.Catch the unique violation at each write site and rethrow it as the same
ValidationError, so the pre-check stays an optimization and the constraint stays the guarantee.🤖 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/service-intervals/rules-service.ts` around lines 161 - 169, Update each write site that persists service rules to catch database unique-constraint violations for the named constraints and rethrow ValidationError(["duplicateName"]). Keep assertNameAvailable as the pre-check optimization, while ensuring concurrent inserts receive the same form-renderable validation error instead of the raw driver error.src/domain/service-intervals/due-service.ts (2)
300-318: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse the two identical origin-date helpers.
firearmOriginDateandaccessoryOriginDatehave identical bodies and identical parameter shapes. OneitemOriginDate(row: { acquiredDate: string | null; createdAt: Date })covers both and removes the risk of the two drifting apart.♻️ Proposed refactor
-function firearmOriginDate(row: { - acquiredDate: string | null; - createdAt: Date; -}): Date { - return row.acquiredDate !== null ? parseISO(row.acquiredDate) : row.createdAt; -} - -/** - * An accessory's origin date (KTD9, updated during implementation) — - * `acquiredDate` when set, else `createdAt`, exactly parallel to - * `firearmOriginDate` above. Added after the plan's original scope; see this - * file's header comment. - */ -function accessoryOriginDate(row: { - acquiredDate: string | null; - createdAt: Date; -}): Date { - return row.acquiredDate !== null ? parseISO(row.acquiredDate) : row.createdAt; -} +/** + * An item's origin date (KTD9, updated during implementation) — `acquiredDate` + * when set, else `createdAt`. Identical for firearms and accessories; see this + * file's header comment for why accessories gained `acquiredDate`. + */ +function itemOriginDate(row: { + acquiredDate: string | null; + createdAt: Date; +}): Date { + return row.acquiredDate !== null ? parseISO(row.acquiredDate) : row.createdAt; +}🤖 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/service-intervals/due-service.ts` around lines 300 - 318, Replace the duplicate firearmOriginDate and accessoryOriginDate helpers with a single itemOriginDate helper using the shared row shape and existing date-selection logic. Update all callers to use itemOriginDate, removing the redundant helper and preserving current behavior.
591-599: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSensitive Data Exposure (CWE-200): Exposure of Sensitive Information to an Unauthorized Actor
Reachability: Internal · Exploitability: Theoretical
Reachability path
● Entry app/(app)/summary/service-backlog-control.tsx:82 submit │ ▼ ● Hop src/domain/service-intervals/events-service.ts:183 loadEffectiveRuleNames │ ▼ ● Sink src/domain/service-intervals/due-service.tsRename
preloadedFirearmsto encode its visibility contract. The current callers passlistFirearms(actorId)results, butloadVisibleItemstrusts this parameter and uses its IDs to filter accessory round counts. Name itvisibleFirearmsAlreadyLoadedto make the required scope explicit.🤖 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/service-intervals/due-service.ts` around lines 591 - 599, Rename the listDueForVisibleCollection parameter preloadedFirearms to visibleFirearmsAlreadyLoaded and update its use when calling loadVisibleItems. Preserve the existing behavior while making clear that callers must provide firearms already filtered to the actor’s visible collection.src/domain/service-intervals/validate.ts (1)
39-43: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd integer validation to
hasThresholdBelowMin.The form component
toRuleInputusesNumber()to parse threshold strings (notNumber.parseInt). This allows fractional values like1.5and invalid values likeNaNto reach the validator. ThehasThresholdBelowMincheck only validates the lower bound; non-integer thresholds pass validation because numeric comparison1.5 < 1andNaN < 1produce the expected false result. These values then fail at database insert time as unhandled driver errors instead of returning aValidationErrorwith a message the form can render.Add
!Number.isInteger(value)to catch non-integer thresholds:Proposed change
function hasThresholdBelowMin(rule: ServiceRuleInput): boolean { return [rule.intervalDays, rule.intervalSessions, rule.intervalRounds].some( - (value) => isSetThreshold(value) && value < MIN_THRESHOLD, + (value) => + isSetThreshold(value) && + (!Number.isInteger(value) || value < MIN_THRESHOLD), ); }🤖 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/service-intervals/validate.ts` around lines 39 - 43, Update hasThresholdBelowMin to treat any set threshold that is not an integer as invalid, alongside the existing MIN_THRESHOLD check, so fractional and NaN values produce the normal validation error path.Source: Coding guidelines
src/domain/service-intervals/events-service.ts (1)
104-157: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAuthorization Bypass (CWE-863): Incorrect Authorization
Reachability: External
Reachability path
● Entry app/(app)/summary/service-backlog-control.tsx:82 submit │ ▼ ● Sink src/domain/service-intervals/events-service.tsAdd a test that verifies authorization parity between single and bulk paths for firearm access tiers.
authorizeEventWritesBatchreimplements firearm authorization logic inline usingvisibleFirearmPermissionsinstead of calling the single-path'sauthorizeUpdatefunction (which itself callsresolvePermission). Both paths currently produce identical outcomes for owner, edit, view, and not-found cases. However, ifresolvePermissionis later enhanced to include additional authorization conditions—archived items, suspended accounts, maintenance gates—the batch path will not inherit those changes, silently creating a looser authorization gate.Add a test that calls both
logServiceEventandlogServiceEventsBulkagainst the same firearm with the same actor under owner, edit, view, and stranger permission states. Verify both paths reject and allow identically.🤖 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/service-intervals/events-service.ts` around lines 104 - 157, Add a test covering authorization parity between logServiceEvent and logServiceEventsBulk for the same firearm and actor across owner, edit, view, and stranger states. Assert both paths allow or reject identically, including the expected authorization error categories, so future changes to single-path authorization cannot leave authorizeEventWritesBatch inconsistent.src/domain/firearms/__tests__/validate.test.ts (1)
123-132: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPass an explicit
asOfto the valid-date test.This test relies on the default
asOf = new Date()."2026-06-14"only stays valid while the system clock is at or after 2026-06-13. Every other date-sensitive test in this file passesasOfexplicitly. Match that pattern so the case is clock-independent.♻️ Proposed fix
test("a real ISO calendar date is valid", () => { expect( - validateFirearm({ - name: "Glock 19", - caliber: "9mm", - ...CLASS, - acquiredDate: "2026-06-14", - }), + validateFirearm( + { + name: "Glock 19", + caliber: "9mm", + ...CLASS, + acquiredDate: "2026-06-14", + }, + new Date(2026, 5, 15), + ), ).toEqual([]); });🤖 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/firearms/__tests__/validate.test.ts` around lines 123 - 132, Update the “a real ISO calendar date is valid” test to pass an explicit fixed asOf date to validateFirearm, matching the other date-sensitive tests and ensuring the 2026-06-14 case remains independent of the system clock.src/domain/summary/__tests__/summary.test.ts (1)
624-658: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExact query-count equality can flake if any other query runs during the measured window.
countPoolQueriespatches the sharedPool.prototype.query, so it counts every query issued by the whole process duringfn.expect(largeCount).toBe(smallCount)then fails if an unrelated concurrent query lands in either window. Prefer an upper bound so the test still proves the property (no per-item query) without depending on process-wide isolation.♻️ Bound the count instead of requiring exact equality
- // Same bounded query count regardless of collection size (Definition of - // Done, U9/U4) — never a per-item query. - expect(largeCount).toBe(smallCount); + // Bounded regardless of collection size (Definition of Done, U9/U4) — + // never a per-item query: 5x the items must not grow the query count. + expect(largeCount).toBeLessThanOrEqual(smallCount);🤖 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/summary/__tests__/summary.test.ts` around lines 624 - 658, Update the query-count assertion in the “inventorySummary's service roll-up loads in a bounded number of queries” test to verify an upper bound rather than exact equality between largeCount and smallCount. Keep the existing measurements and item-count assertions, and ensure the assertion still detects per-item query growth without relying on process-wide query-count isolation.src/domain/summary/summary.ts (1)
262-273: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueForward
firearmstolistDueForVisibleCollectionwhendueEntriesis omitted.When a caller passes
firearmsbut notdueEntries, the due pipeline reloads the visible firearm set itself (getVisibleIds+ select).listDueForVisibleCollectionaccepts apreloadedFirearmsargument for exactly that case. The current signature typesfirearmsasFirearmIdentity[], which is narrower than theFirearmRow[]the due service expects, so forwarding requires widening the parameter type.This is an optimization only; the present callers pass both arguments, so no current path double-loads.
🤖 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/summary/summary.ts` around lines 262 - 273, Update inventorySummary to accept the firearm shape required by listDueForVisibleCollection, and when dueEntries is omitted, pass the resolved or supplied firearms as its preloadedFirearms argument. Preserve the existing dueEntries path and ensure the shared firearm value remains available to both Promise.all operations.app/(app)/summary/page.tsx (1)
24-34: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueMagazine and ammo loads now wait on the due pipeline.
inventorySummaryrunslistMagazinesandlistAmmoin parallel with its own due load. BecausedueEntriesis passed in, those two loads start only afterlistDueForVisibleCollectionfinishes. The result is three sequential phases instead of two. This is latency only; correctness is unaffected.If the extra round-trip time matters on this page, start the due load in the same
Promise.allas the item loads and passfirearmsafter it resolves, or load magazines and ammo in the page and keepinventorySummarypurely computational.🤖 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 `@app/`(app)/summary/page.tsx around lines 24 - 34, Update the summary loading flow around inventorySummary so magazine and ammunition loading overlaps with listDueForVisibleCollection instead of waiting for it; either start the due request in the existing Promise.all and pass the resolved dueEntries to inventorySummary after firearms are available, or move those loads into the page while keeping inventorySummary computational.app/(app)/summary/service-backlog-control.tsx (1)
146-163: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRow
idvalues derive from user data and can contain spaces.
rowKeyembedsrow.ruleName, and rule names accept spaces (for exampleRecoil spring).inputIdtherefore becomes anidattribute with spaces, which is invalid HTML and breaks any CSS orquerySelectorlookup for that element. The prefix also reusesdateId, which belongs to the date input and is unrelated to these rows.Use a dedicated id prefix and a positional suffix.
♻️ Derive row ids from a dedicated prefix and index
const dateId = useId(); const notesId = useId(); const selectAllId = useId(); + const rowIdPrefix = useId();- {backlog.map((row) => { + {backlog.map((row, index) => { const key = rowKey(row); - const inputId = `${dateId}-${key}`; + const inputId = `${rowIdPrefix}-${index}`;🤖 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 `@app/`(app)/summary/service-backlog-control.tsx around lines 146 - 163, Update the backlog map in the row-rendering component to derive each inputId from a dedicated row-specific prefix and the map index, rather than dateId and rowKey. Keep rowKey for selection and React key behavior, while ensuring every checkbox id is stable, unique within the list, and free of user-provided spaces.app/(app)/summary/__tests__/actions.test.ts (1)
126-139: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd the no-revalidation assertion to the
ValidationErrorcase.The
NotFoundErrortest provesrevalidateCallsstays empty. TheValidationErrortest asserts only the mapped codes. Both failures must leave the cache untouched, and only one of them pins that behavior.💚 Pin the cache behavior on the validation path too
expect(result.ok).toBe(false); expect(result.ok === false && result.codes).toEqual(["servicedOnInFuture"]); + expect(revalidateCalls).toEqual([]);🤖 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 `@app/`(app)/summary/__tests__/actions.test.ts around lines 126 - 139, Add an assertion to the “a future servicedOn maps a thrown ValidationError to a failed ActionResult with codes” test verifying that revalidateCalls remains empty, matching the existing NotFoundError test and preserving the cache-untouched behavior for validation failures.app/(app)/settings/service/__tests__/actions.test.ts (1)
198-217: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a delete-failure case to match the update suite.
The delete suite covers only the signed-out and success paths. The update suite already asserts that a
NotFoundError(another owner's default) maps to a non-leaking failure. Mirror it here so a regression in delete error mapping fails a test.As per coding guidelines: "new behavior needs coverage, and bug fixes should include regression tests where practical."💚 Proposed test
+ test("maps a NotFoundError (another owner's default) to a non-leaking failed ActionResult, and does not revalidate", async () => { + currentUserId = "user-1"; + deleteThrows = new NotFoundError(); + + const result = await deleteServiceRuleDefaultAction("default-1"); + + expect(result.ok).toBe(false); + expect(revalidateCalls).toHaveLength(0); + });🤖 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 `@app/`(app)/settings/service/__tests__/actions.test.ts around lines 198 - 217, Add a delete-failure test in the deleteServiceRuleDefaultAction suite that makes deleteServiceRuleDefault throw NotFoundError for another owner’s default, then assert a non-leaking failure result and no revalidation. Mirror the existing update-suite error-mapping setup and assertions, while preserving the unauthenticated and success cases.Source: Coding guidelines
app/(app)/firearms/service-rules-panel.tsx (1)
214-289: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse the four mutation handlers into one helper.
reset,suppress,restore, andremoveare the same shape: run the action in a transition, toast a success message, or toastresult.errorwith the destructive tone. One helper taking the action, the success message, and the failure message removes about 60 lines and keeps the four call sites readable.♻️ Proposed shape
+ function runRuleAction( + action: () => Promise<ActionResult>, + success: string, + failure: string, + ) { + startTransition(async () => { + const result = await action(); + if (result.ok) { + afterMutation(success); + } else { + toast({ message: result.error ?? failure, tone: "destructive" }); + } + }); + }Call sites then read as:
runRuleAction( () => resetServiceRuleAction(parentType, parentId, ruleName), `${ruleName} reset to inherited`, "Could not reset rule.", );🤖 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 `@app/`(app)/firearms/service-rules-panel.tsx around lines 214 - 289, Replace the duplicated logic in reset, suppress, restore, and remove with a shared runRuleAction helper that accepts the action callback, success message, and failure fallback, while preserving the existing transition, success mutation, and destructive error toast behavior. Update each handler to call the helper with its corresponding service action and messages, including removeItemOnlyRuleAction and its “removed” success text.app/(app)/firearms/[id]/page.tsx (1)
82-87: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winWrap the new service loaders in the same
NotFoundError→notFound()guard as their siblings.Lines 69-75 state the convention: if access is revoked between
getFirearmand these calls, aNotFoundErrormust become the page's clean 404 instead of a 500.getItemDueState,listItemRules, andlistServiceHistoryall authorize internally and can throw the same error, but they run unguarded in the samePromise.all.Extract the guard so the three new calls share it.
♻️ Proposed refactor
+ const asNotFound = (error: unknown): never => { + if (error instanceof NotFoundError) notFound(); + throw error; + }; + const [ caliberSuggestions, @@ - getItemDueState(user.id, "firearm", id), + getItemDueState(user.id, "firearm", id).catch(asNotFound), // Raw item-rule rows — only their suppressed names are used here, to // list what's hidden from `serviceRules` above (KTD6). - listItemRules(user.id, "firearm", id), - listServiceHistory(user.id, "firearm", id), + listItemRules(user.id, "firearm", id).catch(asNotFound), + listServiceHistory(user.id, "firearm", id).catch(asNotFound), ]);🤖 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 `@app/`(app)/firearms/[id]/page.tsx around lines 82 - 87, Update the Promise.all loading flow around getItemDueState, listItemRules, and listServiceHistory so all three calls run through the existing NotFoundError-to-notFound() guard used by their sibling loaders. Extract or reuse that guard for the new service loaders, preserving the clean 404 behavior when authorization is revoked.app/(app)/accessories/[id]/page.tsx (1)
40-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe actor-name mapping is duplicated verbatim across both detail pages. Both pages collect distinct non-null
actorIdvalues, callnamesByIds, and mapServiceEventRow[]toServiceHistoryEntry[]with the sameUNKNOWN_ACTOR_LABELand the same raw-id fallback. Two copies of the fallback policy will drift.
app/(app)/accessories/[id]/page.tsx#L40-L61: exportwithActorNamesandUNKNOWN_ACTOR_LABELfrom a shared module next toServiceHistoryEntry, since the helper already exists here in reusable form.app/(app)/firearms/[id]/page.tsx#L105-L125: replace the inlineactorIds/nameById/serviceHistoryblock with a call to the sharedwithActorNames(history)and drop the localUNKNOWN_ACTOR_LABEL.🤖 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 `@app/`(app)/accessories/[id]/page.tsx around lines 40 - 61, The actor-name mapping is duplicated across both detail pages. In app/(app)/accessories/[id]/page.tsx lines 40-61, move/export withActorNames and UNKNOWN_ACTOR_LABEL from a shared module next to ServiceHistoryEntry; in app/(app)/firearms/[id]/page.tsx lines 105-125, replace the local actorIds/nameById/serviceHistory logic with withActorNames(history) and remove the local UNKNOWN_ACTOR_LABEL.app/(app)/firearms/page.tsx (1)
34-48: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueBoth list pages serialize
listDueForVisibleCollectionahead of queries that do not depend on it. The shared root cause is awaiting the due-state pipeline as its own step instead of joining it to the surroundingPromise.all. That pipeline is the heaviest load on each page, so the extra round trips add directly to page latency.
app/(app)/firearms/page.tsx#L34-L48: start the due promise, put it in thePromise.allbesidecalibersForInput,visibleFirearmPermissions, andprimaryThumbnailsFor, then awaitinventorySummaryfrom the resolveddueEntries.app/(app)/accessories/page.tsx#L43-L47: the due call needs onlyfirearms, and nothing in the precedingPromise.alldepends on it — move thelistFirearmsresult into a promise and resolve the due entries in the same batch.🤖 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 `@app/`(app)/firearms/page.tsx around lines 34 - 48, In app/(app)/firearms/page.tsx lines 34-48, start listDueForVisibleCollection as a promise and resolve it within the existing Promise.all alongside calibersForInput, visibleFirearmPermissions, and primaryThumbnailsFor, then pass the resolved dueEntries to inventorySummary. In app/(app)/accessories/page.tsx lines 43-47, move the listFirearms result into the same promise batch and resolve listDueForVisibleCollection there using firearms, preserving the existing dependent processing.src/domain/service-intervals/__tests__/validate.test.ts (1)
43-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an
intervalSessionscase.
validateServiceRuleSettreats three threshold axes. This file exercisesintervalRoundsandintervalDaysonly.intervalSessionsis never validated here, so a regression in the sessions branch ofhasAnyThresholdorhasThresholdBelowMinwould pass this suite.As per coding guidelines: "new behavior needs coverage".
💚 Suggested additions
test("rejects a negative threshold", () => { const rules: ServiceRuleInput[] = [{ name: "Cleaning", intervalDays: -1 }]; expect(validateServiceRuleSet(rules)).toContain("thresholdTooLow"); }); + test("accepts a rule whose only threshold is intervalSessions", () => { + const rules: ServiceRuleInput[] = [ + { name: "Cleaning", intervalSessions: 5 }, + ]; + expect(validateServiceRuleSet(rules)).toEqual([]); + }); + + test("rejects a zero intervalSessions threshold", () => { + const rules: ServiceRuleInput[] = [ + { name: "Cleaning", intervalSessions: 0 }, + ]; + expect(validateServiceRuleSet(rules)).toContain("thresholdTooLow"); + }); +🤖 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/service-intervals/__tests__/validate.test.ts` around lines 43 - 61, Add coverage for the `intervalSessions` threshold in the tests around `validateServiceRuleSet`: include valid positive sessions behavior and invalid zero or negative sessions behavior, matching the existing `intervalRounds` and `intervalDays` assertions. Ensure the tests also cover a sessions-only rule so both `hasAnyThreshold` and `hasThresholdBelowMin` paths are exercised.Source: Coding guidelines
e2e/service-intervals-sharing.spec.ts (1)
148-150: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the actor by seeded identity, not by the user-pool key.
Line 150 asserts the history region contains the literal
"service-intervals-viewer". That passes only while the seeded display name or email embeds the pool key. Use the artifact value already loaded at line 31 so the assertion survives a change to the seeding scheme.♻️ Suggested change
- await expect(history).toContainText("service-intervals-viewer"); + await expect(history).toContainText(grantee.email);🤖 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/service-intervals-sharing.spec.ts` around lines 148 - 150, Update the service history assertion in the relevant test to use the seeded identity artifact loaded near line 31 instead of the hardcoded "service-intervals-viewer" pool key. Preserve the existing history region and "Cleaning" assertions while interpolating the artifact value for the actor check.
🤖 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/[id]/page.tsx:
- Around line 122-127: Update the category-loading logic around
listOwnerAccessoryCategories and ownerCategories so edit-grantees cannot receive
the owner’s unrelated category vocabulary. Gate the owner-category lookup on
isOwner, and for non-owners use the actor’s own categories or another
actor-visible scope, while preserving owner behavior for the accessory owner.
In `@app/`(app)/firearms/service-actions.ts:
- Around line 76-87: The createItemRule error handling should recognize
PostgreSQL unique-constraint violations (code 23505) for the item-rule
constraint and map them through toActionError as codes ["duplicateName"] instead
of the generic error. Update the relevant toActionError mapping used by
createItemRule while preserving existing handling for other database errors.
In `@app/`(app)/firearms/service-history.tsx:
- Around line 299-309: Add a stable React key based on editingEntry.id to the
EditServiceEventForm instance in the editingEntry render branch, so switching
rows remounts the form and reinitializes its state for the selected entry.
In `@app/`(app)/settings/service/actions.ts:
- Around line 28-47: Add runtime validation at the start of
createServiceRuleDefaultAction for input.scope being "firearm" or "accessory"
and input.category and input.name being strings. On invalid input, return the
existing ActionResult validation-error shape without calling
createServiceRuleDefault; preserve the current delegation and revalidation flow
for valid input.
In `@app/`(app)/settings/service/default-rule-form.tsx:
- Around line 255-262: Update the name Input in the default rule form to use
readOnly={nameLocked} instead of disabled={nameLocked}, keeping the locked value
focusable and announced while preserving the existing nameLocked behavior.
- Around line 99-136: Update validateServiceRuleSet to reject threshold values
that are not integers, returning ValidationError before they reach the integer
database columns. Preserve the existing minimum-threshold validation for days,
sessions, and rounds, and apply the integer check to each converted value
produced by toRuleInput.
In `@app/`(app)/summary/service-backlog-control.tsx:
- Around line 109-113: When result.codes is set, check if any codes fall outside
DATE_CODES (such as bulkTooLarge or emptyRuleName). For codes not handled by the
date Field component, map them to appropriate error messages and either set
serverError with a message for the unhandled code or render a fallback message
in the existing error display. Ensure bulkTooLarge maps to a specific message if
one is defined in your firstMessage mapping. Apply this same logic to both the
initial codes check and the other location at lines 179-186 to prevent silent
failures when non-date validation codes are returned from logServiceEventsBulk.
In `@docs/plans/2026-08-02-002-feat-service-intervals-plan.md`:
- Line 212: Align the entire plan’s accessory origin-date contract: use
acquired_date when present, otherwise created_at, and never installed_date.
Update the referenced U1/U6 scope, test cases, and verification steps
consistently, including the R10 statement, so all sections describe and validate
the same fallback behavior.
In `@e2e/service-intervals-sharing.spec.ts`:
- Around line 59-64: Scope the form locator in the rule and service-log flows to
the open dialog by using the dialog locator before selecting the form. Update
the relevant form references in the test so they no longer query all forms on
the page, while preserving the existing field fills and submissions.
In `@e2e/service-intervals.spec.ts`:
- Line 164: Update the threshold formatter used by the interval UI to use
singular “day” when the value is exactly 1, while retaining plural “days” for
other values; then update the barrelRow assertion in the interval test to expect
“of 1 day”.
In `@scripts/seed-demo.ts`:
- Around line 63-69: Update the reset preflight in seed() to query
serviceRuleDefault before the hasInventory() early-return decision. Ensure
owners with service defaults but no inventory still enter reset handling and
delete those defaults before creating new inventory, while preserving the
existing behavior for owners with neither inventory nor defaults.
In `@src/db/migrations/0020_naive_scarlet_spider.sql`:
- Around line 21-50: In the migration’s DO block, acquire a transaction-held
lock on inventory_log before the initial read_count query so legacy writes are
serialized throughout the reconciliation. Use a lock mode that conflicts with
INSERT, UPDATE, and DELETE, and keep the lock acquisition ahead of the INSERT
... SELECT conversion.
In `@src/demo/__tests__/inventory.test.ts`:
- Around line 101-123: The tests call isoDateDaysAgo and todayIso independently,
and if local midnight occurs between these calls, the expected calendar-day
offsets shift by one day causing failures even in correct implementations.
Freeze the current date/time across all test cases using a shared clock mock, or
modify isoDateDaysAgo to accept an optional reference date parameter and pass
the result from todayIso into each isoDateDaysAgo call to ensure both functions
use the same base date throughout each test.
In `@src/demo/inventory.ts`:
- Around line 70-80: Add an accessory service-rule default alongside the
existing firearm defaults in the service-rule configuration, matching the
category of the seeded backdated accessory created in the seed flow. Ensure the
accessory seed uses or resolves this default rule so the cold-start example can
determine its service status and the acquiredDaysAgo behavior produces the
expected due result.
In `@src/domain/firearms/__tests__/service.test.ts`:
- Around line 711-805: The integration test suites create users and query
PostgreSQL but are not gated on DATABASE_URL, causing database connection
attempts during unit-only test runs. At both
src/domain/firearms/__tests__/service.test.ts (lines 711-805, the describe block
starting with "firearms service — acquired date (U6)") and
src/domain/accessories/__tests__/service.test.ts (lines 238-325), define a live
constant as const live = process.env.DATABASE_URL ? describe : describe.skip
before the test suite, then replace the top-level describe call with live to
conditionally skip the entire suite when DATABASE_URL is unset.
In `@src/domain/service-intervals/__tests__/events-service.test.ts`:
- Around line 189-205: The future-date test currently passes for the wrong
ValidationError because the Cleaning rule is not armed. In the test around the
logServiceEvent call, add armCleaningRule for fa before invoking it, then assert
the rejection matches codes ["servicedOnInFuture"] instead of only checking the
error type; apply the same setup and specific-code assertion to the edit-path
test near the referenced section.
In `@src/domain/service-intervals/rules-service.ts`:
- Around line 308-349: Update updateServiceRuleDefault to preserve identity when
the default name changes: either reject renames or migrate all affected
service_rule and service_event name references within the same transaction,
resolving conflicts when an item already uses the target name. Ensure inherited,
overridden, suppressed, and historical event behavior remains associated with
the renamed default, and add coverage for each case.
In `@src/domain/validation-messages.ts`:
- Line 51: Update the duplicateName validation message to use scope-neutral
wording that is accurate for both category defaults and per-item rules. Preserve
assertNameAvailable’s behavior and the existing validation-message key; only
revise the message text so it does not specifically reference a category.
In `@src/lib/dates.ts`:
- Around line 23-27: Update isRealCalendarDate to reject ISO dates with year
"0000" before Date.parse and round-trip validation. Add regression coverage
showing validateFirearm and validateAccessory return their existing invalid date
errors for year-zero inputs.
---
Outside diff comments:
In `@app/`(app)/firearms/firearm-form.tsx:
- Around line 153-159: Update the firearm form’s validation handling to include
acquiredDateInFuture alongside invalidAcquiredDate: add it to focusOrder for
dateId and include it in the date Field error codes used by firstMessage.
Preserve the existing submit-blocking behavior while ensuring future dates
display an error and move focus to the date field.
In `@app/`(app)/summary/page.tsx:
- Around line 58-64: Update the empty-state condition in the summary page to
also require no service items due, using summary.itemsDue alongside the existing
magazine, firearm, and ammo checks. This must allow SummaryTables to render for
accessory-only owners when due entries exist, preserving access to the service
roll-up and bulk mark-serviced control.
---
Nitpick comments:
In `@app/`(app)/accessories/[id]/page.tsx:
- Around line 40-61: The actor-name mapping is duplicated across both detail
pages. In app/(app)/accessories/[id]/page.tsx lines 40-61, move/export
withActorNames and UNKNOWN_ACTOR_LABEL from a shared module next to
ServiceHistoryEntry; in app/(app)/firearms/[id]/page.tsx lines 105-125, replace
the local actorIds/nameById/serviceHistory logic with withActorNames(history)
and remove the local UNKNOWN_ACTOR_LABEL.
In `@app/`(app)/firearms/[id]/page.tsx:
- Around line 82-87: Update the Promise.all loading flow around getItemDueState,
listItemRules, and listServiceHistory so all three calls run through the
existing NotFoundError-to-notFound() guard used by their sibling loaders.
Extract or reuse that guard for the new service loaders, preserving the clean
404 behavior when authorization is revoked.
In `@app/`(app)/firearms/page.tsx:
- Around line 34-48: In app/(app)/firearms/page.tsx lines 34-48, start
listDueForVisibleCollection as a promise and resolve it within the existing
Promise.all alongside calibersForInput, visibleFirearmPermissions, and
primaryThumbnailsFor, then pass the resolved dueEntries to inventorySummary. In
app/(app)/accessories/page.tsx lines 43-47, move the listFirearms result into
the same promise batch and resolve listDueForVisibleCollection there using
firearms, preserving the existing dependent processing.
In `@app/`(app)/firearms/service-rules-panel.tsx:
- Around line 214-289: Replace the duplicated logic in reset, suppress, restore,
and remove with a shared runRuleAction helper that accepts the action callback,
success message, and failure fallback, while preserving the existing transition,
success mutation, and destructive error toast behavior. Update each handler to
call the helper with its corresponding service action and messages, including
removeItemOnlyRuleAction and its “removed” success text.
In `@app/`(app)/settings/service/__tests__/actions.test.ts:
- Around line 198-217: Add a delete-failure test in the
deleteServiceRuleDefaultAction suite that makes deleteServiceRuleDefault throw
NotFoundError for another owner’s default, then assert a non-leaking failure
result and no revalidation. Mirror the existing update-suite error-mapping setup
and assertions, while preserving the unauthenticated and success cases.
In `@app/`(app)/summary/__tests__/actions.test.ts:
- Around line 126-139: Add an assertion to the “a future servicedOn maps a
thrown ValidationError to a failed ActionResult with codes” test verifying that
revalidateCalls remains empty, matching the existing NotFoundError test and
preserving the cache-untouched behavior for validation failures.
In `@app/`(app)/summary/page.tsx:
- Around line 24-34: Update the summary loading flow around inventorySummary so
magazine and ammunition loading overlaps with listDueForVisibleCollection
instead of waiting for it; either start the due request in the existing
Promise.all and pass the resolved dueEntries to inventorySummary after firearms
are available, or move those loads into the page while keeping inventorySummary
computational.
In `@app/`(app)/summary/service-backlog-control.tsx:
- Around line 146-163: Update the backlog map in the row-rendering component to
derive each inputId from a dedicated row-specific prefix and the map index,
rather than dateId and rowKey. Keep rowKey for selection and React key behavior,
while ensuring every checkbox id is stable, unique within the list, and free of
user-provided spaces.
In `@e2e/service-intervals-sharing.spec.ts`:
- Around line 148-150: Update the service history assertion in the relevant test
to use the seeded identity artifact loaded near line 31 instead of the hardcoded
"service-intervals-viewer" pool key. Preserve the existing history region and
"Cleaning" assertions while interpolating the artifact value for the actor
check.
In `@src/domain/firearms/__tests__/validate.test.ts`:
- Around line 123-132: Update the “a real ISO calendar date is valid” test to
pass an explicit fixed asOf date to validateFirearm, matching the other
date-sensitive tests and ensuring the 2026-06-14 case remains independent of the
system clock.
In `@src/domain/service-intervals/__tests__/validate.test.ts`:
- Around line 43-61: Add coverage for the `intervalSessions` threshold in the
tests around `validateServiceRuleSet`: include valid positive sessions behavior
and invalid zero or negative sessions behavior, matching the existing
`intervalRounds` and `intervalDays` assertions. Ensure the tests also cover a
sessions-only rule so both `hasAnyThreshold` and `hasThresholdBelowMin` paths
are exercised.
In `@src/domain/service-intervals/due-service.ts`:
- Around line 300-318: Replace the duplicate firearmOriginDate and
accessoryOriginDate helpers with a single itemOriginDate helper using the shared
row shape and existing date-selection logic. Update all callers to use
itemOriginDate, removing the redundant helper and preserving current behavior.
- Around line 591-599: Rename the listDueForVisibleCollection parameter
preloadedFirearms to visibleFirearmsAlreadyLoaded and update its use when
calling loadVisibleItems. Preserve the existing behavior while making clear that
callers must provide firearms already filtered to the actor’s visible
collection.
In `@src/domain/service-intervals/events-service.ts`:
- Around line 104-157: Add a test covering authorization parity between
logServiceEvent and logServiceEventsBulk for the same firearm and actor across
owner, edit, view, and stranger states. Assert both paths allow or reject
identically, including the expected authorization error categories, so future
changes to single-path authorization cannot leave authorizeEventWritesBatch
inconsistent.
In `@src/domain/service-intervals/rules-service.ts`:
- Around line 536-546: Remove the stale documentation block immediately above
listItemRules in rules-service.ts, since it describes the deleted function
rather than listItemRules. Do not alter listItemRules behavior; only retain the
owner-based defaults detail if it is incorporated into the existing
getItemDueState documentation in due-service.ts.
- Around line 161-169: Update each write site that persists service rules to
catch database unique-constraint violations for the named constraints and
rethrow ValidationError(["duplicateName"]). Keep assertNameAvailable as the
pre-check optimization, while ensuring concurrent inserts receive the same
form-renderable validation error instead of the raw driver error.
In `@src/domain/service-intervals/validate.ts`:
- Around line 39-43: Update hasThresholdBelowMin to treat any set threshold that
is not an integer as invalid, alongside the existing MIN_THRESHOLD check, so
fractional and NaN values produce the normal validation error path.
In `@src/domain/summary/__tests__/summary.test.ts`:
- Around line 624-658: Update the query-count assertion in the
“inventorySummary's service roll-up loads in a bounded number of queries” test
to verify an upper bound rather than exact equality between largeCount and
smallCount. Keep the existing measurements and item-count assertions, and ensure
the assertion still detects per-item query growth without relying on
process-wide query-count isolation.
In `@src/domain/summary/summary.ts`:
- Around line 262-273: Update inventorySummary to accept the firearm shape
required by listDueForVisibleCollection, and when dueEntries is omitted, pass
the resolved or supplied firearms as its preloadedFirearms argument. Preserve
the existing dueEntries path and ensure the shared firearm value remains
available to both Promise.all operations.
In `@src/test-support/factories.ts`:
- Around line 217-233: Update makeServiceRuleDefault so its overrides type omits
ownerId from serviceRuleDefault.$inferInsert, matching the parent-key protection
used by makeServiceRule and makeServiceEvent. Keep the explicit ownerId argument
authoritative while preserving all other override fields.
- Around line 242-258: Update makeServiceRule so that when overrides.suppressed
is true, intervalRounds and the other service-rule threshold fields are
explicitly set to null after applying overrides and parent values. Preserve the
existing defaults for non-suppressed rules, ensuring suppressed factory rows
satisfy the schema CHECK without requiring callers to null thresholds manually.
🪄 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: ab356fc9-2822-409c-8238-10f22fce5bc4
📒 Files selected for processing (85)
CONCEPTS.mdapp/(app)/accessories/[id]/page.tsxapp/(app)/accessories/accessories-view.tsxapp/(app)/accessories/accessory-detail-view.tsxapp/(app)/accessories/accessory-form.tsxapp/(app)/accessories/page.tsxapp/(app)/firearms/[id]/page.tsxapp/(app)/firearms/__tests__/service-actions.test.tsapp/(app)/firearms/firearm-detail-view.tsxapp/(app)/firearms/firearm-form.tsxapp/(app)/firearms/firearms-view.tsxapp/(app)/firearms/log-service-form.tsxapp/(app)/firearms/page.tsxapp/(app)/firearms/range-session-form.tsxapp/(app)/firearms/service-actions.tsapp/(app)/firearms/service-history.tsxapp/(app)/firearms/service-rules-panel.tsxapp/(app)/inventory-log/log-entry-form.tsxapp/(app)/settings/page.tsxapp/(app)/settings/service/__tests__/actions.test.tsapp/(app)/settings/service/actions.tsapp/(app)/settings/service/default-rule-form.tsxapp/(app)/settings/service/page.tsxapp/(app)/settings/service/service-defaults-form.tsxapp/(app)/settings/service/types.tsapp/(app)/summary/__tests__/actions.test.tsapp/(app)/summary/actions.tsapp/(app)/summary/page.tsxapp/(app)/summary/service-backlog-control.tsxapp/(app)/summary/summary-tables.tsxdocs/plans/2026-08-02-002-feat-service-intervals-plan.mde2e/fixtures/user-pool.tse2e/inventory-log.spec.tse2e/service-intervals-bulk-sharing.spec.tse2e/service-intervals-bulk.spec.tse2e/service-intervals-rule-actions.spec.tse2e/service-intervals-sharing.spec.tse2e/service-intervals.spec.tsscripts/seed-demo.tssrc/backup/table-order.tssrc/db/__tests__/migration-0020-service-event-conversion.test.tssrc/db/__tests__/service-intervals-schema.test.tssrc/db/inventory-schema.tssrc/db/migrations/0019_perfect_nebula.sqlsrc/db/migrations/0020_naive_scarlet_spider.sqlsrc/db/migrations/0021_shocking_shooting_star.sqlsrc/db/migrations/meta/0019_snapshot.jsonsrc/db/migrations/meta/0020_snapshot.jsonsrc/db/migrations/meta/0021_snapshot.jsonsrc/db/migrations/meta/_journal.jsonsrc/demo/__tests__/inventory.test.tssrc/demo/inventory.tssrc/domain/accessories/__tests__/service.test.tssrc/domain/accessories/__tests__/validate.test.tssrc/domain/accessories/service.tssrc/domain/accessories/validate.tssrc/domain/firearms/__tests__/service.test.tssrc/domain/firearms/__tests__/validate.test.tssrc/domain/firearms/service.tssrc/domain/firearms/validate.tssrc/domain/inventory-log/__tests__/last-inventoried.test.tssrc/domain/inventory-log/__tests__/service.test.tssrc/domain/inventory-log/__tests__/validate.test.tssrc/domain/inventory-log/constants.tssrc/domain/range-sessions/validate.tssrc/domain/service-intervals/__tests__/backlog.test.tssrc/domain/service-intervals/__tests__/derive.test.tssrc/domain/service-intervals/__tests__/due-service.test.tssrc/domain/service-intervals/__tests__/events-service.test.tssrc/domain/service-intervals/__tests__/rules-service.test.tssrc/domain/service-intervals/__tests__/validate-event.test.tssrc/domain/service-intervals/__tests__/validate.test.tssrc/domain/service-intervals/backlog.tssrc/domain/service-intervals/constants.tssrc/domain/service-intervals/derive.tssrc/domain/service-intervals/due-service.tssrc/domain/service-intervals/events-service.tssrc/domain/service-intervals/rules-service.tssrc/domain/service-intervals/validate-event.tssrc/domain/service-intervals/validate.tssrc/domain/summary/__tests__/summary.test.tssrc/domain/summary/summary.tssrc/domain/validation-messages.tssrc/lib/dates.tssrc/test-support/factories.ts
Resolves CodeRabbit's review. Service history now shows who performed each service by display name rather than a raw account id, resolved once for both detail pages — a view-grantee reading a shared firearm's history sees a name, never another account's address. A rule name that no longer resolves surfaces its validation message on the log-service form instead of failing silently. Also corrects two assertions in the sharing spec that targeted a dialog the rule form does not use, and one that expected an email where the history deliberately renders a display name. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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)/settings/service/actions.ts:
- Around line 39-53: Update validateServiceRuleDefaultInput to accept unknown
and reject null or non-object values before accessing scope, category, or name,
returning the appropriate invalid-input code through
createServiceRuleDefaultAction’s ActionResult envelope. Add regression tests
covering null and undefined action inputs while preserving existing field
validation for objects.
In `@src/domain/service-intervals/actor-names.ts`:
- Around line 45-48: Update the actorName mapping in the event transformation to
return UNKNOWN_ACTOR_LABEL whenever nameById.get(event.actorId) misses,
including non-null deleted actor IDs; preserve named actors and existing
null-actor handling, and add a regression test covering an unresolved non-null
actor identity.
🪄 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: 29daefd0-84e5-461c-bf11-907e5563e6ec
📒 Files selected for processing (31)
app/(app)/accessories/[id]/page.tsxapp/(app)/firearms/[id]/page.tsxapp/(app)/firearms/firearm-form.tsxapp/(app)/firearms/log-service-form.tsxapp/(app)/firearms/service-history.tsxapp/(app)/firearms/service-rules-panel.tsxapp/(app)/settings/service/__tests__/actions.test.tsapp/(app)/settings/service/actions.tsapp/(app)/settings/service/default-rule-form.tsxapp/(app)/summary/__tests__/actions.test.tsapp/(app)/summary/page.tsxapp/(app)/summary/service-backlog-control.tsxdocs/plans/2026-08-02-002-feat-service-intervals-plan.mde2e/service-intervals-sharing.spec.tse2e/service-intervals.spec.tsscripts/seed-demo.tssrc/demo/__tests__/inventory.test.tssrc/demo/inventory.tssrc/domain/accessories/__tests__/validate.test.tssrc/domain/firearms/__tests__/validate.test.tssrc/domain/service-intervals/__tests__/events-service.test.tssrc/domain/service-intervals/__tests__/rules-service.test.tssrc/domain/service-intervals/__tests__/validate.test.tssrc/domain/service-intervals/actor-names.tssrc/domain/service-intervals/due-service.tssrc/domain/service-intervals/rules-service.tssrc/domain/service-intervals/validate.tssrc/domain/summary/__tests__/summary.test.tssrc/domain/validation-messages.tssrc/lib/dates.tssrc/test-support/factories.ts
🚧 Files skipped from review as they are similar to previous changes (21)
- src/demo/tests/inventory.test.ts
- src/domain/accessories/tests/validate.test.ts
- src/domain/firearms/tests/validate.test.ts
- src/domain/validation-messages.ts
- scripts/seed-demo.ts
- e2e/service-intervals.spec.ts
- app/(app)/firearms/log-service-form.tsx
- src/domain/summary/tests/summary.test.ts
- app/(app)/summary/page.tsx
- app/(app)/firearms/firearm-form.tsx
- app/(app)/summary/service-backlog-control.tsx
- app/(app)/firearms/service-history.tsx
- src/domain/service-intervals/rules-service.ts
- src/domain/service-intervals/due-service.ts
- app/(app)/firearms/service-rules-panel.tsx
- e2e/service-intervals-sharing.spec.ts
- src/demo/inventory.ts
- app/(app)/settings/service/default-rule-form.tsx
- docs/plans/2026-08-02-002-feat-service-intervals-plan.md
- app/(app)/accessories/[id]/page.tsx
- app/(app)/summary/tests/actions.test.ts
…id leak, guard malformed default input Three PR #99 review findings closed: Renaming a category default did not repoint the service_rule/service_event rows of items that inherit or override it. Since everything is keyed by rule name, a rename silently stranded every reachable item's service history under the old name, and the rule reappeared under the new name measuring from the item's origin date again instead of its last service. This is the same bug class already fixed for item-rule renames in updateItemRule, so updateServiceRuleDefault now repoints, in the same transaction: an inheriting item's service_event rows, and an overriding/suppressing item's service_rule row alongside its service_event rows. The rename lock (SELECT ... FOR UPDATE) and the pre-write duplicate-name rejection already used for item-rule renames are applied here too, and the whole repoint is scoped strictly to the owner's own items in the exact scope+category being edited. A genuinely item-only rule can never be caught in this repoint: since resolveEffectiveRules matches an item's own rule to a default by name alone within that item's own category, any item-rule row sharing the renamed default's old name is, by that same logic, already an override or suppression of it -- never item-only -- so there is no ambiguous case left to special-case. validateServiceRuleDefaultInput in the settings/service server action read input.scope before confirming input was a non-null object, so a malformed payload threw a raw TypeError instead of returning the normal failed ActionResult. Added the missing guard. The sibling action files (firearms/service-actions.ts, summary/actions.ts) have no equivalent pre-domain input validator of their own, so they don't share this shape and needed no matching change. withActorNames fell back to the raw account id when a service event's actorId was set but the account no longer existed (deleted between the event write and the read) -- exactly the raw-id leak the display-name resolution exists to prevent. It now falls back to UNKNOWN_ACTOR_LABEL like the null-actorId case already does. Tests added for all three: default-rename history repoint (inheriting item, overriding item, cross-owner isolation, duplicate-name rejection, sequential renames), a malformed non-object payload to the settings action, and a service-event actor-id lookup miss. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
coderabbitai
Bot
added
backend
documentation
…nd comments A future acquired date and several rule-action failures were rejected by the server but rendered nothing in the form — the submit button just looked dead. Every validation code a form's actions can return is now surfaced, and rule actions read the specific code instead of falling back to a generic message. The accessory detail page now degrades to a clean 404 like the firearm page when access is revoked mid-load, sharing one helper rather than two copies. All three category-default actions guard a malformed payload, not just create. The summary empty state no longer hides the service roll-up from an owner who has accessories but no firearms, magazines, or ammo. Due state is now a discriminated union, so a tripped axis exists exactly when a rule is due and callers stop null-checking what the logic already guarantees. The exactly-one-parent narrowing lives in one helper instead of three copies. Corrects comment rot the review surfaced: a KTD-7 citation that matched no decision in any plan, a docblock claiming firearms still carry maintenance event types after U5 left only inventoried, two docblocks stranded above the wrong function — one of them the cross-visibility trust contract — and cross-plan citations that read as belonging to this plan. Adds tests for the shared date helpers, which had been verified only through duplicated cases in three consumers. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
Owner
Author
|
Addressed the outside-diff-range findings from the review bodies (no inline thread to resolve on those), plus the review-toolkit findings, in
Fixed. This turned out to be one instance of a class rather than a one-off — three separate reviews found the same shape from different angles: this form,
Fixed. An owner with accessories but no firearms, magazines, or ammo passed all three checks and never saw the roll-up or the bulk control, even with items due — accessory due entries are built independently of the firearm set, so they were reachable in exactly that state.
Both already fixed in Same for the two inline items in the second review body — the Also in this commit, from a parallel review pass:
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/(app)/settings/service/actions.ts (1)
104-121: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject malformed default IDs before calling the domain service.
Line 108 validates only the update payload. Line 120 accepts every string. A non-UUID ID reaches the UUID-backed service query instead of returning
invalidPayload.Validate UUID format in
validateServiceRuleDefaultIdand call it from both actions. Add tests that assert no service call and no revalidation for"not-a-uuid".Proposed validation
+import { isUuid } from "`@/src/lib/uuid`"; + -function validateServiceRuleDefaultId(id: string): string[] { - return typeof id === "string" ? [] : ["invalidPayload"]; +function validateServiceRuleDefaultId(id: unknown): string[] { + return typeof id === "string" && isUuid(id) ? [] : ["invalidPayload"]; } export async function updateServiceRuleDefaultAction(id: string, input: ServiceRuleDefaultUpdateInput) { + const idCodes = validateServiceRuleDefaultId(id); + if (idCodes.length > 0) return { ok: false, codes: idCodes }; const codes = validateServiceRuleDefaultUpdateInput(input); if (codes.length > 0) return { ok: false, codes };As per coding guidelines, “Validate all external and user input at system boundaries and fail fast with clear errors.” As per path instructions, “Server Actions must resolve the session and authorization server-side, validate input, and return non-leaking errors via the ActionResult envelope.”
🤖 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 `@app/`(app)/settings/service/actions.ts around lines 104 - 121, Update both updateServiceRuleDefaultAction and deleteServiceRuleDefaultAction to call validateServiceRuleDefaultId alongside their existing validation, and enhance that validator to reject malformed UUIDs with invalidPayload. Return immediately before invoking updateServiceRuleDefault, the delete service, or revalidation; add tests confirming “not-a-uuid” produces no service call and no path revalidation.Sources: Coding guidelines, Path instructions
🤖 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/[id]/__tests__/service-props.test.ts:
- Around line 33-44: Restore the three spy handles created in the
service-props.test.ts beforeEach by adding their restoration in afterEach. In
actions.test.ts, retain handles for all three spyOn calls and restore each
handle in afterEach, ensuring no mocked implementations leak into later tests.
---
Outside diff comments:
In `@app/`(app)/settings/service/actions.ts:
- Around line 104-121: Update both updateServiceRuleDefaultAction and
deleteServiceRuleDefaultAction to call validateServiceRuleDefaultId alongside
their existing validation, and enhance that validator to reject malformed UUIDs
with invalidPayload. Return immediately before invoking
updateServiceRuleDefault, the delete service, or revalidation; add tests
confirming “not-a-uuid” produces no service call and no path revalidation.
🪄 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: 0c68e77e-875c-4f55-8b17-44d7ed0b7532
📒 Files selected for processing (27)
app/(app)/accessories/[id]/__tests__/service-props.test.tsapp/(app)/accessories/[id]/page.tsxapp/(app)/accessories/page.tsxapp/(app)/firearms/[id]/page.tsxapp/(app)/firearms/__tests__/service-actions.test.tsapp/(app)/firearms/service-actions.tsapp/(app)/firearms/service-history.tsxapp/(app)/firearms/service-rules-panel.tsxapp/(app)/inventory-log/log-entry-form.tsxapp/(app)/settings/service/__tests__/actions.test.tsapp/(app)/settings/service/actions.tsapp/(app)/summary/summary-tables.tsxsrc/db/__tests__/service-intervals-schema.test.tssrc/db/inventory-schema.tssrc/domain/accessories/validate.tssrc/domain/firearms/validate.tssrc/domain/service-intervals/__tests__/backlog.test.tssrc/domain/service-intervals/__tests__/events-service.test.tssrc/domain/service-intervals/__tests__/rules-service.test.tssrc/domain/service-intervals/derive.tssrc/domain/service-intervals/due-service.tssrc/domain/service-intervals/events-service.tssrc/domain/service-intervals/rules-service.tssrc/domain/summary/__tests__/summary.test.tssrc/lib/__tests__/dates.test.tssrc/lib/as-not-found.tssrc/test-support/factories.ts
🚧 Files skipped from review as they are similar to previous changes (17)
- app/(app)/firearms/[id]/page.tsx
- app/(app)/summary/summary-tables.tsx
- app/(app)/firearms/service-history.tsx
- src/domain/firearms/validate.ts
- app/(app)/accessories/page.tsx
- app/(app)/inventory-log/log-entry-form.tsx
- src/domain/service-intervals/tests/backlog.test.ts
- app/(app)/firearms/service-rules-panel.tsx
- src/db/tests/service-intervals-schema.test.ts
- app/(app)/firearms/tests/service-actions.test.ts
- src/domain/summary/tests/summary.test.ts
- src/test-support/factories.ts
- app/(app)/firearms/service-actions.ts
- src/domain/service-intervals/derive.ts
- src/domain/accessories/validate.ts
- src/domain/service-intervals/due-service.ts
- src/domain/service-intervals/tests/events-service.test.ts
bun test app runs every file in one process and Bun's spyOn replacements persist until restored, so six unrestored spies across two new test files could have altered whichever file ran next. Both now restore per handle in afterEach, matching the convention the firearm and summary action tests already follow. This is the same cross-file contamination that already surfaced twice on this branch through mock.module. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
unclesp1d3r
deleted the
10-add-service-interval-tracking-and-in-app-maintenance-reminders-for-firearms-and-accessories
branch
August 5, 2026 00:48
Open
3 tasks done
2 of 4 tasks
3 tasks done
unclesp1d3r
added a commit
that referenced
this pull request
Aug 5, 2026
…ranch (#102) This repo squash-merges with `squash_title: PR_TITLE` and `squash_msg: PR_BODY`, so the PR title and body *are* the commit message and every branch commit message is discarded at merge. That means a breaking change marked only on a branch commit silently disappears. It already happened: #99 retired the `cleaned` and `lubed` inventory-log event types and irreversibly converted their history. The branch carried `feat(service-intervals)!: retire cleaned and lubed log events`, but the squash subject on `main` is `feat(service-intervals): track service intervals and maintenance reminders (#99)` — no `!`, and no `BREAKING CHANGE:` footer either, because the PR body used a prose "Breaking change" heading rather than the conventional footer. Nothing here parses commits to compute a version — releases are cut by pushing a `v*.*.*` tag by hand — so these markers are the only surviving record of what the next version should be. ## Changes `AGENTS.md` gains an explicit rule under **Workflow & boundaries**: the PR title must be a valid Conventional Commit subject, and a breaking change must carry `!` in the title *and* a `BREAKING CHANGE:` footer in the body, with a note that marking it on a branch commit does not work. `.coderabbit.yml` is updated in two places that both described the format without `!`: - `auto_title_instructions` — CodeRabbit generates title suggestions from this, so as written it would never produce the `!` form and could suggest stripping it. Now includes the marker, a breaking example, and why it must not be dropped. - `pre_merge_checks.title.requirements` — now states `!` is valid and must not be flagged, and lists the signals that require it. ## Follow-up, not in this PR Given #99 shipped a breaking change, the next release should be **v2.0.0** rather than v1.6.0. Current latest tag is v1.5.0. ## Test plan - [x] `just ci-check` passes - [x] `.coderabbit.yml` parses as valid YAML (`check-yaml` + js-yaml) - [ ] Confirm CodeRabbit's title check accepts this PR's title, which deliberately uses the plain form (this change is not itself breaking) - [ ] Confirm a future `!` title passes the check rather than being flagged --------- Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #10.
Tracks service intervals for firearms and accessories: an owner sets per-category default maintenance rules, items inherit them live, and due state is derived from real usage rather than stored.
Plan:
docs/plans/2026-08-02-002-feat-service-intervals-plan.mdWhat this does
An owner defines default rules per category — Cleaning, Barrel, and so on — each setting at least one of three thresholds: elapsed days, range sessions, or rounds fired. Every firearm and accessory in that category inherits them immediately, with no per-item setup. An item can override a rule's thresholds, suppress it entirely, or carry item-only rules no default defines.
A rule is due when elapsed days, sessions, or rounds meet or exceed any threshold it sets. Counting measures from the last time that rule was serviced, or from the item's origin date when it never has been. This is binary and advisory: distance past a threshold shows as raw counts, never a severity tier, and nothing is blocked, gated, or escalated.
Counting from day one means a mature collection lights up on arrival — that is deliberate (KD6: honest totals beat zeroed ones), so
/summarycarries a backlog checklist that marks many items serviced in one action.Notable decisions
cleaned/lubedconversion avoid inventing threshold values.parent_typeshape used bygrantandinventory_log— nativeON DELETE CASCADEreplaces a hand-written cleanup trigger.cleanedalready permitted), rule configuration is owner-only, and accessories are owner-only throughout since they are not a grantable parent type.Breaking change
inventoriedis now the only firearm inventory-log event type. Every existingcleanedentry becomes aCleaningservice event and everylubedentry aLubricationone, preserving actor, notes, and insertion time.This migration is irreversible and has no down-migration. It reconciles row counts inside a PL/pgSQL block — rows read must equal rows inserted and rows deleted — and raises on any mismatch, rolling the whole migration transaction back rather than silently losing history. It assumes no other writer is inserting
cleaned/lubedrows during the migration window; that assumption is documented in the migration itself.Converted events carry no rules with them by design. History reattaches once an owner creates a rule of the matching name.
Scope added after the plan was approved
Three items were pulled in during implementation, each recorded in the plan's Scope Boundaries:
Review
Nine simplification findings and every code-review finding were fixed in this branch; none were deferred to follow-up. The review caught, among others:
Test plan
just ci-checkgreen before every one of the 15 commits — lockfile, lint, format, typecheck, pre-commit,bun test, and the full e2e suite. Never bypassed.Asia/Tokyo, andAmerica/New_Yorkdb:generateproduces no diffdata-testidintroduced; e2e targets ARIA roles, accessible names, and visible text