Skip to content

Add the web preview performance panel - #168

Closed
bjdodson-openai wants to merge 1 commit into
bjd/debugger-web-performance-apifrom
bjd/debugger-web-performance
Closed

Add the web preview performance panel#168
bjdodson-openai wants to merge 1 commit into
bjd/debugger-web-performance-apifrom
bjd/debugger-web-performance

Conversation

@bjdodson-openai

Copy link
Copy Markdown
Collaborator

Description

Adds a generation-safe Performance panel over the preceding bounded capture API.

  • Keeps Start, Stop, polling, and completed results bound to one exact target generation.
  • Preserves focus and scroll while rejecting stale or ambiguous capture state.
  • Cleans up ownership on target changes, aborts, and page lifecycle events.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • Documentation improvement
  • Performance optimization
  • Test improvement
  • Other (new debugger capability)

Testing

  • Tests pass locally (bazel test //...)
  • Added/updated tests for changes (if applicable)
  • Tested on multiple platforms (iOS/Android/Web/macOS as applicable)
  • Manual testing performed (describe below)

Testing Details

  • Incremental branch: Panel 25/25, combined focused 114/114, full CLI 347/347, production build, TypeScript, lint, format, syntax, and query passed.
  • Assembled debugger stack: npm test passed 436/436; the CLI production build passed.
  • Focused //src/valdi_modules/src/valdi/web_renderer:test passed.
  • bazel query //... passed.
  • The broad Valdi suite reproduced the established 12 unrelated failures; all new debugger specs passed.

Checklist

  • Code follows project style guidelines
  • Documentation updated (if needed)
  • No breaking changes (or documented in description)
  • Commit messages follow conventional format
  • No secrets, API keys, or internal URLs included

Related Issues

Relates to #154

Additional Context

Stack 15/22. Stacked on #167 (bjd/debugger-web-performance-api). Review this PR as the single incremental commit c1d9db42 against that base; do not merge it before its parent.

@github-actions

Copy link
Copy Markdown

📊 PR Size: size/XL

Total changes: 1707 lines (6 files)

Top files changed:

  • npm_modules/cli/debugger/devtools-panel.js: +679 -4
  • npm_modules/cli/src/debugger/devtoolsPanel.spec.ts: +523 -0
  • npm_modules/cli/debugger/devtools-panel.css: +402 -0
  • npm_modules/cli/debugger/devtools-panel.html: +63 -4
  • docs/docs/performance-tracing.md: +26 -0
  • npm_modules/cli/debugger/README.md: +4 -2

Size calculated as additions + deletions. Labels: XS (<10), S (<50), M (<250), L (<1000), XL (1000+)

@clholgat clholgat left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Review of the web-preview Performance panel. Rendering is safe: every dynamic value (trace names, metric values, labels, search box, duration, paint names, counts) is routed through escapeHtml before innerHTML, and preparePerformanceForTargetChange nulls data/lastTrace/samples on every target change, so no XSS and no cross-generation data bleed. Polling is guarded by pending/snapshotPending, skips when document.hidden, and stops on pagehide. One identity concern inline, plus two low notes below.

Note: please carry this feedback into the squashed PR when the stack is collapsed.

  • 🟢 Low — one-shot Capture sets traceActive/ownerIdentity before issuing the request and does not reset them if the request throws. After a failed Capture the panel is wedged in "Result pending" / Stop-only until the next ~1200 ms poll recomputes owner state from /status — and only while the user stays on the Performance tab. Consider resetting owner state in a catch on the capture path.
  • 🟢 Low — the completionError branch issues its trace/stop call against the currently selected identity, not necessarily perf.ownerIdentity. If the owner belongs to a previous target, the stop clears the wrong session's server state. Local owner state is guarded, but the network call is misdirected; route the stop to perf.ownerIdentity when present.

return `${formatNumber(numeric / 60_000)} min`;
}

function performanceIdentity(target = state.target) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🟠 Med — the frontend "exact identity" guarantee is effectively sessionId-only. inspectedUrl and inspectedTargetNonce are module-level constants read once from the panel URL, so performanceIdentity() rebuilds them identically on every call and performanceIdentityIsCurrent() can never observe an inspectedUrl/targetNonce mismatch — only sessionId can ever differ.

Failure scenario: the panel advertises that Start/Stop/polling/results are bound to the exact inspectedUrl and per-tab targetNonce and "fail closed", but that enforcement lives entirely server-side. If a stale/rebound tab shares a sessionId, the client-side guard would not catch it; safety currently rests solely on #167's server-side nonce validation. Either derive the identity from the actual live target (so the fields can genuinely differ) or drop/soften the client-side guarantee in the docs so it isn't mistaken for real cross-check.

@bjdodson-openai

Copy link
Copy Markdown
Collaborator Author

Superseded by #180, which consolidates this patch into the reviewed debugger capabilities landing unit. The replacement carries forward the feedback and fixes discussed here. Closing this draft to reduce the active stack; this PR and its discussion remain the historical review record.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants