fix(logs): raise in-memory log buffer and support limit/offset - #784
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR expands request-log retention to 2,000 entries, adds filtered pagination and envelope responses to ChangesRequest-log pagination API
Stacked PR target validation
GitHub star test isolation
Test runtime adjustment
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant LogsPage
participant LogsEndpoint
participant RequestLog
LogsPage->>LogsEndpoint: GET /api/logs?limit=2000
LogsEndpoint->>RequestLog: Apply filters and pagination
RequestLog-->>LogsEndpoint: Return logs and pre-pagination total
LogsEndpoint-->>LogsPage: Return timeZone, total, logs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7e59c02a9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Align limit/offset with tail semantics, report filtered total before pagination, and document the logs response shape in the dashboard guide.
Keep upstream useDataSurface loader shape while requesting limit=2000
and parsing the {logs} envelope.
Verdict: approve-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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/scripts/enforce-pr-target.test.cjs:
- Line 68: Add behavioral assertions to the stacked-PR tests around the
fixture’s distinct base/head owners, verifying the matching-owner case and a
mismatched-owner negative case. Exercise the workflow decision rather than only
checking its source text, so the owner from other.base?.repo?.owner is confirmed
to participate in the repository-boundary comparison.
In `@gui/src/pages/Logs.tsx`:
- Around line 367-370: Update the log refresh flow around the fetch and body
parsing to request only a bounded newest page rather than all 2,000 entries.
Preserve the response envelope metadata, use total to determine whether older
pages remain, fetch them with offset-based pagination on demand, and merge
results by requestId while keeping management API state consistent.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f3292994-a66a-4a4b-8687-86b5ca5f1b40
📒 Files selected for processing (20)
.github/scripts/enforce-pr-target.test.cjs.github/workflows/enforce-pr-target.ymldocs-site/src/content/docs/guides/web-dashboard.mddocs-site/src/content/docs/ja/guides/web-dashboard.mddocs-site/src/content/docs/ko/guides/web-dashboard.mddocs-site/src/content/docs/ru/guides/web-dashboard.mddocs-site/src/content/docs/zh-cn/guides/web-dashboard.mdgui/src/pages/Logs.tsxsrc/server/management/logs-usage-routes.tssrc/server/request-log.tstests/claude-messages-endpoint.test.tstests/claude-native-passthrough.test.tstests/helpers/logs-api.tstests/management-api-logs-metrics.test.tstests/openai-api-virtual-models.test.tstests/openai-provider-option-e2e.test.tstests/request-log.test.tstests/server-403-permission-e2e.test.tstests/server-auth.test.tstests/server-combo-failover-e2e.test.ts
Windows runners honor Path over PATH, so the release-helper fakes never ran and preflight saw the real branch instead of the faked main.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/release-helper.test.ts (1)
340-352: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStrengthen the launcher-bypass assertion
The release script uses
Bun.spawnatscripts/release.ts:51andscripts/release.ts:92, but both current calls correctly usecommandInvocation. The test attests/release-helper.test.ts:340-351only checks thatcommandInvocationappears somewhere, so an additional unwrappedBun.spawn,spawn, orexecFilecall would still pass.Assert that every external-command path uses the shared launcher. A focused solution is to inject the launcher or assert that each
Bun.spawnreceives thecommandInvocationresult, while preserving direct spawning as the transport after resolution.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/release-helper.test.ts` around lines 340 - 352, The test around “every external command goes through the shared launcher” is too weak because it only checks that commandInvocation appears somewhere. Strengthen it to verify each external-command invocation, including Bun.spawn, spawn, or execFile paths, receives the result of commandInvocation while preserving direct spawning as the transport after resolution.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/release-helper.test.ts`:
- Around line 340-352: The test around “every external command goes through the
shared launcher” is too weak because it only checks that commandInvocation
appears somewhere. Strengthen it to verify each external-command invocation,
including Bun.spawn, spawn, or execFile paths, receives the result of
commandInvocation while preserving direct spawning as the transport after
resolution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1ca680ec-88a3-4e58-a976-9ef6cb7f65ce
📒 Files selected for processing (1)
tests/release-helper.test.ts
Keep the upstream single Path-key shim env so Windows release-helper tests hit the fake git.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/release-helper.test.ts`:
- Around line 195-221: Resolve the conflict in the test setup around the
release-helper spawn configuration by removing all conflict markers and
retaining the inheritedEnv implementation. Keep the case-insensitive PATH
filtering, platform-specific pathKey selection, and resulting pathValue
assignment so the child process receives exactly one PATH variant.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 151220b5-9ba6-4cae-bb57-7315d8e708c0
📒 Files selected for processing (1)
tests/release-helper.test.ts
Merging current dev brought in a bare-array assertion that conflicts with lidge-jun#726's {timeZone,total,logs} response shape.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/logs-timezone.test.ts (1)
43-82: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftBind the regression test to the production formatter.
Lines 48-54 reimplement
formatLogTimestamp. The assertions at Lines 61-80 call only this copy. A regression ingui/src/pages/Logs.tsxLines 663 or 824 that stops passingserverTimeZonewould still pass. The productionformatLogDateTimepath is also not covered.Extract both pure formatters into a dependency-free module imported by
gui/src/pages/Logs.tsxand this test, or add a GUI component test that renders the production path.As per path instructions, the source behavior change needs a focused regression test that exercises the production implementation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/logs-timezone.test.ts` around lines 43 - 82, Replace the local formatLogTimestamp test helper with coverage of the production formatter in gui/src/pages/Logs.tsx, preferably by extracting formatLogDateTime and the timestamp formatter into a dependency-free shared module imported by both production code and this test. Ensure the regression assertions exercise the serverTimeZone propagation and fallback behavior through the production implementation, including the formatLogDateTime path.Source: Path instructions
gui/src/pages/Logs.tsx (1)
357-377: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClear
serverTimeZonewhenapiBasechanges.If
apiBasechanges whileLogsremains mounted, the previous zone stays in state until a new valid value arrives. If the new/api/settingsrequest fails or omitstimeZone, logs from the new server use the previous server's zone.Reset the state at effect start. Also ignore completions from an obsolete effect.
Proposed fix
useEffect(() => { const controller = new AbortController(); + let active = true; + setServerTimeZone(undefined); void (async () => { try { const res = await fetch(`${apiBase}/api/settings`, { signal: controller.signal }); if (!res.ok) return; const body = await res.json() as { timeZone?: unknown }; - if (typeof body.timeZone === "string" && body.timeZone.trim()) { + if (active && typeof body.timeZone === "string" && body.timeZone.trim()) { setServerTimeZone(body.timeZone.trim()); } } catch { // Offline or an older proxy without the field: keep browser-local formatting. } })(); - return () => controller.abort(); + return () => { + active = false; + controller.abort(); + }; }, [apiBase]);As per path instructions, GUI state must stay consistent with the active management API response.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gui/src/pages/Logs.tsx` around lines 357 - 377, Update the serverTimeZone effect to clear the existing zone when apiBase changes, before starting the settings request. Ensure asynchronous completions from obsolete effects cannot update state after cleanup, while preserving browser-local formatting when the new request fails or omits timeZone.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@gui/src/pages/Logs.tsx`:
- Around line 357-377: Update the serverTimeZone effect to clear the existing
zone when apiBase changes, before starting the settings request. Ensure
asynchronous completions from obsolete effects cannot update state after
cleanup, while preserving browser-local formatting when the new request fails or
omits timeZone.
In `@tests/logs-timezone.test.ts`:
- Around line 43-82: Replace the local formatLogTimestamp test helper with
coverage of the production formatter in gui/src/pages/Logs.tsx, preferably by
extracting formatLogDateTime and the timestamp formatter into a dependency-free
shared module imported by both production code and this test. Ensure the
regression assertions exercise the serverTimeZone propagation and fallback
behavior through the production implementation, including the formatLogDateTime
path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9758626a-ea5d-4f56-99a9-79e1fe1b1744
📒 Files selected for processing (2)
gui/src/pages/Logs.tsxtests/logs-timezone.test.ts
Route tests were spawning real gh.cmd and burning AUTH_TIMEOUT equal to Bun's 5s deadline. Inject a fast fake via setStarDepsForTests, and give the slow compressed trash restore 20s on Windows CI.
Resolve star-state and sidebar-routes conflicts by taking tip's setStarDepsForTests / withStarDeps approach (supersedes this branch's Windows gh hang workaround).
|
[shipping-github] Addressed wake-gate DIRTY — merged upstream |
Give version --help and status --json spawnSync cases 20s so late-suite Windows runners do not fail the 5s Bun default after a full suite.
|
Merging this. Why it helps: dashboard /api/logs was hard-capped at 200 in-memory entries with no pagination, so conversation filters and stats looked incomplete (#726). This raises the buffer to 2,000, adds newest-first limit/offset with a filtered otal, and returns {timeZone,total,logs} so the GUI and docs can page correctly. Ship it. |
1 participant
Summary
Fixes #726
Raises in-memory log buffer and adds limit/offset support for log pagination.
Test plan
bun run typecheckbun run testSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests