Repository navigation
fix(logs): identify decode estimate timing basis - #6582
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (20)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughSuccessful decode-rate results now identify their timing basis as generation-window or legacy post-visible-output. Logs displays a corresponding label, or an unknown-basis label when the field is absent or unrecognized. The basis is derived at read time; stored historical logs are unchanged. ChangesDecode timing basis
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to This change labels which timing window produced each decode-rate estimate in Logs. Existing calculations and stored logs are unchanged, and no merge-blocking issue was found. Owner review and the untested installed-app and cross-platform paths remain normal gates. Security Architecture ReviewSecurity architecture risk: ⚪ Minimal · up to The change adds descriptive timing metadata and labels without changing rate calculations, access controls, or stored logs. Unrecognized and older response values receive fixed unknown-basis labels. No material security risk introduced or worsened by this change was identified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 18.18% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 18 files. (6 skipped: 6 unsupported.)
✨ 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 |
|
✅ Deterministic PR hygiene checks passed. |
|
Current-head update: 5b70da7 includes the dev 584b92a fixture compaction through a normal merge. The previously reported conflict is resolved; GitHub reports mergeable:true. Direct comparison of this PR's runtime, GUI, behavior tests and layout manifest against fcf2ae0 shows no change; the author documents the fixture's 1,988-entry preservation and fresh layout/ratchet validation. Those author counts are not newly executed local checks by me. Exact-head hosted Cross-platform CI 37211022202 and React Doctor 37211022198 completed success. This supersedes the old-head CI and conflict hold below; Draft/readiness and owner UI review remain separate. Keep #6567/#6568's unresolved partial scope and the failed broader local run explicit. No new scan, GUI implementation, approval or merge was performed in this checkpoint. Historical checkpoint: Hosted run Please refresh Hosted verification with the completed result and complete the normal head-bound readiness checklist when ready; Draft and owner/maintainer UI review remain in force. Keep #6567's partial-scope limits explicit rather than treating this as resolution of every throughput/latency concern. No new scan, native/provider call, approval, waiver, integration or deployment was performed. New integration hold: trusted dev advanced to |
Rename the timing-basis labels so they describe how the estimate was timed rather than its age: "Generation window", "After visible output" and "Timing unknown", with matching request-detail labels. Request details now show a visible one-line explanation of the method under the rate. Numbers stay on one line; captions wrap without breaking words. All eleven locales, the docs and the structure contract are updated, and the existing timing-basis test covers the hint for every branch.
|
Maintainer integration (lidge-jun, dev only) — 2026-10-05 bug triage round 2, lane B1.
|
Summary
Logs showed decode-rate estimates computed over two different timing windows under one label, so a generation-window value (~21.8 tok/s) and a post-visible-output value (~134 tok/s) for the same 227 output tokens looked directly comparable. This PR makes each estimate say how it was timed.
displayMetrics.decodeTokPerSecondvalues carrytimingBasis: "generation-window" | "legacy-post-visible-output"(derived at read time; stored JSONL, end-to-end tok/s, request history and aggregates are unchanged).structure/dashboard-and-usage.mdare updated.Refs #6567 (individual Logs timing-basis display only; export/aggregate consistency and the pre-output latency in #6568 are out of scope, so the issue stays open).
Maintainer refresh (lane B1): merged current
dev(37189ca3) into the branch and replaced the age-based labels ("Legacy", "Unknown basis") with method-based wording after a design review. The explanation sentences were checked against the source formulas: generation =lastOutputMs - genStartMs(src/server/management/shared.ts), fallback =durationMs - firstOutputMs. Original implementation by @luvs01.Verification
No local tests were run, per the repository owner's instruction for this round; test evidence is hosted CI at the exact head.
Static checks run at
4e7c4f236b3f9078a236354ae5883ce4053f588c, all exit 0:bun run typecheck,bun run privacy:scan,bun run structure:check,bun scripts/file-size-ratchet.ts,bun run lint:gui,(cd gui && bun run lint:i18n),(cd gui && bun run build),git diff --check. Every new or changed key exists in all 11 locales.The existing
gui/tests/logs-decode-timing-basis.test.tsxwas extended (no new test files): label keys and hint per branch, the visible hint in details, locale coverage of the hint keys, and no hint for absent, too-short or TTFT-missing rates.Rendered with the real
Logspage and production CSS in a sandboxed Vite harness (temporary HOME/OPENCODEX_HOME/CODEX_HOME, fail-closed fetch stub, synthetic request IDs, no proxy), captured with headless Chromium; no console errors and no external requests.Exact-head CI: Cross-platform CI 37321706773 completed success on attempt 1 for
4e7c4f236b3f9078a236354ae5883ce4053f588c(pull_request event): changes, gates, all four Linux test shards, storage policy, API usage, docs site build, structure gate, Docker smoke, keyring (Ubuntu/Windows), npm-global (Ubuntu/Windows) and the finalciaggregate succeeded. The full Windows/macOS matrices, macOS control/widget, setup action, remote helper, desktop shell and the standalone privacy job were skipped by the workflow's change filter (privacy ran insidegates); they are not counted as executed coverage. React Doctor and CodeRabbit passed on this head.List, English, light — method labels under each decode rate:
Request details with a generation-window rate, its explanation, and attempts timed after visible output or with unknown timing:
Korean, dark — an older cached row with unknown timing:
French at 420px · German list
Checklist