-
Notifications
You must be signed in to change notification settings - Fork 669
CONSOLE-5055: AsyncComponent type improvements #16002
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@logonoff: This pull request references CONSOLE-5055 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 story to target the "4.22.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: logonoff 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 |
|
@logonoff: This pull request references CONSOLE-5055 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 story to target the "4.22.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. |
|
@logonoff: This pull request references CONSOLE-5055 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 story to target the "4.22.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. |
|
@logonoff: This pull request references CONSOLE-5055 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 story to target the "4.22.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. |
|
/label px-approved |
📝 WalkthroughWalkthroughThis pull request refactors the AsyncComponent lazy-loading system from class-based to functional component using hooks and Suspense, introducing exponential backoff retry logic with a configurable retry cap. Type signatures across lazy loaders are updated to expect 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/public/components/import-yaml.tsx (1)
16-25:⚠️ Potential issue | 🟡 MinorFix type safety:
initialResource={undefined}violates the required prop definition.
ResourceYAMLEditorPropsdefinesinitialResource: K8sResourceKindas required (not optional). Passingundefinedconflicts with this contract. Increate-yaml.tsx, the pattern correctly uses an empty object{}instead:const initialResource = useMemo(() => { if (!kindObj) { return {}; // Always pass an object } // ... }, []);Either mark
initialResourceas optional inResourceYAMLEditorPropsif undefined should be supported, or pass{}here to align with the type definition and existing usage patterns.
🧹 Nitpick comments (3)
frontend/packages/console-shared/src/components/formik-fields/field-types.ts (1)
166-173: Minor inconsistency between RadioButtonFieldProps and RadioGroupFieldProps value types.
RadioButtonFieldProps.valueacceptsstring | number(line 167), whileRadioGroupOption.valueis constrained tostringonly (line 185). If aRadioButtonFieldis used standalone with a numeric value but the same field is later wrapped in aRadioGroupField, the types won't align.Consider whether both should consistently use
stringorstring | number. If numeric values are genuinely needed for radio buttons,RadioGroupOption.valueshould also supportnumber.frontend/public/components/utils/async.tsx (1)
18-22:sameLoadercomparison is fragile — document limitations and consider alternatives.Comparing functions via
nameandtoString()can produce false positives (different inline functions with identical bodies) or false negatives (minified code where function bodies differ). The current approach handles inline loaders better than referential equality alone, but:
- Arrow functions typically have empty
nameproperties- Minifiers may alter function bodies unpredictably
The comment acknowledges this ("not the best solution"), but consider adding a more robust equality check or documenting that consumers should define loaders outside render cycles when possible.
frontend/public/components/utils/__tests__/async.spec.tsx (1)
98-109: Minor: MissingawaitafteradvanceTimersByTimemay cause timing issues.For consistency with other tests in this file, consider wrapping the timer advancement in
actor awaiting promise resolution:await act(async () => { jest.advanceTimersByTime(1000); });This ensures all pending promise resolutions are flushed before the assertion on line 107.
♻️ Suggested improvement
await waitFor(() => expect(loader).toHaveBeenCalledTimes(1)); - jest.advanceTimersByTime(1000); + await act(async () => { + jest.advanceTimersByTime(1000); + }); expect(loader).toHaveBeenCalledTimes(1);
bed1629 to
be35005
Compare
be35005 to
9cf3f71
Compare
|
@logonoff: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
AsyncComponentto be a functional component that uses React.lazyAsyncComponentnow have proper type checking based on the props of the loaderSummary by CodeRabbit
Bug Fixes
New Features
Tests