fix(rate-widget): cap maxCount to protect property pane, layout sizing, and render (#14833) - #42081
Open
buyua9 wants to merge 2 commits into
Open
fix(rate-widget): cap maxCount to protect property pane, layout sizing, and render (#14833)#42081buyua9 wants to merge 2 commits into
buyua9 wants to merge 2 commits into
Conversation
…g, and render (appsmithorg#14833) The Rating widget's maxCount property had no upper bound. A value like 1_000_000 (typed directly, or supplied by a data binding on page load) crashed the browser with 'Aw, snap!' because the widget rendered one DOM star node per unit of maxCount and asked the layout engine to honour a minWidth proportional to maxCount. The previous attempt (PR appsmithorg#41897, closed for inactivity on 2026-06-28) capped the property pane and the render path but left the two auto-layout sizing surfaces untouched. With a binding returning >= 1_000 the layout engine still produced a minWidth of >= 25_000 px and the browser could crash via the layout box, not via the star render. This commit caps the value uniformly across all three surfaces: * property pane: ValidationTypes.NUMBER params now include min: 1 and max: 100, with an updated helpText that names the allowed range; * getAutoLayoutConfig: widgetSize[0].configuration now routes props.maxCount through getSafeMaxCount before computing minWidth; * getAnvilConfig: widgetSize now routes props.maxCount through getSafeMaxCount before computing minWidth; * getWidgetView: the value forwarded to <RateComponent> is the clamped safeMaxCount, so the render path is defended even if a binding changes maxCount after the property-pane validation pass. The cap and a getSafeMaxCount() helper live in RateWidget/constants.ts so every consumer of maxCount goes through the same source of truth. Defaults (DEFAULT_MAX_RATE_COUNT = 5) preserve today's behaviour for the case where maxCount is missing, non-numeric, or non-positive. A Jest unit test (constants.test.ts) pins the cap so a future regression on any of the three surfaces cannot pass CI silently. Closes appsmithorg#14833
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe Rating widget adds shared ChangesRateWidget max-count safety
Estimated code review effort: 3 (Moderate) | ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/client/src/widgets/RateWidget/constants.ts`:
- Around line 40-52: Update getSafeMaxCount in
app/client/src/widgets/RateWidget/constants.ts:40-52 to parse string inputs with
Number() and reject any parsed value that is not an integer, while preserving
the existing positive-range and maximum clamping behavior. Add assertions in
app/client/src/widgets/RateWidget/constants.test.ts:52-56 confirming both 1.5
and "1.5" return DEFAULT_MAX_RATE_COUNT.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 79312e88-a1af-449b-bd46-dafefdb99560
📒 Files selected for processing (3)
app/client/src/widgets/RateWidget/constants.test.tsapp/client/src/widgets/RateWidget/constants.tsapp/client/src/widgets/RateWidget/widget/index.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
maxCountto the safe 1–100 rangeCloses #14833. Related: #15586. PR #41897 was stale-closed after covering only the property-pane/render surfaces; this patch also closes the layout-sizing gap.
Validation
git diff --checkSummary by CodeRabbit