MM-69835 Update usage of propTypes and defaultProps - #12
Conversation
| // render, without wrapping the component (which would break ref forwarding, | ||
| // static reads, and — for e.g. `bsRole` — class `defaultProps` merging). | ||
| function patchRenderValidation(Component, propName, allowed) { | ||
| const flag = `__bsValidated_${propName}`; |
There was a problem hiding this comment.
Yeah, this is ugly, but I couldn't come up with a better solution than what was generated by Claude without going through each component and manually adding the propTypes and default values
|
@coderabbitai Review this |
|
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughThe change moves several prop validations from declaration time to render time. It adds shared role-error helpers, updates component warnings, changes bootstrap decorators to validate during rendering, updates tests, and makes generated-file patterns recursive. ChangesRuntime validation warnings
Generated file patterns
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Dropdown
participant RoleErrorHelpers
participant warning
Dropdown->>RoleErrorHelpers: check required and duplicate roles
RoleErrorHelpers-->>Dropdown: return warning message or null
Dropdown->>warning: report invalid role configuration
sequenceDiagram
participant DecoratedComponent
participant applyBsProp
participant React
participant warning
DecoratedComponent->>applyBsProp: apply defaults and validate props
applyBsProp->>React: create wrapped element
applyBsProp->>warning: report invalid allowed values
React-->>DecoratedComponent: render element
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ProgressBar.js (1)
21-43: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard empty children before reading
child.type.React.Children.forEachinvokes its callback fornull,undefined, and booleans. When a truthy children array contains one of these values, Line 36 throws beforewarningruns. Skip empty nodes, checkReact.isValidElement(child)before readingtype, and useString(child.type)for unsupported element types. Preserve the existingelement.typecomparison to retain proxy compatibility.🤖 Prompt for 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. In `@src/ProgressBar.js` around lines 21 - 43, Update getInvalidChildError to skip null, undefined, and boolean children before accessing child.type, and only read type after confirming React.isValidElement(child). Preserve the existing element.type comparison for proxy compatibility, and stringify unsupported element types with String(child.type) when constructing the error message.
🤖 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 `@src/Nav.js`:
- Around line 283-287: The warning condition in Nav should use the effective
navbar value, including the navbar state derived from navbarContext when
propsNavbar is absent, rather than checking only propsNavbar. Update the
condition near warning() and align it with the value used by the
navbar-rendering logic so justified Nav components in navbar context trigger the
unsupported-style warning.
In `@src/utils/bootstrapUtils.js`:
- Around line 49-60: Update the validator setup around validatedRender so its
mutable configuration is stored under flag rather than captured permanently in
the first allowed array. When the existing flag is found, refresh that
configuration’s allowed value before returning, so later bsSizes calls validate
against rebuilt values; add coverage using two bsSizes decorators on a class
component.
- Around line 99-118: Update curry to detect React.memo and React.forwardRef
results as supported component types instead of treating them as plain objects.
Apply bsClass, bsStyles, bsSizes, default props, and validation while preserving
each component’s memoization or ref behavior. Add React 19 coverage for all
three decorators on both exotic component types.
---
Outside diff comments:
In `@src/ProgressBar.js`:
- Around line 21-43: Update getInvalidChildError to skip null, undefined, and
boolean children before accessing child.type, and only read type after
confirming React.isValidElement(child). Preserve the existing element.type
comparison for proxy compatibility, and stringify unsupported element types with
String(child.type) when constructing the error message.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b8b0cd44-f0c8-4776-b284-91acae4f62ec
📒 Files selected for processing (12)
.gitattributessrc/ButtonGroup.jssrc/Dropdown.jssrc/MenuItem.jssrc/Nav.jssrc/ProgressBar.jssrc/utils/PropTypes.jssrc/utils/bootstrapUtils.jstest/DropdownSpec.jstest/ProgressBarSpec.jstest/index.jstest/utils/bootstrapUtilsSpec.js
💤 Files with no reviewable changes (1)
- test/index.js
Summary
React 19 makes two changes to those:
propTypesis no longer checked at runtime, so some tests that relied on the warnings printed by that have been replaced with regular runtime checks usingwarningwhich only function when theNODE_ENVisn't production. Some custom validators had to be rewritten for this.defaultProps.The source code for this adds a lot of those by using some helpers like
bsSizewhich had to be rewritten to adapt to that.Ticket Link
https://mattermost.atlassian.net/browse/MM-69835