feat(openapi-sync): virtualize spec diff rendering + spec change block navigation#7848
feat(openapi-sync): virtualize spec diff rendering + spec change block navigation#7848abhishek-bruno wants to merge 1 commit intousebruno:mainfrom
Conversation
- Added a new SpecDiffModal component to display differences between current and updated specs. - Introduced buildRows function to flatten parsed diff data for rendering. - Created DiffRow component for rendering individual rows in the diff view. - Implemented highlightCache for efficient word-level diff highlighting. - Enhanced user experience with navigation controls for changes and loading indicators. - Added tests for buildRows functionality to ensure accurate diff representation.
abhishek-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno,
naman-bruno and
sid-bruno
as code owners
WalkthroughThis PR refactors the OpenAPI spec diff modal from rendering Diff2Html-generated HTML to a custom virtualized row-based renderer. It adds utilities for parsing diffs into flat row arrays, caching word-level highlights, and rendering individual rows, along with change block navigation and updated styling. Changes
Sequence DiagramsequenceDiagram
participant User
participant SpecDiffModal
participant Diff2Html
participant buildRows
participant highlightCache
participant Virtuoso
participant DiffRow
User->>SpecDiffModal: Open modal with unified diff
SpecDiffModal->>Diff2Html: parse(unifiedDiff)
Diff2Html-->>SpecDiffModal: parsed hunks array
SpecDiffModal->>buildRows: buildRows(parsed)
buildRows-->>SpecDiffModal: { rows, changeBlocks }
SpecDiffModal->>highlightCache: createHighlightCache()
highlightCache-->>SpecDiffModal: cache instance
SpecDiffModal->>Virtuoso: render with rows array
Virtuoso->>DiffRow: render each row item
DiffRow->>highlightCache: getWordDiff(left, right)
highlightCache-->>DiffRow: HTML diff fragments
DiffRow-->>Virtuoso: rendered row markup
User->>SpecDiffModal: Click prev/next button
SpecDiffModal->>buildRows: wrapIndex(currentIdx, length)
buildRows-->>SpecDiffModal: wrapped index
SpecDiffModal->>Virtuoso: scrollToIndex(activeBlock)
Virtuoso-->>User: scroll to active change
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Suggested Reviewers
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
packages/bruno-app/src/components/OpenAPISyncTab/SpecDiffModal/__tests__/buildRows.spec.js (2)
73-87: Nit: test-numbering scheme goes 1, 2, 3, 4, (unnumbered), 5, 6, 7.The "matched del+ins pair" test on Line 73 breaks the 1..6 sequence. Either number it (e.g.
4b.or renumber 5→5 and push) or drop numbers entirely — the descriptive name is already enough. No logic impact.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/OpenAPISyncTab/SpecDiffModal/__tests__/buildRows.spec.js` around lines 73 - 87, Update the test title for the "matched del+ins pair → paired row with leftKind=del, rightKind=ins" case to remove or adjust the stray numbering so the test sequence is consistent; locate the test that calls buildRows(parsed) (the test block asserting rows[1].leftKind/rightKind/leftText/rightText etc.) and edit its description string to either add a matching numeric label (e.g., "4b.") or remove numbers entirely so it follows the surrounding tests' naming scheme.
19-147: Consider covering the "unknown line type" safety path.
buildRowshas an explicit safety step for unrecognizedline.typevalues (Pass 1, thei++fallback). It's defensive today but not tested — a test feeding{ type: 'unknown', content: '...' }would pin down the contract and guard against a future refactor silently dropping thei++and infinite-looping.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/OpenAPISyncTab/SpecDiffModal/__tests__/buildRows.spec.js` around lines 19 - 147, The test suite misses a case asserting buildRows handles unexpected line.type values and advances the loop (the Pass 1 i++ fallback); add a unit test that calls buildRows with a parsed hunk containing a line object like { type: 'unknown', content: '...' } and assert it returns without hanging and produces rows/changeBlocks (e.g., no infinite loop and same empty-output behavior or a predictable placeholder), thereby exercising the safety path in the buildRows implementation and ensuring the i++ fallback remains covered.packages/bruno-app/src/components/OpenAPISyncTab/SpecDiffModal/buildRows.js (1)
139-145: Nit:row.leftKind === 'hunk'is already covered by!isChanged.Hunk rows have
leftKind: 'hunk',rightKind: 'hunk'— neither isdel/ins, soisChangedisfalseand!isChangedtriggers. The explicit hunk check is a no-op. Leaving it for readability is fine, but if you want it leaner:♻️ Optional simplification
rows.forEach((row, idx) => { const isChanged = row.leftKind === 'del' || row.rightKind === 'ins'; - if (row.leftKind === 'hunk' || !isChanged) { + if (!isChanged) { currentBlock = null; return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/OpenAPISyncTab/SpecDiffModal/buildRows.js` around lines 139 - 145, The condition "row.leftKind === 'hunk' || !isChanged" is redundant because hunk rows yield !isChanged already; in the rows.forEach loop simplify the branch by removing the explicit hunk check and only test !isChanged (i.e., replace the if (row.leftKind === 'hunk' || !isChanged) { currentBlock = null; return; } with a single !isChanged check) so currentBlock is cleared for unchanged/hunk rows while keeping changed-row logic intact; refer to the rows.forEach loop and the isChanged, row.leftKind/row.rightKind, and currentBlock identifiers when making the change.packages/bruno-app/src/components/OpenAPISyncTab/SpecDiffModal/index.js (3)
109-128: Consider addingdata-testidto the nav buttons for Playwright.Guidelines call for
data-testidon testable elements. Next/Previous change navigation is exactly the kind of behavior a regression test would want to click on —titleattributes aren't stable locators.♻️ Suggested tweak
<button type="button" className="spec-diff-nav-btn" onClick={() => goToChange(currentIndex - 1)} disabled={changeCount === 0} title="Previous change" + data-testid="spec-diff-nav-prev" > <IconChevronUp size={14} strokeWidth={1.75} /> Previous </button> <button type="button" className="spec-diff-nav-btn" onClick={() => goToChange(currentIndex + 1)} disabled={changeCount === 0} title="Next change" + data-testid="spec-diff-nav-next" > <IconChevronDown size={14} strokeWidth={1.75} /> Next </button>As per coding guidelines: "Add
data-testidto testable elements for Playwright."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/OpenAPISyncTab/SpecDiffModal/index.js` around lines 109 - 128, Add stable Playwright testids to the navigation buttons: update the two buttons that call goToChange(currentIndex - 1) and goToChange(currentIndex + 1) (they have className "spec-diff-nav-btn" and use currentIndex/changeCount) to include data-testid attributes (e.g. data-testid="spec-diff-prev-btn" and data-testid="spec-diff-next-btn") so tests can reliably select the Previous and Next change controls.
59-71: Small UX thought: auto-scroll to the first change when the diff loads.
setCurrentIndex(0)is set on parse (Line 47) but we never callscrollToIndexfor it, so the user always lands at the top of the file and has to hit Next to jump to the first change. For small diffs this is fine; for large specs where the interesting changes are thousands of lines deep, it's a friction point.Optional — not a blocker — but easy win:
♻️ Sketch
setRows(built.rows); setChangeBlocks(built.changeBlocks); setCurrentIndex(0); cache.clear(); + // Jump to the first change once Virtuoso is mounted. + if (built.changeBlocks.length > 0) { + // defer until after the Virtuoso has rendered + requestAnimationFrame(() => { + virtuosoRef.current?.scrollToIndex({ + index: built.changeBlocks[0].startIdx, + align: 'center', + behavior: 'auto' + }); + }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/OpenAPISyncTab/SpecDiffModal/index.js` around lines 59 - 71, The current behavior sets setCurrentIndex(0) when parsing but never scrolls the virtuoso to that item; update the logic so the first change is auto-scrolled when diffs load by invoking the same scroll behavior used in goToChange for index 0 (or calling goToChange(0)) right after changeBlocks are populated (e.g., in the parse completion handler or a useEffect that watches changeBlocks length), ensuring you use virtuosoRef.current?.scrollToIndex with the same align/behavior logic and then setCurrentIndex(0) so the user lands on the first change instead of the top of the file.
29-57: Missingcachein the effect dependency array.The effect references
cache(Line 48,cache.clear()) but the dependency array on Line 57 only lists[specDrift?.unifiedDiff].cacheis stable (fromuseState(createHighlightCache)) so this is safe at runtime, but it'll tripreact-hooks/exhaustive-depsand relies on readers knowing that invariant.♻️ Proposed fix
return () => clearTimeout(timer); - }, [specDrift?.unifiedDiff]); + }, [specDrift?.unifiedDiff, cache]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/OpenAPISyncTab/SpecDiffModal/index.js` around lines 29 - 57, The effect uses the stable `cache` (created by `useState(createHighlightCache)`) but does not list it in the dependency array, which trips the react-hooks linter; update the dependency array for the useEffect that parses `specDrift.unifiedDiff` to include `cache` (i.e., deps should be `[specDrift?.unifiedDiff, cache]`), or if you intentionally rely on its stability, explicitly document that by memoizing/creating `cache` with `useRef`/`useMemo` or add an inline eslint-disable comment; adjust around the block referencing `Diff2Html.parse`, `buildRows`, `setRows`, `setChangeBlocks`, `setCurrentIndex`, `setIsRendering`, and `setParseError` accordingly so the linter no longer flags the missing dependency.
🤖 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/bruno-app/src/components/OpenAPISyncTab/SpecDiffModal/buildRows.js`:
- Around line 94-120: The pairing logic in buildRows.js assumes Diff2Html emits
deletions before insertions, which can cause unpaired solo rows for inputs like
[insert, delete]; update the code around the deletions/insertions collection
(the blocks that fill deletions and insertions and the subsequent pairing loop
that calls pairedChangeRow, soloDeletionRow, soloInsertionRow) to either assert
or log a dev-only warning when an insertion is encountered before a preceding
deletion (i.e., unexpected ordering), and add a one-line comment documenting the
required invariant (Diff2Html emits del before ins) so future readers know the
assumption; use the existing variable names deletions, insertions,
pairedChangeRow, soloDeletionRow, and soloInsertionRow to locate the exact place
to add the check/comment.
In
`@packages/bruno-app/src/components/OpenAPISyncTab/SpecDiffModal/highlightCache.js`:
- Around line 11-13: The JSDoc above getWordDiff incorrectly says
"Character-level diff" while the function uses diffHighlight with matching:
'words'; update the comment to "Word-level diff" (or similar) to match the
implementation and other docs; locate the comment directly above the getWordDiff
function and change the wording so it consistently describes word-level diffing
and mentions diffHighlight/matching: 'words' if helpful.
In `@packages/bruno-app/src/components/OpenAPISyncTab/SpecDiffModal/index.js`:
- Around line 74-80: renderItem inside SpecDiffModal is recreated on every
render causing Virtuoso to receive a new function reference; wrap renderItem
with React.useCallback so its identity is stable and only changes when its
dependencies change (rows, activeBlock, cache) — update the renderItem
declaration to use useCallback((index) => <DiffRow row={rows[index]}
active={!!activeBlock && index >= activeBlock.startIdx && index <=
activeBlock.endIdx} cache={cache} />, [rows, activeBlock, cache]).
- Around line 90-105: The wrapper element with class spec-diff-header-left is
unused in StyledWrapper.js; either add a style block for .spec-diff-header-left
inside StyledWrapper (e.g., to control layout/spacing/stacking of
.spec-diff-badges and .spec-diff-subtitle) or remove the wrapper in
SpecDiffModal to simplify markup; update StyledWrapper.js to include a
.spec-diff-header-left rule (targeting spacing, flex/column behavior, and
gap/margin for its children like .spec-diff-badges and .spec-diff-subtitle) if
you want explicit left-side layout control.
In `@packages/bruno-app/src/components/OpenAPISyncTab/StyledWrapper.js`:
- Around line 1532-1536: The current selector "div:last-child" in StyledWrapper
(targeting children of .text-diff-container) is too broad and unintentionally
matches .text-diff-loading and .text-diff-empty; replace the fallback selector
with the specific testid selector (only keep
div[data-testid='virtuoso-scroller']) so only the Virtuoso scroller receives
flex: 1 1 auto; min-height: 0, and remove the broader div:last-child rule. Also
make the theme color usage consistent for the yellow shadow: either remove the
"|| '#f59e0b'" fallback where theme.colors.text.yellow is used (so all usages
rely on the required theme field) or add the same fallback everywhere—update the
box-shadow occurrences that reference theme.colors.text.yellow to match
(.text-diff-loading/.text-diff-empty context is relevant).
---
Nitpick comments:
In
`@packages/bruno-app/src/components/OpenAPISyncTab/SpecDiffModal/__tests__/buildRows.spec.js`:
- Around line 73-87: Update the test title for the "matched del+ins pair →
paired row with leftKind=del, rightKind=ins" case to remove or adjust the stray
numbering so the test sequence is consistent; locate the test that calls
buildRows(parsed) (the test block asserting
rows[1].leftKind/rightKind/leftText/rightText etc.) and edit its description
string to either add a matching numeric label (e.g., "4b.") or remove numbers
entirely so it follows the surrounding tests' naming scheme.
- Around line 19-147: The test suite misses a case asserting buildRows handles
unexpected line.type values and advances the loop (the Pass 1 i++ fallback); add
a unit test that calls buildRows with a parsed hunk containing a line object
like { type: 'unknown', content: '...' } and assert it returns without hanging
and produces rows/changeBlocks (e.g., no infinite loop and same empty-output
behavior or a predictable placeholder), thereby exercising the safety path in
the buildRows implementation and ensuring the i++ fallback remains covered.
In `@packages/bruno-app/src/components/OpenAPISyncTab/SpecDiffModal/buildRows.js`:
- Around line 139-145: The condition "row.leftKind === 'hunk' || !isChanged" is
redundant because hunk rows yield !isChanged already; in the rows.forEach loop
simplify the branch by removing the explicit hunk check and only test !isChanged
(i.e., replace the if (row.leftKind === 'hunk' || !isChanged) { currentBlock =
null; return; } with a single !isChanged check) so currentBlock is cleared for
unchanged/hunk rows while keeping changed-row logic intact; refer to the
rows.forEach loop and the isChanged, row.leftKind/row.rightKind, and
currentBlock identifiers when making the change.
In `@packages/bruno-app/src/components/OpenAPISyncTab/SpecDiffModal/index.js`:
- Around line 109-128: Add stable Playwright testids to the navigation buttons:
update the two buttons that call goToChange(currentIndex - 1) and
goToChange(currentIndex + 1) (they have className "spec-diff-nav-btn" and use
currentIndex/changeCount) to include data-testid attributes (e.g.
data-testid="spec-diff-prev-btn" and data-testid="spec-diff-next-btn") so tests
can reliably select the Previous and Next change controls.
- Around line 59-71: The current behavior sets setCurrentIndex(0) when parsing
but never scrolls the virtuoso to that item; update the logic so the first
change is auto-scrolled when diffs load by invoking the same scroll behavior
used in goToChange for index 0 (or calling goToChange(0)) right after
changeBlocks are populated (e.g., in the parse completion handler or a useEffect
that watches changeBlocks length), ensuring you use
virtuosoRef.current?.scrollToIndex with the same align/behavior logic and then
setCurrentIndex(0) so the user lands on the first change instead of the top of
the file.
- Around line 29-57: The effect uses the stable `cache` (created by
`useState(createHighlightCache)`) but does not list it in the dependency array,
which trips the react-hooks linter; update the dependency array for the
useEffect that parses `specDrift.unifiedDiff` to include `cache` (i.e., deps
should be `[specDrift?.unifiedDiff, cache]`), or if you intentionally rely on
its stability, explicitly document that by memoizing/creating `cache` with
`useRef`/`useMemo` or add an inline eslint-disable comment; adjust around the
block referencing `Diff2Html.parse`, `buildRows`, `setRows`, `setChangeBlocks`,
`setCurrentIndex`, `setIsRendering`, and `setParseError` accordingly so the
linter no longer flags the missing dependency.
🪄 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
Run ID: 8837cc4a-28af-4af9-a14c-0f199f440677
📒 Files selected for processing (7)
packages/bruno-app/src/components/OpenAPISyncTab/SpecDiffModal/DiffRow.jspackages/bruno-app/src/components/OpenAPISyncTab/SpecDiffModal/__tests__/buildRows.spec.jspackages/bruno-app/src/components/OpenAPISyncTab/SpecDiffModal/buildRows.jspackages/bruno-app/src/components/OpenAPISyncTab/SpecDiffModal/highlightCache.jspackages/bruno-app/src/components/OpenAPISyncTab/SpecDiffModal/index.jspackages/bruno-app/src/components/OpenAPISyncTab/StyledWrapper.jspackages/bruno-app/src/components/OpenAPISyncTab/SyncReviewPage/index.js
| // Collect the next run of deletions, then the run of insertions that | ||
| // immediately follows. Pair them 1:1 into side-by-side change rows; | ||
| // any leftovers spill as solo rows. | ||
| // | ||
| // e.g. del A, del B, del C, ins X, ins Y | ||
| // → (A ↔ X) (B ↔ Y) (C ↔ ∅) | ||
| const deletions = []; | ||
| while (i < lines.length && lines[i].type === 'delete') { | ||
| deletions.push(lines[i]); | ||
| i++; | ||
| } | ||
| const insertions = []; | ||
| while (i < lines.length && lines[i].type === 'insert') { | ||
| insertions.push(lines[i]); | ||
| i++; | ||
| } | ||
|
|
||
| const pairCount = Math.min(deletions.length, insertions.length); | ||
| for (let p = 0; p < pairCount; p++) { | ||
| rows.push(pairedChangeRow(deletions[p], insertions[p])); | ||
| } | ||
| for (let p = pairCount; p < deletions.length; p++) { | ||
| rows.push(soloDeletionRow(deletions[p])); | ||
| } | ||
| for (let p = pairCount; p < insertions.length; p++) { | ||
| rows.push(soloInsertionRow(insertions[p])); | ||
| } |
There was a problem hiding this comment.
Pairing assumes Diff2Html's del-before-ins ordering — document or assert.
The collector walks deletions then insertions, so input like [insert, delete] at the same position produces two solo rows (one empty-on-left, one empty-on-right) instead of one paired change row with word-level highlight. Diff2Html normalizes to del → ins in practice, so this is fine today, but the invariant is load-bearing and only implied by the comment example on Line 98.
A one-line comment upgrade (or a dev-only console.warn on unexpected ordering) would future-proof this against a Diff2Html behavior change.
#!/bin/bash
# Confirm Diff2Html emits delete-before-insert consistently.
rg -nP -C3 'type\s*:\s*.insert|LineType\.Insert' packages/bruno-app/public/static/diff2Html*.js 2>/dev/null | head -60🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-app/src/components/OpenAPISyncTab/SpecDiffModal/buildRows.js`
around lines 94 - 120, The pairing logic in buildRows.js assumes Diff2Html emits
deletions before insertions, which can cause unpaired solo rows for inputs like
[insert, delete]; update the code around the deletions/insertions collection
(the blocks that fill deletions and insertions and the subsequent pairing loop
that calls pairedChangeRow, soloDeletionRow, soloInsertionRow) to either assert
or log a dev-only warning when an insertion is encountered before a preceding
deletion (i.e., unexpected ordering), and add a one-line comment documenting the
required invariant (Diff2Html emits del before ins) so future readers know the
assumption; use the existing variable names deletions, insertions,
pairedChangeRow, soloDeletionRow, and soloInsertionRow to locate the exact place
to add the check/comment.
| // Character-level diff for a paired deletion + insertion row. Returns | ||
| // { left, right } HTML strings with <del>/<ins> wrapping changed substrings. | ||
| getWordDiff(leftContent, rightContent) { |
There was a problem hiding this comment.
Doc nit: comment says "character-level" but config is matching: 'words'.
Small inconsistency between the JSDoc on Line 11–12 ("Character-level diff…") and the actual diffHighlight call on Line 30 which uses matching: 'words'. The rest of the file (and the buildRows docstring) consistently calls this "word-level," so a one-word fix keeps the story straight.
📝 Proposed doc tweak
- // Character-level diff for a paired deletion + insertion row. Returns
- // { left, right } HTML strings with <del>/<ins> wrapping changed substrings.
+ // Word-level diff for a paired deletion + insertion row. Returns
+ // { left, right } HTML strings with <del>/<ins> wrapping changed tokens.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/bruno-app/src/components/OpenAPISyncTab/SpecDiffModal/highlightCache.js`
around lines 11 - 13, The JSDoc above getWordDiff incorrectly says
"Character-level diff" while the function uses diffHighlight with matching:
'words'; update the comment to "Word-level diff" (or similar) to match the
implementation and other docs; locate the comment directly above the getWordDiff
function and change the wording so it consistently describes word-level diffing
and mentions diffHighlight/matching: 'words' if helpful.
| const renderItem = (index) => ( | ||
| <DiffRow | ||
| row={rows[index]} | ||
| active={!!activeBlock && index >= activeBlock.startIdx && index <= activeBlock.endIdx} | ||
| cache={cache} | ||
| /> | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether React Compiler is actually enabled for this package and how it treats the file.
fd -t f 'babel.config*' packages/bruno-app -x cat {} \;
fd -t f '.babelrc*' packages/bruno-app -x cat {} \;
fd -t f 'next.config*' packages/bruno-app -x cat {} \;
rg -nP 'babel-plugin-react-compiler|react-compiler' packages/bruno-app -C2
rg -nP '"use memo"|"use no memo"' packages/bruno-app/src/components/OpenAPISyncTab/SpecDiffModalRepository: usebruno/bruno
Length of output: 969
🏁 Script executed:
cat -n packages/bruno-app/src/components/OpenAPISyncTab/SpecDiffModal/index.js | head -100Repository: usebruno/bruno
Length of output: 4558
🏁 Script executed:
# Check if there's a PR description or commit mentioning React Compiler
git log --oneline --all --grep="React Compiler" -- packages/bruno-app/src/components/OpenAPISyncTab/SpecDiffModal | head -20Repository: usebruno/bruno
Length of output: 40
🏁 Script executed:
# Search for how renderItem dependencies (activeBlock, cache, rows) are declared
rg -n "activeBlock|cache.*=|const rows" packages/bruno-app/src/components/OpenAPISyncTab/SpecDiffModal/index.jsRepository: usebruno/bruno
Length of output: 291
Add useCallback to stabilize renderItem reference for Virtuoso.
renderItem recreates on every SpecDiffModal render because it depends on dynamic values (activeBlock changes when currentIndex changes, rows changes on re-parse). React Compiler cannot hoist closures with unstable dependencies, so the function reference changes each time, forcing Virtuoso to potentially re-work visible rows.
Wrap with useCallback:
Suggested fix
const renderItem = useCallback((index) => (
<DiffRow
row={rows[index]}
active={!!activeBlock && index >= activeBlock.startIdx && index <= activeBlock.endIdx}
cache={cache}
/>
), [rows, activeBlock, cache]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-app/src/components/OpenAPISyncTab/SpecDiffModal/index.js`
around lines 74 - 80, renderItem inside SpecDiffModal is recreated on every
render causing Virtuoso to receive a new function reference; wrap renderItem
with React.useCallback so its identity is stable and only changes when its
dependencies change (rows, activeBlock, cache) — update the renderItem
declaration to use useCallback((index) => <DiffRow row={rows[index]}
active={!!activeBlock && index >= activeBlock.startIdx && index <=
activeBlock.endIdx} cache={cache} />, [rows, activeBlock, cache]).
| <div className="spec-diff-header"> | ||
| <div className="spec-diff-header-left"> | ||
| <div className="spec-diff-badges"> | ||
| <div>Endpoint Changes:</div> | ||
| {modifiedCount > 0 && <StatusBadge status="warning">Updated: {modifiedCount}</StatusBadge>} | ||
| {addedCount > 0 && <StatusBadge status="success">Added: {addedCount}</StatusBadge>} | ||
| {removedCount > 0 && <StatusBadge status="danger">Removed: {removedCount}</StatusBadge>} | ||
| {versionLabel && <StatusBadge>{versionLabel}</StatusBadge>} | ||
| </div> | ||
|
|
||
| <p className="spec-diff-subtitle"> | ||
| {specDrift?.storedSpecMissing | ||
| ? 'The current spec file is missing. The full remote spec is shown below.' | ||
| : 'Side-by-side diff of your current spec vs the updated spec from the spec URL.'} | ||
| </p> | ||
| <p className="spec-diff-subtitle"> | ||
| {specDrift?.storedSpecMissing | ||
| ? 'The current spec file is missing. The full remote spec is shown below.' | ||
| : 'Side-by-side diff of your current spec vs the updated spec from the spec URL.'} | ||
| </p> | ||
| </div> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether .spec-diff-header-left is styled anywhere.
rg -nP '\bspec-diff-header-left\b' packages/bruno-app/srcRepository: usebruno/bruno
Length of output: 183
🏁 Script executed:
#!/bin/bash
# Find StyledWrapper file in or near the SpecDiffModal directory
fd -type f "StyledWrapper" packages/bruno-app/src/components/OpenAPISyncTab/SpecDiffModal/Repository: usebruno/bruno
Length of output: 229
🏁 Script executed:
#!/bin/bash
# If not found in SpecDiffModal, search more broadly
fd -type f "StyledWrapper" packages/bruno-app/src/components/OpenAPISyncTab/Repository: usebruno/bruno
Length of output: 229
🏁 Script executed:
#!/bin/bash
# Also check for any CSS files in the directory
fd -type f "\.(css|scss)" packages/bruno-app/src/components/OpenAPISyncTab/SpecDiffModal/Repository: usebruno/bruno
Length of output: 229
🏁 Script executed:
#!/bin/bash
# Find StyledWrapper file
fd "StyledWrapper" packages/bruno-app/src/components/OpenAPISyncTab/SpecDiffModal/Repository: usebruno/bruno
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Search for any style-related files (js, css, scss) in the SpecDiffModal directory
ls -la packages/bruno-app/src/components/OpenAPISyncTab/SpecDiffModal/Repository: usebruno/bruno
Length of output: 483
🏁 Script executed:
#!/bin/bash
# Search more broadly for StyledWrapper in OpenAPISyncTab
fd "StyledWrapper" packages/bruno-app/src/components/OpenAPISyncTab/Repository: usebruno/bruno
Length of output: 124
🏁 Script executed:
#!/bin/bash
# Get file size
wc -l packages/bruno-app/src/components/OpenAPISyncTab/StyledWrapper.jsRepository: usebruno/bruno
Length of output: 129
🏁 Script executed:
#!/bin/bash
# Search for spec-diff-header styles in StyledWrapper.js
rg -n "spec-diff-header" packages/bruno-app/src/components/OpenAPISyncTab/StyledWrapper.jsRepository: usebruno/bruno
Length of output: 89
🏁 Script executed:
#!/bin/bash
# Read the spec-diff-header style definition and surrounding area
sed -n '1675,1720p' packages/bruno-app/src/components/OpenAPISyncTab/StyledWrapper.jsRepository: usebruno/bruno
Length of output: 1350
🏁 Script executed:
#!/bin/bash
# Also search the entire file for any occurrence of spec-diff-header-left
rg -n "spec-diff-header-left" packages/bruno-app/src/components/OpenAPISyncTab/StyledWrapper.jsRepository: usebruno/bruno
Length of output: 40
.spec-diff-header-left has no styles defined in StyledWrapper.js.
The JSX uses spec-diff-header-left as a wrapper for the badges and subtitle, but it's not styled anywhere. The parent .spec-diff-header sets display: flex; align-items: flex-end; justify-content: space-between;, so the unstyled child will render correctly with default block behavior, but if your intent was to control stacking or spacing on the left side, add the style or remove the wrapper.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-app/src/components/OpenAPISyncTab/SpecDiffModal/index.js`
around lines 90 - 105, The wrapper element with class spec-diff-header-left is
unused in StyledWrapper.js; either add a style block for .spec-diff-header-left
inside StyledWrapper (e.g., to control layout/spacing/stacking of
.spec-diff-badges and .spec-diff-subtitle) or remove the wrapper in
SpecDiffModal to simplify markup; update StyledWrapper.js to include a
.spec-diff-header-left rule (targeting spacing, flex/column behavior, and
gap/margin for its children like .spec-diff-badges and .spec-diff-subtitle) if
you want explicit left-side layout control.
| > div[data-testid='virtuoso-scroller'], | ||
| > div:last-child { | ||
| flex: 1 1 auto; | ||
| min-height: 0; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does react-virtuoso always render its scroller element with data-testid="virtuoso-scroller" in latest version?
💡 Result:
Yes—at least in the current upstream implementation, the underlying scroller element rendered by react-virtuoso’s default Scroller component includes data-testid="virtuoso-scroller". In the repository source, the buildScroller implementation renders ScrollerComponent with data-testid="virtuoso-scroller" (and also data-virtuoso-scroller={true}) on the scroller DOM node. Caveat: react-virtuoso supports customization of the Scroller element via props/components; if you replace ScrollerComponent, you may no longer get this test id.
Citations:
- 1: https://github.com/petyosi/react-virtuoso/blob/master/packages/react-virtuoso/src/Virtuoso.tsx
- 2: https://virtuoso.dev/react-virtuoso/virtuoso/custom-scroll-container/
🏁 Script executed:
# Find theme definitions to check for theme.colors.text.yellow
fd -t f -e 'js' -e 'ts' -e 'json' | xargs rg 'colors.*text.*yellow|theme.*colors' --type-list | head -20Repository: usebruno/bruno
Length of output: 448
🏁 Script executed:
# Search for theme files in the Bruno app
find . -path '*/theme*' -o -name '*theme*' -o -name '*colors*' | grep -E '\.(js|ts|json)$' | head -20Repository: usebruno/bruno
Length of output: 1104
🏁 Script executed:
# Look for the StyledWrapper.js file and check its imports/context
fd -t f 'StyledWrapper.js' -E 'node_modules'Repository: usebruno/bruno
Length of output: 22991
🏁 Script executed:
# Check the component structure and how loading/empty states are rendered
rg 'text-diff-loading|text-diff-empty' -B 2 -A 2Repository: usebruno/bruno
Length of output: 3071
🏁 Script executed:
# Check theme files for yellow color definition
cat packages/bruno-app/src/themes/dark/dark.js | head -100Repository: usebruno/bruno
Length of output: 2243
🏁 Script executed:
# Check light theme
cat packages/bruno-app/src/themes/light/light.js | head -100Repository: usebruno/bruno
Length of output: 2296
🏁 Script executed:
# Search for theme.colors.text.yellow pattern in StyledWrapper
rg 'colors\.text\.yellow' packages/bruno-app/src/components/OpenAPISyncTab/Repository: usebruno/bruno
Length of output: 651
🏁 Script executed:
# Check for color-mix usage and any fallbacks in StyledWrapper
rg 'color-mix' packages/bruno-app/src/components/OpenAPISyncTab/StyledWrapper.js -A 2 -B 2Repository: usebruno/bruno
Length of output: 993
🏁 Script executed:
# Get the exact line 1559 from StyledWrapper.js
sed -n '1555,1565p' packages/bruno-app/src/components/OpenAPISyncTab/StyledWrapper.jsRepository: usebruno/bruno
Length of output: 426
🏁 Script executed:
# Search for theme.colors.text mapping/definition
rg 'theme\.colors\.text|colors\.text\s*=' packages/bruno-app/src -A 5 -B 2 | head -100Repository: usebruno/bruno
Length of output: 8945
🏁 Script executed:
# Look for where yellow is mapped to theme
rg 'text.*yellow|yellow.*text' packages/bruno-app/src/themes/ -A 2 -B 2Repository: usebruno/bruno
Length of output: 507
🏁 Script executed:
# Check package.json for Electron version
cat packages/bruno-app/package.json | rg 'electron' -A 2 -B 2Repository: usebruno/bruno
Length of output: 40
🏁 Script executed:
# Check for Electron minimum version or compatibility notes
rg 'electron' packages/bruno-app/src -l | head -5Repository: usebruno/bruno
Length of output: 219
🏁 Script executed:
# Search for where yellow is added to theme colors
rg 'yellow' packages/bruno-app/src/themes/ -B 3 -A 3Repository: usebruno/bruno
Length of output: 9236
🏁 Script executed:
# Check the theme index to see how colors are mapped
cat packages/bruno-app/src/themes/index.js | head -80Repository: usebruno/bruno
Length of output: 2082
🏁 Script executed:
# Check the package.json for Electron version
cat packages/bruno-app/package.json | grep -A 5 -B 5 '"electron"'Repository: usebruno/bruno
Length of output: 40
🏁 Script executed:
# Search for any mentions of color-mix or browser compatibility
rg 'color-mix|caniuse|browser|support' packages/bruno-app -i | head -20Repository: usebruno/bruno
Length of output: 2371
🏁 Script executed:
# Get electron version from package.json
cat packages/bruno-app/package.json | jq '.devDependencies.electron' 2>/dev/null || grep -A 1 '"electron"' packages/bruno-app/package.jsonRepository: usebruno/bruno
Length of output: 63
🏁 Script executed:
# Check for color-mix fallbacks or polyfills
rg 'color-mix' packages/bruno-app/src -B 5 -A 5 | grep -E 'fallback|polyfill|rgba|rgba' -iRepository: usebruno/bruno
Length of output: 360
🏁 Script executed:
# Verify the exact line 1559 context - does it have fallback?
sed -n '1555,1565p' packages/bruno-app/src/components/OpenAPISyncTab/StyledWrapper.jsRepository: usebruno/bruno
Length of output: 426
🏁 Script executed:
# Check all other yellow usages to confirm fallback pattern
rg 'colors\.text\.yellow' packages/bruno-app/src/components/OpenAPISyncTab/StyledWrapper.js -B 1 -A 1Repository: usebruno/bruno
Length of output: 581
🏁 Script executed:
# Look at the parent structure to confirm .text-diff-loading/.text-diff-empty are direct children
rg -A 30 'text-diff-container' packages/bruno-app/src/components/OpenAPISyncTab/SpecDiffModal/index.js | head -50Repository: usebruno/bruno
Length of output: 1462
The :last-child fallback selector will match loading/empty states when rendered.
When isRendering is true, .text-diff-loading becomes the last child of .text-diff-container; when there's a parse error or no rows to display, .text-diff-empty becomes the last child. Both will receive flex: 1 1 auto; min-height: 0; as a side effect. While these styles don't break anything (the divs are already flex-centered), the selector is broader than intended.
Since react-virtuoso reliably renders with data-testid="virtuoso-scroller", consider tightening to just the testid selector unless you need the fallback for custom Virtuoso implementations in this codebase.
Additionally, note the inconsistency: Line 1559 applies theme.colors.text.yellow without a fallback in the box-shadow, while lines 1564–1565 use it with || '#f59e0b'. Since yellow is a required theme field (all themes define it), either remove all fallbacks or keep them consistent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-app/src/components/OpenAPISyncTab/StyledWrapper.js` around
lines 1532 - 1536, The current selector "div:last-child" in StyledWrapper
(targeting children of .text-diff-container) is too broad and unintentionally
matches .text-diff-loading and .text-diff-empty; replace the fallback selector
with the specific testid selector (only keep
div[data-testid='virtuoso-scroller']) so only the Virtuoso scroller receives
flex: 1 1 auto; min-height: 0, and remove the broader div:last-child rule. Also
make the theme color usage consistent for the yellow shadow: either remove the
"|| '#f59e0b'" fallback where theme.colors.text.yellow is used (so all usages
rely on the required theme field) or add the same fallback everywhere—update the
box-shadow occurrences that reference theme.colors.text.yellow to match
(.text-diff-loading/.text-diff-empty context is relevant).
1 participant
Description
JIRA
Screen.Recording.2026-04-24.at.1.33.25.PM.mov
Summary
Diff2Html.html()+innerHTMLwith a virtualized row renderer (react-virtuoso), so only visible rows are in the DOM. Unblocks the main thread on large specs — 50k-line diffs no longer freeze the app.buildRows.js(pure, testable), word-level highlighting intohighlightCache.js, and the row component intoDiffRow.js.Why
On large specs (tested with 50k-line diffs), the previous flow hung the renderer's main thread for seconds — the modal appeared unresponsive, the close button didn't react, and the app felt frozen. The new virtualized renderer keeps DOM size bounded regardless of diff size and defers parsing via
setTimeout(0)so the spinner paints first.Notes
buildRows.jsis covered by 8 unit tests including a regression test pinning the change-block count against fixture shapes.SpecDiffModalandDiffRowboth compile cleanly under React Compiler — all manualuseMemo/useCallbackdropped except where they earn their keep..d2h-*class stack is replaced with a custom.diff-row/.diff-cell-*grid layout.Test plan
@@ ... @@) render as full-width rows and don't count as change blocks.Contribution Checklist:
Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.
Publishing to New Package Managers
Please see here for more information.
Summary by CodeRabbit
Release Notes
New Features
Improvements