Skip to content

feat: merge Limits + Usage into unified Spend page#24093

Merged
DanielleMaywood merged 6 commits into
mainfrom
danielle/unified-spend-page
Apr 9, 2026
Merged

feat: merge Limits + Usage into unified Spend page#24093
DanielleMaywood merged 6 commits into
mainfrom
danielle/unified-spend-page

Conversation

@DanielleMaywood

@DanielleMaywood DanielleMaywood commented Apr 7, 2026

Copy link
Copy Markdown
Contributor

This PR was written by Coder Agent on behalf of Danielle Maywood

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

  • AgentSettingsSpendPage container, AgentSettingsSpendPageView unified view with 3 SectionHeader sections
  • SpendDrillInView extracted for per-user drill-in
  • formatUsageDateRange + dateRange.ts shared date helpers
  • Router: /limits and /usage replaced by /spend
  • Sidebar: Limits+Usage merged into Spend (WalletIcon), Analytics renamed to Insights
  • Deleted: AgentSettingsLimitsPage, AgentSettingsUsagePageView, LimitsTab/LimitsTab.tsx, LimitsTab/index.ts
  • Default page size changed from 10 to 25 (inherits usePaginatedQuery default)

@github-actions github-actions Bot added the community Pull Requests and issues created by the community. label Apr 7, 2026
@DanielleMaywood DanielleMaywood force-pushed the danielle/analytics-datepicker branch from 0d52625 to d18e3a3 Compare April 7, 2026 13:35
@DanielleMaywood DanielleMaywood force-pushed the danielle/unified-spend-page branch from f110100 to 27b6526 Compare April 7, 2026 13:35
@DanielleMaywood DanielleMaywood force-pushed the danielle/analytics-datepicker branch from d18e3a3 to f36406b Compare April 7, 2026 13:40
@DanielleMaywood DanielleMaywood force-pushed the danielle/unified-spend-page branch 2 times, most recently from d66dff4 to a02da70 Compare April 7, 2026 13:51
@DanielleMaywood DanielleMaywood force-pushed the danielle/analytics-datepicker branch from f36406b to 847cb45 Compare April 7, 2026 13:51
@DanielleMaywood DanielleMaywood force-pushed the danielle/unified-spend-page branch from a02da70 to 668072a Compare April 7, 2026 13:56
@DanielleMaywood DanielleMaywood force-pushed the danielle/analytics-datepicker branch 2 times, most recently from 421a732 to 581dfdc Compare April 7, 2026 15:54
@DanielleMaywood DanielleMaywood force-pushed the danielle/unified-spend-page branch 3 times, most recently from c22d94b to badd0db Compare April 7, 2026 16:01
@DanielleMaywood DanielleMaywood force-pushed the danielle/analytics-datepicker branch 2 times, most recently from 4fe05f9 to f04c99f Compare April 7, 2026 16:17
@DanielleMaywood DanielleMaywood force-pushed the danielle/unified-spend-page branch 2 times, most recently from 4bd794a to 6a95e39 Compare April 7, 2026 16:24
@DanielleMaywood DanielleMaywood force-pushed the danielle/analytics-datepicker branch from f04c99f to 069be14 Compare April 7, 2026 16:24
@DanielleMaywood DanielleMaywood force-pushed the danielle/unified-spend-page branch from 6a95e39 to ff09998 Compare April 7, 2026 17:54
@DanielleMaywood DanielleMaywood force-pushed the danielle/analytics-datepicker branch from 069be14 to 37b0cbb Compare April 7, 2026 17:54
@DanielleMaywood DanielleMaywood force-pushed the danielle/unified-spend-page branch 2 times, most recently from e5e2eb7 to 1f5c719 Compare April 8, 2026 08:09
@DanielleMaywood DanielleMaywood changed the base branch from danielle/analytics-datepicker to main April 8, 2026 08:09
@DanielleMaywood DanielleMaywood removed the community Pull Requests and issues created by the community. label Apr 8, 2026
@DanielleMaywood DanielleMaywood requested a review from mafredri April 8, 2026 09:05
@DanielleMaywood DanielleMaywood force-pushed the danielle/unified-spend-page branch from 323ed66 to aa15780 Compare April 8, 2026 09:14
DanielleMaywood added a commit that referenced this pull request Apr 8, 2026
- 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
@DanielleMaywood DanielleMaywood force-pushed the danielle/unified-spend-page branch from 5a59726 to 445e9ee Compare April 8, 2026 15:35

Copy link
Copy Markdown
Contributor Author

R6 findings addressed in 445e9ee:

  1. selectedUserId now uses searchParams.get("user") || null to treat empty strings as no selection.
  2. SpendDrillInView now imports and uses the existing BackButton component instead of inlining identical markup.
  3. Redundant null guard on spendLimitMicros simplified — the second check is always true when enabled is true.
  4. Added paginatedChatCostUsers factory test covering queryPayload() and queryKey() shape. 53/53 chats tests pass.

🤖 This comment was written by Coder Agent on behalf of Danielle Maywood 🤖

@DanielleMaywood DanielleMaywood requested a review from mafredri April 8, 2026 15:37
DanielleMaywood added a commit that referenced this pull request Apr 8, 2026
- 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
@DanielleMaywood DanielleMaywood force-pushed the danielle/unified-spend-page branch from 445e9ee to 4a83e19 Compare April 8, 2026 15:48

@mafredri mafredri left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread site/src/api/queries/chats.test.ts Outdated

const query = chatCostUsers(params);
// queryPayload returns the original payload.
expect(result.queryPayload()).toEqual(payload);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 = {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

🤖

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bisky, explain why this needs PaginatedChats. Isn't this scope creep?

DanielleMaywood added a commit that referenced this pull request Apr 8, 2026
- 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
@DanielleMaywood DanielleMaywood force-pushed the danielle/unified-spend-page branch from 4a83e19 to a59b3ee Compare April 8, 2026 15:58
DanielleMaywood added a commit that referenced this pull request Apr 8, 2026
- 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
@DanielleMaywood DanielleMaywood force-pushed the danielle/unified-spend-page branch from a59b3ee to 572283b Compare April 8, 2026 18:07
@DanielleMaywood DanielleMaywood requested a review from mafredri April 8, 2026 18:08

@mafredri mafredri left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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");
},
};

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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">

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

🤖

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 /> },

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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} />

🤖

DanielleMaywood added a commit that referenced this pull request Apr 8, 2026
- 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
@DanielleMaywood DanielleMaywood force-pushed the danielle/unified-spend-page branch from 572283b to f06a57f Compare April 8, 2026 18:37
@DanielleMaywood DanielleMaywood requested a review from mafredri April 8, 2026 18:45

@mafredri mafredri left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(),
};

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 }).

🤖

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 }).

🤖

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why must the test prove limit and offset? Test name says username is the UUT.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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">

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

🤖

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

🤖

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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.

Copy link
Copy Markdown
Contributor Author

🤖 This comment was written by Coder Agent on behalf of Danielle Maywood 🤖

Thanks R9! All 34 resolved, clean trace — great to see.

On the P3 findings:

  1. "Agent" column label — Intentional. "Agent" is the correct terminology across both the admin Spend page and the personal Analytics page. No chatColumnLabel prop needed — we want consistency here.

  2. Client-side pagination visible at >10 items on Analytics — Acknowledged. Not a functional bug, and the pagination is a strict UX improvement for users who have enough data to trigger it.

  3. Default page size changed to 25 — This comes from usePaginatedQuery's default. Will note in the PR description.

  4. Table container border / SectionHeader layout — These are incidental to the shared component cleanup. The new layout is intentionally uniform.

@DanielleMaywood DanielleMaywood marked this pull request as ready for review April 8, 2026 19:14
@coder-tasks

coder-tasks Bot commented Apr 8, 2026

Copy link
Copy Markdown
Contributor

Documentation Check

Updates Needed

  • docs/ai-coder/agents/platform-controls/usage-insights.md — Navigation paths are now stale. Line 8 says Navigate to **Agents** > **Settings** > **Limits**. and line 56 says Navigate to **Agents** > **Settings** > **Usage**. — both should be updated to Navigate to **Agents** > **Settings** > **Spend**. since the two pages were merged into a single /spend route.
  • docs/ai-coder/agents/platform-controls/usage-insights.md — The sidebar item Analytics was renamed to Insights in this PR. The doc title (# Usage and Analytics) and the cross-reference in index.md (See [Usage & Analytics](./usage-insights.md)) should be updated to reflect the new naming.

Automated review via Coder Tasks

@DanielleMaywood DanielleMaywood changed the title feat(site): merge Limits + Usage into unified Spend page feat: merge Limits + Usage into unified Spend page Apr 8, 2026
- 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 mafredri left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(),
};

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why must the test prove limit and offset? Test name says username is the UUT.

total_runtime_ms: 0,
}));

export const PaginatedModels: Story = {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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">

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hisoka, did you actually prove that the admin can't access the users chat?

@mafredri mafredri left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread site/src/pages/AgentsPage/AgentSettingsSpendPageView.tsx
- Assert limit/offset forwarding in paginatedChatCostUsers test (F35)
- Use handler methods instead of raw setters for cross-controller
  dismissal to preserve cleanup invariants (F37)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants