CONSOLE-3769: Refactor weird getResourceSidebarSamples
#15915
Conversation
|
Skipping CI for Draft Pull Request. |
|
@logonoff: This pull request references CONSOLE-3769 which is a valid jira issue. 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. |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
bot
added
the
do-not-merge/work-in-progress
WalkthroughThe pull request refactors sample utility functionality from a traditional utility module to a React hook-based approach. A new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Rationale: Substantial refactoring involving 378-line hook creation replacing 405-line utility deletion across multiple import paths. Complexity stems from: (1) significant new hook logic integrating samples, YAML generation, extension subscriptions, and i18n; (2) consistent consumption pattern updates across five component files; (3) import path migrations with type-only semantics; (4) translation handling moved into hook internals; (5) verifying no breaking changes for consumers of 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
openshift-ci
bot
added
component/core
|
@logonoff: This pull request references CONSOLE-3769 which is a valid jira issue. 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. |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
@logonoff: This pull request references CONSOLE-3769 which is a valid jira issue. 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 |
openshift-ci
bot
added
px-approved
|
@logonoff: This pull request references CONSOLE-3769 which is a valid jira issue. 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. |
![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
🤖 Fix all issues with AI agents
In @frontend/packages/console-shared/src/hooks/useResourceSidebarSamples.ts:
- Around line 362-369: The type assertion for extensionSamples is using
Exclude<Sample, 'id'> which is incorrect because Exclude removes union members;
replace it with Omit<Sample, 'id'> so the spread of sample.spec is correctly
typed; update the map callback where extensionSamples is defined (reference
symbols: extensionSamples, yamlSamplesData, Sample) to use Omit<Sample, 'id'>
instead of Exclude<Sample, 'id'>.
🧹 Nitpick comments (5)
frontend/packages/console-shared/src/components/formik-fields/CodeEditorField.tsx (1)
50-54: Consider memoizing the FirehoseResult object to avoid unnecessary recalculations.Creating the object inline on every render
{ data: sampleResources, loaded, loadError }is fine ifuseResourceSidebarSamplesdoesn't perform expensive operations. However, if sample filtering becomes costly or ifuseDefaultSamples()internally relies on referential stability, this could cause performance regressions on rapid re-renders.If you observe performance issues, wrap with
useMemo:const yamlSamplesList = useMemo( () => ({ data: sampleResources, loaded, loadError }), [sampleResources, loaded, loadError], ); const { samples, snippets } = useResourceSidebarSamples(model, yamlSamplesList);Not blocking, but worth noting for future-proofing.
frontend/packages/console-shared/src/hooks/useResourceSidebarSamples.ts (4)
68-91: Consider memoizinguseClusterRoleBindingSamplesreturn value.This hook creates a new array on every render. Since the samples are static (only dependent on
t), wrapping the return inuseMemowith[t]as dependency would prevent unnecessary downstream re-renders.♻️ Suggested improvement
const useClusterRoleBindingSamples = (): Sample[] => { const { t } = useTranslation('console-shared'); - return [ + return React.useMemo(() => [ { title: t('Allow reading Nodes in the core API groups (for ClusterRoleBinding)'), // ... rest of samples }, - ]; + ], [t]); };
93-343: Performance concern:useDefaultSamplescreates newImmutableMapon every render.This hook builds a new
ImmutableMapwith nested sample arrays on every invocation. SinceuseResourceSidebarSamplesis called fromEditYAMLInner, any re-render of that component triggers full reconstruction of these samples.For a "chill" review, I'll note this as something to consider optimizing later—especially if users report sluggishness in the YAML editor. Wrapping the entire chain in
useMemowith dependencies on[t, addActions, catalogItemTypes, perspectives, clusterRoleBindingSamples]would be the fix.♻️ Suggested memoization pattern
const useDefaultSamples = () => { const { t } = useTranslation('console-shared'); const [addActions] = useResolvedExtensions<AddAction>(isAddAction); const [catalogItemTypes] = useResolvedExtensions<CatalogItemType>(isCatalogItemType); const [perspectives] = useResolvedExtensions<Perspective>(isPerspective); const clusterRoleBindingSamples = useClusterRoleBindingSamples(); - return ImmutableMap<GroupVersionKind, Sample[]>() + return React.useMemo(() => ImmutableMap<GroupVersionKind, Sample[]>() .setIn( [referenceForModel(BuildConfigModel)], // ... all the sample definitions ) - ); + ), [t, addActions, catalogItemTypes, perspectives, clusterRoleBindingSamples]); };
345-378: Hook output not memoized—may cause unnecessary re-renders.The returned
{ snippets, samples }object and the filtered arrays are new references on every render. Components consuming these (likeCodeEditorSidebar) will re-render even when the underlying data hasn't changed.Consider memoizing the return value, especially since filtering is performed on each call:
♻️ Suggested memoization
export const useResourceSidebarSamples = (kindObj: K8sKind, yamlSamplesList: FirehoseResult) => { const defaultSamples = useDefaultSamples(); - if (!kindObj) { - return { snippets: [], samples: [] }; - } - - const yamlSamplesData = !_.isEmpty(yamlSamplesList) - ? _.filter( - yamlSamplesList.data, - (sample: K8sResourceKind) => - sample.spec.targetResource.apiVersion === apiVersionForModel(kindObj) && - sample.spec.targetResource.kind === kindObj.kind, - ) - : []; - - const existingSamples = defaultSamples.get(referenceForModel(kindObj)) || []; - // ... rest of logic - - return { snippets, samples }; + return React.useMemo(() => { + if (!kindObj) { + return { snippets: [], samples: [] }; + } + + const yamlSamplesData = !_.isEmpty(yamlSamplesList) + ? _.filter( + yamlSamplesList.data, + (sample: K8sResourceKind) => + sample.spec.targetResource.apiVersion === apiVersionForModel(kindObj) && + sample.spec.targetResource.kind === kindObj.kind, + ) + : []; + + const existingSamples = defaultSamples.get(referenceForModel(kindObj)) || []; + const extensionSamples = !_.isEmpty(yamlSamplesData) + ? yamlSamplesData.map((sample: K8sResourceKind) => ({ + id: sample.metadata.uid, + ...(sample.spec as Omit<Sample, 'id'>), + })) + : []; + + const allSamples = [...existingSamples, ...extensionSamples]; + const snippets = allSamples.filter((sample: Sample) => sample.snippet); + const samples = allSamples.filter((sample: Sample) => !sample.snippet); + + return { snippets, samples }; + }, [kindObj, yamlSamplesList, defaultSamples]); };
352-359: Defensive check: handle potential undefinedsample.spec.targetResource.If
yamlSamplesList.datacontains malformed entries withoutspec.targetResource, the filter will throw. Consider adding a guard:♻️ Defensive filter
const yamlSamplesData = !_.isEmpty(yamlSamplesList) ? _.filter( yamlSamplesList.data, (sample: K8sResourceKind) => + sample.spec?.targetResource && sample.spec.targetResource.apiVersion === apiVersionForModel(kindObj) && sample.spec.targetResource.kind === kindObj.kind, ) : [];
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (9)
frontend/packages/console-shared/locales/en/console-shared.jsonfrontend/packages/console-shared/src/components/editor/CodeEditorSidebar.tsxfrontend/packages/console-shared/src/components/formik-fields/CodeEditorField.tsxfrontend/packages/console-shared/src/hooks/useResourceSidebarSamples.tsfrontend/packages/console-shared/src/utils/index.tsfrontend/packages/console-shared/src/utils/sample-utils.tsfrontend/public/components/edit-yaml.tsxfrontend/public/components/sidebars/resource-sidebar-samples.tsxfrontend/public/components/sidebars/resource-sidebar.tsx
💤 Files with no reviewable changes (2)
- frontend/packages/console-shared/src/utils/index.ts
- frontend/packages/console-shared/src/utils/sample-utils.ts
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/packages/console-shared/src/components/formik-fields/CodeEditorField.tsx (1)
frontend/packages/console-shared/src/hooks/useResourceSidebarSamples.ts (1)
useResourceSidebarSamples(345-378)
frontend/public/components/edit-yaml.tsx (2)
frontend/packages/console-shared/src/hooks/useResourceSidebarSamples.ts (1)
useResourceSidebarSamples(345-378)frontend/packages/console-dynamic-plugin-sdk/src/extensions/console-types.ts (1)
AccessReviewResourceAttributes(82-89)
🔇 Additional comments (13)
frontend/public/components/sidebars/resource-sidebar.tsx (1)
15-15: LGTM — Import path consolidation forSampletype.Centralizing the
Sampletype export alongside theuseResourceSidebarSampleshook improves discoverability and keeps related concerns co-located. No runtime impact.frontend/public/components/sidebars/resource-sidebar-samples.tsx (2)
11-11: LGTM — Import path updated forSampletype.Consistent with the broader refactor to source the
Sampletype from the hook module.
96-107: No action needed — the type system already enforces synchronouslazyYaml.The
Sampletype definition (lazyYaml?: () => string) is already synchronous, and all built-in implementations across the codebase follow this contract. No async or Promise-returning implementations exist, so the code change is safe and requires no additional verification from extension providers.frontend/packages/console-shared/locales/en/console-shared.json (1)
298-314: LGTM — Translation key reordering with no functional impact.JSON object key order is not significant for i18n lookups. The translation values remain unchanged.
frontend/packages/console-shared/src/components/editor/CodeEditorSidebar.tsx (1)
3-8: LGTM — Type-only imports are a best practice.Using
import typeforJSONSchema7,K8sKind, andSampleensures these are erased at compile time, aiding bundler tree-shaking and clarifying intent. Good hygiene.frontend/packages/console-shared/src/components/formik-fields/CodeEditorField.tsx (1)
18-18: LGTM — Hook-based approach is idiomatic React.Replacing the imperative
getResourceSidebarSamplesutility withuseResourceSidebarSampleshook aligns with React patterns and makes the data flow more declarative.frontend/public/components/edit-yaml.tsx (6)
15-16: LGTM on the import change.Clean migration from utility function to hook-based approach. The import path is correct and aligns with the new hook module location.
215-217: Hook integration looks correct.The
modelis derived before the hook call, anduseResourceSidebarSamplesis invoked unconditionally at the component's top level—respecting React's rules of hooks. The fallback toprops.modelwhengetModel(props.obj)returns null is a sensible default.One minor note:
modelis recomputed on every render. IfgetModelorprops.objchanges frequently, consider memoizing this value. That said, sincegetModelis already auseCallback, the current approach is acceptable for now.
265-289: LocalobjModelscoping is clean.Good refactor to use a locally-scoped
objModelfor buildingAccessReviewResourceAttributes. This avoids confusion with the outermodelconstant and makes the callback's intent clearer. The dependency array correctly includesgetModel.
469-511: LGTM onupdateYAMLrefactor.Consistent use of local
objModelfork8sCreate/k8sUpdatecalls. The dependency array properly includesgetModel.
544-569: LGTM onvalidaterefactor.Same local
objModelpattern applied consistently. The namespace handling logic correctly referencesobjModel.namespaced.
793-793: Sidebar content now correctly derives from hook output.The
hasSidebarContentcheck and sidebar rendering at lines 793 and 964-972 properly consumesamplesandsnippetsfrom the hook. The conditionalcreate ? samples : []pattern is preserved.frontend/packages/console-shared/src/hooks/useResourceSidebarSamples.ts (1)
33-46: Well-structuredSampletype.Clean type definition with optional fields for flexibility. The
lazyYamlfunction pattern is a nice touch for deferring expensive YAML generation until needed.
| const extensionSamples = !_.isEmpty(yamlSamplesData) | ||
| ? yamlSamplesData.map((sample: K8sResourceKind) => { | ||
| return { | ||
| id: sample.metadata.uid, | ||
| ...(sample.spec as Exclude<Sample, 'id'>), | ||
| }; | ||
| }) | ||
| : []; |
There was a problem hiding this comment.
Type assertion uses incorrect utility type.
Exclude<Sample, 'id'> is semantically incorrect here. Exclude<T, U> operates on union types to remove members, not on object types to omit properties. The correct utility is Omit<Sample, 'id'>.
This won't cause runtime issues since TypeScript erases types at runtime, but it's misleading for anyone reading the code and may cause unexpected type-checking behavior.
🐛 Proposed fix
const extensionSamples = !_.isEmpty(yamlSamplesData)
? yamlSamplesData.map((sample: K8sResourceKind) => {
return {
id: sample.metadata.uid,
- ...(sample.spec as Exclude<Sample, 'id'>),
+ ...(sample.spec as Omit<Sample, 'id'>),
};
})
: [];📝 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.
| const extensionSamples = !_.isEmpty(yamlSamplesData) | |
| ? yamlSamplesData.map((sample: K8sResourceKind) => { | |
| return { | |
| id: sample.metadata.uid, | |
| ...(sample.spec as Exclude<Sample, 'id'>), | |
| }; | |
| }) | |
| : []; | |
| const extensionSamples = !_.isEmpty(yamlSamplesData) | |
| ? yamlSamplesData.map((sample: K8sResourceKind) => { | |
| return { | |
| id: sample.metadata.uid, | |
| ...(sample.spec as Omit<Sample, 'id'>), | |
| }; | |
| }) | |
| : []; |
🤖 Prompt for AI Agents
In @frontend/packages/console-shared/src/hooks/useResourceSidebarSamples.ts
around lines 362 - 369, The type assertion for extensionSamples is using
Exclude<Sample, 'id'> which is incorrect because Exclude removes union members;
replace it with Omit<Sample, 'id'> so the spread of sample.spec is correctly
typed; update the map callback where extensionSamples is defined (reference
symbols: extensionSamples, yamlSamplesData, Sample) to use Omit<Sample, 'id'>
instead of Exclude<Sample, 'id'>.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: logonoff, vojtechszocs 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 |
|
/assign @yapei |
|
/label acknowledge-critical-fixes-only |
openshift-ci
bot
added
the
acknowledge-critical-fixes-only
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
/retest |
|
Given those tests didn't fail in #15904 maybe that was a flake /unhold |
openshift-ci
bot
removed
the
do-not-merge/hold
/verified by @yapei |
openshift-ci-robot
added
the
verified
|
@yapei: This PR has been marked as verified by 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. |
|
/retest |
1 similar comment
|
/retest |
|
@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. |
|
/hold Revision 61f112b was retested 3 times: holding |
openshift-ci
bot
added
the
do-not-merge/hold
4 participants
Cherrypick from #15904
Refactors
getResourceSidebarSamplesintouseResourceSidebarSamplesSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.