fix(gui): render dashboard log timestamps in server timezone - #790
Conversation
|
Warning Review limit reached
Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
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: c36cc46ba8
ℹ️ 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".
c36cc46 to
616eaa0
Compare
|
This pull request currently targets @Wibias Please retarget this PR to Its title has been prefixed with This pull request is being kept as a draft automatically. Once every issue above is resolved, it will be marked ready for review again. |
github-actions
Bot
changed the title
616eaa0 to
58662bb
Compare
Wibias
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 58662bb2ad
ℹ️ 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".
[shipping-github] StatusNot merge-ready yet. This PR is intentionally stacked on
Next: merge #784 to |
Return server timeZone from /api/logs, validate client ICU support, cache the timezone with log rows, and tolerate legacy array responses.
58662bb to
fd43fcb
Compare
Verdict: approve-comment
|
The user's constraint is that their GUI layout must not change. #790's direction is right and its defect is real -- dashboard timestamps render in browser-local time -- but it carries two problems, and only one was about layout. It rewrites /api/logs from an array to a {timeZone, logs} envelope while leaving four array consumers unfixed, and the single test it touches accepts both shapes, so it passes with the production change reverted. The narrower path already exists inside that same PR: it adds timeZone to /api/settings and then never uses it, carrying the same value over two routes. SettingsData already flows through the GUI, so the fix needs no new fetch, no session-cache schema change, and no /api/logs change -- only a timezone argument threaded into two formatters at two call sites. Records the RangeError guard (an ICU-unknown zone would otherwise kill the row render), the browser-TZ != server-TZ test that #790's version could not fail on, and the grep that proves no JSX or CSS moved.
) A proxy running in KST viewed from a UTC browser reported every request nine hours off: `formatLogTimestamp` and `formatLogDateTime` called `toLocaleTimeString(localeTag)` with no zone, so the viewer's machine decided what time the server logged something. The zone rides on `/api/settings`, which is already an object and already flows through the GUI. PR #790 fixed the same defect by rewriting `/api/logs` from a bare array into a `{timeZone, logs}` envelope -- while leaving four suites that read that response as an array untouched (server-auth:1623, claude-native-passthrough:119, openai-provider-option-e2e:489, server-403-permission-e2e:86). It also added a session-cache schema that accepts both the old and new shapes. Curiously that PR already added `timeZone` to `/api/settings` and then never read it, carrying one value over two routes. Layout is untouched, which was the constraint here. The GUI diff is two call sites gaining a third argument; every `className`, element and stylesheet is byte-identical. `git diff -- gui/` filtered for className/element/style changes returns the two timestamp lines and nothing else. The formatters swallow `RangeError`: a zone string this browser's ICU build does not know would otherwise take the whole row render down, and a timestamp in the wrong zone beats an empty log list. The fetch is a one-shot on mount rather than part of the 2s log poll, since the server's zone cannot change while the page is open, and an older proxy without the field simply keeps browser-local formatting. Tests, both verified by ablation rather than assumed: - removing `timeZone` from /api/settings fails the settings test (4/1) - dropping the formatter argument fails the rendering test (4/1) That second one is the case #790's own test could not catch: it compared the response to itself and passed with the production change reverted. The new test fixes 00:30 UTC and asserts Seoul reads 9:30 while UTC reads 12:30, so an ignored argument shows up as a wrong hour. There is also a test asserting /api/logs is still a bare array, placed at the source so the contract fails here rather than in four unrelated suites. The detail dialog gets the same zone; leaving it on browser time would put two different times for one request on screen at once. Co-authored-by: Wibias <37517432+Wibias@users.noreply.github.com>
|
Closing — the defect is real and now fixed on The
Related: the one test this PR does change ( The zone was already available. This PR adds What landed instead, keeping your
The branch is staging: it merges into |
Selection rule is narrow on purpose: an entry must trace to a triage row and must not be waiting on maintainer approval or third-party evidence. That is what keeps the list short -- most open PRs are blocked by approval, not by code. F1 is first because three other stream items sit on it. closeCurrentToolCall() at bridge.ts:378-417 hardcodes status: "completed", and both failure paths call it unconditionally (:776 on adapter error, :835 on implicit EOF). So a stream that ends in failure still hands the client a completed tool call, with response.failed arriving afterward. That is why the adapter-side attempt at #658 was reverted in 95d8ed7, and it is what PR #781 already fixes with failCurrentToolCall(). F2 must not land before F1. Relaxing EOF without a cancel path would let a truncated tool call through as success. Also recorded: bridge.ts:436-441 still says it synthesizes response.completed, but :838 emits response.incomplete. The comment outlived the code. Three tests already on dev would survive reverting the behavior they claim to cover. With #790 and #758 last round, that is a pattern rather than three accidents, so it gets its own issue.
2 participants
Summary
Fixes #725
Renders dashboard log timestamps in the server timezone.
Stack
Stacks on #784 (
fix/726-log-pagination→dev). Land the pagination PR first, then retarget this PR todevif needed.Test plan
bun run typecheckbun run test