fix(grid): fix user passed scrollY value is null #3658
Conversation
WalkthroughAdds a null-safe access when computing rowHeight in the table component, changing scrollY.rHeight to scrollY?.rHeight to avoid runtime errors when scrollY is undefined. No API/signature changes. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/vue/src/grid/src/table/src/table.ts (1)
713-715: Prefer nullish coalescing (??) over logical OR (||) for numeric fallbacksUsing
||will treat0as falsy and fall back to defaults. If0is ever a valid (even if unlikely) value forrHeight, it would be unintentionally overridden.??expresses the intent of “use default only when null/undefined.”- optimizeOpts.value?.scrollY?.rHeight || - GlobalConfig.rowHeight[tinyTheme.value]?.[vSize.value || 'default'] || - 40 + optimizeOpts.value?.scrollY?.rHeight ?? + GlobalConfig.rowHeight[tinyTheme.value]?.[vSize.value || 'default'] ?? + 40
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/vue/src/grid/src/table/src/table.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/vue/src/grid/src/table/src/table.ts (2)
packages/vue/src/grid-toolbar/src/index.ts (1)
vSize(313-315)packages/vue/src/grid/src/grid/grid.ts (1)
vSize(181-183)
🔇 Additional comments (1)
packages/vue/src/grid/src/table/src/table.ts (1)
713-715: Null-safe access to scrollY prevents TypeError — LGTMChanging
scrollY.rHeighttoscrollY?.rHeightcorrectly handles the case when users passscrollY: null, avoiding runtime crashes during row height computation. Fallback order remains intact.
| const rowHeight = hooks.computed( | ||
| () => | ||
| optimizeOpts.value?.scrollY.rHeight || GlobalConfig.rowHeight[tinyTheme.value]?.[vSize.value || 'default'] || 40 | ||
| optimizeOpts.value?.scrollY?.rHeight || | ||
| GlobalConfig.rowHeight[tinyTheme.value]?.[vSize.value || 'default'] || | ||
| 40 | ||
| ) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add a regression test and scan for other unguarded scrollY dereferences
Given this bug class, add a test that mounts the grid with optimization: { scrollY: null } and asserts no error and that rowHeight resolves to theme/default values. Also, scan the repo for scrollY. accesses lacking optional chaining to prevent similar crashes elsewhere.
I can draft the unit test and open a follow-up PR to harden other call sites. To quickly scan the codebase, run:
🏁 Script executed:
#!/bin/bash
# Find potentially unsafe scrollY dereferences
rg -nP -C2 --type=ts '\bscrollY\.[A-Za-z_]\w*' -g '!**/dist/**' -g '!**/node_modules/**'
# Also check optimizeOpts-based paths
rg -nP -C2 --type=ts 'optimizeOpts(?:\.value)?\.scrollY\.[A-Za-z_]\w*' -g '!**/dist/**' -g '!**/node_modules/**'Length of output: 2551
Guard all scrollY accesses and add a regression test
I’ve confirmed several unguarded scrollY dereferences beyond the original snippet. To prevent crashes when optimization.scrollY is null or undefined, please:
- In
packages/vue/src/grid/src/table/src/utils/computeScrollLoad.ts
• Line 37: replacescrollY.vSizewithscrollY?.vSize
• Line 41: guard!scrollY.oSizeas!scrollY?.oSize
• Line 46: guardif (!scrollY.rSize)asif (!scrollY?.rSize)
• (And similarly for any other directscrollY.*usages) - In
packages/vue/src/grid/src/table/src/methods.tsline 293
• ChangescrollY && scrollY.gt > 0toscrollY?.gt > 0
After adding these guards (and providing sensible defaults where needed), please write a regression test that mounts the grid with:
optimization: { scrollY: null }and asserts:
- No runtime error is thrown.
rowHeightfalls back to the theme’s default (e.g.GlobalConfig.rowHeight[tinyTheme.value]['default']or40).
This will catch any remaining unguarded access and prevent similar crashes in the future.
🤖 Prompt for AI Agents
In packages/vue/src/grid/src/table/src/table.ts around lines 711-716 and in
packages/vue/src/grid/src/table/src/utils/computeScrollLoad.ts (line ~37, 41, 46
and any other direct scrollY.* usages) and
packages/vue/src/grid/src/table/src/methods.ts line 293, guard all dereferences
of optimization.scrollY with optional chaining (e.g. replace scrollY.vSize with
scrollY?.vSize, !scrollY.oSize with !scrollY?.oSize, if (!scrollY.rSize) with if
(!scrollY?.rSize), and change scrollY && scrollY.gt > 0 to scrollY?.gt > 0), and
provide sensible defaults where needed so code still falls back to GlobalConfig
rowHeight or 40; then add a regression test that mounts the grid with
optimization: { scrollY: null } and asserts no runtime error and rowHeight falls
back to the theme default.
PR
修复用户主动将scrollY 传值为Null时的报错
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit