fix(report): redesign evaluation card, add run duration, fix suite labeling - #227
Merged
jithin23-kv merged 3 commits intoJul 30, 2026
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe change adds suite identifiers and run durations to unified and SDK reports, expands report view models, replaces the shared HTML report layout, and routes SDK and extension report generation through the shared renderer with updated judgment details and fixtures. ChangesReport pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SDK
participant Extension
participant ReportViewModel
participant renderReport
SDK->>ReportViewModel: Adapt SDK results
Extension->>ReportViewModel: Adapt extension report
ReportViewModel->>renderReport: Render shared HTML report
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
core/src/report/render.ts (1)
560-560: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winModule-level
transcriptIdCountermakes renderer output non-deterministic across calls.The counter is never reset, so rendering the same
ReportViewModeltwice (e.g. a long-lived MCP server, or snapshot tests) yields differentdata-for/data-targetvalues, and the value grows unbounded for the process lifetime. The IDs only need to be unique within one document — derive them from the evaluator/result indices instead.♻️ Proposed refactor to derive IDs from indices
-let transcriptIdCounter = 0;function resultDetailCard( r: ResultViewModel, index: number, _mode: "agent" | "mcp", - showTestHeading: boolean + showTestHeading: boolean, + evalIndex: number ): string {- const tId = `t${transcriptIdCounter++}`; + const tId = `t${evalIndex}-${index}`;And at the call site (line 200):
- .map((r, i) => resultDetailCard(r, i, model.mode, showTestHeading)) + .map((r, i) => resultDetailCard(r, i, model.mode, showTestHeading, idx))Also applies to: 667-667
🤖 Prompt for 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. In `@core/src/report/render.ts` at line 560, Remove the module-level transcriptIdCounter and make transcript identifiers deterministic per render by deriving them from the evaluator/result indices. Update the relevant transcript ID generation and its call site in the renderer so repeated rendering of the same ReportViewModel produces identical data-for/data-target values while remaining unique within a document.
🤖 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 `@core/src/report/render.ts`:
- Around line 449-451: Remove the inline onclick handler from the copy button in
the report ID markup and store the escaped report ID in a data attribute such as
data-copy. In the existing script block, add the delegated .copy-btn click
listener to read that attribute, write it via navigator.clipboard, and update
the icon color while preserving the current copy behavior.
In `@runners/extension/popup.js`:
- Around line 1402-1434: Update toReportViewModel to populate the summary
duration from summary.durationMs, using the popup state’s run start/end delta
when that is the source of duration for extension runs. Ensure the resulting
summary includes the same duration value expected by the Executive Summary
instead of leaving it undefined.
- Around line 1157-1164: Update resolveSuiteLabel so the suite lookup safely
handles an absent state.catalog.suites collection, not just an absent catalog.
Extend the optional chaining at the suites access before calling find, while
preserving the existing Custom Suite fallback and fullSuite logic.
---
Nitpick comments:
In `@core/src/report/render.ts`:
- Line 560: Remove the module-level transcriptIdCounter and make transcript
identifiers deterministic per render by deriving them from the evaluator/result
indices. Update the relevant transcript ID generation and its call site in the
renderer so repeated rendering of the same ReportViewModel produces identical
data-for/data-target values while remaining unique within a document.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 07c19fd0-6513-4020-834d-f52ef5b373a5
📒 Files selected for processing (18)
core/src/browser.tscore/src/execute/aggregate.tscore/src/execute/runAll.tscore/src/execute/runAllBrowser.tscore/src/execute/types.tscore/src/report/buildReport.tscore/src/report/render.tscore/src/report/types.tscore/tests/orchestrator.equivalence.test.tsrunners/cli/tests/jsonlEventListener.test.tsrunners/cli/tests/knowledge.test.tsrunners/extension/popup.jsrunners/sdk/src/report.tsrunners/sdk/src/run.tsrunners/sdk/src/types.tsrunners/sdk/tests/opfor-class.test.tsrunners/sdk/tests/report.test.tsrunners/sdk/tests/types.test.ts
Collaborator
Author
|
Addressed all three CodeRabbit findings in 392878b:
Build, lint, and all three test suites (core 184/184, SDK 49/49, CLI 31/31) still green. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
runners/extension/popup.js (2)
1161-1167: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winBase the suite label on completed evaluators.
Line 1164 checks the selected suite, so a user-stopped run can still display the full suite ID despite unrun evaluators. Derive
fullSuitefrom completed, non-cancelledstate.resultsIDs instead.🤖 Prompt for 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. In `@runners/extension/popup.js` around lines 1161 - 1167, Update resolveSuiteLabel so fullSuite is determined from state.results entries that completed successfully and are not cancelled, using their evaluator IDs rather than state.selectedEvaluators. Require those completed IDs to exactly match suite.evaluatorIds, preserving the existing "Custom Suite" fallback for incomplete or cancelled runs.
3106-3112: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRestore the start time for long-running recovered runs.
When the popup reopens after more than five minutes,
popupQueueActiveis false even ifopforRunStatus.runningis true. Line 3111 is then skipped, leavingrunStartedAtnull and omitting duration from the final report. Restore the timestamp from the matching persisted snapshot independently of the freshness check.🤖 Prompt for 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. In `@runners/extension/popup.js` around lines 3106 - 3112, Restore runStartedAt from the matching persisted opforPopupRun snapshot independently of the popupQueueActive check, so recovered runs with opforRunStatus.running retain their original start time even after the freshness window expires. Keep the existing fallback behavior when no persisted timestamp is available.
🤖 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.
Outside diff comments:
In `@runners/extension/popup.js`:
- Around line 1161-1167: Update resolveSuiteLabel so fullSuite is determined
from state.results entries that completed successfully and are not cancelled,
using their evaluator IDs rather than state.selectedEvaluators. Require those
completed IDs to exactly match suite.evaluatorIds, preserving the existing
"Custom Suite" fallback for incomplete or cancelled runs.
- Around line 3106-3112: Restore runStartedAt from the matching persisted
opforPopupRun snapshot independently of the popupQueueActive check, so recovered
runs with opforRunStatus.running retain their original start time even after the
freshness window expires. Keep the existing fallback behavior when no persisted
timestamp is available.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2ab1dfb1-b151-408b-bf5d-5dbeb9ed2aba
📒 Files selected for processing (2)
core/src/report/render.tsrunners/extension/popup.js
Collaborator
Author
|
Addressed the latest round in 191adc3 (these were flagged as "outside diff range" / nitpick, so no inline thread to reply to directly — replying here instead):
Build, lint, and all three suites still green (core 184/184, SDK 49/49, CLI 31/31). |
arunSunnyKVS
approved these changes
Jul 30, 2026
This was referenced Aug 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The Evaluation Results card in the shared HTML report (used by CLI, MCP server, SDK, and the browser extension) didn't match the Figma design: a redundant two-level accordion (click the card to open, then click again to expand the transcript), boxed grey "meta grid" for Verdict/Score/Confidence/Pattern, a Standards chip that didn't make sense once shown, wrong turn styling (attacker/agent not visually distinguished, no highlight for judge-flagged turns), and mismatched header typography (pill badges instead of plain colored text).
Separately, two functional bugs surfaced during review:
Solution
failingTurnsactually flagged (previously this checked a per-turn judge field that is never populated in the real pipeline — the judgement is only ever made once, over the whole transcript).durationMsfield, captured via wall-clock timing inrunAll/runAllBrowser, surfaced as a 5th box in the Executive Summary ("12m 34s").suiteIdfield that's the real suite id only when the full suite was actually run; otherwise "Custom Suite" — computed from the evaluator selection mode in the CLI/SDK/MCP path, and from a selected-vs-suite-membership comparison in the extension.knowledge.test.tsexpected a vuln-class id of"prompt-injection"; the actual category id — matching both the directory name and the README's own frontmatter — is"injection").Changes
core/src/report/render.ts,core/src/report/types.ts,core/src/report/buildReport.ts— report template/view-modelcore/src/execute/runAll.ts,runAllBrowser.ts,aggregate.ts,types.ts— duration + suite-label computationrunners/sdk/src/run.ts,report.ts,types.ts— threadsuiteId/durationMsthroughrunners/extension/popup.js— suite-label fix for the extension's own report buildercore/src/browser.ts— re-exportrenderReport/view-model types for the extension bundlecore/tests/,runners/cli/tests/,runners/sdk/tests/Issue
N/A
How to test
npm run buildfrom repo root, thennpm test,npm test --workspace=runners/cli,npm test --workspace=runners/sdk— all green (core 184/184, CLI 31/31, SDK 49/49).opfor runwith a full suite selected → "Evaluation Suite" shows the suite id. Re-run with--evaluators(a hand-picked subset) → shows "Custom Suite" and a Duration box in the Executive Summary.FailingTurns→ only those turns render with the pink dashed highlight; others render as plain text/bubble.Screenshots
Not attached — verified via synthetic reports rendered locally through headless Chrome during development (see conversation history for before/after captures).
Summary by CodeRabbit