test(payouts): e2e coverage for reprice/pay/revert/pay and member payout view - #83
Merged
Conversation
…nation Phase 1 shipped the usable core; this specs what makes it pleasant, plus the nine defects it knowingly deferred. One PR. Two decisions worth a reviewer's attention: - Derived payment state comes from paidAmount, not a fold of payout_payment events. payout_payment.at is defaultNow() = transaction START time, so a transaction that starts earlier can take the operation lock later and insert an out-of-order timestamp; id is defaultRandom() and no tiebreak. The fold the phase-1 design called for would read a reverted payment as paid. Fixing it properly needs a monotonic sequence column, i.e. a migration. - esi-ui.open_window.v1 goes into EVE_SSO_SCOPES globally, which flips every existing character to needs_reauth on the next token-health run. Verified that sync keeps working (contacts.ts gates per job), so it is a fleet-wide false alarm that self-heals as members log in, not an outage. Recorded in the spec rather than discovered on deploy day. Also corrects the handover: multiple pools per operation already works end to end. It is a test gap, not a feature gap.
- Payment ordering: write `at` as clock_timestamp() inside the existing operation lock, define display order as (at asc, id asc). Removes the reversed pay/revert display without a sequence-column migration. - Manual prices: exactly two decimals, exact cents x qty, bounded against the numeric(20,2) range. Names the appraised/manual inconsistency. - Absurd quantities: bound at MAX_SAFE_INTEGER (a correctness bound, since loot_item.qty is bigint mode:number) plus a separate total-value bound. - Pagination: three queries, not four scoped. The payment query is deleted, not bounded; payout_payment has no operationId anyway. - "Your payouts": finalized operations only, with the reasoning.
Thirteen TDD tasks covering the phase-2 design: revert, manual item pricing, additive roster entry, the open-info express path, member-facing payout rows, and all nine deferred defects. Also corrects three stale claims in the spec, each verified against the source: phase 1 already tests both loot_item constraints through addAppraisedPool, so only direct-insert and the totalValue < 0 branch are missing; four sites compare granted scopes against config, not three, and the admin table's comparison lives in account-view.ts rather than the page that renders it.
Define the missing revertPaymentAction, re-read the open-info target server-side instead of trusting a caller-supplied character id, branch ESI failures instead of calling every one "offline", enforce the IEEE-754 exactness bound on appraised line totals, clamp payment timestamps forward under the operation lock, resolve the payment actor, and add the rollout and completion-workflow steps.
computeSplit accepted a negative total and a corp share outside 0-100, producing a plausible split that only died later as a raw check-constraint error. Also drops perShareCents, which nothing read, and adds MAX_MONEY_CENTS for the callers that have to keep money inside numeric(20,2).
…d ones A zero-quantity line vanished with no signal, a bare quantity became an item literally named "12", and a 30-digit quantity died downstream as a raw Postgres error. parseLootPaste now returns those lines with a reason, and appraiseLoot carries them to the caller. Nothing is rejected wholesale: a mostly-good paste still appraises. Bounding the quantity did not make the line total exact - the total is a float product, and past 2^53 cents the representable values are 16 cents apart, so 1000000.01 ISK x 1e9 units stored a cent low with nothing to catch it. The product is now bounded too, before the BigInt conversion launders it, and a line past ~90 trillion ISK says so by name.
…sally recordPayment and loadParticipantOperationId threw bare Errors that callers could not tell from a programming mistake; they now throw PayoutNotFoundError alongside the existing forbidden/locked pair. payout_payment.at defaulted to now(), which is transaction START time, so a transaction that took the operation lock later could still record an earlier instant than the event before it. The insert now supplies a clock reading taken from inside the lock and clamped forward past that participant's latest row, so a participant's history is strictly increasing rather than merely usually increasing — clock_timestamp() alone repeats at clock resolution and can step backwards under NTP. The cost is that after a backwards step the timestamp reads later than the wall clock until it catches up; order is what a reader of this history needs, and the instant is what is traded for it. No migration: the column default is untouched, it is simply not what the writer uses.
paidAmount, not the payment log, is now what says whether someone has been paid — one column written under the same lock that reads it. Reverting does not reopen the operation's numbers.
…c overflow shares is numeric(6,2); 10000 was a raw Postgres error. Bounded with a readable message in the service and the action rather than widening the column, which would cost a migration for the sake of a sentence. The action appends its bound to the format/positivity order #74 established rather than replacing it: the guard it would have replaced throws, and throwing from that control is the defect #74 removed.
Additive, unlike setRoster, so share edits survive. Resolves through the same path the paste does, so an alt collapses into their main instead of drawing a second share, and a repeated unresolved name is refused rather than warned about after the fact. Both refusals redirect with a code the page renders, following the conversion #74 made across this file: a blank name box is not a fault on our end, and error.tsx is the only thing a throw can say.
Manual prices are exact to the cent, so the line total is a bigint product with nothing to round. Line and pool totals past numeric(20,2) now say so instead of surfacing a Postgres overflow, and the loot_item constraints get tests that name them.
listPayoutOperations issued four queries, three unbounded: every loot_pool, payout_participant and payout_payment row in the database, folded in memory. The payment query is deleted rather than scoped -- paidAmount already answers what it was consulted for, and payout_payment has no operationId to scope by. The other two are scoped to the page, which keyset pagination now bounds. The cursor is composite (occurredAt, id): occurredAt is not unique and the id is a random uuid, so a single-column cursor skips every operation sharing a date across a page boundary.
An Older link carrying the composite cursor, matching the audit pager. The heading no longer states a count unless the page provably holds every operation -- a page count presented as a total is wrong the moment a 51st operation exists. A cursor past the end gets an exit link rather than looking like an empty database.
The scope belongs to the token making the call, so the gate reads the main character's persisted scopes column rather than config: config says what login asks for, and an operator who authorized before the scope existed has a valid session without it.
Second documented exception to enqueue-don't-execute, justified in the code: the call persists no state anywhere, so a lost call is a re-click and a duplicate opens the window twice. The action takes a participant id and re-reads the recipient character from the database, so an operator cannot aim their own token at an arbitrary character by editing the posted id. Failures are classified from what ESI actually said -- offline only when its own body says so -- and the 30s client timeout, which is a DOMException rather than an EsiError, no longer escapes as a raw 500. EVE_SSO_SCOPES gains esi-ui.open_window.v1. This flips every existing character to needs_reauth until its holder logs in again and writes one audit row each -- noise, not an outage, since every job gates on the scopes it actually needs. docs/ops.md now says so where the variable is documented.
eslint's no-html-link-for-pages fires on a literal <a href="/payouts"> because that string matches a static page route exactly -- the "Older" pager link escapes it only because its href is a template literal, not a bare string. Matches the repo convention (Link for static in-page routes; <a> for hrefs built from an expression) already used for the other links on this page.
Add the fly secrets set / fly deploy rollout for an already-running deployment to the "Adding an SSO scope" section, transcribed from Task 10's Step 23 -- without it a maintainer reading only docs/ops.md would learn what the scope change does but not that they must run it, and skipping it is a silent no-op. Also note in getOpenInfoTarget's docblock that it takes no actor and calls no operator guard, so a future second caller must add its own gate.
…add one participant at a time
…l members what they are owed The history list names the operator who recorded each event, resolved the way audit rows resolve an actor -- account id to that account's main character's name. payout_payment.actor has been written since phase 1 and never read; the column already existed, so nothing migrates. It renders 'unknown' when the actor's account was deleted (the FK is on delete set null) or has no main character, which are indistinguishable here. revertPaymentAction catches nothing. Every failure revertPayment can raise is authorization or lifecycle state, never something the operator typed, and #74 drew exactly that line across this file: input rejections redirect with an ?error= code, everything else belongs on error.tsx. This action has no input to reject, so it adds no code to the page's ERRORS map. The revert control arms before it fires, joining the delete-pool, remove- participant, Finalize and first-mark-paid controls #74 already armed inside this table's ConfirmArmScope. Also fixes a defect flagged by earlier tasks: the page's arm-gating const for "mark paid" (renamed anyPaid -> firstPayment) now derives from `locked` (hasPayments) instead of `paymentState === "paid"`. Reverting the only payment on an operation left `locked` true but flipped the old anyPaid to false, re-arming a confirm step for a door that was already shut for good.
…r payout view end to end
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 12 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
The unit-price control threw on a malformed value while every other money input on the page redirects with a typed ?error= code, so a pasted comma-grouped number sent the operator to the generic error boundary with no idea which field was wrong. Add type=number/step to match the shares input, redirect on rejection with a new price_invalid message, and cover it end to end. Also pin the payout.item_repriced audit target and the MAX_MONEY_CENTS boundary, two cheap gaps the same review flagged.
# Conflicts: # src/app/payouts/[id]/page.tsx
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 27
🤖 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 @.env.example:
- Line 41: Quote the space-separated value assigned to EVE_SSO_SCOPES, matching
the quoted configuration in the operational documentation, so dotenv tooling
preserves the complete scope list.
In `@docs/superpowers/plans/2026-08-04-fight-payout-tracking-phase-2.md`:
- Around line 2614-2633: Update setItemPriceAction to route invalid, negative,
and out-of-range prices through the established operationFailed error contract
instead of allowing plain Error instances to escape. Add or reuse price-specific
ERRORS entries and ensure the operation detail page maps those codes for
error.tsx, preserving the existing successful pricing and revalidation flow.
- Line 77: Fix all reported Markdown lint violations in the document: add
appropriate language tags to every fenced code block triggering MD040, insert
blank lines immediately before and after the fences at the locations reported
for MD031, and rewrite “#74 left this file...” as valid prose such as “Issue `#74`
left this file...” to resolve MD018.
- Around line 6493-6500: Update the payment-status assertions in the finalize
and pay-again flows, including the matching section around the second referenced
range, so they verify an exact paid status rather than using
toContainText("paid"), which also matches unpaid. Assert the row’s status text
exactly or additionally confirm that unpaid is absent.
- Line 25: Keep the open-information error classifier outside the pure
`src/core/` boundary: update the proposed `open-info-error.ts` and its consumers
so they do not import the runtime `EsiError` from `@/lib/esi/client`. Place the
classifier beside `EsiError`, or move the shared error type into a
dependency-neutral core module, while preserving the existing classification
behavior.
- Around line 4728-4743: Update the dropped-reason validation in the decoder to
use an own-property check against DROPPED_REASONS instead of the
inherited-property-inclusive in operator. Keep only reasons that are direct keys
of DROPPED_REASONS before returning the cleaned sample, so crafted values such
as toString are rejected.
- Around line 653-664: Validate quantity text in parseQty before any item is
emitted: accept only plain digits or correctly grouped comma-formatted digits,
and reject non-finite or non-integer results. Update the quantity parsing paths
using QTY_PREFIX, QTY_SUFFIX, QTY_COMMA, and QTY_ONLY so malformed values such
as “1,,2” cannot produce an item with qty: NaN.
- Around line 946-1029: The timestamp test in “payment history is ordered as it
happened” incorrectly requires cross-participant timestamps and UUID ordering.
Replace its two-participant scenario with a single participant performing pay →
revert → pay, and assert the resulting payment timestamps/order according to the
per-participant guarantee enforced by nextPaymentAt; remove expectations that
timestamps or ordering are strictly increasing across different participants.
- Around line 4472-4492: Replace the proposed ?dropped= payload flow around
encodeDropped and the page rendering logic so operator-pasted line samples are
never placed in the URL. Carry only non-sensitive counts and reason codes, or
use a short-lived opaque server-side reference, while preserving the existing
behavior of rendering no notice for malformed or unrecognized values.
- Around line 551-565: Update the line-total calculation used by appraiseLoot to
use exact decimal arithmetic rather than multiplying floating-point price and
quantity values before rounding. Ensure values such as price 48804.84 with
quantity 1845177173 produce the exact cent total, while preserving the existing
Number.MAX_SAFE_INTEGER validation and formatted totalValue outputs; expand the
nearby boundary coverage if needed to exercise this precision case.
In `@docs/superpowers/specs/2026-08-04-fight-payout-tracking-phase-2-design.md`:
- Around line 61-67: Add language identifiers to the opening fenced code blocks
around the transaction-order examples at the referenced sections, using text or
another accurate identifier. Update both affected fences while leaving their
example content unchanged.
- Around line 72-84: The design documentation must not claim causal or strictly
increasing ordering while accepting clock rollback and equal timestamps. Update
the timestamp-ordering discussion, the causal-order claim near the referenced
phase-2 behavior, and the strict-increase test specification to consistently
require only deterministic `(at asc, id asc)` ordering, unless the
implementation is changed to enforce monotonic per-operation ordering.
In `@e2e/payouts.spec.ts`:
- Around line 856-864: Update the first “mark paid” click in the payout flow to
use the rowFor("Brain Tartare") locator, matching the row-scoped action already
used later, instead of relying on .first(). Keep the existing confirmation and
exact status assertion unchanged.
In `@src/app/account/account-payouts.tsx`:
- Around line 4-6: Extract the duplicated fmtDate helper into a shared module
under src/app/_components/ and update the account payouts page plus both payout
page modules to import and reuse it. Remove their local function definitions
while preserving the existing YYYY-MM-DD formatting behavior.
In `@src/app/account/page.tsx`:
- Around line 131-135: Update the account page data-loading flow around
canReadPayouts and listAccountPayouts to start both independent database reads
concurrently and await their combined results, preserving the existing arguments
and result assignments.
In `@src/app/payouts/`[id]/page.tsx:
- Around line 515-540: Add numeric input attributes to the unit-price field in
the form using `setItemPriceAction`, including appropriate numeric type, step,
and nonnegative minimum constraints. Update `setItemPriceAction` to validate the
submitted price server-side, add the corresponding item-price `ERRORS` code, and
redirect with that code instead of throwing when validation fails.
In `@src/app/payouts/actions.ts`:
- Around line 269-286: Update the unitPrice validation in setItemPriceAction to
route invalid typed input through operationFailed instead of throwing, using a
new price_invalid error code while preserving the existing validation rule. Add
the matching price_invalid message to the detail page ERRORS map and include the
code and expected message in the relevant bypassClientGuard coverage table in
e2e/payouts.spec.ts.
In `@src/app/payouts/dropped.ts`:
- Around line 71-82: In the dropped-sample filter within the clean-building
flow, replace the DROPPED_REASONS `in` check with an Object.hasOwn check so only
declared reasons pass the type predicate. Extend the relevant payout-dropped
test case to include a `"toString"` reason and verify it is rejected.
In `@src/app/payouts/page.tsx`:
- Around line 146-155: Replace the “Older” anchor in the nextCursor pagination
block with the existing next/link Link component, preserving its href,
className, label, and accessibility markup. Do not retain a plain anchor unless
the implementation documents a specific reason.
In `@src/services/payouts.ts`:
- Around line 337-355: Extend the duplicate validation in the roster-entry
handling flow around the accountId check so a resolved entry also compares its
displayName case-insensitively against existing unresolved rows. Reuse the
existing clash detection and PayoutDuplicateParticipantError behavior, while
preserving the current accountId duplicate check and unresolved-name handling.
In `@tests/account-payouts.test.ts`:
- Around line 50-56: Update the test case “shows the exact stored amount and
each paid state” to assert the paid and unpaid states without substring
matching, using exact or boundary-aware checks so an “unpaid” row cannot satisfy
the “paid” assertion. Preserve the existing amount assertions and verify that
both distinct states are rendered.
In `@tests/payment-history.test.ts`:
- Around line 26-46: Update the PaymentHistory test to assert that the rendered
HTML contains the accessible disclosure aria-label built from participantName
"Brain Tartare", using the existing render helper and the component’s
established label format.
In `@tests/payout-dropped.test.ts`:
- Around line 39-44: The long-line truncation test should use the shared
DROPPED_LINE_CHARS constant rather than the literal 120. Import
DROPPED_LINE_CHARS alongside DROPPED_SAMPLE_LIMIT and assert the decoded line
length equals that cap, preserving the existing truncation scenario in the test.
In `@tests/payout-loot.test.ts`:
- Around line 441-542: Add a multi-item test within the setItemPrice suite,
using the existing seeding helpers or setup patterns to create two loot lines in
one pool, then reprice only one line via setItemPrice. Assert the pool total
equals the unchanged sibling line total plus the repriced line total, proving
pool-total recalculation includes all item rows rather than only the updated
line.
In `@tests/payout-schema.test.ts`:
- Around line 113-172: Extract the duplicated payoutOperation and lootPool
insertion setup from the tests into a local helper that creates the operation
and returns the created pool. Update both “loot_item_qty_ck” and
“loot_item_price_ck” tests to use this helper while preserving their existing
assertions and test data.
In `@tests/payout-view.test.ts`:
- Around line 536-552: Add coverage for the null actor branch in the test
covering getPayoutOperationDetail: import the payoutPayment schema symbol,
capture the recorded payment, update its actor column to null after recording
it, then fetch the detail and assert the payment remains present with a null
actorName. Preserve the existing missing-main-character case separately.
In `@tests/payouts-service.test.ts`:
- Around line 585-589: Update the descriptive comment above the mutating export
tests to say “All fourteen mutating exports are exercised here,” leaving the
listed exports and addFlatPool coverage note unchanged.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b52644e7-db61-453b-b071-6b5b6a2337e5
📒 Files selected for processing (36)
.env.exampledocs/ops.mddocs/superpowers/plans/2026-08-04-fight-payout-tracking-phase-2.mddocs/superpowers/specs/2026-08-04-fight-payout-tracking-phase-2-design.mde2e/account.spec.tse2e/payouts.spec.tssrc/app/account/account-payouts.tsxsrc/app/account/page.tsxsrc/app/payouts/[id]/page.tsxsrc/app/payouts/[id]/payment-history.tsxsrc/app/payouts/access.tssrc/app/payouts/actions.tssrc/app/payouts/dropped.tssrc/app/payouts/page.tsxsrc/core/loot-paste.tssrc/core/open-info-error.tssrc/core/payout-split.tssrc/lib/esi/client.tssrc/services/appraisal.tssrc/services/payout-loot.tssrc/services/payout-view.tssrc/services/payouts.tssrc/services/tokens.tstests/account-payouts.test.tstests/appraisal.test.tstests/esi-client.test.tstests/open-info-error.test.tstests/payment-history.test.tstests/payout-dropped.test.tstests/payout-loot.test.tstests/payout-parse.test.tstests/payout-schema.test.tstests/payout-split.test.tstests/payout-view.test.tstests/payouts-service.test.tstests/tokens.test.ts
|
|
||
| **Round once, at the line total.** Never round a per-unit price before multiplying by quantity — the error would scale with quantity instead of being confined to the single rounding at the line total. A manual price is already at cent precision, so for manual items per-unit and line-total rounding coincide and the product is exact — it is a `bigint` multiply with nothing to round. An **appraised** line total is not exact by the same argument: it is a float product, and rounding once removes the per-unit error but not IEEE-754's own. That is what `MAX_EXACT_LINE_CENTS` (Task 2) bounds, and bounding `MAX_LOOT_QTY` does not substitute for it. | ||
|
|
||
| **`src/core/` is pure.** No database, no network, no imports outside `src/core/`. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Keep the classifier out of the pure-core dependency boundary.
Line [25] forbids src/core/ from importing outside modules. The proposed open-info-error.ts imports the runtime EsiError class from @/lib/esi/client. This makes the pure layer depend on the ESI client and can introduce a dependency cycle.
Move the classifier beside EsiError, or move the shared error type to a dependency-neutral core module. An existing boundary violation in another file does not satisfy this constraint.
Also applies to: 3811-3820
🤖 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 `@docs/superpowers/plans/2026-08-04-fight-payout-tracking-phase-2.md` at line
25, Keep the open-information error classifier outside the pure `src/core/`
boundary: update the proposed `open-info-error.ts` and its consumers so they do
not import the runtime `EsiError` from `@/lib/esi/client`. Place the classifier
beside `EsiError`, or move the shared error type into a dependency-neutral core
module, while preserving the existing classification behavior.
| Grep evidence for the deletion (run before editing, expect only the two lines inside | ||
| `payout-split.ts` itself): | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the reported Markdown lint violations.
markdownlint-cli2 reports repeated MD040 warnings for fenced blocks without language tags. It also reports MD031 at Lines [2676] and [3381], and MD018 at Line [6358]. Add fence languages, blank lines around fences, and change #74 left this file... to valid prose such as Issue #74 left this file....
Also applies to: 2676-2676, 3381-3381, 6358-6358
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 77-77: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/superpowers/plans/2026-08-04-fight-payout-tracking-phase-2.md` at line
77, Fix all reported Markdown lint violations in the document: add appropriate
language tags to every fenced code block triggering MD040, insert blank lines
immediately before and after the fences at the locations reported for MD031, and
rewrite “#74 left this file...” as valid prose such as “Issue `#74` left this
file...” to resolve MD018.
Source: Linters/SAST tools
Comment on lines
+653
to
+664
| // "12x Foo", "12 Foo" — qty (with optional comma grouping) leads the line. | ||
| const QTY_PREFIX = /^(\d[\d,]*)\s*x?\s+(.+)$/i; | ||
| // "Foo x12" — qty trails the line behind a literal "x". | ||
| const QTY_SUFFIX = /^(.+?)\s+x\s*(\d[\d,]*)$/i; | ||
| // "Foo, 12" — qty trails behind a comma. | ||
| const QTY_COMMA = /^(.+),\s*(\d[\d,]*)$/; | ||
| // "12", "1,234" — a line that is nothing but a quantity, with no item at all. | ||
| const QTY_ONLY = /^[\d,]+$/; | ||
|
|
||
| function parseQty(text: string): number { | ||
| return Number(text.replace(/,/g, "")); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject malformed comma grouping before emitting an item.
The quantity patterns accept values such as 1,,2x Foo. parseQty converts that value to NaN. Both quantity checks then return false for NaN, so the parser emits an item with qty: NaN.
Require valid comma grouping or reject non-finite and non-integer quantities immediately after parsing.
Also applies to: 701-761
🤖 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 `@docs/superpowers/plans/2026-08-04-fight-payout-tracking-phase-2.md` around
lines 653 - 664, Validate quantity text in parseQty before any item is emitted:
accept only plain digits or correctly grouped comma-formatted digits, and reject
non-finite or non-integer results. Update the quantity parsing paths using
QTY_PREFIX, QTY_SUFFIX, QTY_COMMA, and QTY_ONLY so malformed values such as
“1,,2” cannot produce an item with qty: NaN.
Comment on lines
+39
to
+44
| it("truncates an absurdly long line rather than shipping it whole", () => { | ||
| const report = decodeDropped( | ||
| encodeDropped([{ line: "x".repeat(5000), reason: "quantity-only" }]), | ||
| ); | ||
| expect(report?.sample[0].line.length).toBeLessThanOrEqual(120); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Assert against DROPPED_LINE_CHARS instead of the literal 120.
The test hardcodes 120 and uses toBeLessThanOrEqual. If DROPPED_LINE_CHARS is raised later, the assertion still passes and stops pinning the cap. The file already imports DROPPED_SAMPLE_LIMIT from the same module, so importing the second constant keeps both bounds pinned the same way.
♻️ Proposed refactor
import {
+ DROPPED_LINE_CHARS,
DROPPED_SAMPLE_LIMIT,
decodeDropped,
encodeDropped,
} from "`@/app/payouts/dropped`"; it("truncates an absurdly long line rather than shipping it whole", () => {
const report = decodeDropped(
encodeDropped([{ line: "x".repeat(5000), reason: "quantity-only" }]),
);
- expect(report?.sample[0].line.length).toBeLessThanOrEqual(120);
+ expect(report?.sample[0].line).toHaveLength(DROPPED_LINE_CHARS);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it("truncates an absurdly long line rather than shipping it whole", () => { | |
| const report = decodeDropped( | |
| encodeDropped([{ line: "x".repeat(5000), reason: "quantity-only" }]), | |
| ); | |
| expect(report?.sample[0].line.length).toBeLessThanOrEqual(120); | |
| }); | |
| it("truncates an absurdly long line rather than shipping it whole", () => { | |
| const report = decodeDropped( | |
| encodeDropped([{ line: "x".repeat(5000), reason: "quantity-only" }]), | |
| ); | |
| expect(report?.sample[0].line).toHaveLength(DROPPED_LINE_CHARS); | |
| }); |
🤖 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 `@tests/payout-dropped.test.ts` around lines 39 - 44, The long-line truncation
test should use the shared DROPPED_LINE_CHARS constant rather than the literal
120. Import DROPPED_LINE_CHARS alongside DROPPED_SAMPLE_LIMIT and assert the
decoded line length equals that cap, preserving the existing truncation scenario
in the test.
Comment on lines
+441
to
+542
| describe("setItemPrice", () => { | ||
| /** One appraised pool holding a single Tritanium line, priced by triff. */ | ||
| async function seedPricedItem(qty: number) { | ||
| const { operatorId, operationId } = await seedOperation(); | ||
| const { poolId } = await ctx.db.transaction((tx) => | ||
| addAppraisedPool(tx, operatorId, operationId, { | ||
| rawPaste: `${qty}x Tritanium`, | ||
| pricingMode: "sell_best", | ||
| stationId: 60003760, | ||
| appraisal: { | ||
| items: [ | ||
| { | ||
| typeId: 34, | ||
| name: "Tritanium", | ||
| qty, | ||
| unitPrice: "5.00", | ||
| totalValue: centsToIsk(500n * BigInt(qty)), | ||
| priceSource: "triff", | ||
| }, | ||
| ], | ||
| dropped: [], | ||
| totalValue: centsToIsk(500n * BigInt(qty)), | ||
| }, | ||
| }), | ||
| ); | ||
| const [item] = await ctx.db | ||
| .select() | ||
| .from(lootItem) | ||
| .where(eq(lootItem.poolId, poolId)); | ||
| return { operatorId, operationId, poolId, itemId: item.id }; | ||
| } | ||
|
|
||
| it("computes the line total as an exact bigint product at a quantity floats would drift on", async () => { | ||
| // 12,345,678,901 x 7.77 ISK = 95,925,925,060.77 ISK. In cents that product | ||
| // is 9,592,592,506,077 — past nothing on its own, but the float route | ||
| // (12345678901 * 7.77) yields 95925925060.76999..., which renders as a | ||
| // different line total. bigint has nothing to drift. | ||
| const QTY = 12345678901; | ||
| const { operatorId, operationId, poolId, itemId } = await seedPricedItem(QTY); | ||
|
|
||
| await ctx.db.transaction((tx) => setItemPrice(tx, operatorId, itemId, "7.77")); | ||
|
|
||
| const [item] = await ctx.db.select().from(lootItem).where(eq(lootItem.id, itemId)); | ||
| expect(item.unitPrice).toBe("7.77"); | ||
| expect(item.totalValue).toBe("95925925060.77"); | ||
| expect(item.priceSource).toBe("manual"); | ||
| // A manual price is already at cent precision, so unit x qty reproduces | ||
| // the line total exactly — unlike an appraised item, whose unitPrice is a | ||
| // lossy 2dp rendering of a sub-cent market price. | ||
| expect(iskToCents(item.unitPrice) * BigInt(item.qty)).toBe( | ||
| iskToCents(item.totalValue), | ||
| ); | ||
|
|
||
| // The pool total is re-derived from its item rows, and recalculate ran. | ||
| const [pool] = await ctx.db.select().from(lootPool).where(eq(lootPool.id, poolId)); | ||
| expect(pool.totalValue).toBe("95925925060.77"); | ||
| expect(await soleParticipantAmount(operationId)).toBe("95925925060.77"); | ||
| }); | ||
|
|
||
| it("keeps rawPaste verbatim so the pool can still be re-appraised", async () => { | ||
| const { operatorId, poolId, itemId } = await seedPricedItem(3); | ||
| await ctx.db.transaction((tx) => setItemPrice(tx, operatorId, itemId, "10.00")); | ||
| const [pool] = await ctx.db.select().from(lootPool).where(eq(lootPool.id, poolId)); | ||
| expect(pool.rawPaste).toBe("3x Tritanium"); | ||
| }); | ||
|
|
||
| it("rejects a line total past what numeric(20,2) can hold, with a readable error", async () => { | ||
| const { operatorId, itemId } = await seedPricedItem(1000); | ||
| // 1000 x 9999999999999999.99 is ~1e21, past the column's range. (The | ||
| // brief's own "999999999999999.99" is one digit short: at qty 1000 that | ||
| // line totals 999999999999999990.00, which is still under numeric(20,2)'s | ||
| // 999999999999999999.99 ceiling and would not reject.) | ||
| await expect( | ||
| ctx.db.transaction((tx) => | ||
| setItemPrice(tx, operatorId, itemId, "9999999999999999.99"), | ||
| ), | ||
| ).rejects.toThrow(/largest value this system can record/); | ||
| }); | ||
|
|
||
| it("rejects a negative unit price before it reaches the column", async () => { | ||
| const { operatorId, itemId } = await seedPricedItem(2); | ||
| await expect( | ||
| ctx.db.transaction((tx) => setItemPrice(tx, operatorId, itemId, "-1.00")), | ||
| ).rejects.toThrow(/cannot be negative/); | ||
| }); | ||
|
|
||
| it("refuses once the operation is finalized, because it moves money", async () => { | ||
| const { operatorId, operationId, itemId } = await seedPricedItem(2); | ||
| await ctx.db.transaction((tx) => finalizeOperation(tx, operatorId, operationId)); | ||
| await expect( | ||
| ctx.db.transaction((tx) => setItemPrice(tx, operatorId, itemId, "9.00")), | ||
| ).rejects.toThrow(PayoutLockedError); | ||
| }); | ||
|
|
||
| it("rejects a non-operator actor at the service layer", async () => { | ||
| const { itemId } = await seedPricedItem(2); | ||
| const green = await seedAccount(ctx.db, { tier: "green", status: "active" }); | ||
| await expect( | ||
| ctx.db.transaction((tx) => setItemPrice(tx, green.id, itemId, "9.00")), | ||
| ).rejects.toThrow(PayoutForbiddenError); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a multi-item pool case for the pool-total re-derivation.
Every test here uses a pool with exactly one item. The pool total then equals the repriced line total, so the sibling sum at src/services/payout-loot.ts lines 200-204 cannot be distinguished from "write the new line total to the pool". A regression that ignored sibling rows would still pass this suite.
Add one pool with two items, reprice one, and assert the pool total is the sum of both lines.
♻️ Sketch of the added case
+ it("re-derives the pool total from every item, not just the repriced one", async () => {
+ const { operatorId, operationId } = await seedOperation();
+ const { poolId } = await ctx.db.transaction((tx) =>
+ addAppraisedPool(tx, operatorId, operationId, {
+ rawPaste: "1x Tritanium\n1x Pyerite",
+ pricingMode: "sell_best",
+ stationId: 60003760,
+ appraisal: {
+ items: [
+ { typeId: 34, name: "Tritanium", qty: 1, unitPrice: "5.00", totalValue: "5.00", priceSource: "triff" },
+ { typeId: 35, name: "Pyerite", qty: 1, unitPrice: "7.00", totalValue: "7.00", priceSource: "triff" },
+ ],
+ dropped: [],
+ totalValue: "12.00",
+ },
+ }),
+ );
+ const items = await ctx.db.select().from(lootItem).where(eq(lootItem.poolId, poolId));
+ const tritanium = items.find((i) => i.name === "Tritanium")!;
+ await ctx.db.transaction((tx) => setItemPrice(tx, operatorId, tritanium.id, "10.00"));
+ const [pool] = await ctx.db.select().from(lootPool).where(eq(lootPool.id, poolId));
+ expect(pool.totalValue).toBe("17.00"); // 10.00 + the untouched 7.00
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| describe("setItemPrice", () => { | |
| /** One appraised pool holding a single Tritanium line, priced by triff. */ | |
| async function seedPricedItem(qty: number) { | |
| const { operatorId, operationId } = await seedOperation(); | |
| const { poolId } = await ctx.db.transaction((tx) => | |
| addAppraisedPool(tx, operatorId, operationId, { | |
| rawPaste: `${qty}x Tritanium`, | |
| pricingMode: "sell_best", | |
| stationId: 60003760, | |
| appraisal: { | |
| items: [ | |
| { | |
| typeId: 34, | |
| name: "Tritanium", | |
| qty, | |
| unitPrice: "5.00", | |
| totalValue: centsToIsk(500n * BigInt(qty)), | |
| priceSource: "triff", | |
| }, | |
| ], | |
| dropped: [], | |
| totalValue: centsToIsk(500n * BigInt(qty)), | |
| }, | |
| }), | |
| ); | |
| const [item] = await ctx.db | |
| .select() | |
| .from(lootItem) | |
| .where(eq(lootItem.poolId, poolId)); | |
| return { operatorId, operationId, poolId, itemId: item.id }; | |
| } | |
| it("computes the line total as an exact bigint product at a quantity floats would drift on", async () => { | |
| // 12,345,678,901 x 7.77 ISK = 95,925,925,060.77 ISK. In cents that product | |
| // is 9,592,592,506,077 — past nothing on its own, but the float route | |
| // (12345678901 * 7.77) yields 95925925060.76999..., which renders as a | |
| // different line total. bigint has nothing to drift. | |
| const QTY = 12345678901; | |
| const { operatorId, operationId, poolId, itemId } = await seedPricedItem(QTY); | |
| await ctx.db.transaction((tx) => setItemPrice(tx, operatorId, itemId, "7.77")); | |
| const [item] = await ctx.db.select().from(lootItem).where(eq(lootItem.id, itemId)); | |
| expect(item.unitPrice).toBe("7.77"); | |
| expect(item.totalValue).toBe("95925925060.77"); | |
| expect(item.priceSource).toBe("manual"); | |
| // A manual price is already at cent precision, so unit x qty reproduces | |
| // the line total exactly — unlike an appraised item, whose unitPrice is a | |
| // lossy 2dp rendering of a sub-cent market price. | |
| expect(iskToCents(item.unitPrice) * BigInt(item.qty)).toBe( | |
| iskToCents(item.totalValue), | |
| ); | |
| // The pool total is re-derived from its item rows, and recalculate ran. | |
| const [pool] = await ctx.db.select().from(lootPool).where(eq(lootPool.id, poolId)); | |
| expect(pool.totalValue).toBe("95925925060.77"); | |
| expect(await soleParticipantAmount(operationId)).toBe("95925925060.77"); | |
| }); | |
| it("keeps rawPaste verbatim so the pool can still be re-appraised", async () => { | |
| const { operatorId, poolId, itemId } = await seedPricedItem(3); | |
| await ctx.db.transaction((tx) => setItemPrice(tx, operatorId, itemId, "10.00")); | |
| const [pool] = await ctx.db.select().from(lootPool).where(eq(lootPool.id, poolId)); | |
| expect(pool.rawPaste).toBe("3x Tritanium"); | |
| }); | |
| it("rejects a line total past what numeric(20,2) can hold, with a readable error", async () => { | |
| const { operatorId, itemId } = await seedPricedItem(1000); | |
| // 1000 x 9999999999999999.99 is ~1e21, past the column's range. (The | |
| // brief's own "999999999999999.99" is one digit short: at qty 1000 that | |
| // line totals 999999999999999990.00, which is still under numeric(20,2)'s | |
| // 999999999999999999.99 ceiling and would not reject.) | |
| await expect( | |
| ctx.db.transaction((tx) => | |
| setItemPrice(tx, operatorId, itemId, "9999999999999999.99"), | |
| ), | |
| ).rejects.toThrow(/largest value this system can record/); | |
| }); | |
| it("rejects a negative unit price before it reaches the column", async () => { | |
| const { operatorId, itemId } = await seedPricedItem(2); | |
| await expect( | |
| ctx.db.transaction((tx) => setItemPrice(tx, operatorId, itemId, "-1.00")), | |
| ).rejects.toThrow(/cannot be negative/); | |
| }); | |
| it("refuses once the operation is finalized, because it moves money", async () => { | |
| const { operatorId, operationId, itemId } = await seedPricedItem(2); | |
| await ctx.db.transaction((tx) => finalizeOperation(tx, operatorId, operationId)); | |
| await expect( | |
| ctx.db.transaction((tx) => setItemPrice(tx, operatorId, itemId, "9.00")), | |
| ).rejects.toThrow(PayoutLockedError); | |
| }); | |
| it("rejects a non-operator actor at the service layer", async () => { | |
| const { itemId } = await seedPricedItem(2); | |
| const green = await seedAccount(ctx.db, { tier: "green", status: "active" }); | |
| await expect( | |
| ctx.db.transaction((tx) => setItemPrice(tx, green.id, itemId, "9.00")), | |
| ).rejects.toThrow(PayoutForbiddenError); | |
| }); | |
| }); | |
| describe("setItemPrice", () => { | |
| /** One appraised pool holding a single Tritanium line, priced by triff. */ | |
| async function seedPricedItem(qty: number) { | |
| const { operatorId, operationId } = await seedOperation(); | |
| const { poolId } = await ctx.db.transaction((tx) => | |
| addAppraisedPool(tx, operatorId, operationId, { | |
| rawPaste: `${qty}x Tritanium`, | |
| pricingMode: "sell_best", | |
| stationId: 60003760, | |
| appraisal: { | |
| items: [ | |
| { | |
| typeId: 34, | |
| name: "Tritanium", | |
| qty, | |
| unitPrice: "5.00", | |
| totalValue: centsToIsk(500n * BigInt(qty)), | |
| priceSource: "triff", | |
| }, | |
| ], | |
| dropped: [], | |
| totalValue: centsToIsk(500n * BigInt(qty)), | |
| }, | |
| }), | |
| ); | |
| const [item] = await ctx.db | |
| .select() | |
| .from(lootItem) | |
| .where(eq(lootItem.poolId, poolId)); | |
| return { operatorId, operationId, poolId, itemId: item.id }; | |
| } | |
| it("computes the line total as an exact bigint product at a quantity floats would drift on", async () => { | |
| // 12,345,678,901 x 7.77 ISK = 95,925,925,060.77 ISK. In cents that product | |
| // is 9,592,592,506,077 — past nothing on its own, but the float route | |
| // (12345678901 * 7.77) yields 95925925060.76999..., which renders as a | |
| // different line total. bigint has nothing to drift. | |
| const QTY = 12345678901; | |
| const { operatorId, operationId, poolId, itemId } = await seedPricedItem(QTY); | |
| await ctx.db.transaction((tx) => setItemPrice(tx, operatorId, itemId, "7.77")); | |
| const [item] = await ctx.db.select().from(lootItem).where(eq(lootItem.id, itemId)); | |
| expect(item.unitPrice).toBe("7.77"); | |
| expect(item.totalValue).toBe("95925925060.77"); | |
| expect(item.priceSource).toBe("manual"); | |
| // A manual price is already at cent precision, so unit x qty reproduces | |
| // the line total exactly — unlike an appraised item, whose unitPrice is a | |
| // lossy 2dp rendering of a sub-cent market price. | |
| expect(iskToCents(item.unitPrice) * BigInt(item.qty)).toBe( | |
| iskToCents(item.totalValue), | |
| ); | |
| // The pool total is re-derived from its item rows, and recalculate ran. | |
| const [pool] = await ctx.db.select().from(lootPool).where(eq(lootPool.id, poolId)); | |
| expect(pool.totalValue).toBe("95925925060.77"); | |
| expect(await soleParticipantAmount(operationId)).toBe("95925925060.77"); | |
| }); | |
| it("re-derives the pool total from every item, not just the repriced one", async () => { | |
| const { operatorId, operationId } = await seedOperation(); | |
| const { poolId } = await ctx.db.transaction((tx) => | |
| addAppraisedPool(tx, operatorId, operationId, { | |
| rawPaste: "1x Tritanium\n1x Pyerite", | |
| pricingMode: "sell_best", | |
| stationId: 60003760, | |
| appraisal: { | |
| items: [ | |
| { typeId: 34, name: "Tritanium", qty: 1, unitPrice: "5.00", totalValue: "5.00", priceSource: "triff" }, | |
| { typeId: 35, name: "Pyerite", qty: 1, unitPrice: "7.00", totalValue: "7.00", priceSource: "triff" }, | |
| ], | |
| dropped: [], | |
| totalValue: "12.00", | |
| }, | |
| }), | |
| ); | |
| const items = await ctx.db.select().from(lootItem).where(eq(lootItem.poolId, poolId)); | |
| const tritanium = items.find((i) => i.name === "Tritanium")!; | |
| await ctx.db.transaction((tx) => setItemPrice(tx, operatorId, tritanium.id, "10.00")); | |
| const [pool] = await ctx.db.select().from(lootPool).where(eq(lootPool.id, poolId)); | |
| expect(pool.totalValue).toBe("17.00"); // 10.00 + the untouched 7.00 | |
| }); | |
| it("keeps rawPaste verbatim so the pool can still be re-appraised", async () => { | |
| const { operatorId, poolId, itemId } = await seedPricedItem(3); | |
| await ctx.db.transaction((tx) => setItemPrice(tx, operatorId, itemId, "10.00")); | |
| const [pool] = await ctx.db.select().from(lootPool).where(eq(lootPool.id, poolId)); | |
| expect(pool.rawPaste).toBe("3x Tritanium"); | |
| }); | |
| it("rejects a line total past what numeric(20,2) can hold, with a readable error", async () => { | |
| const { operatorId, itemId } = await seedPricedItem(1000); | |
| // 1000 x 9999999999999999.99 is ~1e21, past the column's range. (The | |
| // brief's own "999999999999999.99" is one digit short: at qty 1000 that | |
| // line totals 999999999999999990.00, which is still under numeric(20,2)'s | |
| // 999999999999999999.99 ceiling and would not reject.) | |
| await expect( | |
| ctx.db.transaction((tx) => | |
| setItemPrice(tx, operatorId, itemId, "9999999999999999.99"), | |
| ), | |
| ).rejects.toThrow(/largest value this system can record/); | |
| }); | |
| it("rejects a negative unit price before it reaches the column", async () => { | |
| const { operatorId, itemId } = await seedPricedItem(2); | |
| await expect( | |
| ctx.db.transaction((tx) => setItemPrice(tx, operatorId, itemId, "-1.00")), | |
| ).rejects.toThrow(/cannot be negative/); | |
| }); | |
| it("refuses once the operation is finalized, because it moves money", async () => { | |
| const { operatorId, operationId, itemId } = await seedPricedItem(2); | |
| await ctx.db.transaction((tx) => finalizeOperation(tx, operatorId, operationId)); | |
| await expect( | |
| ctx.db.transaction((tx) => setItemPrice(tx, operatorId, itemId, "9.00")), | |
| ).rejects.toThrow(PayoutLockedError); | |
| }); | |
| it("rejects a non-operator actor at the service layer", async () => { | |
| const { itemId } = await seedPricedItem(2); | |
| const green = await seedAccount(ctx.db, { tier: "green", status: "active" }); | |
| await expect( | |
| ctx.db.transaction((tx) => setItemPrice(tx, green.id, itemId, "9.00")), | |
| ).rejects.toThrow(PayoutForbiddenError); | |
| }); | |
| }); |
🤖 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 `@tests/payout-loot.test.ts` around lines 441 - 542, Add a multi-item test
within the setItemPrice suite, using the existing seeding helpers or setup
patterns to create two loot lines in one pool, then reprice only one line via
setItemPrice. Assert the pool total equals the unchanged sibling line total plus
the repriced line total, proving pool-total recalculation includes all item rows
rather than only the updated line.
Comment on lines
+113
to
+172
| it("rejects a non-positive loot item qty (loot_item_qty_ck)", async () => { | ||
| const [op] = await ctx.db | ||
| .insert(payoutOperation) | ||
| .values({ name: "Op", occurredAt: new Date() }) | ||
| .returning(); | ||
| const [pool] = await ctx.db | ||
| .insert(lootPool) | ||
| .values({ | ||
| operationId: op.id, | ||
| valuationSource: "flat", | ||
| totalValue: "0", | ||
| notes: "note", | ||
| }) | ||
| .returning(); | ||
| await expectCheckViolation( | ||
| ctx.db | ||
| .insert(lootItem) | ||
| .values({ poolId: pool.id, name: "Nothing", qty: 0, priceSource: "unresolved" }), | ||
| "loot_item_qty_ck", | ||
| ); | ||
| }); | ||
|
|
||
| it("rejects a negative loot item unit price or total (loot_item_price_ck)", async () => { | ||
| const [op] = await ctx.db | ||
| .insert(payoutOperation) | ||
| .values({ name: "Op", occurredAt: new Date() }) | ||
| .returning(); | ||
| const [pool] = await ctx.db | ||
| .insert(lootPool) | ||
| .values({ | ||
| operationId: op.id, | ||
| valuationSource: "flat", | ||
| totalValue: "0", | ||
| notes: "note", | ||
| }) | ||
| .returning(); | ||
| await expectCheckViolation( | ||
| ctx.db.insert(lootItem).values({ | ||
| poolId: pool.id, | ||
| name: "Owed", | ||
| qty: 1, | ||
| unitPrice: "-1.00", | ||
| priceSource: "manual", | ||
| }), | ||
| "loot_item_price_ck", | ||
| ); | ||
| // The constraint covers totalValue as well, and no service-level test | ||
| // reaches that half of it. | ||
| await expectCheckViolation( | ||
| ctx.db.insert(lootItem).values({ | ||
| poolId: pool.id, | ||
| name: "Owed", | ||
| qty: 1, | ||
| unitPrice: "1.00", | ||
| totalValue: "-1.00", | ||
| priceSource: "manual", | ||
| }), | ||
| "loot_item_price_ck", | ||
| ); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Look for an existing loot-pool seeding helper before adding a new one.
fd -e ts . tests | xargs rg -n -C3 'lootPool\)?\s*$|function .*[Pp]ool|insert\(lootPool\)'Repository: guarzo/authGD
Length of output: 8037
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- tests/payout-schema.test.ts outline ---'
ast-grep outline tests/payout-schema.test.ts --lang typescript || true
printf '%s\n' '--- tests/payout-schema.test.ts ---'
cat -n tests/payout-schema.test.ts | sed -n '1,190p'
printf '%s\n' '--- helper declarations and test setup ---'
rg -n -C3 '^(const|function|async function|describe|beforeEach|afterEach|it\()|seed|create.*Pool|make.*Pool|setup' tests/payout-schema.test.ts tests --glob '*.ts' | sed -n '1,240p'Repository: guarzo/authGD
Length of output: 20854
Extract the repeated operation-and-pool setup.
Both tests use identical setup. No existing helper provides it. Extract a local helper that returns the created pool.
🤖 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 `@tests/payout-schema.test.ts` around lines 113 - 172, Extract the duplicated
payoutOperation and lootPool insertion setup from the tests into a local helper
that creates the operation and returns the created pool. Update both
“loot_item_qty_ck” and “loot_item_price_ck” tests to use this helper while
preserving their existing assertions and test data.
Comment on lines
+536
to
+552
| // Both nulls are reachable and neither is an error: `payout_payment.actor` | ||
| // is `on delete set null`, and an account need not have a main character at | ||
| // all. The row must still come back — history is append-only, and losing an | ||
| // event because nobody can be named would be the worse failure. | ||
| it("leaves actorName null when there is no main character to name the actor by", async () => { | ||
| const { operator, operationId, byName } = await seedOperation({ | ||
| totalValue: "300.00", | ||
| names: ["A"], | ||
| }); | ||
| await ctx.db.transaction((tx) => finalizeOperation(tx, operator.id, operationId)); | ||
| await ctx.db.transaction((tx) => recordPayment(tx, operator.id, byName.get("A")!.id)); | ||
|
|
||
| const detail = await getPayoutOperationDetail(ctx.db, operationId); | ||
| const a = detail!.participants.find((p) => p.displayName === "A")!; | ||
| expect(a.payments).toHaveLength(1); | ||
| expect(a.payments[0].actorName).toBeNull(); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Cover the null actor column, not only the missing main character.
The comment names two reachable nulls: payout_payment.actor set to null by on delete set null, and an account with no main character. The test below exercises only the second one. getPayoutOperationDetail resolves the name with payment.actor === null ? null : (actorNameById.get(payment.actor) ?? null), so the first branch has no coverage. A change that dereferenced payment.actor unconditionally would still pass this suite.
Add a case that nulls the actor column after the payment is recorded.
♻️ Sketch of the added case
+ it("leaves actorName null when the payment's actor row is gone", async () => {
+ const { operator, operationId, byName } = await seedOperation({
+ totalValue: "300.00",
+ names: ["A"],
+ });
+ await ctx.db.transaction((tx) => finalizeOperation(tx, operator.id, operationId));
+ await ctx.db.transaction((tx) => recordPayment(tx, operator.id, byName.get("A")!.id));
+ // What `on delete set null` leaves behind once the operator's account is gone.
+ await ctx.db
+ .update(payoutPayment)
+ .set({ actor: null })
+ .where(eq(payoutPayment.participantId, byName.get("A")!.id));
+
+ const detail = await getPayoutOperationDetail(ctx.db, operationId);
+ const a = detail!.participants.find((p) => p.displayName === "A")!;
+ expect(a.payments).toHaveLength(1); // the event survives
+ expect(a.payments[0].actorName).toBeNull();
+ });payoutPayment needs adding to the schema import at lines 5-11.
🤖 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 `@tests/payout-view.test.ts` around lines 536 - 552, Add coverage for the null
actor branch in the test covering getPayoutOperationDetail: import the
payoutPayment schema symbol, capture the recorded payment, update its actor
column to null after recording it, then fetch the detail and assert the payment
remains present with a null actorName. Preserve the existing
missing-main-character case separately.
…eads listPayoutOperations skips its loot_pool child query when the first page comes back empty, so renaming loot_pool no longer made /payouts throw and six boundary tests stopped exercising the boundary. Rename payout_operation instead: the operations query has no such short-circuit, and it still meets the constraints the technique needs - read by the page body, not by the guard, not by metadata.
A bound on the product's magnitude was doing duty as a bound on the error in computing it. They are not the same thing: `MAX_EXACT_LINE_CENTS` makes the answer representable, but the double `price` still carries ~1.1e-16 relative error into the multiply, which near 9e15 cents is already about a whole cent. 48804.84 ISK x 1,845,177,173 units sits under the bound and stored 9005357669991731 where the true total is 9005357669991732. So the multiply moves into bigint over the price's own decimal expansion, and the bound goes back to being what it says it is: a ceiling on a single line, refused by name rather than stored wrong. Rounding still happens once, at the line total, half away from zero — the tie-break `Math.round` used, so the only totals that move are the ones the float got wrong. Also from the same review pass: - `decodeDropped` filtered its reason with `in`, which walks the prototype chain, so a crafted `?dropped=` carrying "constructor" or "toString" passed the allowlist and rendered whatever Object.prototype resolved to. - Two tests asserted on "paid", which "unpaid" contains, and so passed on a render where the paid badge was missing entirely; one e2e mark-paid clicked `.first()` rather than the row under test. - A docblock counted fifteen guarded mutations and listed fourteen. - `.env.example` left a space-separated scope list unquoted.
Owner
Author
|
Worked through all 27 CodeRabbit findings. Six fixed in Fixed
The root error was conceptual, and it was written down in a docblock as if it were a proof:
Already fixed before this review
Declined
Findings against VerificationFull gate set on the merged tree, after the fixes:
|
This was referenced Aug 4, 2026
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
Task 13 of the fight-payout-tracking phase-2 plan: closes the e2e coverage gap left by Tasks 1-12. No source changes — this PR is test-only.
?error=table-driven test from 12 to 22 codes (addsshares_rangeand theparticipant_*/open_info_*codes).payout_paymenthistory and thepayout.payment_revertedaudit entry targets the operation.While writing the pay/revert/pay-again test, found and fixed three locator/assertion bugs in the new test itself (not the app): a
getByLabelsubstring collision, atoHaveCount(0)assertion that doesn't account for<details>keeping collapsed content in the DOM, a redundant second click that closed an already-open disclosure, and — the one that actually caused the reported flake — atoContainText("paid")assertion that passed instantly because "unpaid" contains "paid" as a substring, so it never actually waited for the state transition before the test raced ahead to read the DB directly.Test plan
TEST_DATABASE_URL=... npm test— 71 files, 804 tests, all passednpm run typecheck— cleannpm run lint— 0 errors (3 pre-existing unrelated warnings)npm run format:check— cleannpm run test:e2e— 129/129 passed (full suite, including all 5 new tests)npm run build— succeededSummary by CodeRabbit
New Features
Bug Fixes
Documentation