Skip to content

fix: avoid stale scroll retry sync#1495

Merged
zombieJ merged 1 commit into
masterfrom
codex/virtual-scroll-hover-source
Jul 7, 2026
Merged

fix: avoid stale scroll retry sync#1495
zombieJ merged 1 commit into
masterfrom
codex/virtual-scroll-hover-source

Conversation

@zombieJ

@zombieJ zombieJ commented Jul 2, 2026

Copy link
Copy Markdown
Member

Fixes ant-design/ant-design#57850

Summary

  • avoid stale delayed scrollLeft retries overriding newer table scroll sync
  • move the retry equality check into the timeout so the latest scroll position can win

Test Plan

  • npm test -- tests/Table.spec.jsx -t "does not retry stale scrollLeft sync"
  • GitHub Actions: test / react component workflow

Summary by CodeRabbit

  • Bug Fixes
    • 优化表格横向滚动同步逻辑,避免在滚动状态已变化时重复写入滚动位置,减少不必要的滚动抖动。
    • 补充滚动同步相关测试,覆盖异步场景下的旧值回写问题,提升滚动行为稳定性。

@vercel

vercel Bot commented Jul 2, 2026

Copy link
Copy Markdown

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

Project Deployment Actions Updated (UTC)
table Ready Ready Preview, Comment Jul 7, 2026 8:35am

@coderabbitai

coderabbitai Bot commented Jul 2, 2026

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ce897afb-a149-4910-84c8-e9e0a70bfd5b

📥 Commits

Reviewing files that changed from the base of the PR and between 204d749 and 98cdb2d.

📒 Files selected for processing (2)
  • src/Table.tsx
  • tests/Table.spec.jsx

Walkthrough

修改了 Table.tsxforceScrollsetTimeout 回调逻辑,在回调执行时新增二次判等,仅当 target.scrollLeft 与期望值仍不一致时才写回,避免重复赋值。同时新增测试用例验证该行为。

Changes

forceScroll 滚动同步修复

Layer / File(s) Summary
回调内二次判等与验证测试
src/Table.tsx, tests/Table.spec.jsx
forceScrollsetTimeout 回调新增对 target.scrollLeft 与期望值的二次比较,仅在不一致时才写回,避免过期回写;新增测试用例 does not retry stale scrollLeft sync 使用假计时器与自定义 scrollLeft 访问器验证该行为。

Estimated code review effort: 1 (Trivial) | ~5 minutes

Sequence Diagram(s)

sequenceDiagram
  participant TableBody as 表体滚动
  participant forceScroll as forceScroll
  participant Timer as setTimeout回调
  participant TableHeader as 表头scrollLeft

  TableBody->>forceScroll: 触发滚动事件
  forceScroll->>Timer: 设置延迟同步
  Timer->>Timer: 比较target.scrollLeft与期望值
  alt 值不一致
    Timer->>TableHeader: 写入scrollLeft
  else 值一致
    Timer-->>TableHeader: 跳过写入
  end
Loading

Suggested reviewers: afc163

Poem

小兔子敲代码,滚轮转得欢,
表头表体不再错乱一片,
二次判等把旧值挡在门外,
快滚慢滚都稳稳当当。🐇✨

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/virtual-scroll-hover-source

Warning

Tools execution failed with the following error:

Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error)


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown

React Doctor found 15 issues in 2 files · 15 warnings · score 74 / 100 (Needs work) · vs master

15 warnings

src/Table.tsx

  • ⚠️ L202 Large component is hard to read and change no-giant-component
  • ⚠️ L210 Event logic handled in an effect no-event-handler
  • ⚠️ L395 Missing effect dependencies exhaustive-deps
  • ⚠️ L395 Missing effect dependencies exhaustive-deps
  • ⚠️ L457 Pure function rebuilt every render prefer-module-scope-pure-function
  • ⚠️ L555 Missing effect dependencies exhaustive-deps
  • ⚠️ L563 Derived value copied into state no-derived-state
  • ⚠️ L565 Missing effect dependencies exhaustive-deps
  • ⚠️ L581 Missing effect dependencies exhaustive-deps
  • ⚠️ L627 Missing effect dependencies exhaustive-deps
  • ⚠️ L659 Duplicate data in server props server-dedup-props
  • ⚠️ L749 JSX element passed as a prop jsx-no-jsx-as-prop
  • ⚠️ L765 JSX element passed as a prop jsx-no-jsx-as-prop
  • ⚠️ L999 Non-component export in component file only-export-components

tests/Table.spec.jsx

  • ⚠️ L1252 Component defined inside a component no-unstable-nested-components

Reviewed by React Doctor for commit 98cdb2d. See inline comments for fixes.

@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown

❌ Deploy failed

PR preview ❌ Failed ❌ Failed
🔗 Preview https://react-component-table-preview-pr-1495.surge.sh (may be unavailable)
📝 Commit98cdb2d
🪵 LogsView logs
📋 Build log (last lines)
npm error     @eslint-community/eslint-utils@"^4.9.1" from @typescript-eslint/utils@8.63.0
npm error     node_modules/@typescript-eslint/utils
npm error       @typescript-eslint/utils@"8.63.0" from @typescript-eslint/eslint-plugin@8.63.0
npm error       node_modules/@typescript-eslint/eslint-plugin
npm error         peerOptional @typescript-eslint/eslint-plugin@"^8.0.0" from eslint-plugin-jest@29.15.4
npm error         node_modules/eslint-plugin-jest
npm error         1 more (typescript-eslint)
npm error       3 more (@typescript-eslint/type-utils, eslint-plugin-jest, typescript-eslint)
npm error     @eslint-community/eslint-utils@"^4.8.0" from eslint@10.6.0
npm error   10 more (@eslint/compat, @eslint/js, ...)
npm error
npm error Could not resolve dependency:
npm error peer eslint@"^3 || ^4 || ^5 || ^6 || ^7 || ^8 || ^9.7" from eslint-plugin-react@7.37.5
npm error node_modules/eslint-plugin-react
npm error   dev eslint-plugin-react@"^7.37.5" from the root project
npm error
npm error Conflicting peer dependency: eslint@9.39.4
npm error node_modules/eslint
npm error   peer eslint@"^3 || ^4 || ^5 || ^6 || ^7 || ^8 || ^9.7" from eslint-plugin-react@7.37.5
npm error   node_modules/eslint-plugin-react
npm error     dev eslint-plugin-react@"^7.37.5" from the root project
npm error
npm error Fix the upstream dependency conflict, or retry
npm error this command with --force or --legacy-peer-deps
npm error to accept an incorrect (and potentially broken) dependency resolution.
npm error
npm error
npm error For a full report see:
npm error /home/runner/.npm/_logs/2026-07-07T08_36_07_051Z-eresolve-report.txt
npm error A complete log of this run can be found in: /home/runner/.npm/_logs/2026-07-07T08_36_07_051Z-debug-0.log

🤖 Powered by surge-preview

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a mechanism to track the active scroll source (header, body, summary, or sticky scrollbar) using hover states (onMouseEnter) to improve scroll synchronization across different table sections. It also includes corresponding unit tests. The review feedback highlights two critical issues: first, a potential bug in getScrollSource where undefined or null values could incorrectly match and resolve the scroll source; second, the lack of a reset mechanism for hoverEle when the mouse leaves the table, which could break scroll synchronization during keyboard navigation.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread src/Table.tsx Outdated
Comment thread src/Table.tsx Outdated
@codecov

codecov Bot commented Jul 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.04%. Comparing base (204d749) to head (98cdb2d).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1495   +/-   ##
=======================================
  Coverage   99.04%   99.04%           
=======================================
  Files          45       45           
  Lines        1362     1362           
  Branches      409      409           
=======================================
  Hits         1349     1349           
  Misses         13       13           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@zombieJ

zombieJ commented Jul 6, 2026

Copy link
Copy Markdown
Member Author

@gemini-code-assist review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a mechanism to track the active scroll source (using hoverEle and mouse enter/leave events) to prevent scroll synchronization conflicts between the table body, header, summary, and sticky scrollbar. The reviewer identified two critical issues with this implementation: first, dragging the scrollbar can cause the mouse to drift over the table body, triggering onMouseEnter and freezing the scrollbar, which can be fixed by checking that no mouse buttons are pressed (e.buttons === 0) before updating the hovered element; second, only 'body' scroll events should be ignored when the body is not the hovered element, as other sources are directly user-triggered and should always be processed.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread src/Table.tsx Outdated
Comment thread src/Table.tsx Outdated
Comment thread src/Table.tsx Outdated
Comment thread src/Table.tsx Outdated
@zombieJ zombieJ force-pushed the codex/virtual-scroll-hover-source branch from 2824b16 to 98cdb2d Compare July 7, 2026 08:34
@zombieJ zombieJ changed the title fix: sync table scroll by hover source fix: avoid stale scroll retry sync Jul 7, 2026
@zombieJ zombieJ marked this pull request as ready for review July 7, 2026 08:45
@zombieJ zombieJ merged commit cbcfd8a into master Jul 7, 2026
13 of 14 checks passed
@zombieJ zombieJ deleted the codex/virtual-scroll-hover-source branch July 7, 2026 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant