fix: prevent diagnostics retention scroll lag - #703
Conversation
Astro-Han
added
bug
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR refactors and implements a complete renderer diagnostics pipeline, splitting a 639-line monolithic module into four focused submodules (types, sanitize, slice, recorder) with comprehensive tests, then re-exporting via a barrel. It adds event allowlisting, payload sanitization, byte-budget capping with retention headroom, serialized write queueing, and high-frequency rate-limiting to prevent diagnostic collection from degrading UI performance. ChangesRenderer Diagnostics Capture and Retention
Sequence Diagram(s)sequenceDiagram
participant Input as RendererDiagnosticInput
participant Sanitize as sanitizeRendererDiagnosticEvent
participant Validate as validateEventName
participant SizeCheck as jsonBytes
participant Output as RendererDiagnosticEvent
Input->>Sanitize: unknown input
Sanitize->>Validate: check event name allowlist
Validate-->>Sanitize: valid or undefined
Sanitize->>SizeCheck: measure payload bytes
SizeCheck-->>Sanitize: byte size
Sanitize-->>Output: sanitized event or undefined
sequenceDiagram
participant Client as Client Code
participant Recorder as createRendererDiagnosticsRecorder
participant Queue as enqueueWrite
participant RateLimit as rateLimitByWindow
participant Retention as maybeFlushRetention
participant FS as Filesystem
Client->>Recorder: record(diagnosticInput)
Recorder->>Recorder: sanitizeRendererDiagnosticEvent
Recorder->>RateLimit: check highFrequencyDrop
RateLimit-->>Recorder: drop or proceed
Recorder->>Queue: append to JSONL
Queue->>FS: write appended event
Recorder->>Retention: periodic maybeFlushRetention
Retention->>FS: read existing JSONL
Retention->>Retention: filter by age + cap by bytes
Retention->>FS: write to temp, rename atomically
FS-->>Retention: on rename error, cleanup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
github-actions
Bot
added
platform
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Suggested priority: P2 (includes user-path files (packages/desktop-electron/src/main/renderer-diagnostics-recorder.ts, packages/desktop-electron/src/main/renderer-diagnostics-sanitize.test.ts, packages/desktop-electron/src/main/renderer-diagnostics-sanitize.ts, packages/desktop-electron/src/main/renderer-diagnostics-slice.test.ts, packages/desktop-electron/src/main/renderer-diagnostics-slice.ts, packages/desktop-electron/src/main/renderer-diagnostics-types.ts, packages/desktop-electron/src/main/renderer-diagnostics.test.ts, packages/desktop-electron/src/main/renderer-diagnostics.ts)).
P1/P0 are reserved for maintainer confirmation. Please relabel manually if this is a release blocker, security issue, data-loss risk, or updater/runtime failure.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the renderer diagnostics system by modularizing the logic into separate files for recording, sanitization, slicing, and type definitions. The review feedback identifies performance bottlenecks in the retention and event capping logic, specifically pointing out that using Array.prototype.shift() and Array.prototype.splice() inside loops results in O(N^2) complexity. It is recommended to optimize these pruning operations to avoid potential main-process blocking when processing large diagnostic logs.
3e023f5 to
35a3e4d
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/desktop-electron/src/main/renderer-diagnostics-recorder.ts`:
- Around line 94-118: readEventReport currently returns a status ("ok" |
"missing" | "corrupt") but readEvents discards that and only returns events,
causing flushRetentionNow to treat a read error-as-corrupt as an empty event
list and potentially truncate the log; update the flow so flushRetentionNow
calls readEventReport (not readEvents) and checks the returned status,
immediately aborting/returning if status === "corrupt" (or handling "missing"
appropriately) before performing the temp-file rename; reference the functions
readEventReport, readEvents, and flushRetentionNow when making the change.
🪄 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: CHILL
Plan: Pro Plus
Run ID: a81d26ab-f103-423a-8dc9-2264b83fbde5
📒 Files selected for processing (8)
packages/desktop-electron/src/main/renderer-diagnostics-recorder.tspackages/desktop-electron/src/main/renderer-diagnostics-sanitize.test.tspackages/desktop-electron/src/main/renderer-diagnostics-sanitize.tspackages/desktop-electron/src/main/renderer-diagnostics-slice.test.tspackages/desktop-electron/src/main/renderer-diagnostics-slice.tspackages/desktop-electron/src/main/renderer-diagnostics-types.tspackages/desktop-electron/src/main/renderer-diagnostics.test.tspackages/desktop-electron/src/main/renderer-diagnostics.ts
Prepare PawWork v2026.5.18 for the stable desktop release. - Bump the desktop package version to 2026.5.18. - Scope the diagnostics unreadable-file retention test to POSIX permission semantics so Windows advisory does not fail on chmod behavior that Windows does not enforce the same way. Verification: - Focused desktop diagnostics test passed locally: 11 pass / 0 fail. - Release typecheck passed locally for packages/desktop-electron. - PR #706 CI passed, including ci, desktop-smoke, e2e-artifacts, CodeQL, dependency-review, label-policy, commit-lint, and title lint. Release notes: - Drafted against the merged range since v2026.5.17: #691, #692, #693, #694, #702, and #703. - Cold-read review completed before merge; wording was tightened to avoid overclaiming diagnostics impact and to keep verification short.
1 participant
Summary
session.timeline.scroll_controllerrenderer diagnostics so scroll-heavy sessions cannot flood the log.Why
Installed builds could become scroll-laggy once
renderer-diagnostics.jsonlsat near its 20 MiB cap. The trace did not show a renderer scroll/layout long task. Instead, clearing the diagnostics logs dropped INP from about 240ms to 16-40ms, and the log was dominated bysession.timeline.scroll_controllerevents. The fix removes the noisy source and leaves headroom after retention so the app does not immediately re-enter the same rewrite path.Related Issue
Closes #700
Human Review Status
Pending. A human should make the final merge decision after reviewing the final diff and verification evidence.
Review Focus
session.timeline.scroll_controllerbelongs in the same high-frequency retention bucket as scroll and perf samples.Risk Notes
Diagnostics may retain fewer older events when the file exceeds the cap because cleanup now targets 80% of the max. This is intentional; incident and session export behavior remains capped and sanitized. No user data migration, permissions, dependencies, or visible UI changes.
How To Verify
Screenshots or Recordings
Not required. This change is in desktop diagnostics retention and has no visible UI surface.
Checklist
bug,enhancement,task, ordocumentation), at least one primary routing label (app,ui,platform,harness, orci), and exactly one priority label (P0toP3), or I requested maintainer labelingdev, and my PR title and commit messages use Conventional Commits in EnglishSummary by CodeRabbit
Refactoring
New Features
Tests