perf(desktop): paginate usage activity - #4539
Conversation
15e250d to
0bb0081
Compare
|
CC @liuxiaocs7 |
|
Thanks for working on this. The core pagination behavior appears to address #4531, and CI is green. I found the following issues before merge: Standards
Spec / verification
|
22d229c to
201f380
Compare
Done |
|
@liuxiaocs7 The latest CI run failed on the unrelated |
|
Thank you for the analysis. However, I don’t have permission to rerun the CI checks. Could you please push an empty commit to trigger CI again? |
201f380 to
189a421
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
The pagination itself is sound and the numbers justify it, but one normal-path reset case is missing and the diff is larger than the problem needs.
On the stale approval: the +1 was submitted against 201f380d, and the branch was force-pushed to 189a4215 afterwards. I diffed the PR's own files at both SHAs, and features/usage-activity/index.ts, features/usage-activity/ui/usage-activity-pagination.tsx and settings/usage-settings-page.tsx are byte identical. The only differences are unrelated About-page story fixtures and the two regenerated inventory files, all coming from main. So the delta since the approval is a pure rebase, and the earlier round of feedback (barrel split, rowIndexStart/rowCount, reset coverage, HStack from @maka/ui, the 10-run median/p95 benchmark) is already addressed at this head.
What I verified and found no problem with:
- Totals the user sees are unchanged. The tab count still reads
stats.logs.length,recordCountstill readsfilteredLogs.length, the metric cards still readstats.summary.*, androwCountpassed to the table is the full filtered count, not the page length.rowIndexStartandrowCountare real props in@astryxdesign/core@0.5.2. - The story fails on old behavior through the production component: without pagination the first render has 53 rows, so
toHaveLength(50)breaks. The fixture giving every record the same provider is deliberate and correct here: it keeps the row set constant across the filter change, so returning to page 1 can only come fromreset().
P3: shortUsageActivitySessionId has nothing to do with pagination, but it now lives in ui/usage-activity-pagination.tsx and its only caller is still usageSessionDisplayLabel in usage-settings-page.tsx. The move turns a private helper into a cross-file export plus a barrel entry plus a renderer-architecture.json dependency edge, and buys nothing. Leaving it (and the parseDesktopSessionKey import) where it was would shrink this PR.
Also note the branch is currently CONFLICTING. usage-settings-page.tsx is a real code conflict; renderer-architecture.json and the two astryx-surface-file-inventory files just need regenerating after the rebase.
| </>; | ||
| } | ||
| return ( | ||
| <UsageActivityPagination items={props.logs} pageSize={USAGE_REQUESTS_PAGE_SIZE}> |
There was a problem hiding this comment.
P2: changing the time range does not return the table to page 1. setRange (line 120) only persists, the surface then refetches, and UsageActivityPagination keeps whatever page number it held. A user sitting on page 5 of a 90d history switches to 7d and lands on page 5 of a completely different dataset, or gets silently clamped to its last page. The same applies to Refresh: new records are prepended, so the page 2 window shifts under the reader and boundary rows repeat. The reset wiring added for search, status and clear filters covers the filter paths but not the dataset paths.
Smallest fix: <UsageActivityPagination key={usageDraft.range} items={props.logs} pageSize={USAGE_REQUESTS_PAGE_SIZE}>, so a range change remounts and resets. To cover the refetch shift too, also reset when the items identity changes.
| reset(): void; | ||
| } | ||
|
|
||
| export function UsageActivityPagination<Item>(props: { |
There was a problem hiding this comment.
P3: @astryxdesign/core@0.5.2 already ships the seam this reimplements. useTablePagination is the Table plugin for exactly this shape (consumer owns the page state, the plugin renders Pagination around the table, with position, align, size and label). The PR uses paginateData from the same module but routes around the plugin with a render-prop component, a new feature directory and a barrel.
The smaller equivalent: hold page in UsageRequestsPanel (above the showDetails early return), call paginateData directly, and pass the plugin through UsageStatsTable. That deletes features/usage-activity/ entirely, the UsageActivityPage type, and the renderer-architecture.json plus astryx-surface-file-inventory churn, and turns the reset into a plain setPage(1). I have not rendered the plugin's chrome to confirm it matches the current HStack + Pagination layout; if it does not, the fallback is still to move the state into UsageRequestsPanel and drop the wrapper while keeping the hand-placed Pagination.
| ); | ||
| } | ||
|
|
||
| export function shortUsageActivitySessionId(sessionKey: string) { |
There was a problem hiding this comment.
P3: this helper is about session ids, not pagination, and its only caller is still usageSessionDisplayLabel in usage-settings-page.tsx. Keeping it there (along with the parseDesktopSessionKey import) avoids a new public export, a barrel entry and a dependency edge in renderer-architecture.json.
Large usage histories rebuilt every activity row on each tab visit. Limiting each page to 50 records keeps repeated navigation responsive. CLOSES apache#4531 Generated-by: OpenAI Codex Signed-off-by: Jiawei Zhao <Phoenix500526@163.com>
189a421 to
9741434
Compare
|
@Astro-Han, thanks for the review. I rebased the branch and applied the simplification: the separate usage-activity feature, pagination wrapper, barrel export, and architecture edge are gone, while shortUsageActivitySessionId and parseDesktopSessionKey remain in the existing Usage view. Pagination now uses the Astryx table primitives directly in that view. The story covers returning from page 2 after model search, status changes, clearing filters, range changes, and refresh, and it retains the page-2 rowIndexStart and full rowCount assertions. The PR is now limited to the two existing Usage UI files and their Storybook coverage. The current CI failure is the unrelated transcript-scroll timing test; the pagination and local verification paths are unchanged. |
Astro-Han
left a comment
There was a problem hiding this comment.
The simplification landed well — the separate feature, the wrapper, the barrel export and the architecture edge are all gone, and the change is now two files that already existed plus their stories. That is the shape I was hoping for. shortUsageActivitySessionId and parseDesktopSessionKey staying put is right.
You're also right about CI: the failure is transcript-scroll.spec.ts:746, unrelated to this PR. I've rerun it. Worth knowing what it actually is, since it will keep hitting your PRs: it expected turn-fixture-1 and got turn-prompt-rail-111 — a turn from a different fixture Session. That is the same fixture race #4714 diagnoses for prompt-rail.spec.ts, showing up in a second spec file.
P2 — page reset is implemented three times over.
UsageRequestsPanel now resets the page from three independent places:
-
key={${usageDraft.range}:${usageRefreshRevision}}remounts the panel on range change and on refresh; -
useEffect(() => setPage(1), [props.logs]); - three manual
setPage(1)calls — model filter, status, clear filters.
filteredLogs is memoized on [stats, usageDraft.status, normalizedModelFilter], so its identity already changes for every one of those cases: a filter edit changes normalizedModelFilter, a status change changes usageDraft.status, and both a range change and a refresh produce a new stats. The effect alone covers all three triggers.
So (2) subsumes (1) and (3). Dropping them takes the usageRefreshRevision state with it — it exists only to feed that key — and removes the setPage(1) calls interleaved into the three handlers, which are the part most likely to drift when a fourth filter is added later and someone forgets the reset.
One detail if you keep the effect as the single owner: logs={showRequestDetails ? filteredLogs : []} builds a fresh [] on every render, so the effect fires on each render while details are collapsed. It's harmless today (the state is already 1, so React bails out, and the panel early-returns the Banner anyway), but hoisting that empty array to a module constant makes the dependency honest.
Nothing here is wrong as it stands — the behaviour is correct in all three paths. It's one mechanism where there are currently three.
Summary
The slowdown came from Settings → Usage → Activity log rendering every
matching record whenever the tab was opened. With hundreds of records, React
rebuilt the entire table—including per-row tooltips and actions—on every visit.
Paginate the log at 50 records per page so only the visible rows are mounted.
Searching, changing status, or clearing filters returns the table to the first
page.
Fixes #4531
Verification
npm --workspace @maka/desktop run typechecknpm exec -- biome linton the changed TypeScript filesnpm run format:check(1,857 files)npm run check:renderer-architecture -- --base upstream/main(61 tests)npm --workspace @maka/desktop run build-storybooknpm --workspace @maka/desktop run smoke:storybook(502 renders)on the same build: all rows median 1,616.9 ms / p95 1,670.2 ms; 50 per page
median 74.0 ms / p95 80.0 ms; 21.9× faster by median
The benchmark measures Activity log rendering only; it does not include
loading the usage history from the backend.
UI
Before Change
After Change
Benchmark result
Save this snippet as
/tmp/maka-usage-benchmark.cjs, then run it from a Makacheckout with dependencies and Playwright Chromium installed.
$ cd maka-agent $ node /tmp/maka-usage-benchmark.cjs All rows: median 1616.9 ms, p95 1670.2 ms 50/page: median 74.0 ms, p95 80.0 ms Speedup: 21.9x by medianAI use
Select exactly one:
Tool(s) and scope: OpenAI Codex assisted with implementation, tests,
benchmarking, review, and verification.
Checklist
Does this PR entail a change in behavior?