test(shipping): give the suite teeth — role gates, audit assertions, RPC integration (SR-4) - #62
Merged
Merged
Conversation
…RPC integration (SR-4) The suite was structurally blind to the three things CLAUDE.md declares sacred: you could delete every writeAudit call, downgrade every editor gate to viewer, or regress any RPC body, and 200+ tests stayed green. - NEW role-gates.test.ts: one table pins the min-role all 29 shipping withRole call sites request (the per-route mocks discard it); a count assertion catches unregistered new routes - writeAudit assertions added to the 9 mutation suites that mocked it without ever asserting (parcels create/update, shipments create/update, attach, advance, pack-request update/attach, request update) + not- called-on-4xx checks on the parcels paths - NEW export suite: BOM bytes, ¥ formatting, audit row, and formula- injection neutralization incl. the leading-whitespace bypass - parcels suite: proves the route actually passes SANITIZED search terms into .or() (the pure-function test alone couldn't) - NEW lib/shipping/__tests__/rpc.integration.test.ts (RUN_DB_TESTS-gated, dev-branch only): pins the SQL invariants nothing executable covered — advance skip-matrix (never touches picked_up), attach in-txn re-check, detach roundtrip, partial-pack-attach non-approval + re-run approval, one-open-request 23505, confirm-by-token idempotency, revert reason on the timeline, bulk-receive id sets, reassign, token-uniqueness trigger Migration 20260703000006: writing the integration suite exposed that 20260703000001's approval predicate (newly-moved == total) could NEVER approve on the sanctioned re-run flow (the re-run only moves stragglers). Approval now checks "every linked parcel sits on this batch"; the route prefers the RPC's authoritative flag with a pre-apply count fallback. 299 tests green (25 skipped: DB-gated suites); tsc clean; build green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThis PR fixes the ChangesPack request approval fix and shipping test coverage
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant AttachRoute as attach/route.ts
participant RPC as admin_attach_pack_request
participant DB as Parcels/Shipment tables
Client->>AttachRoute: PATCH attach pack request
AttachRoute->>RPC: call admin_attach_pack_request(shipment_id)
RPC->>DB: lock request, aggregate linked parcel IDs
RPC->>DB: count parcels on target shipment vs total
DB-->>RPC: v_on_batch, v_total
RPC->>RPC: approved = (v_on_batch == v_total)
RPC-->>AttachRoute: { approved, total, attached }
AttachRoute->>AttachRoute: prefer r.approved, else fallback count check
AttachRoute-->>Client: response with approval status
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bia-admin/app/api/admin/shipping/pack-requests/[id]/attach/route.ts (1)
127-140: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winStale comment contradicts the new approval logic.
Lines 129-130 still state
Compute approved from the counts (not the RPC's key), but the code right below (Lines 137-140) now does the opposite: it prefersr.approvedwhen the RPC provides it, falling back to the count comparison only otherwise. This will mislead future readers about the actual precedence.✏️ Proposed fix
// attached < total means some parcels were ineligible (not received_cn) and // the RPC skipped them — the request then stays attachable for a re-run. - // Compute `approved` from the counts (not the RPC's key) so this route - // stays correct against the pre-20260703000001 RPC, which always approved. const attachedCount = r.attached ?? 0; const total = r.total ?? 0; const skipped = total - attachedCount;🤖 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 `@bia-admin/app/api/admin/shipping/pack-requests/`[id]/attach/route.ts around lines 127 - 140, The comment above the approval calculation in the attach route is stale and contradicts the current logic. Update the nearby explanatory comment in the route handler so it matches the behavior in `approved`: `r.approved` is authoritative when present, and the count comparison is only the fallback. Keep the wording aligned with the `attachedCount`, `total`, and `approved` symbols so future readers understand the actual precedence.
🧹 Nitpick comments (3)
bia-admin/app/api/admin/shipping/shipments/[id]/export/__tests__/route.test.ts (2)
97-103: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider asserting
entity_typetoo.The audit assertion checks
action,entity_id, andpayloadbut notentity_type: "shipment". Minor completeness gap for pinning the full audit contract.🤖 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 `@bia-admin/app/api/admin/shipping/shipments/`[id]/export/__tests__/route.test.ts around lines 97 - 103, The audit test for the shipment export route only verifies action, entity_id, and payload, so it does not fully pin the audit contract. Update the assertion in the export route test to also check entity_type: "shipment" in the writeAuditMock call, using the existing shipment export test case as the place to tighten the expectation.
119-124: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFragile cell-splitting via
split('","').Splitting rows on the literal
'","'delimiter assumes no cell content ever contains that exact sequence. It works for current fixtures but is brittle if test data changes.🤖 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 `@bia-admin/app/api/admin/shipping/shipments/`[id]/export/__tests__/route.test.ts around lines 119 - 124, The test in route.test.ts is using a brittle CSV cell parser by splitting each row with split('","'), which will break if any exported field contains that exact sequence. Update the assertion logic in the shipment export test to parse cells in a way that respects CSV quoting, or otherwise inspect the exported rows without relying on a literal delimiter. Keep the existing safety check in the test, but apply it through a more robust row/cell iteration approach around the text processing block.bia-admin/lib/shipping/__tests__/rpc.integration.test.ts (1)
116-135: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInconsistent
errorassertions before accessing.datarisk unhelpful failures.Several RPC calls destructure only
dataand immediately dereference it (e.g. Line 118const { data } = await admin.rpc(...)then Line 123data.received_at; similarly Lines 262-272, 280-289, 328-333, 352-357) without first assertingerroris null. Other tests in the same file (e.g. Lines 211-224, 381-388, 401-406) do checkerrorexplicitly. On an unexpected RPC error, the un-checked variants will throw a genericCannot read properties of nullinstead of a clear assertion message, making live-DB failures harder to diagnose.♻️ Example fix pattern
- const { data } = await admin.rpc("admin_patch_parcel", { + const { data, error } = await admin.rpc("admin_patch_parcel", { p_id: p.id, p_actor_user_id: ACTOR, p_patch: { status: "received_cn" }, }); + expect(error).toBeNull(); expect(data.received_at).toBeTruthy();Also applies to: 262-291, 328-333, 352-357
🤖 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 `@bia-admin/lib/shipping/__tests__/rpc.integration.test.ts` around lines 116 - 135, Several rpc integration tests in admin_patch_parcel and related cases destructure only data and use it before confirming the RPC error state, which can hide the real failure; update these calls to assert error is null (or otherwise check the RPC result) immediately after each admin.rpc invocation before dereferencing data, matching the safer pattern already used elsewhere in this test file, so failures in admin_patch_parcel and similar helpers surface a clear assertion instead of a null property access.
🤖 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 `@bia-admin/app/api/admin/shipping/pack-requests/`[id]/attach/route.ts:
- Around line 127-140: The comment above the approval calculation in the attach
route is stale and contradicts the current logic. Update the nearby explanatory
comment in the route handler so it matches the behavior in `approved`:
`r.approved` is authoritative when present, and the count comparison is only the
fallback. Keep the wording aligned with the `attachedCount`, `total`, and
`approved` symbols so future readers understand the actual precedence.
---
Nitpick comments:
In
`@bia-admin/app/api/admin/shipping/shipments/`[id]/export/__tests__/route.test.ts:
- Around line 97-103: The audit test for the shipment export route only verifies
action, entity_id, and payload, so it does not fully pin the audit contract.
Update the assertion in the export route test to also check entity_type:
"shipment" in the writeAuditMock call, using the existing shipment export test
case as the place to tighten the expectation.
- Around line 119-124: The test in route.test.ts is using a brittle CSV cell
parser by splitting each row with split('","'), which will break if any exported
field contains that exact sequence. Update the assertion logic in the shipment
export test to parse cells in a way that respects CSV quoting, or otherwise
inspect the exported rows without relying on a literal delimiter. Keep the
existing safety check in the test, but apply it through a more robust row/cell
iteration approach around the text processing block.
In `@bia-admin/lib/shipping/__tests__/rpc.integration.test.ts`:
- Around line 116-135: Several rpc integration tests in admin_patch_parcel and
related cases destructure only data and use it before confirming the RPC error
state, which can hide the real failure; update these calls to assert error is
null (or otherwise check the RPC result) immediately after each admin.rpc
invocation before dereferencing data, matching the safer pattern already used
elsewhere in this test file, so failures in admin_patch_parcel and similar
helpers surface a clear assertion instead of a null property access.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8bf40fe4-a5c1-4c04-b890-477cd6833aaa
📒 Files selected for processing (14)
bia-admin/app/api/admin/shipping/__tests__/role-gates.test.tsbia-admin/app/api/admin/shipping/pack-requests/[id]/__tests__/route.test.tsbia-admin/app/api/admin/shipping/pack-requests/[id]/attach/__tests__/route.test.tsbia-admin/app/api/admin/shipping/pack-requests/[id]/attach/route.tsbia-admin/app/api/admin/shipping/parcels/[id]/__tests__/route.test.tsbia-admin/app/api/admin/shipping/parcels/__tests__/route.test.tsbia-admin/app/api/admin/shipping/requests/[id]/__tests__/route.test.tsbia-admin/app/api/admin/shipping/shipments/[id]/__tests__/route.test.tsbia-admin/app/api/admin/shipping/shipments/[id]/advance-parcels/__tests__/route.test.tsbia-admin/app/api/admin/shipping/shipments/[id]/attach/__tests__/route.test.tsbia-admin/app/api/admin/shipping/shipments/[id]/export/__tests__/route.test.tsbia-admin/app/api/admin/shipping/shipments/__tests__/route.test.tsbia-admin/lib/shipping/__tests__/rpc.integration.test.tssupabase/migrations/20260703000006_pack_request_approve_on_batch.sql
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
1 participant
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.
Summary
Final PR of the 2026-07-03 shipping refinement — the P1 test-blindness cluster. Before this, three guardrail regressions passed the whole suite silently: deleting every
writeAuditcall, downgradingwithRole("editor")toviewer, or regressing any RPC body (all route tests mockrpc()).New coverage
role-gates.test.ts— one table pins the min-role of all 29 shippingwithRolecall sites (the withRole mock returns 204 before the handler body runs, so no supabase mocks needed). A count assertion flags unregistered new routes.shipment.exportaudit, and formula-injection neutralization including the leading-whitespace bypass.GET /parcelsactually feedssanitizeSearchTermoutput into.or()— the unit test alone couldn't catch the route dropping the call.rpc.integration.test.ts(RUN_DB_TESTS-gated, dev branch only, marked fixtures + cleanup): executable pins for the most-patched SQL surface — advance skip-matrix (never touchespicked_up), attach in-txn re-check, detach roundtrip, pack-request partial/re-run approval, one-open-request23505, confirm-token idempotency, revert reason on the timeline, bulk-receive id sets, reassign, token-uniqueness trigger.Bug found by writing the tests → migration
2026070300000620260703000001approved a pack request only when newly-moved count == total — so the sanctioned re-run flow (partial attach → stragglers received → attach again) could never reachapproved: the re-run only moves the stragglers. Approval now checks "every linked parcel sits on this batch after the attach";attachedstill reports newly-moved for the officer's toast; the route prefers the RPC's flag with a pre-apply count fallback.Verification
299 passed / 25 skipped (DB-gated), tsc clean, lint clean (repo files), production build green.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes