performance of test suites#3137
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 1cc21f9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
This public PR was merged directly in the public repo. The matching monorepo PR was left open for manual follow-up because Matching internal PR: #99 |
TL;DRReplaces heavy Key changesNew
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Good change — replacing Monaco Editor with Streamdown for read-only JSON views is a meaningful performance win, especially in dataset tables where many instances can be rendered simultaneously. The new ReadOnlyJsonView component is clean and well-scoped. No blocking issues.
Claude Opus ｜ 𝕏
| className, | ||
| maxHeight = '200px', | ||
| }) => { | ||
| const markdown = `\`\`\`json\n${value}\n\`\`\``; |
There was a problem hiding this comment.
Nit: if value is ever "undefined" (from JSON.stringify(undefined)), this renders a code block containing the literal text undefined. The callers today guard against this, but since this is a reusable component you may want to handle the edge case — e.g. early-return with an empty state or default to "null".
| )} | ||
| style={{ maxHeight }} | ||
| > | ||
| <Streamdown>{markdown}</Streamdown> |
There was a problem hiding this comment.
Streamdown is designed for streaming markdown rendering. For a static, pre-computed string like this, it adds overhead (streaming parser, incremental DOM updates) that a simpler syntax-highlighted <pre> block wouldn't. Consider whether a lighter alternative like shiki (already common in the ecosystem) or even a plain <code> block with a JSON highlight pass would be more appropriate for a purely static read-only view. That said, Streamdown is already used widely in this codebase, so the consistency argument is reasonable — just flagging the tradeoff.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
This is a clean, focused performance optimization that replaces heavy Monaco-based ExpandableJsonEditor components with a lightweight ReadOnlyJsonView component using Streamdown for read-only JSON display contexts. The refactoring is correct and follows existing codebase patterns.
🔴❗ Critical (0) ❗🔴
None.
🟠⚠️ Major (0) 🟠⚠️
None.
🟡 Minor (0) 🟡
None.
💭 Consider (1) 💭
💭 1) read-only-json-view.tsx:20-27 Keyboard accessibility for scrollable region
Issue: The scrollable container uses overflow-auto but lacks tabIndex={0} for keyboard focus.
Why: Users navigating with keyboard cannot scroll JSON content that overflows. This is a WCAG 2.1.1 consideration for scrollable containers. However, this pattern is consistent with other overflow-auto usages across the codebase — it's not a regression introduced by this PR.
Fix: Optionally add tabIndex={0} and role="region" with an aria-label, but this is a codebase-wide pattern that could be addressed separately.
✅ APPROVE
Summary: This is a well-executed performance optimization. The new ReadOnlyJsonView component is appropriately lightweight, uses Streamdown consistently with other parts of the codebase, and the refactoring correctly removes Monaco overhead where editing is not needed. The changeset is properly scoped. Ship it! 🚀
Discarded (5)
| Location | Issue | Reason Discarded |
|---|---|---|
read-only-json-view.tsx:18 |
Markdown string recreated on render | React Compiler handles optimization; LOW confidence finding |
read-only-json-view.tsx:27 |
Inline style for maxHeight | Pragmatic for dynamic values; Tailwind arbitrary values would be less readable |
read-only-json-view.tsx:13 |
"View" suffix instead of "Editor" | Intentional naming to distinguish read-only from editable components |
read-only-json-view.tsx:8 |
Value prop expects pre-formatted string | Matches existing ExpandableJsonEditor pattern |
evaluation-job-results.tsx |
Duplicate OutputCollapsible/MetadataCollapsible | Pre-existing duplication not introduced by this PR |
Reviewers (3)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-frontend |
3 | 0 | 1 | 0 | 0 | 0 | 2 |
pr-review-consistency |
3 | 0 | 0 | 0 | 0 | 0 | 3 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 6 | 0 | 1 | 0 | 0 | 0 | 5 |
Preview URLsUse these stable preview aliases for testing this PR:
These point to the same Vercel preview deployment as the bot comment, but they stay stable and easier to find. Raw Vercel deployment URLs
|
There was a problem hiding this comment.
Clean, well-scoped performance improvement. Replacing per-row Monaco editor instances with a lightweight Streamdown-based markdown renderer for read-only JSON display is the right call — Monaco is far too heavy for static content in table cells and collapsibles. Streamdown is already an existing dependency with established usage patterns across the codebase, and ExpandableJsonEditor remains intact for its 5 editable consumers. No issues found.
Claude Opus ｜ 𝕏
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
This is a clean, focused performance optimization that replaces heavy Monaco-based ExpandableJsonEditor components with a lightweight ReadOnlyJsonView component using Streamdown for read-only JSON display contexts. The implementation is correct, follows existing codebase patterns, and the refactoring is well-scoped.
🔴❗ Critical (0) ❗🔴
None.
🟠⚠️ Major (0) 🟠⚠️
None.
🟡 Minor (0) 🟡
None.
💭 Consider (1) 💭
💭 1) .changeset/red-flowers-beg.md More descriptive changeset message
Issue: The changeset message "improve performance of dataset loading" is functional but could be more specific about what changed.
Why: Per AGENTS.md guidance, changelog messages should help consumers understand what changed. A more descriptive message would clarify: (1) what was replaced (Monaco Editor), (2) what replaced it (Streamdown), and (3) the scope (datasets + evaluation results).
Fix: Consider updating to: "Replace Monaco Editor with lightweight Streamdown for read-only JSON display in datasets and evaluation results, improving render performance"
Refs: AGENTS.md changeset guidelines
✅ APPROVE
Summary: This is a well-executed performance optimization. The new ReadOnlyJsonView component is appropriately lightweight, uses Streamdown consistently with other parts of the codebase, and the refactoring correctly removes Monaco overhead where editing is not needed. The changeset is properly scoped. Ship it! 🚀
Discarded (2)
| Location | Issue | Reason Discarded |
|---|---|---|
read-only-json-view.tsx:13 |
Export pattern (const FC vs function) inconsistent with ExpandableJsonEditor |
ReadOnlyJsonView actually follows the majority pattern in the editors directory; the inconsistency is in ExpandableJsonEditor (pre-existing, out of scope) |
| Various | All prior review findings (accessibility, undefined handling, Streamdown overhead, etc.) | Already raised in prior reviews — cannot re-raise per No Duplication Principle |
Reviewers (3)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-frontend |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-standards |
1 | 0 | 1 | 0 | 0 | 0 | 0 |
pr-review-consistency |
1 | 0 | 0 | 0 | 0 | 0 | 1 |
| Total | 2 | 0 | 1 | 0 | 0 | 0 | 1 |
Note: This is a re-review with no new commits since the prior automated review. All prior findings have been excluded per the No Duplication Principle.
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Delta Review
This is a re-review scoped to changes since the last automated review (fa6a7d81cbfb). The only change is an improved changeset message:
-improve performance of dataset loading
+Improve performance of datasets and evaluation results loading This update:
- ✅ Fixes sentence casing (per AGENTS.md guidelines)
- ✅ Expands scope to accurately reflect that both datasets AND evaluation results are affected
- ✅ Addresses feedback from the prior review suggesting a more descriptive message
🔴❗ Critical (0) ❗🔴
None.
🟠⚠️ Major (0) 🟠⚠️
None.
🟡 Minor (0) 🟡
None.
✅ APPROVE
Summary: The delta consists solely of an improved changeset message that addresses prior review feedback. The core implementation (replacing Monaco Editor with Streamdown for read-only JSON views) was already approved in the previous review. Ship it! 🚀
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| Total | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: Delta-only review — no sub-reviewers dispatched as the only change is a changeset message update.
1 participant
No description provided.