Skip to content

feat: add renderer diagnostics observability#392

Merged
Astro-Han merged 16 commits intodevfrom
codex/i389-renderer-diagnostics
May 3, 2026
Merged

feat: add renderer diagnostics observability#392
Astro-Han merged 16 commits intodevfrom
codex/i389-renderer-diagnostics

Conversation

@Astro-Han
Copy link
Copy Markdown
Owner

@Astro-Han Astro-Han commented May 2, 2026

Summary

Adds a default-on renderer diagnostics pipeline for session rendering issues:

  • Records sanitized renderer diagnostics from the session UI into a bounded Electron main-process JSONL log.
  • Adds diagnostics export through Help -> Export Diagnostics Log..., session export, and problem reports.
  • Emits session submit, view-state, timeline, scroll, composer dock, visibility, perf, and derived incident events.
  • Adds truncation/failure handling so diagnostics never block app behavior, session export, or problem report creation.
  • Adds unit and E2E coverage for the diagnostics flow and send/scroll stability.

Why

Issue #389 needs better observability for recurring session jump-to-top and render flicker bugs. The goal is to stop relying only on Computer Use and visual inspection by giving manually shared problem reports/session exports enough structured renderer evidence to diagnose these failures.

Related Issue

Closes #389

Human Review Status

Pending. A human should make the final merge decision after reviewing the final diff and verification evidence.

Review Focus

Please focus on:

  • Privacy/sanitization boundaries for renderer diagnostics fields.
  • Whether diagnostics export and problem report attachment fail open correctly.
  • Whether the renderer instrumentation is scoped enough and does not perturb session scrolling/rendering.
  • Desktop menu copy/i18n and platform impact on macOS/Windows.

Risk Notes

  • New local diagnostic JSONL is written under Electron user data and retained with size/time caps.
  • The Help menu has one visible native-menu copy change: Export Diagnostics Log... / 导出诊断日志....
  • The saved diagnostics file is user-chosen via native save dialog; problem reports/session exports can include a capped diagnostics slice.
  • No dependencies, migrations, credentials, deletion behavior, or generated source files were introduced.
  • Labels: maintainer labeling requested.

How To Verify

Latest local verification on head 70f8cfc79:

Desktop focused tests: 40 passed, 0 failed
bun --cwd packages/desktop-electron test src/main/renderer-diagnostics.test.ts src/main/feedback.test.ts src/main/ipc-window-config.test.ts

App focused tests: 685 passed, 0 failed
bun --cwd packages/app test src/context/renderer-diagnostics.test.ts src/pages/session/session-view-controller.test.ts

E2E focused tests: 1 passed, 0 failed
bun --cwd packages/app playwright test e2e/session/session-renderer-diagnostics.spec.ts --reporter=line

Desktop typecheck: passed
bun --cwd packages/desktop-electron typecheck

App typecheck: passed
bun --cwd packages/app typecheck

Diff whitespace check: passed
git diff --check

Screenshots or Recordings

Not captured. The visible UI change is a native menu item and native save dialog title; coverage is via menu/i18n tests. Manual visual follow-up is still useful on macOS/Windows native menus.

Checklist

  • Human review status is stated above as pending, approved, or not required
  • I linked the related issue, or stated why there is no issue
  • This PR has type, scope, and priority labels, or I requested maintainer labeling
  • I described the review focus and any meaningful risks
  • I listed the relevant verification steps and the key result for each
  • I did not introduce unrelated refactors, dependencies, generated files, or file changes beyond the stated scope
  • I manually checked visible UI or copy changes when needed, with screenshots or recordings
  • I considered macOS and Windows impact for desktop, packaging, updater, signing, paths, shell, or permissions changes
  • I called out docs, release notes, dependencies, permissions, credentials, deletion behavior, generated content, or local file changes when relevant
  • I reviewed the final diff for unrelated changes and suspicious dependency changes
  • I am targeting dev, and my PR title and commit messages use Conventional Commits in English

Summary by CodeRabbit

  • New Features

    • "Export Diagnostics Log" menu command and renderer APIs to record/export diagnostics.
  • Enhancements

    • Session view emits scroll, timeline, visibility and performance telemetry; derived incident signals reported and sampled.
    • Diagnostics included in feedback/problem reports and session exports with sanitization, retention, and safe truncation.
    • Composer/dock resize and scroll metrics reported; timeline mount/visible/unmount and submit events emitted.
  • Tests

    • Extensive e2e and unit tests for capture, sanitization, retention, slicing, export, and incident detection.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 2, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 6421f0ff-0e10-42b2-93af-de334c0d9e2d

📥 Commits

Reviewing files that changed from the base of the PR and between 45e1c2b and 70f8cfc.

📒 Files selected for processing (3)
  • packages/desktop-electron/src/main/index.ts
  • packages/desktop-electron/src/main/renderer-diagnostics.test.ts
  • packages/desktop-electron/src/main/renderer-diagnostics.ts
✅ Files skipped from review due to trivial changes (1)
  • packages/desktop-electron/src/main/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/desktop-electron/src/main/renderer-diagnostics.ts

📝 Walkthrough

Walkthrough

Adds a renderer diagnostics subsystem: in-renderer emitters, incident detection, timeline/scroll/perf instrumentation, preload+IPC hooks, a main-process sanitizer/recorder/slicer/export, session-export & problem-report integration, menu/export wiring, and tests including an E2E probe.

Changes

Renderer diagnostics end-to-end

Layer / File(s) Summary
Types & API contracts
packages/app/src/context/platform.tsx, packages/app/src/app.tsx, packages/app/src/desktop-api.ts, packages/desktop-electron/src/preload/types.ts
Add RendererDiagnosticInput / RendererDiagnosticsExportResult types and extend Platform / Window.api / ElectronAPI with emitRendererDiagnostic and exportDiagnosticsLog.
Browser emitter & incident detection
packages/app/src/context/renderer-diagnostics.ts, packages/app/src/context/renderer-diagnostics.test.ts
Add emitter that ensures monotonic_ms, global incident detector (scroll-jump, timeline remount, visible-clear), and createSessionPerformanceDiagnostics (FPS/jank/long-task/CLS sampling); unit tests.
App instrumentation
packages/app/src/components/prompt-input/submit.ts, packages/app/src/pages/session.tsx, packages/app/src/pages/session/message-timeline.tsx
Emit diagnostics for submits, view/identity transitions, timeline mount/unmount/visible, and session.scroll.sample; compute message/part counts and wire perf diagnostics.
Scroll-dock wiring
packages/app/src/pages/session/use-session-scroll-dock.ts, packages/app/src/pages/session/use-session-timeline-interaction.ts, packages/app/src/pages/session/use-session-scroll-dock.test.ts
Add optional onDockHeightChange callback, compute previous/current dock height and scroll metrics, call callback when height changes, and forward events to diagnostics emitter; tests added.
Session refresh diagnostics
packages/app/src/pages/session/use-session-refresh-effects.ts
Wrap session/todo sync calls to emit session.data.refresh diagnostics with phases, durations, and cache flags; accepts optional emitRendererDiagnostic.
E2E capture probe
packages/app/e2e/session/session-renderer-diagnostics.spec.ts
Playwright E2E test injects capture script, seeds session turns, scrolls timeline, submits prompt, asserts scroll-anchor stability, bounded viewport metrics, and presence/counts of specific diagnostics.
Preload & IPC wiring
packages/desktop-electron/src/preload/index.ts, packages/desktop-electron/src/main/ipc.ts, packages/desktop-electron/src/main/ipc-window-config.test.ts
Expose emitRendererDiagnostic and exportDiagnosticsLog via preload; add IPC handlers for recording, exporting, and slicing diagnostics, forwarding window context.
Main-process recorder & utilities
packages/desktop-electron/src/main/renderer-diagnostics.ts, packages/desktop-electron/src/main/renderer-diagnostics.test.ts
New recorder: sanitizer/allowlist, per-event size limits, URL/token scrubbing, protected-event preservation, rate-limits for high-frequency events, JSONL persistence, retention trimming (age + bytes), serialized writes, slicing with byte-cap/truncation, export generation; tests added.
Export & session integration
packages/desktop-electron/src/main/server-client.ts, packages/desktop-electron/src/main/server-client.test.ts, packages/desktop-electron/src/main/index.ts
Attach diagnostics slice into session export JSON when export body is an object; initialize recorder at app launch; add menu export flow and IPC/export handlers.
Problem report & feedback integration
packages/desktop-electron/src/main/problem-report.ts, packages/desktop-electron/src/main/problem-report.test.ts, packages/desktop-electron/src/main/feedback.ts, packages/desktop-electron/src/main/feedback.test.ts
Include renderer diagnostics slice in problem report payload/summary; implement truncation of non-protected events to meet max-bytes, track omitted bytes, and fallback on diagnostics fetch/write failures.
Menu labels & templates
packages/desktop-electron/src/main/menu-labels.ts, packages/desktop-electron/src/main/menu-template.ts, packages/desktop-electron/src/main/menu-template.test.ts, packages/desktop-electron/src/main/menu-labels.test.ts
Add localized Export Diagnostics Log... labels, extend MenuTemplateDeps with exportDiagnosticsLog, and wire Help menu items to invoke the export handler.

Sequence Diagram(s)

sequenceDiagram
    participant Renderer as Browser Renderer
    participant Emitter as Renderer Emitter
    participant Preload as Preload API (window.api)
    participant IPC as IPC Main
    participant Recorder as Diagnostics Recorder
    participant File as Local JSONL Storage
    participant Report as Problem Report Builder

    Renderer->>Emitter: emit(session.timeline.mount / scroll.sample / perf.sample / action.submit)
    Emitter->>Emitter: normalize(monotonic_ms), detect incidents
    Emitter->>Preload: window.api.emitRendererDiagnostic(event)
    Preload->>IPC: invoke "renderer-diagnostics:record" (includes windowID)
    IPC->>Recorder: record(sanitized event)
    Recorder->>File: append JSONL line (serialize + retention flush)
    Recorder-->>Recorder: rate-limit / cap by bytes / retention

    Note over Report,IPC: During session export or problem report
    Report->>IPC: request renderer diagnostics slice (sessionID, windowID, maxBytes)
    IPC->>Recorder: slice(...)
    Recorder->>Report: RendererDiagnosticsSlice (ok/truncated/missing/expired)
    Report->>File: include slice in payload and save report
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Suggested labels

enhancement, P2, app, desktop

Poem

🐇 I logged each hop and tiny blink,

Anchors kept while timelines sink,
Frames and jumps reduced to trace,
So devs can follow every pace,
A rabbit's note to guard the link.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'feat: add renderer diagnostics observability' clearly and concisely summarizes the main change—adding a renderer diagnostics pipeline for session rendering observability. It is specific and follows Conventional Commits format.
Description check ✅ Passed The PR description is comprehensive and covers all required sections from the template: Summary, Why, Related Issue, Human Review Status, Review Focus, Risk Notes, How To Verify, and Checklist. All critical sections are complete.
Linked Issues check ✅ Passed The PR comprehensively addresses issue #389 by implementing the core objectives: capturing local renderer diagnostics (via JSONL logging with retention), emitting session/scroll/timeline/perf events, sanitizing sensitive data, integrating with session export and problem reports, and adding thorough test coverage including E2E regression probes.
Out of Scope Changes check ✅ Passed All changes are directly aligned with the stated objectives. The implementation adds the diagnostics pipeline (emitter, recorder, sanitizer, export), UI instrumentation, menu integration, and comprehensive tests. No unrelated refactors, dependencies, or generated files were introduced.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/i389-renderer-diagnostics

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get your free trial and get 200 agent minutes per Slack user (a $50 value).


Review rate limit: 7/10 reviews remaining, refill in 12 minutes and 21 seconds.

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a comprehensive renderer diagnostics system for the desktop application, enabling the capture and recording of performance metrics, scroll behavior, and session state transitions. The implementation includes a main-process recorder that persists events to a JSONL log with retention and byte-capping logic, alongside a renderer-side emitter that integrates with SolidJS components. Feedback focuses on critical performance issues in the log pruning logic, specifically the $O(N^2)$ complexity of the retention flush and the excessive disk I/O caused by pruning on every event. Additionally, the data sanitizer regex is noted to be overly aggressive, potentially stripping valid technical identifiers like domain-based provider names.

Comment thread packages/desktop-electron/src/main/renderer-diagnostics.ts Outdated
Comment thread packages/desktop-electron/src/main/renderer-diagnostics.ts Outdated
Comment thread packages/desktop-electron/src/main/renderer-diagnostics.ts Outdated
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
packages/desktop-electron/src/main/problem-report.ts (1)

491-499: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Validate the new truncation field in isTruncation().

parseProblemReportPayload() can now return a Payload whose truncation.omittedRendererDiagnosticsBytes is missing or non-numeric, because the guard never checks it.

Suggested fix
 function isTruncation(value: unknown): value is Payload["truncation"] {
   if (!isRecord(value)) return false
   return (
     isFiniteNumber(value.omittedMessages) &&
     isFiniteNumber(value.omittedLogBytes) &&
     isFiniteNumber(value.omittedSessionInfoBytes) &&
     isFiniteNumber(value.omittedFailedExportErrorBytes) &&
+    isFiniteNumber(value.omittedRendererDiagnosticsBytes) &&
     isFiniteNumber(value.omittedDiagnosticsBytes)
   )
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/desktop-electron/src/main/problem-report.ts` around lines 491 - 499,
The type guard isTruncation currently omits checking
omittedRendererDiagnosticsBytes, so update isTruncation (the function checking
Payload["truncation"]) to also require
isFiniteNumber(value.omittedRendererDiagnosticsBytes); ensure the guard returns
false if that property is missing or non-numeric by adding it alongside the
other isFiniteNumber checks so parseProblemReportPayload can't produce a Payload
with an invalid truncation field.
🧹 Nitpick comments (2)
packages/desktop-electron/src/main/ipc-window-config.test.ts (1)

20-26: ⚡ Quick win

Avoid locking this test to src/main/ipc.ts as the wiring anchor.

This test currently enshrines string-based channel registration inside ipc.ts, which makes future migration to module-level register*Ipc() bootstrap wiring harder. Prefer asserting the bootstrap integration point instead of file-content substrings.

Based on learnings: In Astro-Han/pawwork (packages/desktop-electron/src/main/), the bootstrap entry (packages/desktop-electron/src/main/index.ts) should directly call each module’s exported register*Ipc() function and should not centralize sub-module IPC registrations through src/main/ipc.ts.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/desktop-electron/src/main/ipc-window-config.test.ts` around lines 20
- 26, The test is brittle because it asserts string substrings from
src/main/ipc.ts instead of verifying the bootstrap wiring; update
packages/desktop-electron/src/main/ipc-window-config.test.ts to assert that the
bootstrap entry (src/main/index.ts) invokes the module-level registration
function(s) (e.g., registerRendererDiagnosticsIpc or the generic register*Ipc
exports) rather than checking for specific channel strings; modify the test to
import the bootstrap (index.ts) and spy/mock the exported register*Ipc
function(s) from the diagnostics module (or assert that index.ts imports and
calls registerRendererDiagnosticsIpc) so the test validates integration at the
bootstrap level and not file content.
packages/app/src/app.tsx (1)

50-51: ⚡ Quick win

Align Window.api typing with diagnostics export surface.

Line 91 adds emitRendererDiagnostic, but the paired exportDiagnosticsLog method (already exposed on preload API) is missing from this interface. Adding it here keeps app-side typing consistent and avoids drift.

♻️ Suggested typing alignment
-import type { RendererDiagnosticInput } from "@/context/platform"
+import type { RendererDiagnosticInput, RendererDiagnosticsExportResult } from "@/context/platform"
...
     api?: {
       setDesktopContext?: (context: DesktopContext) => Promise<void>
       emitRendererDiagnostic?: (event: RendererDiagnosticInput) => Promise<void>
+      exportDiagnosticsLog?: () => Promise<RendererDiagnosticsExportResult>
       getAboutInfo?: () => Promise<AboutInfo>

Also applies to: 89-99

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/app/src/app.tsx` around lines 50 - 51, The Window.api type is
missing the exportDiagnosticsLog method causing app-side typing drift; update
the Window.api interface (the same area that currently declares
emitRendererDiagnostic) to also include exportDiagnosticsLog with the same
signature as the preload API—use the existing RendererDiagnosticInput type if
appropriate and ensure the method name and parameter/return types match the
exported preload surface so both emitRendererDiagnostic and exportDiagnosticsLog
are declared consistently.
🤖 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/app/e2e/session/session-renderer-diagnostics.spec.ts`:
- Around line 39-44: Replace the outright replacement of
win.api.emitRendererDiagnostic with a wrapper that delegates to the existing
implementation: capture the original function (e.g., const originalEmit =
win.api?.emitRendererDiagnostic), then assign a new async emitRendererDiagnostic
that calls await originalEmit?.call(win.api, event) (if present) and still
pushes a deep-cloned event into win.__pawwork_renderer_diagnostics; keep other
win.api properties untouched and preserve async/return behavior so a broken
preload/IPC handler is still exercised by the test while recording the event.

In `@packages/app/src/components/prompt-input/submit.ts`:
- Around line 523-540: The diagnostics are reading mutable state after calling
removeCommentItems() and clearInput(), and the fire-and-forget
emitRendererDiagnostic(...) silently drops rejections; capture metrics into
local variables (e.g., const commentCount = input.commentCount(), promptLen =
input.promptLength(currentPrompt), imageCount = images.length) before calling
removeCommentItems() / clearInput(), then call emitRendererDiagnostic with those
captured values and handle async failures (await the promise and wrap in
try/catch or attach a .catch to log/report errors) so failures aren’t ignored;
update references to messageID, session.id, model.providerID/model.modelID as
before when building the payload.

In `@packages/app/src/context/renderer-diagnostics.ts`:
- Around line 169-185: The emitted fps should be the real frames-per-second over
the sample period, not raw frameCount; in flush() replace fps: frameCount with a
computed value framesPerSecond = Math.round(frameCount / (sampleDurationMs /
1000)), where sampleDurationMs is the elapsed milliseconds since the previous
flush (track a lastFlushTimestamp set when diagnostics start and update it at
the end of flush(), initialize it on start); use framesPerSecond in the event
payload (and keep rounding/undefined handling consistent with other metrics).

In `@packages/app/src/pages/session/message-timeline.tsx`:
- Around line 739-755: The scroll diagnostic currently calls
emitRendererDiagnostic on every scroll event (hot path), causing excessive IPC
and jank; change it to throttle via requestAnimationFrame (or an RAF-backed
boolean guard) so emitRendererDiagnostic(...) runs at most once per animation
frame instead of per event, and clamp the computed distance_from_bottom (max -
el.scrollTop) to a non-negative value before including it in data; update the
same call site that uses emitRendererDiagnostic, visibleRangeData(),
props.hasScrollGesture(), props.scroll, and staging.isStaging() to use the
RAF-throttled emitter and the clamped distance metric.

In `@packages/app/src/pages/session/use-session-scroll-dock.ts`:
- Around line 172-179: The diagnostics hook input.onDockHeightChange is
currently invoked directly and can throw, breaking resize/scroll recovery; wrap
the invocation of input.onDockHeightChange (inside the dockHeight !==
previousDockHeight branch) in a try/catch so any exception is caught and does
not propagate, and log the error (e.g., via console.error or existing logger)
while keeping the conditional call (input.onDockHeightChange?.(...)) behavior
intact so diagnostics failures are fail-open.

In `@packages/desktop-electron/src/main/feedback.ts`:
- Around line 221-226: Wrap the await deps.rendererDiagnostics(context) call in
a timeout guard (e.g., Promise.race) so it cannot hang indefinitely; if the
timeout elapses, treat it as a handled error by calling
deps.onHandledError?.("renderer diagnostics slice timed out", timeoutError) and
set rendererDiagnostics = emptyRendererDiagnosticsSlice("write_failed", new
Date(generatedAt)) — mirror the fail-open behavior used for session export; use
a short configurable timeout constant (e.g., TIMEOUT_MS) and reference
deps.rendererDiagnostics, deps.onHandledError, emptyRendererDiagnosticsSlice,
and generatedAt when implementing.

In `@packages/desktop-electron/src/main/renderer-diagnostics.ts`:
- Around line 446-468: The record() function still writes events to disk even
when options.disabled is true; add an early check at the top of record (before
sanitizing/enqueueing) to honor options.disabled and immediately return { ok:
false, reason: "disabled" } (matching slice() behavior) so no further processing
(sanitizeRendererDiagnosticEvent, enqueueWrite, appendFile, flushRetention)
occurs when disabled.

---

Outside diff comments:
In `@packages/desktop-electron/src/main/problem-report.ts`:
- Around line 491-499: The type guard isTruncation currently omits checking
omittedRendererDiagnosticsBytes, so update isTruncation (the function checking
Payload["truncation"]) to also require
isFiniteNumber(value.omittedRendererDiagnosticsBytes); ensure the guard returns
false if that property is missing or non-numeric by adding it alongside the
other isFiniteNumber checks so parseProblemReportPayload can't produce a Payload
with an invalid truncation field.

---

Nitpick comments:
In `@packages/app/src/app.tsx`:
- Around line 50-51: The Window.api type is missing the exportDiagnosticsLog
method causing app-side typing drift; update the Window.api interface (the same
area that currently declares emitRendererDiagnostic) to also include
exportDiagnosticsLog with the same signature as the preload API—use the existing
RendererDiagnosticInput type if appropriate and ensure the method name and
parameter/return types match the exported preload surface so both
emitRendererDiagnostic and exportDiagnosticsLog are declared consistently.

In `@packages/desktop-electron/src/main/ipc-window-config.test.ts`:
- Around line 20-26: The test is brittle because it asserts string substrings
from src/main/ipc.ts instead of verifying the bootstrap wiring; update
packages/desktop-electron/src/main/ipc-window-config.test.ts to assert that the
bootstrap entry (src/main/index.ts) invokes the module-level registration
function(s) (e.g., registerRendererDiagnosticsIpc or the generic register*Ipc
exports) rather than checking for specific channel strings; modify the test to
import the bootstrap (index.ts) and spy/mock the exported register*Ipc
function(s) from the diagnostics module (or assert that index.ts imports and
calls registerRendererDiagnosticsIpc) so the test validates integration at the
bootstrap level and not file content.
🪄 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: fc7e3280-b9b7-4318-ad00-ed3ff5d3994d

📥 Commits

Reviewing files that changed from the base of the PR and between a689708 and dc1a25b.

📒 Files selected for processing (29)
  • packages/app/e2e/session/session-renderer-diagnostics.spec.ts
  • packages/app/src/app.tsx
  • packages/app/src/components/prompt-input/submit.ts
  • packages/app/src/context/platform.tsx
  • packages/app/src/context/renderer-diagnostics.test.ts
  • packages/app/src/context/renderer-diagnostics.ts
  • packages/app/src/desktop-api.ts
  • packages/app/src/pages/session.tsx
  • packages/app/src/pages/session/message-timeline.tsx
  • packages/app/src/pages/session/use-session-scroll-dock.test.ts
  • packages/app/src/pages/session/use-session-scroll-dock.ts
  • packages/app/src/pages/session/use-session-timeline-interaction.ts
  • packages/desktop-electron/src/main/feedback.test.ts
  • packages/desktop-electron/src/main/feedback.ts
  • packages/desktop-electron/src/main/index.ts
  • packages/desktop-electron/src/main/ipc-window-config.test.ts
  • packages/desktop-electron/src/main/ipc.ts
  • packages/desktop-electron/src/main/menu-labels.test.ts
  • packages/desktop-electron/src/main/menu-labels.ts
  • packages/desktop-electron/src/main/menu-template.test.ts
  • packages/desktop-electron/src/main/menu-template.ts
  • packages/desktop-electron/src/main/problem-report.test.ts
  • packages/desktop-electron/src/main/problem-report.ts
  • packages/desktop-electron/src/main/renderer-diagnostics.test.ts
  • packages/desktop-electron/src/main/renderer-diagnostics.ts
  • packages/desktop-electron/src/main/server-client.test.ts
  • packages/desktop-electron/src/main/server-client.ts
  • packages/desktop-electron/src/preload/index.ts
  • packages/desktop-electron/src/preload/types.ts

Comment thread packages/app/e2e/session/session-renderer-diagnostics.spec.ts
Comment thread packages/app/src/components/prompt-input/submit.ts Outdated
Comment thread packages/app/src/context/renderer-diagnostics.ts
Comment thread packages/app/src/pages/session/message-timeline.tsx Outdated
Comment thread packages/app/src/pages/session/use-session-scroll-dock.ts
Comment thread packages/desktop-electron/src/main/feedback.ts
Comment thread packages/desktop-electron/src/main/renderer-diagnostics.ts
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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/desktop-electron/src/main/renderer-diagnostics.ts`:
- Around line 266-279: parseEventLine currently returns any object-shaped data
from disk; update parseEventLine (the function returning
RendererDiagnosticEvent) to re-sanitize fields before returning: validate
event.name and level against the known allowlists (e.g., permitted event name
strings and allowed level values) and reject if not in those sets, and rebuild
the returned data object by copying only the allowed keys from the original
value.data (using your established allowlist for data fields) so no unexpected
properties are passed through to exportRendererDiagnosticsLog() or slice(); keep
all other existing shape checks and return undefined on any mismatch.
- Around line 510-517: The high-frequency throttling uses Date.now(), causing
divergence from the recorder's injected clock; change the timestamp source in
the block that computes `current` (where `key`, `current`, `previous`,
`lastHighFrequency`, and `highFrequencyIntervalMs` are used) to use the
recorder's injected time function (e.g., `options.now()` or the same `now` used
elsewhere) instead of `Date.now()` so rate limiting and retention follow the
same clock as the rest of the recorder.
🪄 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: a37a79ff-b098-402b-92c8-1d8837496a2b

📥 Commits

Reviewing files that changed from the base of the PR and between a7f1aec and 1373c4b.

📒 Files selected for processing (11)
  • packages/app/e2e/session/session-renderer-diagnostics.spec.ts
  • packages/app/src/app.tsx
  • packages/app/src/components/prompt-input/submit.ts
  • packages/app/src/context/renderer-diagnostics.ts
  • packages/app/src/pages/session/message-timeline.tsx
  • packages/app/src/pages/session/use-session-scroll-dock.ts
  • packages/desktop-electron/src/main/feedback.test.ts
  • packages/desktop-electron/src/main/feedback.ts
  • packages/desktop-electron/src/main/problem-report.ts
  • packages/desktop-electron/src/main/renderer-diagnostics.test.ts
  • packages/desktop-electron/src/main/renderer-diagnostics.ts
✅ Files skipped from review due to trivial changes (3)
  • packages/app/src/app.tsx
  • packages/desktop-electron/src/main/renderer-diagnostics.test.ts
  • packages/app/src/components/prompt-input/submit.ts
🚧 Files skipped from review as they are similar to previous changes (6)
  • packages/app/src/pages/session/use-session-scroll-dock.ts
  • packages/desktop-electron/src/main/feedback.ts
  • packages/app/e2e/session/session-renderer-diagnostics.spec.ts
  • packages/desktop-electron/src/main/feedback.test.ts
  • packages/desktop-electron/src/main/problem-report.ts
  • packages/app/src/context/renderer-diagnostics.ts

Comment thread packages/desktop-electron/src/main/renderer-diagnostics.ts
Comment thread packages/desktop-electron/src/main/renderer-diagnostics.ts
Copy link
Copy Markdown
Owner Author

@Astro-Han Astro-Han left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR #392 Code Review: Renderer Diagnostics Observability

Overall this is a high-quality PR with clear architecture, comprehensive tests, and solid fail-open design. The main concerns are around type safety boundaries, performance details, and code organization.

P0 — Blocking (must fix)

  1. eventMatchesSession accesses data.*_session_id fields not in the allowlist for non-transition events — silent logic failure.
  2. PerformanceObserver uses deprecated entryTypes parameter — will trigger warnings in newer Chromium.
  3. flushRetention has a race condition when called concurrently (not serialized through enqueueWrite).

P1 — Serious (strongly recommended)

  1. stringField URL regex is overly broad and drops legitimate endpoint_kind values.
  2. capEvents has O(n²) JSON stringify overhead during truncation loops.
  3. session.timeline.visible effect dependency tracking creates large strings on every render.
  4. requestAnimationFrame in onScroll may fire after unmount.
  5. rendererDiagnosticsSlice IPC handler declares unused directory parameter.
  6. Truncation loop can delete protected events if all remaining are protected.

P2 — Moderate (recommended)

  1. highFrequencyIntervalMs default 250ms may miss fast scroll jumps.
  2. FPS calculation is inaccurate when tab is backgrounded.
  3. emitRendererDiagnostic silently swallows all errors even in dev.
  4. Menu placement of Export Diagnostics is inconsistent with conditional Report Problem.
  5. SliceInput.now is required in type but omitted in public API.
  6. route_ready and visible_ready always have identical values.

P3 — Minor / Style

16–24. Code organization (566-line file), test splitting, indentation consistency, threshold tuning, unit assumptions, and type safety nits.

See inline comments for specific locations and suggestions.

Comment thread packages/desktop-electron/src/main/renderer-diagnostics.ts
Comment thread packages/app/src/context/renderer-diagnostics.ts
Comment thread packages/desktop-electron/src/main/renderer-diagnostics.ts
Comment thread packages/desktop-electron/src/main/renderer-diagnostics.ts
Comment thread packages/desktop-electron/src/main/renderer-diagnostics.ts
Comment thread packages/app/src/pages/session/message-timeline.tsx
Comment thread packages/app/src/pages/session/message-timeline.tsx
Comment thread packages/desktop-electron/src/main/ipc.ts
Comment thread packages/desktop-electron/src/main/problem-report.ts
Comment thread packages/desktop-electron/src/main/renderer-diagnostics.ts
Comment thread packages/app/src/context/renderer-diagnostics.ts
Comment thread packages/app/src/components/prompt-input/submit.ts
Comment thread packages/desktop-electron/src/main/menu-template.ts
Comment thread packages/desktop-electron/src/main/renderer-diagnostics.ts
Comment thread packages/app/src/pages/session.tsx
Comment thread packages/desktop-electron/src/main/renderer-diagnostics.ts
Comment thread packages/desktop-electron/src/main/renderer-diagnostics.ts
Comment thread packages/app/src/context/renderer-diagnostics.ts
Comment thread packages/app/src/context/renderer-diagnostics.ts
Comment thread packages/desktop-electron/src/main/server-client.ts
Comment thread packages/desktop-electron/src/main/index.ts
Comment thread packages/desktop-electron/src/main/problem-report.test.ts
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 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/app/src/pages/session.tsx`:
- Around line 137-176: The emitRendererDiagnostic call inside the
createEffect/on callbacks can reject and cause unhandled promise rejections;
make these diagnostics "fail open" by ensuring the returned promise is
swallowed—attach a .catch(() => {}) (or equivalent try/catch around an awaited
call) to the emitRendererDiagnostic invocation in both places so any rejection
is suppressed; locate the calls to emitRendererDiagnostic inside the
createEffect/on callback(s) (symbols: createEffect, on, emitRendererDiagnostic,
timelineMessageMetrics) and add the catch handler to silently ignore errors.

In `@packages/app/src/pages/session/use-session-refresh-effects.ts`:
- Around line 26-63: syncSessionWithDiagnostics currently re-reads
input.timelineSessionID() inside the .then/.catch, causing end/failed events to
use a different session id if navigation occurs; capture the start-time IDs up
front (e.g., const startedVisibleId = input.timelineSessionID() and any other
session id used at start) and use those variables for
visible_session_id/timeline_session_id in the emitted *_end and *_failed events
instead of calling input.timelineSessionID() again; apply the same change to
syncTodoWithDiagnostics (and any similar helpers) so start and completion/failed
diagnostics consistently reference the same session ids.
- Around line 22-24: The emitRefresh helper currently calls
input.emitRendererDiagnostic?.(event) without handling rejected promises so
failures produce unhandled rejections; update emitRefresh to call
input.emitRendererDiagnostic?.(event) and explicitly handle the returned Promise
(e.g., chain .catch(...) or await inside a try/catch) to swallow or log errors
from the IPC/logger path; reference emitRefresh, input.emitRendererDiagnostic
and RendererDiagnosticInput when locating the call and ensure any caught error
is handled (logged via existing logger) or ignored so refreshes fail open.

In `@packages/desktop-electron/src/main/ipc.ts`:
- Around line 361-370: The call to deps.rendererDiagnosticsSlice(...) in the
export-session path can hang; wrap that await in the same timeout guard used by
feedback code so the slice fetch is bounded (e.g., Promise.race with a timeout
promise or reuse the existing timeout helper), and on timeout or rejection fall
back to rendererDiagnostics = emptyRendererDiagnosticsSlice("write_failed", new
Date()); adjust the surrounding try/catch so timeouts are handled the same way
feedback.ts does and ensure you reference the same
SESSION_EXPORT_RENDERER_DIAGNOSTICS_MAX_BYTES and windowID parameters when
invoking rendererDiagnosticsSlice.
🪄 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: 216f7e1a-1198-4780-a754-08fd3af67e36

📥 Commits

Reviewing files that changed from the base of the PR and between 1373c4b and 39b4ec6.

📒 Files selected for processing (12)
  • packages/app/src/context/renderer-diagnostics.test.ts
  • packages/app/src/context/renderer-diagnostics.ts
  • packages/app/src/pages/session.tsx
  • packages/app/src/pages/session/message-timeline.tsx
  • packages/app/src/pages/session/use-session-refresh-effects.ts
  • packages/desktop-electron/src/main/feedback.test.ts
  • packages/desktop-electron/src/main/feedback.ts
  • packages/desktop-electron/src/main/index.ts
  • packages/desktop-electron/src/main/ipc.ts
  • packages/desktop-electron/src/main/renderer-diagnostics.test.ts
  • packages/desktop-electron/src/main/renderer-diagnostics.ts
  • packages/desktop-electron/src/renderer/index.tsx
✅ Files skipped from review due to trivial changes (4)
  • packages/desktop-electron/src/main/feedback.test.ts
  • packages/app/src/pages/session/message-timeline.tsx
  • packages/desktop-electron/src/main/renderer-diagnostics.test.ts
  • packages/desktop-electron/src/main/renderer-diagnostics.ts
🚧 Files skipped from review as they are similar to previous changes (3)
  • packages/app/src/context/renderer-diagnostics.test.ts
  • packages/desktop-electron/src/main/index.ts
  • packages/app/src/context/renderer-diagnostics.ts

Comment thread packages/app/src/pages/session.tsx
Comment thread packages/app/src/pages/session/use-session-refresh-effects.ts
Comment thread packages/app/src/pages/session/use-session-refresh-effects.ts
Comment thread packages/desktop-electron/src/main/ipc.ts
@Astro-Han Astro-Han force-pushed the codex/i389-renderer-diagnostics branch from 09b17ac to ec54e50 Compare May 3, 2026 00:47
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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/desktop-electron/src/main/index.ts`:
- Around line 585-587: The adapter currently ignores the IPC-provided window
context when calling reportProblem; update the exported adapter entry (the arrow
for reportProblem) to accept and forward the second argument so the call becomes
something like reportProblem(input, context) instead of only
reportProblem(input); ensure the symbol referenced in the diff (reportProblem)
and the IPC caller in packages/desktop-electron/src/main/ipc.ts
(deps.reportProblem(input, { windowID: win?.id })) are aligned and update any
TypeScript types/signature for reportProblem to accept the optional context
parameter so windowID is preserved through to the actual report handler.

In `@packages/desktop-electron/src/main/renderer-diagnostics.ts`:
- Around line 566-575: The code sets a module-scoped flag writeFailed in
record() on any write error and then causes slice() to always return
emptyRendererDiagnosticsSlice("write_failed", ...) even after the underlying log
becomes readable again; change this so writeFailed is only a transient failure
indicator: clear writeFailed when a subsequent readEventReport() or a successful
record() completes (e.g., set writeFailed = false after readEventReport()
succeeds or after successful writes), and ensure slice() only returns the
"write_failed" empty slice when the most recent I/O attempt actually failed
rather than relying on a permanent flag; update references in record(), slice(),
and the readEventReport() call to implement this behavior.
🪄 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: 26367ebf-bd7f-44a2-a605-158e39651e01

📥 Commits

Reviewing files that changed from the base of the PR and between 39b4ec6 and a392526.

📒 Files selected for processing (10)
  • packages/app/src/context/renderer-diagnostics.ts
  • packages/app/src/pages/session.tsx
  • packages/app/src/pages/session/message-timeline.tsx
  • packages/app/src/pages/session/use-session-refresh-effects.ts
  • packages/app/src/pages/session/use-session-timeline-data.ts
  • packages/desktop-electron/src/main/index.ts
  • packages/desktop-electron/src/main/ipc.ts
  • packages/desktop-electron/src/main/problem-report.test.ts
  • packages/desktop-electron/src/main/problem-report.ts
  • packages/desktop-electron/src/main/renderer-diagnostics.ts
✅ Files skipped from review due to trivial changes (1)
  • packages/desktop-electron/src/main/problem-report.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
  • packages/app/src/pages/session/message-timeline.tsx
  • packages/desktop-electron/src/main/problem-report.ts
  • packages/app/src/context/renderer-diagnostics.ts
  • packages/app/src/pages/session.tsx
  • packages/app/src/pages/session/use-session-refresh-effects.ts

Comment thread packages/desktop-electron/src/main/index.ts Outdated
Comment thread packages/desktop-electron/src/main/renderer-diagnostics.ts
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
packages/desktop-electron/src/main/renderer-diagnostics.test.ts (1)

437-439: ⚡ Quick win

Use allowlisted incident field names in this fixture.

For incident.session_timeline_remount, Line 438 currently uses mounts/unmounts, so the sanitizer can drop them silently and the test still passes without validating incident payload retention.

💡 Suggested diff
     const second = sanitizeRendererDiagnosticEvent(
       {
         name: "incident.session_timeline_remount",
-        data: { mounts: 2, unmounts: 1 },
+        data: { timeline_mount_count: 2, timeline_unmount_count: 1 },
       },
       { appLaunchID: "launch_1", windowID: 1, now: () => new Date("2026-05-02T10:01:00.000Z") },
     )
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/desktop-electron/src/main/renderer-diagnostics.test.ts` around lines
437 - 439, The fixture for the incident named
"incident.session_timeline_remount" uses non-allowlisted fields "mounts" and
"unmounts" which the sanitizer will drop; update the object in
renderer-diagnostics.test.ts (the fixture with name
"incident.session_timeline_remount") to use the sanitizer's allowlisted incident
field names instead—replace "mounts"/"unmounts" with the exact allowed keys from
the incident field allowlist used by the sanitizer (consult the sanitizer's
allowlist and use those key names) so the test validates payload retention
correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@packages/desktop-electron/src/main/renderer-diagnostics.test.ts`:
- Around line 437-439: The fixture for the incident named
"incident.session_timeline_remount" uses non-allowlisted fields "mounts" and
"unmounts" which the sanitizer will drop; update the object in
renderer-diagnostics.test.ts (the fixture with name
"incident.session_timeline_remount") to use the sanitizer's allowlisted incident
field names instead—replace "mounts"/"unmounts" with the exact allowed keys from
the incident field allowlist used by the sanitizer (consult the sanitizer's
allowlist and use those key names) so the test validates payload retention
correctly.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: e8b9501d-23c9-44e7-b843-c37e9b65eb7f

📥 Commits

Reviewing files that changed from the base of the PR and between a392526 and 45e1c2b.

📒 Files selected for processing (4)
  • packages/app/src/pages/session.tsx
  • packages/desktop-electron/src/main/index.ts
  • packages/desktop-electron/src/main/renderer-diagnostics.test.ts
  • packages/desktop-electron/src/main/renderer-diagnostics.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • packages/desktop-electron/src/main/index.ts
  • packages/desktop-electron/src/main/renderer-diagnostics.ts

@Astro-Han Astro-Han merged commit 9adb5c7 into dev May 3, 2026
28 checks passed
@Astro-Han Astro-Han deleted the codex/i389-renderer-diagnostics branch May 3, 2026 02:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Task] Add renderer diagnostics for session jump and flicker observability

1 participant