fix(history): join PymtHouse tickets onto MCP job_* Cost - #75
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
🟡 Changes recommended
The client can request more than 50 gatewayRequestId values (e.g., after loading more history pages or adding a detail id), which will trigger a 400 from the route and break Cost joining.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates the Home/History “Cost” join logic to fetch signed-ticket usage rows by specific gateway_request_ids (instead of relying on whichever correlated rows appear on the first usage page), and updates the BFF + route handling to support that targeted lookup.
Changes:
- Home History now requests correlated signed-ticket rows for the current set of visible (and open-detail)
gatewayRequestIds to populate Cost. - The BFF forwards
gatewayRequestIdquery params to PymtHouse and skips cursor pagination when IDs are provided. - The
account-requestsAPI route adds a fast-path for ID lookups (no page-walking) and persists fee metadata for those matched rows.
File summaries
| File | Description |
|---|---|
| tests/contracts/home-history-surface.test.tsx | Asserts the History surface requests correlated tickets including the specific gatewayRequestId. |
| tests/contracts/account-history.test.ts | Verifies the BFF appends repeated gatewayRequestId params and omits cursor when IDs are provided. |
| tests/contracts/account-history-route.test.ts | Verifies the route fast-path filters by gatewayRequestId and avoids pagination walking. |
| lib/console/useAccountRequests.ts | Extends the hook to accept gatewayRequestIds and append them to the request URL + cache keying. |
| lib/console/pymthouse-bff.ts | Adds gatewayRequestIds forwarding to /api/v1/user/usage/requests and disables cursor when present. |
| components/console/CallsSection.tsx | Collects history + detail gatewayRequestIds and fetches correlated signed-ticket rows for Cost joining. |
| app/api/pymthouse/account-requests/route.ts | Adds ID-based lookup path with input validation and shared fee persistence helper. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🔵 Needs a closer look
The client can request >50 gatewayRequestId values (e.g., 50 history rows + 1 detail row), which triggers a 400 from the route and breaks Cost correlation for all rows.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
lib/console/useAccountRequests.ts:60
idKeyis derived viagatewayRequestIds.join("\0"), which can collide (e.g.,["a", "b"]and["a\0b"]produce the same key). Using a collision-free encoding avoids stale closures/cache scopes if an ID ever contains the delimiter.
components/console/CallsSection.tsx:45
gatewayRequestIdscan exceed the route limit of 50 (history fetches 50 items, and adding a non-visible detail run can make it 51; loading more history can also push it higher). In that case/api/pymthouse/account-requestsreturns 400 and all Cost values will fail to correlate. Cap the list to 50 and ensure the inspected run’sgatewayRequestIdis kept when present.
return [...ids];
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The changes are consistent end-to-end (client → route → BFF → upstream) and are covered by updated/added contract tests validating the new lookup behavior.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
There are a couple of concrete API/parameter-semantics issues (cursor + gatewayRequestId ambiguity and includeCorrelated bypass for gatewayRequestId lookups) that should be addressed before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 3
- Review effort level: Lite
| const push = (raw: string) => { | ||
| const id = raw.trim(); | ||
| if (!id || seen.has(id) || out.length >= MAX_GATEWAY_REQUEST_IDS) return; | ||
| seen.add(id); | ||
| out.push(id); | ||
| }; |
| const params = new URLSearchParams({ limit: "50" }); | ||
| if (includeCorrelated) params.set("includeCorrelated", "1"); | ||
| if (cursor) params.set("cursor", cursor); | ||
| for (const id of takeGatewayRequestIds(gatewayRequestIds)) { | ||
| params.append("gatewayRequestId", id); | ||
| } |
There was a problem hiding this comment.
🟡 Changes recommended
The new empty-match warning log includes user/request identifiers (sensitive in centralized logs) and should be redacted/adjusted before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
lib/console/useAccountRequests.ts:114
useEffectdepends on thegatewayRequestIdsarray identity. If a caller passes a freshly-allocated array with the same ids (common with inline props), this effect will abort and refetch even thoughscope/idKeyhaven’t logically changed. Consider stabilizing ids inside the hook (e.g., keep a ref updated each render or memoize a normalized/capped list keyed byidKey) and then remove the raw array reference from the dependency list.
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Lite
| if (scoped.length === 0) { | ||
| console.warn("[account-requests] live Cost lookup returned no matches", { | ||
| externalUserId: session.externalUserId, | ||
| appId, | ||
| probeGatewayRequestId: "job_713a57c61e3d4976", | ||
| requestedGatewayRequestIds: gatewayRequestIds.slice(0, 10), | ||
| }); | ||
| } |
There was a problem hiding this comment.
🟡 Changes recommended
The by-id lookup path can exceed the intended upstream page-walk cap (worst case 1 + MAX_LOOKUP_PAGES fetches), which should be corrected to avoid unnecessary latency/load.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Lite
| for ( | ||
| let page = 0; | ||
| matchedByGateway.size < wanted.size && next && page < MAX_LOOKUP_PAGES; | ||
| page++ | ||
| ) { |
There was a problem hiding this comment.
🟡 Changes recommended
A new warning log in account-requests includes user-identifying data, and the correlated lookup can amplify upstream requests (up to 40 extra pages), which should be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
app/api/pymthouse/account-requests/route.ts:159
- The new warning log includes
externalUserId(a user identifier) and a hardcodedprobeGatewayRequestId, which risks leaking sensitive data into logs and looks like leftover debug. Prefer logging only non-identifying context (e.g., appId, count, and a small sample of requested ids).
{ ...payload, items: scoped, nextCursor: null },
{ headers: PYMTHOUSE_NO_STORE_HEADERS }
);
}
if (includeCorrelated) {
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Lite
| const MAX_LEGACY_LOOKUP_PAGES = 5; | ||
| const MAX_CORRELATED_LOOKUP_PAGES = 40; |
| let next = payload.nextCursor; | ||
| for ( | ||
| let page = 0; | ||
| matchedByGateway.size < wanted.size && | ||
| next && | ||
| page < MAX_CORRELATED_LOOKUP_PAGES; | ||
| page++ |
There was a problem hiding this comment.
🔵 Needs a closer look
The client hook currently appends gatewayRequestId params even when includeCorrelated is false (route rejects this), and the route logs a hard-coded probe id that should be removed to avoid confusing/unnecessary operational logging.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
lib/console/useAccountRequests.ts:33
fetchRequestsPageappendsgatewayRequestIdparams even whenincludeCorrelatedis false. The route explicitly rejectsgatewayRequestIdunlessincludeCorrelated=1, so any future caller that passes ids without correlation enabled will hard-fail with a 400. Gate the query params behindincludeCorrelated(or assert) so the hook can’t produce an invalid request shape.
app/api/pymthouse/account-requests/route.ts:152
- The warning payload includes a hard-coded
probeGatewayRequestIdthat isn’t used by the code path and reads like a real request id. This can confuse operational debugging and risks leaking an identifier into logs unnecessarily; logging the requested ids (already included) should be sufficient.
console.warn("[account-requests] live Cost lookup returned no matches", {
externalUserId: session.externalUserId,
appId,
probeGatewayRequestId: "job_713a57c61e3d4976",
requestedGatewayRequestIds: gatewayRequestIds.slice(0, 10),
});
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The functional changes align with the PR description and are covered by targeted contract tests, with only minor performance/logging nits noted.
Review details
Suppressed comments (2)
lib/console/useAccountRequests.ts:178
loadMoreis re-created whenevergatewayRequestIdschanges by reference, which can cause avoidable re-renders. Since the request identity is represented byidKey/scope, depending onidKeyavoids churn when the ids are the same.
} finally {
if (generation.current === id) appendBusy.current = false;
}
}, [enabled, state, scope, includeCorrelated, gatewayRequestIds]);
app/api/pymthouse/account-requests/route.ts:152
- The warning log includes a hard-coded
probeGatewayRequestIdvalue that is unrelated to the request being processed, which can mislead debugging and makes log output look like real customer data.
console.warn("[account-requests] live Cost lookup returned no matches", {
externalUserId: session.externalUserId,
appId,
probeGatewayRequestId: "job_713a57c61e3d4976",
requestedGatewayRequestIds: gatewayRequestIds.slice(0, 10),
});
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Lite
| controller.abort(); | ||
| appendController.current?.abort(); | ||
| }; | ||
| }, [scope, enabled, refresh, includeCorrelated]); | ||
| }, [scope, enabled, refresh, includeCorrelated, gatewayRequestIds]); |
eliteprox
changed the title
There was a problem hiding this comment.
🟡 Changes recommended
The includeCorrelated=1 route path can drop valid tickets when upstream omits clientId, which can regress Cost correlation for consumers not using the gatewayRequestId lookup mode.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
lib/console/useAccountRequests.ts:63
idKeyis derived from the rawgatewayRequestIdsarray, but the actual request is normalized/capped viatakeGatewayRequestIds(...). If the caller passes >50 ids (or ids with duplicates/whitespace),scopewill churn and trigger refetches even though the outgoing query params are unchanged. DeriveidKeyfrom the normalized/capped ids so the cache scope matches the real request.
const idKey = gatewayRequestIds.join("\0");
- Files reviewed: 15/15 changed files
- Comments generated: 1
- Review effort level: Lite
| const scoped = payload.items.filter( | ||
| (item) => | ||
| item.externalUserId === session.externalUserId && | ||
| item.clientId === appId | ||
| ); |
Production usage ids are CloudEvent 8-hex, not console job_* keys. Keep the current-month ticket feed and match by capability and time so the request drawer can show a real fee.
d86687b to
39de9f6
Compare
There was a problem hiding this comment.
🟡 Changes recommended
There’s a small but real type-safety regression in CallsSection where feeByGateway can be inferred as Map<any, any> due to returning an untyped new Map().
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Lite
| } | ||
| return fees; | ||
| }, [billingRows]); | ||
| if (!billingRows) return new Map(); |
Summary
c9a1fae7); MCP runs storejob_*. Exact by-id lookup is ignored, so Cost was always—./me/usage/requestsfeed (no 365-day window, no drop of unmatched ids) and join tickets onto runs by exact id first, then nearest unused same-capability ticket in a 15-minute window.billing_usageevents are not used for the drawer.Test plan
/home?request=<run-uuid>for a succeeded MCP image (e.g. ideogram) and confirm Cost is a dollar amount, not——