feat: merge Limits + Usage into unified Spend page#24093
Conversation
0d52625 to
d18e3a3
Compare
f110100 to
27b6526
Compare
d18e3a3 to
f36406b
Compare
d66dff4 to
a02da70
Compare
f36406b to
847cb45
Compare
a02da70 to
668072a
Compare
421a732 to
581dfdc
Compare
c22d94b to
badd0db
Compare
4fe05f9 to
f04c99f
Compare
4bd794a to
6a95e39
Compare
f04c99f to
069be14
Compare
6a95e39 to
ff09998
Compare
069be14 to
37b0cbb
Compare
e5e2eb7 to
1f5c719
Compare
323ed66 to
aa15780
Compare
- Fix goToFirstPage race with setSearchParams (C2) - Preserve query params in /usage → /spend redirect (C3) - Reset ChatCostSummaryView pagination on data change (C4) - Extract GroupOverrideController and UserOverrideController (C5) - Make isLoadingConfig inline instead of blocking page (C6) - Add click-to-drill-in story (C7) - Rename selectedUser → drillInUser for clarity (C8) - Rename hasExplicitDateRange → endDateIsExclusive (C9) - Extract named constants for magic numbers (C10) - Remove stray whitespace expressions (C11) - Rename usage vestiges in search params and descriptions - View reduced from 710 lines / 12 useState to 500 lines / 0 useState
5a59726 to
445e9ee
Compare
|
R6 findings addressed in 445e9ee:
|
- Fix goToFirstPage race with setSearchParams (C2) - Preserve query params in /usage → /spend redirect (C3) - Reset ChatCostSummaryView pagination on data change (C4) - Extract GroupOverrideController and UserOverrideController (C5) - Make isLoadingConfig inline instead of blocking page (C6) - Add click-to-drill-in story (C7) - Rename selectedUser → drillInUser for clarity (C8) - Rename hasExplicitDateRange → endDateIsExclusive (C9) - Extract named constants for magic numbers (C10) - Remove stray whitespace expressions (C11) - Rename usage vestiges in search params and descriptions - View reduced from 710 lines / 12 useState to 500 lines / 0 useState
445e9ee to
4a83e19
Compare
mafredri
left a comment
There was a problem hiding this comment.
Round 7. All 27 prior findings resolved. One new P1: the paginatedChatCostUsers test has TypeScript errors that will fail CI.
Severity count: 1 P1, 2 P3.
The P1 is a fix-chain regression from the R6 test addition. queryPayload() and queryKey() are called without the required QueryPageParams fields (limit, offset, searchParams). pnpm exec tsc --noEmit confirms these are the only two type errors in the repo. The fix is straightforward: pass dummy values for the required fields.
The test also claims to verify username coercion ("coerces empty username") but never calls queryFn, where the coercion lives. Fixing the type errors and adding a queryFn call would address both F28 and F29 together.
Hisoka noted that formatUsageDateRange's .utc() formatting overcorrects for users east of UTC (Pacific/Auckland verified). This is a pre-existing issue rooted in DateRangePicker.toBoundary producing local midnight, not introduced by this PR. The old code had the inverse bug (correct for picker, wrong for URL sharing). Neither approach handles both; the fix belongs in DateRangePicker. Noted, not blocking.
🤖 This review was automatically generated with Coder Agents.
|
|
||
| const query = chatCostUsers(params); | ||
| // queryPayload returns the original payload. | ||
| expect(result.queryPayload()).toEqual(payload); |
There was a problem hiding this comment.
P1 Two TypeScript errors that will fail CI. queryPayload() is called with zero arguments but requires QueryPageParams. queryKey() receives { payload, pageNumber: 2 } but QueryPageParamsWithPayload also requires limit, offset, and searchParams. (Mafuuu P1)
The test also claims to verify username coercion ("coerces empty username") but never calls queryFn where the coercion payload.username || undefined lives. (Bisky P3, Knov P4)
Fix both: pass complete params and add a queryFn assertion:
const params = { payload, pageNumber: 2, limit: 25, offset: 25, searchParams: new URLSearchParams() };
expect(result.queryPayload(params)).toMatchObject({ ...payload, username: undefined });
expect(result.queryKey(params)).toEqual(["chats", "costUsers", { ...payload, username: undefined }, 2]);
await result.queryFn(params);
expect(API.experimental.getChatCostUsers).toHaveBeenCalledWith(expect.objectContaining({ username: undefined }));🤖
| total_runtime_ms: 0, | ||
| })); | ||
|
|
||
| export const PaginatedModels: Story = { |
There was a problem hiding this comment.
P3 PaginatedModels tests model table pagination but there is no equivalent PaginatedChats story. The chat table uses the identical slice/clamp/PaginationWidgetBase pattern with separate state (chatPage/setChatPage). Third round this gap has been noted. (Bisky P3)
🤖
There was a problem hiding this comment.
Bisky, explain why this needs PaginatedChats. Isn't this scope creep?
- Fix goToFirstPage race with setSearchParams (C2) - Preserve query params in /usage → /spend redirect (C3) - Reset ChatCostSummaryView pagination on data change (C4) - Extract GroupOverrideController and UserOverrideController (C5) - Make isLoadingConfig inline instead of blocking page (C6) - Add click-to-drill-in story (C7) - Rename selectedUser → drillInUser for clarity (C8) - Rename hasExplicitDateRange → endDateIsExclusive (C9) - Extract named constants for magic numbers (C10) - Remove stray whitespace expressions (C11) - Rename usage vestiges in search params and descriptions - View reduced from 710 lines / 12 useState to 500 lines / 0 useState
4a83e19 to
a59b3ee
Compare
- Fix goToFirstPage race with setSearchParams (C2) - Preserve query params in /usage → /spend redirect (C3) - Reset ChatCostSummaryView pagination on data change (C4) - Extract GroupOverrideController and UserOverrideController (C5) - Make isLoadingConfig inline instead of blocking page (C6) - Add click-to-drill-in story (C7) - Rename selectedUser → drillInUser for clarity (C8) - Rename hasExplicitDateRange → endDateIsExclusive (C9) - Extract named constants for magic numbers (C10) - Remove stray whitespace expressions (C11) - Rename usage vestiges in search params and descriptions - View reduced from 710 lines / 12 useState to 500 lines / 0 useState
a59b3ee to
572283b
Compare
mafredri
left a comment
There was a problem hiding this comment.
Round 8. All 30 prior findings resolved. PR was rebased onto a new base.
Severity count: 1 P2, 3 P3.
The P2 is a re-evaluation of the timezone issue. In R3, we switched isMidnight to UTC methods to fix cross-timezone URL sharing. In R5, we downgraded the inverse problem (DateRangePicker produces local midnight) as pre-existing. Meruem's R8 analysis changes that assessment: the R3 fix regressed the common case. DateRangePicker's toBoundary produces local midnight. After URL round-trip (toISOString), local midnight becomes non-midnight UTC. isMidnight returns false for every non-UTC user who picks a date. The old code used local-time methods, which were correct for the picker case. The fix should be reverted to local-time methods to match toBoundary's contract.
Meruem: "The R3 finding that motivated the switch to UTC was testing with a UTC midnight string, which is not what the DateRangePicker produces. The fix broke the common case without helping the edge case."
PS. Hisoka noted existingGroupIds/existingUserIds in controller props are computed but never consumed by the view (dead entries). Meruem noted all dateRange tests use UTC midnight strings, masking the real picker boundary format.
🤖 This review was automatically generated with Coder Agents.
| * inclusive end date instead. | ||
| */ | ||
| function isMidnight(date: Date): boolean { | ||
| return ( |
There was a problem hiding this comment.
P2 isMidnight checks getUTCHours(), but DateRangePicker.toBoundary produces local midnight via dayjs(to).startOf("day").add(1, "day").toDate(). After URL round-trip (.toISOString()), local midnight becomes non-midnight UTC for every non-UTC user. isMidnight returns false, toInclusiveDateRange does not adjust, and both the picker button label and drill-in date range show the exclusive boundary (one day too many). (Hisoka P2, Meruem P2)
The R3 fix that switched to UTC was motivated by a test using "2025-06-08T00:00:00Z" (UTC midnight), which is not what the DateRangePicker actually produces. The fix broke the common case (same-timezone picker usage) without helping the edge case (cross-timezone URL sharing, where neither approach works).
Fix: revert to local-time methods (getHours(), getMinutes(), etc.) to match toBoundary's convention. Or use dayjs(date).isSame(dayjs(date).startOf("day")) which is timezone-agnostic.
🤖
| await canvas.findByText("Back"); | ||
| }, | ||
| }; | ||
|
|
There was a problem hiding this comment.
P3 SpendDrillInLoading play function asserts "Back", which is present in all three branches (loading, error, success). If the loading branch regressed to render only the back button without the spinner, this story would still pass. Third round this has been raised. (Mafuuu P3, Meruem P3)
Fix: await expect(canvas.getByRole("status")).toBeInTheDocument();
🤖
| {pagedModels.map((model) => ( | ||
| <TableRow key={model.model_config_id}> | ||
| <TableCell>{model.display_name || model.model}</TableCell> | ||
| <TableCell className="text-content-secondary"> |
There was a problem hiding this comment.
P3 Chat title links in the cost breakdown table navigate to /agents/${chat.root_chat_id}. When an admin drills into another user's spend data, these root_chat_id values belong to that user's chats. The backend will 403 or 404 because the admin does not own the chat. The user clicks a title and lands on a dead end. The old code rendered plain text. (Hisoka P3)
Options: conditionally render links only when viewing own data, add admin read access for chat metadata, or keep as plain text when the viewer is not the chat owner.
🤖
There was a problem hiding this comment.
Hisoka, did you actually prove that the admin can't access the users chat?
| { index: true, element: <Navigate to="behavior" replace /> }, | ||
| { path: "behavior", element: <BehaviorRouteElement /> }, | ||
| { path: "usage", element: <UsageRouteElement /> }, | ||
| { path: "spend", element: <AgentSettingsSpendPage /> }, |
There was a problem hiding this comment.
P3 AgentSettingsSpendPage is rendered without the now prop, making the default date range non-deterministic. Other pages in the same routing table pass now={fixedNow}. This will cause flaky visual regression snapshots if Chromatic runs at different times. (Meruem P3)
Fix: <AgentSettingsSpendPage now={fixedNow} />
🤖
- Fix goToFirstPage race with setSearchParams (C2) - Preserve query params in /usage → /spend redirect (C3) - Reset ChatCostSummaryView pagination on data change (C4) - Extract GroupOverrideController and UserOverrideController (C5) - Make isLoadingConfig inline instead of blocking page (C6) - Add click-to-drill-in story (C7) - Rename selectedUser → drillInUser for clarity (C8) - Rename hasExplicitDateRange → endDateIsExclusive (C9) - Extract named constants for magic numbers (C10) - Remove stray whitespace expressions (C11) - Rename usage vestiges in search params and descriptions - View reduced from 710 lines / 12 useState to 500 lines / 0 useState
572283b to
f06a57f
Compare
mafredri
left a comment
There was a problem hiding this comment.
Round 9. All 34 prior findings resolved. Mafuuu found 0 issues after a full four-mode trace including running both test suites (12/12 dateRange, 53/53 chats).
Severity count: 2 P3 (inline), 1 P3 (body).
The isMidnight revert to local-time methods is correct. Kite confirmed the local-time convention matches DateRangePicker's toBoundary and the roundtrip is consistent within a single timezone. The cross-timezone URL-sharing edge case (cosmetic label difference) is documented in a code comment. No data correctness impact.
New findings are shared-component side effects and a test assertion gap:
P3 (body) Shared ChatCostSummaryView changes affect the personal Analytics page at /agents/analytics: (1) table header renamed from "Conversation" to "Agent" (Hisoka), (2) client-side pagination at 10 rows now shows on Analytics for users with >10 models/chats (Kite), (3) table container border removed (Hisoka Note), (4) SectionHeader action layout shifted inline (Kite). None are functional bugs, but they are undocumented visual changes to a page outside this PR's stated scope. Consider whether the label rename warrants a chatColumnLabel prop on ChatCostSummaryView to let each consumer set its own terminology.
Kite: "The risk profile is uniform across all pieces. Nothing here warrants a separate PR."
PS. The default page size changed from 10 to 25 (via usePaginatedQuery default) -- minor UX improvement not called out in the PR description.
🤖 This review was automatically generated with Coder Agents.
| limit: 25, | ||
| offset: 25, | ||
| searchParams: new URLSearchParams(), | ||
| }; |
There was a problem hiding this comment.
P3 Test asserts expect.objectContaining({ username: undefined }) but does not verify limit and offset are forwarded. If someone removes pagination parameter forwarding from the queryFn, this test still passes. (Bisky P3, Kite P3)
One-line fix: expect.objectContaining({ username: undefined, limit: 25, offset: 25 }).
🤖
There was a problem hiding this comment.
Round 10: still open, 2 rounds of author silence, re-raised by 4/4 reviewers. The test provides limit: 25, offset: 25 in pageParams but the assertion is expect.objectContaining({ username: undefined }). If limit/offset forwarding in the queryFn were removed, this test still passes and pagination silently breaks.
One-line fix: expect.objectContaining({ username: undefined, limit: 25, offset: 25 }).
🤖
There was a problem hiding this comment.
Why must the test prove limit and offset? Test name says username is the UUT.
There was a problem hiding this comment.
🤖 This comment was written by Coder Agent on behalf of Danielle Maywood 🤖
Fixed in 74a17a5. The assertion now checks { username: undefined, limit: 25, offset: 25 } to verify pagination parameter forwarding.
| aria-label="Cost breakdown by conversation" | ||
| > | ||
| <div> | ||
| <Table aria-label="Cost breakdown by agent"> |
There was a problem hiding this comment.
P3 "Conversation" renamed to "Agent" in the table header, empty-state label, and aria-label. ChatCostSummaryView is shared with the personal Analytics page at /agents/analytics. A non-admin user reviewing their own cost breakdowns now sees "Cost breakdown by agent" where "Cost breakdown by conversation" was the more natural label. (Hisoka P3)
Option: accept a chatColumnLabel prop (defaulting to "Agent" for admin Spend, "Conversation" for personal Analytics).
🤖
There was a problem hiding this comment.
Round 10: 2 rounds of author silence. 3/4 reviewers re-raised. Hisoka resolved by design, arguing both consumers now display agent sessions and the terminology is consistent.
Response needed: if the rename to "Agent" is intentional for both the admin Spend drill-in and the personal Analytics page, an acknowledgment closes this. If the personal Analytics page should say "Conversation", a chatColumnLabel prop on ChatCostSummaryView would let each consumer set its own label.
🤖
There was a problem hiding this comment.
🤖 This comment was written by Coder Agent on behalf of Danielle Maywood 🤖
Intentional. "Agent" is the correct terminology for both the admin Spend page and the personal Analytics page — both display agent sessions under /agents/ routes. No chatColumnLabel prop needed.
Thanks R9! All 34 resolved, clean trace — great to see. On the P3 findings:
|
Documentation CheckUpdates Needed
Automated review via Coder Tasks |
- Deep-link search, page, and date range via URL search params - Remove double page-reset in search onChange handler - Add redirect routes for removed /limits and /usage paths - Render configError inline instead of blocking entire page - Fix duplicate user_id in mock data (user-3 → user-4) - Extract SpendDrillInView into components/SpendDrillInView - Extract DefaultLimitController into components/LimitsTab/ - Remove pass-through handleDelete* wrappers - Add missing stories: config-error, users-loading, users-error - Add dateRange.test.ts with 11 unit tests
Replace hand-rolled page/offset/limit URL params with the standard usePaginatedQuery hook and PaginationContainer component. This aligns the spend page with every other paginated page in the codebase and picks up prefetching, invalid-page correction, and keepPreviousData for free. - Add paginatedChatCostUsers query factory in chats.ts - Replace manual page/offset state with usePaginatedQuery - Replace PaginationWidgetBase + PaginationAmount with PaginationContainer - Collapse 7 pagination-related view props into a single usersQuery prop - Search filter reset uses goToFirstPage() per codebase convention
- Fix goToFirstPage race with setSearchParams (C2) - Preserve query params in /usage → /spend redirect (C3) - Reset ChatCostSummaryView pagination on data change (C4) - Extract GroupOverrideController and UserOverrideController (C5) - Make isLoadingConfig inline instead of blocking page (C6) - Add click-to-drill-in story (C7) - Rename selectedUser → drillInUser for clarity (C8) - Rename hasExplicitDateRange → endDateIsExclusive (C9) - Extract named constants for magic numbers (C10) - Remove stray whitespace expressions (C11) - Rename usage vestiges in search params and descriptions - View reduced from 710 lines / 12 useState to 500 lines / 0 useState
mafredri
left a comment
There was a problem hiding this comment.
Round 10. No PR-specific code changes since R9 (delta is entirely rebase). All R8 findings resolved. Two R9 findings remain open after 2 rounds of author silence.
Severity count: 2 open P3 (from R9), 1 new P3 note.
Findings 35 and 36 have gone 2 rounds without author response. Both are minor P3s. Stating what response is needed:
F35 (#comment-3053554521): paginatedChatCostUsers test does not assert limit/offset forwarding. 4/4 reviewers re-raised. One-line fix: add limit: 25, offset: 25 to the expect.objectContaining call. Response needed: fix or acknowledge the gap is intentional.
F36 (#comment-3053554525): "Conversation" renamed to "Agent" in shared ChatCostSummaryView, affecting the personal Analytics page. 3/4 reviewers re-raised. Hisoka resolved by design: "Both consumers display agent sessions. The terminology is now consistent product vocabulary." Response needed: confirm the rename is intentional for both contexts, or add a chatColumnLabel prop.
New: Bisky noted SpendUserDrillIn story asserts user profile elements but not cost content from the summary. If ChatCostSummaryView were disconnected, the story would still pass. Not blocking.
Bisky: "Oh, these [dateRange tests] are lovely. Pure functions, deterministic inputs, no timezone ambiguity. Real gems."
Hisoka: "Silence on the rest."
🤖 This review was automatically generated with Coder Agents.
| limit: 25, | ||
| offset: 25, | ||
| searchParams: new URLSearchParams(), | ||
| }; |
There was a problem hiding this comment.
Why must the test prove limit and offset? Test name says username is the UUT.
| total_runtime_ms: 0, | ||
| })); | ||
|
|
||
| export const PaginatedModels: Story = { |
There was a problem hiding this comment.
Bisky, explain why this needs PaginatedChats. Isn't this scope creep?
| {pagedModels.map((model) => ( | ||
| <TableRow key={model.model_config_id}> | ||
| <TableCell>{model.display_name || model.model}</TableCell> | ||
| <TableCell className="text-content-secondary"> |
There was a problem hiding this comment.
Hisoka, did you actually prove that the admin can't access the users chat?
mafredri
left a comment
There was a problem hiding this comment.
Round 11. No code changes since R9. Both contested findings closed by reviewer consensus.
F35 (test limit/offset assertion): human reviewer's defense accepted by 4/4 reviewers. The test's stated scope is username coercion and query key structure. Pagination parameter forwarding is the framework's responsibility. Closed.
F36 (Conversation->Agent rename): 3/4 reviewers close it. The terminology is consistent with the product surface (/agents/ routes, sidebar labels, page headers). Hisoka: "The 3 rounds of silence are a process concern; the code is done." Closed.
Severity count: 1 new P3.
Meruem found one structural observation: cross-controller dismissal uses raw setters (setShowUserForm(false); setEditingUserOverride(null)) instead of calling the handlers (handleShowUserFormChange(false)) which already encapsulate this cleanup. If either handler gains additional cleanup logic, these call sites silently skip it. The dual interface (raw setters + handlers) invites the wrong call.
Bisky: "Oh, this suite looks lovely now. Twelve rounds and it shows. Every story I inspected has a real assertion that would break if the feature it claims to test were removed. Genuine stones, all of them."
Hisoka: "11 rounds smoothed every edge I could find."
🤖 This review was automatically generated with Coder Agents.
- Assert limit/offset forwarding in paginatedChatCostUsers test (F35) - Use handler methods instead of raw setters for cross-controller dismissal to preserve cleanup invariants (F37)
What
Merges the separate Limits and Usage pages into a single Spend page.
Why
The Limits and Usage pages were closely related but lived on separate routes. Combining them gives admins a single place to configure limits and immediately see the effect on usage.
This PR does the structural reorganization only. Visual overhaul (CollapsibleSection cards) will follow separately.
Changes
AgentSettingsSpendPagecontainer,AgentSettingsSpendPageViewunified view with 3 SectionHeader sectionsSpendDrillInViewextracted for per-user drill-informatUsageDateRange+dateRange.tsshared date helpers/limitsand/usagereplaced by/spendAgentSettingsLimitsPage,AgentSettingsUsagePageView,LimitsTab/LimitsTab.tsx,LimitsTab/index.tsusePaginatedQuerydefault)