fix: add pagination support for recap trace list#596
fix: add pagination support for recap trace list#596anandgupta42 merged 5 commits intoAltimateAI:mainfrom
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 7 minutes and 24 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdded paginated trace listing: new Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as "CLI: trace list"
participant Trace as "Trace.listTracesPaginated"
participant Store as "Filesystem / Trace.listTraces"
User->>CLI: run `trace list --offset N --limit M`
CLI->>Trace: listTracesPaginated(dir, {offset:N, limit:M})
Trace->>Store: listTraces(dir) (reads files / parses JSON)
Store-->>Trace: full trace array (or [] on failure)
Trace-->>CLI: { traces: slice(offset, limit), total, offset, limit }
CLI->>CLI: render header, items, footer ("Showing X–Y of total"), next-page hint
CLI-->>User: paginated listing output
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/src/altimate/observability/tracing.ts`:
- Around line 999-1005: The returned pagination metadata may include NaN,
fractional or infinite values for offset/limit; update the logic that computes
offset and limit in tracing.ts to first coerce inputs to finite integers (use
Number.isFinite and Math.trunc), fall back to the existing defaults (0 for
offset, 20 for limit), then clamp with Math.max(0, offset) and Math.max(1,
limit) before using them for slice and returning them in the response (refer to
the offset and limit variables used in this block).
🪄 Autofix (Beta)
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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b341b9b3-16f3-4876-977b-f1b4c8a4f3fd
📒 Files selected for processing (3)
packages/opencode/src/altimate/observability/tracing.tspackages/opencode/src/cli/cmd/trace.tspackages/opencode/src/cli/cmd/tui/component/dialog-trace-list.tsx
VJ-yadav
force-pushed
the
fix/recap-pagination
branch
from
a7968fc to
b018d62
Compare
❌ Tests — Failures DetectedTypeScript — 15 failure(s)
cc @VJ-yadav |
Review — Ready after rebaseScope: Fixes #418 — recap pagination broken for large lists. All 4 subtasks in the issue are addressed. Analysis
Files changed (3): CI status (commit
Recommendation cc @anandgupta42 — rebase and re-run CI; if green, ready for |
VJ-yadav
force-pushed
the
fix/recap-pagination
branch
from
b018d62 to
dbfa95f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/src/cli/cmd/trace.ts`:
- Around line 171-174: Replace the logical-OR fallbacks for pagination with
nullish coalescing so explicit zeros are preserved: in the call to
Trace.listTracesPaginated (the invocation using tracesDir and the args object),
change args.offset || 0 and args.limit || 20 to args.offset ?? 0 and args.limit
?? 20 so that a provided 0 is passed through to listTracesPaginated (which
performs validation/clamping).
🪄 Autofix (Beta)
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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a9e48842-6741-4a1e-9e40-fe5417610e8f
📒 Files selected for processing (3)
packages/opencode/src/altimate/observability/tracing.tspackages/opencode/src/cli/cmd/trace.tspackages/opencode/src/cli/cmd/tui/component/dialog-trace-list.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/opencode/src/cli/cmd/tui/component/dialog-trace-list.tsx
- packages/opencode/src/altimate/observability/tracing.ts
- Add `listTracesPaginated()` method to Trace class returning page of traces with total count, offset, and limit metadata - Add `--offset` CLI option to `trace list` command for navigating past the first page of results - Wire CLI handler to use `listTracesPaginated()` instead of manual array slicing - Remove hardcoded `.slice(0, 50)` cap in TUI dialog so all traces are accessible via built-in scrolling - Show "Showing X-Y of N" pagination footer with next-page hint - Distinguish empty results (no traces exist) from offset-past-end (offset exceeds total count) with clear messaging Closes AltimateAI#418 Co-Authored-By: Vijay Yadav <vijay@studentsucceed.com>
Coerce offset/limit to finite integers via Number.isFinite and Math.trunc before using them for slice and returning in metadata. Prevents NaN, fractional, or infinite values from producing invalid display ranges. Co-Authored-By: Vijay Yadav <vijay@studentsucceed.com>
anandgupta42
force-pushed
the
fix/recap-pagination
branch
from
dbfa95f to
b30e7d4
Compare
Code review (GPT + Gemini) flagged two follow-ups: 1. TUI freeze risk after removing .slice(0, 50) — DialogSelect creates reactive nodes per item via Solid's <For>, so unbounded rendering can lag noticeably on trace directories with thousands of entries. 2. listTracesPaginated had no unit tests for boundary math. Fixes: - Cap dialog-trace-list at MAX_TUI_ITEMS=500 with an explicit '... N more not shown' footer row pointing users to 'altimate-code trace list --offset N' for the full set. 500 is 10× the old cap and covers the vast majority of real usage, while leaving TUI rendering bounded. The onSelect handler ignores the truncation marker row. - Add 10 listTracesPaginated regression tests covering: empty dir, bounded page size, multi-page traversal, offset === total, offset > total, negative offset clamping, non-positive limit clamping, fractional truncation, NaN → default fallback, and default options. The I/O-layer optimization (slice filenames before JSON.parse) called out by Gemini is a separate perf concern that requires embedding timestamps in trace filenames; tracked as a follow-up.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/opencode/test/altimate/tracing.test.ts">
<violation number="1" location="packages/opencode/test/altimate/tracing.test.ts:746">
P2: Reusing a single `Trace` instance across multiple `startTrace`/`endTrace` cycles causes spans to accumulate — each successive trace file will contain all spans from earlier iterations. Create a new tracer per iteration, matching the pattern used elsewhere in this file (line ~706).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
Consensus code-review applied — fixes pushedRan a 4-participant consensus review (Claude + GPT 5.4 + Gemini 3.1 Pro + self-review). Findings and actionsFinding 1 — TUI freeze risk from uncapped list (both models, MAJOR) Finding 2 — No unit tests for Finding 3 — "Fake pagination" at I/O layer (both models, CRITICAL/MAJOR) Finding 4 — CLI input validation (both models, MINOR) Test coverageAll 75 tracing tests pass (65 pre-existing + 10 new). TypeScript clean. All CI green. Pushed as |
- trace.ts:172-173 (coderabbit): switch pagination fallbacks from || to ?? so an explicit --offset 0 / --limit 0 reaches listTracesPaginated() for clamping, rather than being swallowed as falsy and replaced with defaults. The API already clamps limit<1 to 1, so --limit 0 now routes through that clamp path correctly. - tracing.test.ts:746 (cubic): create a fresh Trace instance per iteration inside seedTraces() rather than reusing a single tracer. Reusing a tracer across startTrace/endTrace cycles accumulates spans in memory across iterations. Matches the pattern used by the maxFiles test.
E2E verification completeRan end-to-end CLI scenarios against a real seeded traces directory (15 traces) and visually inspected the TUI dialog options at multiple scales. All verified. CLI e2e — seeded 15 traces in
|
| Scenario | Command | Output |
|---|---|---|
| Default | trace list |
Shows 1-15 of 15, no next-page hint ✓ |
| Limit=5 | trace list --limit 5 |
Shows 1-5 of 15 + Next page: --offset 5 --limit 5 ✓ |
| Middle page | trace list --offset 5 --limit 5 |
Shows 6-10 of 15 + next-page hint ✓ |
| Last page | trace list --offset 10 --limit 5 |
Shows 11-15 of 15, no next-page hint ✓ |
| Past end | trace list --offset 9999 |
No traces on this page (offset 9999 past end of 15 traces) + Try: --offset 0 --limit 5 ✓ |
| Negative offset | --offset -5 --limit 3 |
Clamped to offset=0, shows 1-3 ✓ |
| Zero limit | --limit 0 |
Clamped to limit=1, shows 1-1 ✓ |
| Fractional | --offset 2.9 --limit 3.7 |
Truncated to offset=2 limit=3, shows 3-5 ✓ |
The ?? fix (nullish coalescing) preserves explicit 0 values correctly: --limit 0 reaches the API's clamp-to-1 path rather than being promoted to the 20 default.
TUI visual inspection — dialog options at multiple scales
Replicated the options() memo from dialog-trace-list.tsx with fake trace data and rendered what the dialog would show:
| Trace count | Rows rendered | Truncation marker | Rationale |
|---|---|---|---|
| 0 | 0 | — | Empty state ✓ |
| 10 | 10 | — | Under cap, all visible ✓ |
| 500 (exactly at cap) | 500 | no | Edge-case: cap unreached ✓ |
| 501 | 501 (500 + marker) | ... 1 more not shown |
Just-over-cap triggers marker ✓ |
| 600 | 501 (500 + marker) | ... 100 more not shown + Showing 500 of 600 — use CLI --offset to navigate |
Gemini's freeze-risk scenario ✓ |
| 5000 | 501 (500 + marker) | ... 4500 more not shown |
Extreme case — saves 4500 reactive nodes ✓ |
Marker row specifics:
title: "... N more not shown"category: "Older"(groups below date categories)footer: "Showing 500 of <total> — use CLI --offset to navigate"value: "__truncated__"— onSelect handler explicitly no-ops on this value- Error/crashed trace statuses render with
[error]/[crashed]prefixes ✓
What's covered now
- 10 unit tests —
listTracesPaginatedboundary math - 8 CLI e2e scenarios — verified against real seeded traces
- 6 TUI scale scenarios — verified against fake data at 0/10/500/501/600/5000 items
- Status badge rendering — verified
[error]prefix - TypeScript + all existing tracing tests — green
All CI checks CLEAN. Ready for merge.
3 participants
Summary
Fixes #418 — recap pagination broken for large lists.
.slice(0, 50)cap so all traces are accessible via the dialog's built-in scrolling--offsetoption: added--offsetflag totrace listcommand, enabling navigation past the first pagelistTracesPaginated(): added paginated variant oflistTraces()that returns{ traces, total, offset, limit }metadata, used by the CLI handlerTest Plan
altimate-code trace list— shows first 20 traces with "Showing 1-20 of N" footeraltimate-code trace list --limit 5— shows 5 traces with next-page hintaltimate-code trace list --offset 5 --limit 5— shows traces 6-10altimate-code trace list --offset 9999— shows "offset past end" message, not "No traces found"altimate-code trace listwith 0 traces — shows "No traces found" with setup hintaltimate-code trace view <bad-id>— fallback list shows pagination metadataChecklist
listTracesPaginatedclamps offset/limit to safe valuesSummary by CodeRabbit