Skip to content

perf(ui): add virtual scrolling to file preview modal code viewer#101319

Merged
shakkernerd merged 7 commits into
openclaw:mainfrom
xianshishan:perf/virtual-scroll-code-viewer-v2
Jul 7, 2026
Merged

perf(ui): add virtual scrolling to file preview modal code viewer#101319
shakkernerd merged 7 commits into
openclaw:mainfrom
xianshishan:perf/virtual-scroll-code-viewer-v2

Conversation

@xianshishan

@xianshishan xianshishan commented Jul 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

The file preview modal in Skill Workshop renders entire file contents as a single giant <pre> text node. For large support files (up to 256 KB / ~5000 lines), the browser struggles to layout and paint this monolithic DOM node, causing severe lag and freezing when scrolling through code.

Fixes #99062

Why This Change Was Made

Replace the single <pre> element with a virtual scrolling approach:

  • Only lines visible in the viewport (plus a 30-line overscan buffer above and below) are rendered as individual <div class="code-line"> elements
  • Top and bottom spacer <div> elements maintain correct scrollbar dimensions (totalLines * LINE_HEIGHT = 22px)
  • Scroll events are throttled via requestAnimationFrame to avoid layout thrashing
  • A ResizeObserver on the scroll container recalculates the visible range when the container resizes
  • resetCodeScroll() clears the virtual range and resets .detail-body.scrollTop to 0 when switching files or filtering, preventing blank/stale content
  • connectedCallback / disconnectedCallback handle lifecycle cleanup (rAF cancellation, ResizeObserver teardown)

For a typical 256 KB file with ~5000 lines, DOM nodes drop from 1 giant text node to ~70 lightweight <div> elements at any time.

User Impact

  • Scrolling through large code files in the preview modal is now smooth and responsive
  • No visual change to the UI — identical appearance, dramatically better performance
  • File list sidebar navigation is unaffected
  • File switching and filtering correctly reset the scroll position to the top

Evidence

image

The file display area has been refactored into additional div elements; implementing virtual scrolling will help alleviate performance bottlenecks.

Changes

  • ui/src/components/file-preview-modal.ts (+98/-10): Replaced single <pre> with virtual-scrolled <div class="code-line"> elements
    • Added handleCodeScroll with rAF throttling for scroll-driven range recalculation
    • Added recalcVisibleRange for computing visible line range from scroll position and container height
    • Added ResizeObserver for container resize-triggered range recalculation
    • Added connectedCallback / disconnectedCallback for lifecycle cleanup
    • Added resetCodeScroll that clears virtual indices and resets scrollTop to 0 on file/query change
    • Replaced .pre CSS with .code-line and .code-vscroll styles

Approach Validation

  • Virtual scrolling is a well-established technique used by code editors and large-list UIs (VS Code, Monaco, React Virtuoso)
  • LINE_HEIGHT (22px) matches line-height from the replaced .pre style (13px font * 1.7 = ~22px)
  • OVERSCAN (30 lines) provides sufficient buffer for smooth scrolling without excessive DOM nodes
  • rAF throttling prevents layout thrashing from rapid scroll events
  • ResizeObserver handles viewport resizing without polling
  • Lifecycle cleanup (cancelAnimationFrame, ResizeObserver.disconnect) prevents memory leaks on element removal

@openclaw-barnacle openclaw-barnacle Bot added app: web-ui App: web-ui size: S triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. and removed triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. labels Jul 7, 2026
@clawsweeper

clawsweeper Bot commented Jul 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 7, 2026, 12:10 AM ET / 04:10 UTC.

Summary
The branch changes ui/src/components/file-preview-modal.ts to render support-file contents as virtualized fixed-height code lines with spacer-backed scrolling, rAF scroll handling, and ResizeObserver cleanup.

PR surface: Source +88. Total +88 across 1 file.

Reproducibility: yes. from source, though not from a live measured browser run: current main renders supported 256 KiB support files in one <pre>. The PR-introduced reset defect is also source-reproducible by scrolling deep, switching/filtering, and observing that resetCodeScroll() leaves the real scrollTop unchanged.

Review metrics: none identified.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #99062
Summary: This PR is the current open candidate fix for the canonical Skill Workshop large-file preview freeze issue; the earlier same-author PR was closed due merge conflicts.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🦪 silver shellfish
Patch quality: 🦐 gold shrimp
Result: blocked until stronger real behavior proof is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Reset .detail-body.scrollTop when activePath, query, or files change before recalculating virtual lines.
  • Attach redacted browser proof showing large-file scrolling and file-switch/filter reset behavior in Skill Workshop.

Proof guidance:

  • [P1] Needs stronger real behavior proof before merge: The screenshot shows a small 2.7 KB file and DOM line elements, but it does not show after-fix large-file scrolling or file-switch/filter reset behavior; add redacted browser proof, then update the PR body to trigger review or ask a maintainer for @clawsweeper re-review.

Risk before merge

  • [P1] Merging as-is can leave the virtualized code pane blank or on the wrong slice after file switching or filtering because the actual .detail-body scroll offset can remain deep in the previous file.
  • [P1] The current proof screenshot does not show the reported large-file scrolling path or the reset behavior in a real Skill Workshop setup.

Maintainer options:

  1. Repair scroll reset before merge (recommended)
    Update the reset path to set the real code-pane scroll container back to the top before recalculating virtual lines, then verify switching and filtering after a deep scroll.
  2. Pause for contributor proof
    Keep the PR open but do not merge until the author supplies redacted browser evidence for large-file scrolling and reset behavior in Skill Workshop.

Next step before merge

  • [P1] The PR needs contributor-supplied real browser proof plus a focused scroll-reset repair, so it should stay human-handled unless maintainers explicitly opt into repair after the proof gate is satisfied.

Security
Cleared: The diff only changes a Control UI Lit component and does not introduce dependency, workflow, secret, package-resolution, or code-execution surface changes.

Review findings

  • [P2] Reset the real code scroller on file changes — ui/src/components/file-preview-modal.ts:572-577
Review details

Best possible solution:

Land the virtualized modal viewer after it resets the actual code-pane scroll position on file/query/files changes and the contributor adds redacted browser proof for large-file scrolling plus reset behavior.

Do we have a high-confidence way to reproduce the issue?

Yes from source, though not from a live measured browser run: current main renders supported 256 KiB support files in one <pre>. The PR-introduced reset defect is also source-reproducible by scrolling deep, switching/filtering, and observing that resetCodeScroll() leaves the real scrollTop unchanged.

Is this the best way to solve the issue?

No as submitted. Virtualizing the modal is the right owner boundary, but this implementation needs to reset the actual scroll container and include real browser proof before it is the best fix.

Full review comments:

  • [P2] Reset the real code scroller on file changes — ui/src/components/file-preview-modal.ts:572-577
    updated() calls resetCodeScroll() when the active file, query, or file list changes, but this helper only clears virtual indices and leaves .detail-body.scrollTop at the previous offset. After a user scrolls deep into a large file, switching files or filtering can render the top slice while the viewport remains down in spacer space, leaving blank or wrong content until another scroll/resize recalculates from that stale offset.
    Confidence: 0.9

Overall correctness: patch is incorrect
Overall confidence: 0.86

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 1d128b4dd232.

Label changes

Label changes:

  • add P2: This is a bounded Control UI performance bug fix for supported Skill Workshop files, with one blocking component defect and a proof gap.
  • add merge-risk: 🚨 other: The virtualized preview can show blank or stale content after file switching/filtering if the real scroll container keeps its previous offset.
  • add proof: 📸 screenshot: Contributor real behavior proof includes screenshot evidence. The screenshot shows a small 2.7 KB file and DOM line elements, but it does not show after-fix large-file scrolling or file-switch/filter reset behavior; add redacted browser proof, then update the PR body to trigger review or ask a maintainer for @clawsweeper re-review.
  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The screenshot shows a small 2.7 KB file and DOM line elements, but it does not show after-fix large-file scrolling or file-switch/filter reset behavior; add redacted browser proof, then update the PR body to trigger review or ask a maintainer for @clawsweeper re-review.

Label justifications:

  • P2: This is a bounded Control UI performance bug fix for supported Skill Workshop files, with one blocking component defect and a proof gap.
  • merge-risk: 🚨 other: The virtualized preview can show blank or stale content after file switching/filtering if the real scroll container keeps its previous offset.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The screenshot shows a small 2.7 KB file and DOM line elements, but it does not show after-fix large-file scrolling or file-switch/filter reset behavior; add redacted browser proof, then update the PR body to trigger review or ask a maintainer for @clawsweeper re-review.
  • proof: 📸 screenshot: Contributor real behavior proof includes screenshot evidence. The screenshot shows a small 2.7 KB file and DOM line elements, but it does not show after-fix large-file scrolling or file-switch/filter reset behavior; add redacted browser proof, then update the PR body to trigger review or ask a maintainer for @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +88. Total +88 across 1 file.

View PR surface stats
Area Files Added Removed Net
Source 1 98 10 +88
Tests 0 0 0 0
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 1 98 10 +88

What I checked:

  • Current main still renders the full file in one pre block: The current modal still renders file.contents directly inside <pre class="pre">, so main has not already implemented the requested large-file rendering fix. (ui/src/components/file-preview-modal.ts:447, 1d128b4dd232)
  • Supported input can be large enough to hit this path: Skill Workshop docs allow support files up to 256 KiB each and 2 MiB total, matching the scale in the linked report. Public docs: docs/tools/skill-workshop.md. (docs/tools/skill-workshop.md:326, 1d128b4dd232)
  • PR reset helper leaves the real scroll offset unchanged: The PR resets virtual indices in resetCodeScroll() but does not set .detail-body.scrollTop back to 0 before rendering the new file/query slice. (ui/src/components/file-preview-modal.ts:572, 13f14b48bc0b)
  • Real behavior proof is insufficient: The attached screenshot shows a small 2.7 KB file and DOM line elements, but it does not demonstrate large-file smooth scrolling or file-switch/filter reset behavior. (13f14b48bc0b)
  • Earlier duplicate branch is closed, not canonical: The previous virtual-scroll PR was closed by the author due merge conflicts, so this PR is the current open candidate rather than being superseded by that closed branch. (f937ad65db40)
  • Feature history points to Skill Workshop UI owners: Merged PR metadata shows shakkernerd added the Skill Workshop Control UI including the original file preview modal, while steipete recently changed the current modal and test files. (ui/src/components/file-preview-modal.ts:1, f78bb34cb4eb)

Likely related people:

  • shakkernerd: Merged PR metadata for feat: add Skill Workshop Control UI #88756 shows this person added the Skill Workshop Control UI, including the original file preview modal, its tests, controller, and view. (role: introduced feature surface; confidence: high; commits: f78bb34cb4eb; files: ui/src/ui/components/file-preview-modal.ts, ui/src/ui/components/file-preview-modal.test.ts, ui/src/ui/controllers/skill-workshop.ts)
  • steipete: Merged PR metadata for fix(ui): remove redundant file-preview Escape hint #100528 shows recent work on the current file preview modal and its tests after the UI path move. (role: recent adjacent contributor; confidence: medium; commits: 797bca251e88; files: ui/src/components/file-preview-modal.ts, ui/src/components/file-preview-modal.test.ts)
  • Ayaan Zaidi: Current-main blame shows the present ui/src/components/file-preview-modal.ts file was reintroduced into its current path in commit ec289357a450, so this is useful provenance for path history but weaker ownership evidence for the behavior. (role: recent path mover; confidence: low; commits: ec289357a450; files: ui/src/components/file-preview-modal.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added proof: 📸 screenshot Contributor real behavior proof includes screenshot evidence. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. labels Jul 7, 2026
@shakkernerd shakkernerd self-assigned this Jul 7, 2026
@shakkernerd
shakkernerd force-pushed the perf/virtual-scroll-code-viewer-v2 branch from 13f14b4 to d032a61 Compare July 7, 2026 17:36
@shakkernerd
shakkernerd merged commit 53d0c4c into openclaw:main Jul 7, 2026
81 of 84 checks passed
@shakkernerd

shakkernerd commented Jul 7, 2026

Copy link
Copy Markdown
Member

Merged using rebase.

What changed after review:

  • Replaced the fixed-height line virtualizer with cached, wrapped text chunks using browser containment.
  • Preserved full-file copy, file-switch scroll reset, modal focus, and narrow-screen wrapping.
  • Added focused regression coverage and updated the changelog credit.

Verification:

  • Rebased/refreshed onto current origin/main before pushing.
  • Local focused Vitest: 7/7 passed; Control UI production build passed.
  • Browser proof: approximately 254 KB / 5,000 lines, deep scrolling, wide and narrow wrapping, full copy, and file-switch reset.
  • Auto-review: clean with no accepted/actionable findings.
  • GitHub checks: no required checks configured; current-head admission, security, dependency, workflow-sanity, and Real behavior proof checks were green at merge.

Landed on main:

Thanks @xianshishan!

@github-actions github-actions Bot mentioned this pull request Jul 7, 2026
Open
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Jul 8, 2026
@shakkernerd
docs: update file preview changelog (openclaw#101319)
f9f1b1c
giodl73-repo pushed a commit to giodl73-repo/openclaw that referenced this pull request Jul 8, 2026
@shakkernerd @giodl73-repo
docs: update file preview changelog (openclaw#101319)
f86581d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@shakkernerd shakkernerd

Labels

app: web-ui App: web-ui merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. P2 Normal backlog priority with limited blast radius. proof: 📸 screenshot Contributor real behavior proof includes screenshot evidence. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: M status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

File preview modal freezes when rendering large code files in Skill Workshop

2 participants

@xianshishan @shakkernerd