Skip to content

fix/last row issue for 1mln row offset#874

Merged
revolist merged 2 commits into
mainfrom
fix/render-offset-logic
May 27, 2026
Merged

fix/last row issue for 1mln row offset#874
revolist merged 2 commits into
mainfrom
fix/render-offset-logic

Conversation

@revolist

@revolist revolist commented May 27, 2026

Copy link
Copy Markdown
Owner

Summary by CodeRabbit

  • Tests

    • Added end-to-end tests covering final-row/column behavior when scrolling to the end of very large grids, verifying rendering, viewport bounds, and focus alignment.
    • Added unit tests verifying scroll-dimension and render-offset calculations for compressed row/column rendering.
  • Bug Fixes

    • Improved scroll coordinate handling to ensure accurate viewport positioning and render offsets in compressed-scroll scenarios.
  • Documentation

    • Enhanced inline documentation for scroll-dimension and viewport coordinate behavior.

Review Change Stack

@coderabbitai

coderabbitai Bot commented May 27, 2026

Copy link
Copy Markdown

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: ce9d5a09-8280-4c0b-aa11-ba5d16a92e6a

📥 Commits

Reviewing files that changed from the base of the PR and between 51536cd and 3596d9c.

📒 Files selected for processing (1)
  • src/store/vp/viewport.helpers.ts

Walkthrough

Replaces clamped viewport coordinate delegation with explicit logical-bound clamping in DimensionProvider; adds JSDoc for scroll dimensions and new unit + e2e tests verifying renderOffset and final-row/column behavior under compressed physical scrolling.

Changes

Compressed-scroll boundary rendering

Layer / File(s) Summary
Core renderOffset calculation fix
src/services/dimension.provider.ts
setViewPortCoordinate now computes renderCoordinate directly via Math.max/Math.min bounded by 0 and scrollDimension.logicalScrollSize, replacing the prior clampViewportCoordinate delegation. Includes comments on logical-to-physical scroll offset behavior and initialization dependencies.
ScrollDimension type documentation
src/services/scroll.dimension.helpers.ts
JSDoc comments added to dimensional fields (contentSize, clientSize, viewportSize, physicalContentSize, logicalScrollSize) clarifying their semantics in compressed-scroll calculations.
Unit tests for renderOffset boundary storage
test/scroll.dimension.spec.ts
Three new test cases verify DimensionProvider stores correct renderOffset at scroll boundaries: standard compressed row rendering, custom-sized tail row, and compressed column rendering.
E2E tests for final compressed scroll viewport behavior
e2e/virtualization.spec.ts
Two new Playwright tests verify final row and column cells remain within viewport bounds when scrolling to dataset ends; validate physical scroll compression and focus alignment with large row/column counts and custom dimension definitions.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Possibly related PRs

  • revolist/revogrid#854: Both PRs modify DimensionProvider.setViewPortCoordinate to compute renderOffset using logical scroll coordinates instead of the previous clamping approach.
  • revolist/revogrid#831: Extends virtualization E2E coverage; the new final-compressed tests build on infrastructure introduced there.

Suggested labels

bug

Suggested reviewers

  • m2a2x

Poem

🐰 I hopped down the scroll to the very last line,
Where pixels compress but logical edges shine.
Bounds kept in check from zero to the end,
The final cell stays — no offset to mend.
Tests clap their paws; the viewport aligns.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title references a specific performance/scale issue (1mln row offset) and indicates a fix, but lacks clarity about the core change—which involves correcting renderOffset logic for compressed scroll positioning at dataset boundaries. Consider a more descriptive title such as 'Fix renderOffset calculation for compressed scroll at dataset boundaries' to better clarify the actual technical change.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ 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 fix/render-offset-logic

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint skipped: no ESLint configuration detected in root package.json. To enable, add eslint to devDependencies.


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 and usage tips.

@revolist revolist merged commit 70bb45e into main May 27, 2026
4 of 5 checks passed
@revolist revolist deleted the fix/render-offset-logic branch May 27, 2026 12:35
@sonarqubecloud

Copy link
Copy Markdown

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.

2 participants