OU-1384: Remove some of the set-state-in-effect overrides#1006
OU-1384: Remove some of the set-state-in-effect overrides#1006PeterYurkovich wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@PeterYurkovich: This pull request references OU-1384 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: PeterYurkovich The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
WalkthroughThree components replace ChangesChart Height Computation Refactor
Autocomplete Suggestions Refactor
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (1)
web/src/components/console/public/components/autocomplete.tsx (1)
151-151: ⚡ Quick winUse a named export for
AutocompleteInput(Line 151).Please replace the default export with a named export to match the repository TSX component convention.
Suggested diff
-const AutocompleteInput: FC<AutocompleteInputProps> = (props) => { +export const AutocompleteInput: FC<AutocompleteInputProps> = (props) => { ... }; - -export default AutocompleteInput;As per coding guidelines, “Always use named exports for React components instead of default exports.”
🤖 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 `@web/src/components/console/public/components/autocomplete.tsx` at line 151, The AutocompleteInput component is currently exported as a default export on line 151. Replace the default export statement with a named export to align with the repository's TSX component convention. Change the export statement from `export default AutocompleteInput;` to a named export format `export { AutocompleteInput };`. Additionally, update all import statements throughout the codebase that currently import AutocompleteInput as a default import to use the named import syntax instead.Source: Coding guidelines
🤖 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 `@web/src/components/Incidents/AlertsChart/AlertsChart.tsx`:
- Line 48: Both chart components violate the TSX component contract by using
inline prop typing and default exports. Fix both files to follow the consistent
pattern: (1) In web/src/components/Incidents/AlertsChart/AlertsChart.tsx at line
48, replace the inline props destructuring { theme: 'light' | 'dark' } with a
named AlertsChartProps type and declare the component as const AlertsChart:
FC<AlertsChartProps>; then at line 280, replace the default export with a named
export. (2) In web/src/components/Incidents/IncidentsChart/IncidentsChart.tsx at
lines 59-75, replace the inline props typing with a named IncidentsChartProps
type and declare the component as const IncidentsChart: FC<IncidentsChartProps>;
then at line 299, replace the default export (including the memo wrapper) with a
named export form using memo.
---
Nitpick comments:
In `@web/src/components/console/public/components/autocomplete.tsx`:
- Line 151: The AutocompleteInput component is currently exported as a default
export on line 151. Replace the default export statement with a named export to
align with the repository's TSX component convention. Change the export
statement from `export default AutocompleteInput;` to a named export format
`export { AutocompleteInput };`. Additionally, update all import statements
throughout the codebase that currently import AutocompleteInput as a default
import to use the named import syntax instead.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: dfdedcd6-5559-4da6-b1b2-93b775cbe6a6
📒 Files selected for processing (3)
web/src/components/Incidents/AlertsChart/AlertsChart.tsxweb/src/components/Incidents/IncidentsChart/IncidentsChart.tsxweb/src/components/console/public/components/autocomplete.tsx
| export const DEFAULT_CHART_CONTAINER_HEIGHT = 300; | ||
| export const DEFAULT_CHART_HEIGHT = 250; | ||
|
|
||
| const AlertsChart = ({ theme }: { theme: 'light' | 'dark' }) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Both modified incident chart components violate the same TSX component contract (default export + non-FC declaration). Apply one consistent component pattern across both files: define a Props type, declare const Component: FC<Props>, and use named exports.
web/src/components/Incidents/AlertsChart/AlertsChart.tsx#L48-L48: replace inline props typing withAlertsChartProps+FC<AlertsChartProps>.web/src/components/Incidents/AlertsChart/AlertsChart.tsx#L280-L280: replace default export with named export.web/src/components/Incidents/IncidentsChart/IncidentsChart.tsx#L59-L75: replace inline props typing withIncidentsChartProps+FC<IncidentsChartProps>.web/src/components/Incidents/IncidentsChart/IncidentsChart.tsx#L299-L299: replaceexport default memo(IncidentsChart)with named export form (e.g., namedmemoexport).
As per coding guidelines, “Use functional components with explicit type annotations and FC type for React components” and “Always use named exports for React components instead of default exports.”
📍 Affects 2 files
web/src/components/Incidents/AlertsChart/AlertsChart.tsx#L48-L48(this comment)web/src/components/Incidents/AlertsChart/AlertsChart.tsx#L280-L280web/src/components/Incidents/IncidentsChart/IncidentsChart.tsx#L59-L75web/src/components/Incidents/IncidentsChart/IncidentsChart.tsx#L299-L299
🤖 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 `@web/src/components/Incidents/AlertsChart/AlertsChart.tsx` at line 48, Both
chart components violate the TSX component contract by using inline prop typing
and default exports. Fix both files to follow the consistent pattern: (1) In
web/src/components/Incidents/AlertsChart/AlertsChart.tsx at line 48, replace the
inline props destructuring { theme: 'light' | 'dark' } with a named
AlertsChartProps type and declare the component as const AlertsChart:
FC<AlertsChartProps>; then at line 280, replace the default export with a named
export. (2) In web/src/components/Incidents/IncidentsChart/IncidentsChart.tsx at
lines 59-75, replace the inline props typing with a named IncidentsChartProps
type and declare the component as const IncidentsChart: FC<IncidentsChartProps>;
then at line 299, replace the default export (including the memo wrapper) with a
named export form using memo.
Source: Coding guidelines
2 participants
These lint rule updates provide refactors of react code to remove the set-state-in-effect overrides. You can read more about this rule here: https://react.dev/reference/eslint-plugin-react-hooks/lints/set-state-in-effect
These fixes were done by eliminating excess state tracking and returning values directly from useMemo's rather than triggering useEffects off of changes to the useMemo value
Summary by CodeRabbit