BA-3305-fe-clean-up-console-errors-in-base-app-frontend-and-template-projects#391
Conversation
lucas-caribe
requested review from
anicioalexandre,
matheusysd and
priscilladeroode
as code owners
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAcross 24 ranges in 20 files, MUI styled component declarations are migrated to the ChangesshouldForwardProp Rollout and Related Fixes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
packages/components/modules/profiles/web/ProfileSettingsComponent/index.tsxESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. packages/components/modules/profiles/web/ProfileSettingsComponent/styled.tsxESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. packages/design-system/components/web/buttons/IconButton/styled.tsxESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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
`@packages/components/modules/navigations/web/__shared__/NavSectionHorizontal/NavList/NavItem/styled.tsx`:
- Around line 125-138: In the StyledLink component, add 'disabled' to the
shouldForwardProp filter condition alongside 'hasTabLayout' and 'active' to
prevent the custom disabled prop from being forwarded to the DOM Link element.
Additionally, fix the invalid CSS value for pointerEvents on line 128 by
changing it from 'cursor' to 'auto', which is a valid CSS value for the
pointer-events property when the component is not disabled.
In
`@packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/index.tsx`:
- Around line 53-59: Remove the unnecessary key props from the FlagImage,
CountryTitle, and Typography elements within the MenuItem component. These
elements are static siblings not part of a dynamic list, so React keys are not
required for them. The MenuItem itself already has the appropriate key for list
reconciliation based on defaultCountry.iso2, making the individual key props on
the child elements redundant and serving no functional purpose.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b5d4e018-b9bb-48e6-8944-a7eafb91c539
📒 Files selected for processing (17)
packages/components/modules/__shared__/web/ActionsOverlay/styled.tsxpackages/components/modules/comments/web/CommentItem/styled.tsxpackages/components/modules/messages/web/AllChatRoomsList/ChatRoomItem/styled.tsxpackages/components/modules/messages/web/ChatRoom/ChatRoomHeader/styled.tsxpackages/components/modules/messages/web/ChatRoomsComponent/styled.tsxpackages/components/modules/messages/web/MessagesList/MessagesGroup/UserMessage/MessageItem/styled.tsxpackages/components/modules/navigations/web/Header/styled.tsxpackages/components/modules/navigations/web/__shared__/NavSectionHorizontal/NavList/NavItem/styled.tsxpackages/components/modules/profiles/web/ProfileComponent/BlockButtonWithDialog/styled.tsxpackages/design-system/components/web/avatars/AvatarWithPlaceholder/styled.tsxpackages/design-system/components/web/avatars/CircledAvatar/styled.tsxpackages/design-system/components/web/avatars/ClickableAvatar/styled.tsxpackages/design-system/components/web/buttons/FileUploadButton/index.tsxpackages/design-system/components/web/dialogs/Dialog/styled.tsxpackages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/index.tsxpackages/design-system/components/web/inputs/TextareaField/styled.tsxpackages/design-system/providers/web/SnackbarProvider/ProgressBar/styled.tsx
| export const StyledLink = styled(Link, { | ||
| shouldForwardProp: (prop) => prop !== 'hasTabLayout' && prop !== 'active', | ||
| })<StyledLinkProps>(({ theme, disabled, hasTabLayout, active }) => ({ | ||
| pointerEvents: disabled ? 'none' : 'cursor', | ||
| color: 'inherit', | ||
| textDecoration: 'none', | ||
| display: 'flex', | ||
| alignItems: 'center', | ||
| borderBottom: hasTabLayout && active ? `2px solid ${theme.palette.grey[800]}` : 'none', | ||
| '&:hover': { | ||
| backgroundColor: hasTabLayout ? theme.palette.action.hover : 'transparent', | ||
| }, | ||
| variants: [], | ||
| })) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify valid CSS pointer-events values in MDN or W3C specs
echo "Valid CSS pointer-events values per MDN:"
echo "- auto"
echo "- none"
echo "- visiblePainted, visibleFill, visibleStroke, visible (SVG only)"
echo "- painted, fill, stroke, all (SVG only)"
echo "- inherit, initial, revert, unset"
echo ""
echo "'cursor' is NOT a valid value for pointer-events."Repository: silverlogic/baseapp-frontend
Length of output: 317
🏁 Script executed:
cat -n packages/components/modules/navigations/web/__shared__/NavSectionHorizontal/NavList/NavItem/styled.tsx | sed -n '120,145p'Repository: silverlogic/baseapp-frontend
Length of output: 827
🏁 Script executed:
# Search for any other StyledLink components or similar patterns to understand the context
rg "shouldForwardProp" packages/components/modules/navigations/web/ -A 3Repository: silverlogic/baseapp-frontend
Length of output: 3407
🏁 Script executed:
# Check if disabled prop is used elsewhere in this file
rg "disabled" packages/components/modules/navigations/web/__shared__/NavSectionHorizontal/NavList/NavItem/styled.tsx -nRepository: silverlogic/baseapp-frontend
Length of output: 242
Add disabled to shouldForwardProp filter and fix invalid CSS value for pointerEvents.
Two issues in StyledLink (lines 125-138):
-
Missing
shouldForwardPropfilter fordisabled: Thedisabledprop is destructured and used in the styling function (line 128), but it is not filtered in theshouldForwardPropconfiguration. Next.jsLinkdoes not have a nativedisabledprop, so this custom prop will be forwarded to the DOM, causing a console warning—contradicting this PR's objective to eliminate console errors. -
Invalid CSS value: Line 128 sets
pointerEvents: 'cursor', but'cursor'is not a valid CSS value for thepointer-eventsproperty. Valid values are'auto','none','inherit', etc. This should be'auto'.
🐛 Proposed fix
export const StyledLink = styled(Link, {
- shouldForwardProp: (prop) => prop !== 'hasTabLayout' && prop !== 'active',
+ shouldForwardProp: (prop) => prop !== 'hasTabLayout' && prop !== 'active' && prop !== 'disabled',
})<StyledLinkProps>(({ theme, disabled, hasTabLayout, active }) => ({
- pointerEvents: disabled ? 'none' : 'cursor',
+ pointerEvents: disabled ? 'none' : 'auto',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const StyledLink = styled(Link, { | |
| shouldForwardProp: (prop) => prop !== 'hasTabLayout' && prop !== 'active', | |
| })<StyledLinkProps>(({ theme, disabled, hasTabLayout, active }) => ({ | |
| pointerEvents: disabled ? 'none' : 'cursor', | |
| color: 'inherit', | |
| textDecoration: 'none', | |
| display: 'flex', | |
| alignItems: 'center', | |
| borderBottom: hasTabLayout && active ? `2px solid ${theme.palette.grey[800]}` : 'none', | |
| '&:hover': { | |
| backgroundColor: hasTabLayout ? theme.palette.action.hover : 'transparent', | |
| }, | |
| variants: [], | |
| })) | |
| export const StyledLink = styled(Link, { | |
| shouldForwardProp: (prop) => prop !== 'hasTabLayout' && prop !== 'active' && prop !== 'disabled', | |
| })<StyledLinkProps>(({ theme, disabled, hasTabLayout, active }) => ({ | |
| pointerEvents: disabled ? 'none' : 'auto', | |
| color: 'inherit', | |
| textDecoration: 'none', | |
| display: 'flex', | |
| alignItems: 'center', | |
| borderBottom: hasTabLayout && active ? `2px solid ${theme.palette.grey[800]}` : 'none', | |
| '&:hover': { | |
| backgroundColor: hasTabLayout ? theme.palette.action.hover : 'transparent', | |
| }, | |
| variants: [], | |
| })) |
🤖 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
`@packages/components/modules/navigations/web/__shared__/NavSectionHorizontal/NavList/NavItem/styled.tsx`
around lines 125 - 138, In the StyledLink component, add 'disabled' to the
shouldForwardProp filter condition alongside 'hasTabLayout' and 'active' to
prevent the custom disabled prop from being forwarded to the DOM Link element.
Additionally, fix the invalid CSS value for pointerEvents on line 128 by
changing it from 'cursor' to 'auto', which is a valid CSS value for the
pointer-events property when the component is not disabled.
| <FlagImage key="flag" iso2={defaultCountry.iso2} /> | ||
| <CountryTitle key="name" {...countryNameProps}> | ||
| {defaultCountry.name} | ||
| </CountryTitle> | ||
| <Typography key="dialCode" {...countryDialCodeProps}> | ||
| +{defaultCountry.dialCode} | ||
| </Typography> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for key props on static children within MenuItem or similar list item components
# Search for MenuItem with multiple children that have key props
rg -n -A 10 'MenuItem.*key=' --type=tsx --type=ts | rg 'key="'Repository: silverlogic/baseapp-frontend
Length of output: 100
🏁 Script executed:
cat packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/index.tsx | head -80Repository: silverlogic/baseapp-frontend
Length of output: 1793
🏁 Script executed:
rg -n -B 5 -A 15 'FlagImage.*key="flag"' --type-listRepository: silverlogic/baseapp-frontend
Length of output: 462
🏁 Script executed:
rg -n -B 5 -A 15 'FlagImage.*key' packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/Repository: silverlogic/baseapp-frontend
Length of output: 2275
🏁 Script executed:
rg -n 'MenuItem' packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/ -A 10 | head -30Repository: silverlogic/baseapp-frontend
Length of output: 3080
🏁 Script executed:
# Search for similar patterns: elements with key props inside MenuItem or similar components
rg 'key="[a-z]+"' packages/design-system/components/web/ -B 3 -A 1 | grep -E '(MenuItem|key=)' | head -40Repository: silverlogic/baseapp-frontend
Length of output: 546
🏁 Script executed:
# Check if there are other similar patterns in the codebase with multiple children having keys
rg -n '<[A-Z]\w+.*key=' packages/design-system/components/ | grep -v 'defaultCountries.map\|\.map\|for\|forEach' | head -20Repository: silverlogic/baseapp-frontend
Length of output: 1074
Remove unnecessary key props from static siblings.
The FlagImage, CountryTitle, and Typography elements are static siblings within MenuItem, not part of an array. React keys are required only for elements in dynamic lists. The MenuItem itself already has the proper key for list reconciliation (key={defaultCountry.iso2}). The key props on these static children should be removed as they serve no functional purpose.
🤖 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
`@packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/index.tsx`
around lines 53 - 59, Remove the unnecessary key props from the FlagImage,
CountryTitle, and Typography elements within the MenuItem component. These
elements are static siblings not part of a dynamic list, so React keys are not
required for them. The MenuItem itself already has the appropriate key for list
reconciliation based on defaultCountry.iso2, making the individual key props on
the child elements redundant and serving no functional purpose.
… feature/BA-3305-fe-clean-up-console-errors-in-base-app-frontend-and-template-projects
anicioalexandre
left a comment
anicioalexandre
left a comment
There was a problem hiding this comment.
friendly reminder to version these changes
… feature/BA-3305-fe-clean-up-console-errors-in-base-app-frontend-and-template-projects
|
2 participants
Acceptance Criteria
User Story
The BaseApp frontend and frontend template projects currently produce avoidable warnings and errors in the browser console, which make debugging harder and reduce overall code quality. These include common React issues such as invalid DOM props and missing keys in lists.
This effort focuses on identifying and resolving all simple and low-effort console errors, while documenting and creating follow-up work for any issues that require deeper investigation.
Acceptance Criteria
This story is done when:
All easily fixable console warnings and errors (e.g., invalid DOM props, missing key props in lists) are resolved in both baseapp-frontend and baseapp-frontend-template, so that the console is clean during normal usage.
The applications can be used in development mode without recurring console noise, so that developers can focus on meaningful issues.
Any console errors or warnings identified during the process that are not trivial to fix are documented, so that they are not lost or ignored.
Follow-up stories are created for non-trivial issues discovered during the cleanup, so that they can be properly prioritized and addressed later.
The cleanup does not introduce regressions in UI behavior or functionality, so that improvements in code quality do not negatively impact the user experience.
Approvd
https://app.approvd.io/silverlogic/BA/stories/46077
Summary by CodeRabbit
Release Notes
Bug Fixes
Accessibility
Refactor
Chores