Skip to content

fix(knowledge): preserve scroll position when toggling tokenizer in chunk viewer#4643

Open
waleedlatif1 wants to merge 2 commits into
stagingfrom
waleedlatif1/kb-tokenizer-scroll
Open

fix(knowledge): preserve scroll position when toggling tokenizer in chunk viewer#4643
waleedlatif1 wants to merge 2 commits into
stagingfrom
waleedlatif1/kb-tokenizer-scroll

Conversation

@waleedlatif1
Copy link
Copy Markdown
Collaborator

Summary

  • Toggling the Tokenizer switch in the KB chunk viewer no longer scrolls the content back to the top
  • Captures scrollTop from the active element (textarea or tokenized div) before the toggle, then restores it on the newly-mounted element via useLayoutEffect

Type of Change

  • Bug fix

Testing

Tested manually

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@vercel
Copy link
Copy Markdown

vercel Bot commented May 17, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped May 17, 2026 6:19pm

Request Review

@cursor
Copy link
Copy Markdown

cursor Bot commented May 17, 2026

PR Summary

Low Risk
Low risk UI-only behavior change that adds scroll position capture/restore when switching between textarea and tokenized rendering.

Overview
Fixes a UX bug in chunk-editor.tsx where toggling Tokenizer reset the viewer/editor scroll position to the top.

The toggle now captures the active element’s scrollTop (textarea vs tokenized div) and restores it after the new view mounts via useLayoutEffect, using refs to avoid running on initial render.

Reviewed by Cursor Bugbot for commit 0e1e331. Configure here.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 17, 2026

Greptile Summary

This PR fixes a UX bug where toggling the Tokenizer switch in the Knowledge Base chunk viewer would reset the scroll position to the top. The fix captures scrollTop from the active element (textarea or tokenized div) synchronously before the toggle, then restores it on the newly-mounted element via useLayoutEffect.

  • Adds tokenizedScrollRef and preservedScrollTopRef to track and restore scroll state across the two views.
  • Introduces handleTokenizerChange to capture scroll before calling setTokenizerOn, and a hasToggledTokenizerRef guard to prevent the useLayoutEffect from zeroing scroll on initial mount.
  • useLayoutEffect runs synchronously after the DOM mutation but before paint, ensuring the user never sees a scroll flicker when switching views.

Confidence Score: 5/5

Safe to merge — the change is narrowly scoped to scroll-position bookkeeping in a single UI component with no side effects on data, saving, or network calls.

The fix is small and self-contained: two new refs, one new callback, and one layout effect. The hasToggledTokenizerRef guard correctly prevents the effect from running on initial mount, the scroll capture happens synchronously before state update, and useLayoutEffect restores it before the browser paints, so there is no visible flicker. No data paths, API calls, or shared state are touched.

No files require special attention.

Important Files Changed

Filename Overview
apps/sim/app/workspace/[workspaceId]/knowledge/[id]/[documentId]/components/chunk-editor/chunk-editor.tsx Adds scroll-position preservation across tokenizer toggle using useLayoutEffect and a hasToggledTokenizerRef guard; logic is correct and the guard addresses the previously raised initial-mount concern.

Sequence Diagram

sequenceDiagram
    participant User
    participant TokenizerToggle
    participant handleTokenizerChange
    participant preservedScrollTopRef
    participant React State
    participant useLayoutEffect
    participant DOM

    User->>TokenizerToggle: clicks toggle
    TokenizerToggle->>handleTokenizerChange: onCheckedChange(newValue)
    handleTokenizerChange->>DOM: read scrollTop from active element
    handleTokenizerChange->>preservedScrollTopRef: store scrollTop
    handleTokenizerChange->>React State: setTokenizerOn(newValue)
    React State->>DOM: unmount old element, mount new element
    DOM->>useLayoutEffect: fires synchronously (before paint)
    useLayoutEffect->>preservedScrollTopRef: read stored scrollTop
    useLayoutEffect->>DOM: set scrollTop on newly-mounted element
    DOM->>User: renders at preserved scroll position
Loading

Reviews (2): Last reviewed commit: "fix(knowledge): skip scroll restore on i..." | Re-trigger Greptile

@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@cursor review

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 0e1e331. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant