Skip to content

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

Closed
xianshishan wants to merge 1 commit into
openclaw:mainfrom
xianshishan:perf/virtual-scroll-code-viewer
Closed

perf(ui): add virtual scrolling to file preview modal code viewer#99063
xianshishan wants to merge 1 commit into
openclaw:mainfrom
xianshishan:perf/virtual-scroll-code-viewer

Conversation

@xianshishan

@xianshishan xianshishan commented Jul 2, 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

Before

After

Changes

  • ui/src/ui/components/file-preview-modal.ts:
    • 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

@clawsweeper

clawsweeper Bot commented Jul 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 2, 2026, 9:30 PM ET / 01:30 UTC.

Summary
This PR replaces the Skill Workshop support-file preview <pre> with a virtualized line renderer and scroll/resize range tracking in ui/src/ui/components/file-preview-modal.ts.

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 large support-file contents as one full <pre>, and the linked issue gives the Skill Workshop path. The PR-introduced reset defect is also source-reproducible because the virtual range resets without resetting the real scroll container.

Review metrics: none identified.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #99062
Summary: This PR is the open candidate fix for the canonical large-file Skill Workshop preview freeze issue; the Escape-label PR only overlaps by file.

Members:

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

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🦐 gold shrimp
Result: blocked until 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 the actual .detail-body.scrollTop when activePath, query, or files change.
  • Attach redacted browser proof showing large-file scrolling and file-switch/filter reset behavior.

Proof guidance:

  • [P1] Needs real behavior proof before merge: Missing: the PR body still has TODO proof placeholders and no redacted screenshot, recording, log, live output, or artifact showing the changed Skill Workshop behavior; update the PR body after adding proof to trigger a fresh ClawSweeper review.

Mantis proof suggestion
A short browser recording would directly show whether large-file scrolling and reset behavior work in the real Skill Workshop UI. A maintainer can ask Mantis to capture proof by posting this exact PR comment:

@openclaw-mantis visual task: verify Skill Workshop support-file preview scrolls smoothly on a large file and switching/filtering resets the code pane without blank content.

Risk before merge

  • [P1] Merging as-is can leave the virtualized code pane blank or showing the wrong slice after file switching or filtering because the real .detail-body scroll offset is not reset.
  • [P1] No redacted browser proof currently shows the after-fix large-file scrolling path or the file-switch/filter reset behavior in a real Skill Workshop setup.

Maintainer options:

  1. Fix scroll reset and prove the UI path (recommended)
    Update the reset path to put .detail-body.scrollTop back at the top before recalculating virtual lines, then attach redacted browser proof for large-file scrolling and switching/filtering.
  2. Pause until contributor proof exists
    Keep the PR open but do not merge until the author supplies observable after-fix proof from a real Skill Workshop setup.

Next step before merge

  • [P1] The PR needs contributor-supplied real behavior proof plus a focused scroll-reset repair, so this should stay human-handled unless maintainers explicitly opt into repair.

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/ui/components/file-preview-modal.ts:549-554
Review details

Best possible solution:

Land the virtualized modal code viewer after the branch 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 large support-file contents as one full <pre>, and the linked issue gives the Skill Workshop path. The PR-introduced reset defect is also source-reproducible because the virtual range resets without resetting the real scroll container.

Is this the best way to solve the issue?

No for this branch as submitted. Virtualizing the modal is the right owner boundary, but the implementation must 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/ui/components/file-preview-modal.ts:549-554
    updated() invokes 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 the stale offset.
    Confidence: 0.88

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 4440226d52c2.

Label changes

Label justifications:

  • P2: This is a bounded Control UI performance bug fix with limited blast radius, but the PR still has a merge-blocking component defect and missing proof.
  • merge-risk: 🚨 other: The virtualized code pane can show blank or stale content after file switching/filtering if the real scroll container keeps its old offset.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: Missing: the PR body still has TODO proof placeholders and no redacted screenshot, recording, log, live output, or artifact showing the changed Skill Workshop behavior; update the PR body after adding proof to trigger a fresh ClawSweeper 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:

Likely related people:

  • shakkernerd: GitHub path history shows this person extracted the file preview modal component and carried several Skill Workshop UI and support-file follow-up commits around the affected view/controller surface. (role: feature owner; confidence: high; commits: fd2c65f59b10, 40f99e474a35, d1378650bb39; files: ui/src/ui/components/file-preview-modal.ts, ui/src/ui/components/file-preview-modal.test.ts, ui/src/ui/views/skill-workshop.ts)
  • Solvely-Colin: Recent history shows adjacent Skill Workshop filtered navigation work in the controller/view path that feeds the modal's active file and query behavior. (role: adjacent area contributor; confidence: medium; commits: a34e822cd4b9, 211321ce5c49; files: ui/src/ui/controllers/skill-workshop.ts, ui/src/ui/views/skill-workshop.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 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. labels Jul 2, 2026
@xianshishan
xianshishan force-pushed the perf/virtual-scroll-code-viewer branch from db3d498 to bcd3d92 Compare July 2, 2026 11:55
@clawsweeper clawsweeper Bot mentioned this pull request Jul 2, 2026
Closed
@xianshishan
xianshishan force-pushed the perf/virtual-scroll-code-viewer branch from bcd3d92 to 78b39e3 Compare July 2, 2026 11:56
@xianshishan
perf(ui): add virtual scrolling to file preview modal code viewer
f937ad6 
Replace single <pre> element with virtual scrolling to improve
rendering performance for large files. Also fixes lint issues with
no-underscore-dangle and curly rules.
@xianshishan
xianshishan force-pushed the perf/virtual-scroll-code-viewer branch from 78b39e3 to f937ad6 Compare July 2, 2026 12:07
@clawsweeper clawsweeper Bot added status: 🛠️ actively grinding The PR author has acted after the latest ClawSweeper review and work remains. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. and removed status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. status: 🛠️ actively grinding The PR author has acted after the latest ClawSweeper review and work remains. labels Jul 2, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jul 3, 2026
Closed
@xianshishan

xianshishan commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

Closing due to merge conflicts with main. Will re-open a clean PR after rebasing.

@xianshishan xianshishan closed this Jul 7, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jul 7, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

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. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: S 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

1 participant

@xianshishan