CONSOLE-5018: Refactor UNSAFE_ class components to FC#16062
CONSOLE-5018: Refactor UNSAFE_ class components to FC#16062logonoff wants to merge 4 commits intoopenshift:mainfrom
UNSAFE_ class components to FC#16062Conversation
Can't use both renderWithProviders and a router provider
dead code
openshift-ci-robot
added
the
jira/valid-reference
|
@logonoff: This pull request references CONSOLE-5018 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 |
openshift-ci
bot
added
component/helm
Replace the class component with hooks (useState, useEffect, useMemo, useCallback, useRef) and replace withTranslation HOC with useTranslation hook. Derived data (resourceMap, items, displayTitle) is now computed via useMemo instead of stored in state. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace class component with hooks, merge StorageClassDropdownInner and StorageClassDropdown into a single component, and replace withTranslation HOC with useTranslation hook. Derived data (items, defaultClass, displayTitle) is now computed via useMemo. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
logonoff
force-pushed
the
CONSOLE-5018-status
branch
from
4f39335 to
4437283
Compare
📝 WalkthroughWalkthroughThis pull request modernizes the OpenShift Console frontend through multiple refactoring efforts. Class-based components are converted to functional components using React hooks (ResourceDropdown, StorageClassDropdown), type definitions are consolidated to reference shared prop interfaces, and obsolete health monitoring components and their status utilities are removed entirely. Test infrastructure is updated to use consistent provider-based rendering. The onChange callback signature in ResourceDropdown is updated to enforce stricter typing. These changes improve maintainability and reduce code duplication across the console codebase. 🚥 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 |
|
@logonoff: This pull request references CONSOLE-5018 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
frontend/packages/helm-plugin/src/components/details-page/__tests__/HelmReleaseDetails.spec.tsx (1)
66-103: Optional: extract a local render helper to reduce repetition.This file repeats the same render invocation in multiple tests; a tiny helper would reduce noise and make intent clearer.
♻️ Suggested refactor
describe('HelmReleaseDetails', () => { + const renderLoadedDetails = () => + renderWithProviders(<LoadedHelmReleaseDetails {...loadedHelmReleaseDetailsProps} />); + beforeEach(() => { @@ it('should show the loading box if helm release data is not loaded', () => { loadedHelmReleaseDetailsProps.helmRelease.loaded = false; - renderWithProviders(<LoadedHelmReleaseDetails {...loadedHelmReleaseDetailsProps} />); + renderLoadedDetails(); @@ it('should show an error if helm release data could not be loaded', () => { loadedHelmReleaseDetailsProps.helmRelease.loadError = new Error('An error!'); - renderWithProviders(<LoadedHelmReleaseDetails {...loadedHelmReleaseDetailsProps} />); + renderLoadedDetails(); @@ it('should show the loading box if secret is not loaded', () => { @@ - renderWithProviders(<LoadedHelmReleaseDetails {...loadedHelmReleaseDetailsProps} />); + renderLoadedDetails(); @@ it('should show the status box if there is an error loading the secret', () => { @@ - renderWithProviders(<LoadedHelmReleaseDetails {...loadedHelmReleaseDetailsProps} />); + renderLoadedDetails(); @@ it('should render the DetailsPage component when secret gets loaded', () => { - renderWithProviders(<LoadedHelmReleaseDetails {...loadedHelmReleaseDetailsProps} />); + renderLoadedDetails(); @@ it('should show the ErrorPage404 for an incorrect release name in the url', () => { @@ - renderWithProviders(<LoadedHelmReleaseDetails {...loadedHelmReleaseDetailsProps} />); + renderLoadedDetails();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/helm-plugin/src/components/details-page/__tests__/HelmReleaseDetails.spec.tsx` around lines 66 - 103, Tests repeat renderWithProviders(<LoadedHelmReleaseDetails {...loadedHelmReleaseDetailsProps} />) in many it blocks; extract a local helper (e.g., const renderComponent = () => renderWithProviders(<LoadedHelmReleaseDetails {...loadedHelmReleaseDetailsProps} />)) at the top of the describe or beforeEach and replace direct renderWithProviders calls with renderComponent to reduce duplication and improve readability while continuing to mutate loadedHelmReleaseDetailsProps in each test.frontend/packages/console-shared/src/components/dropdown/ResourceDropdown.tsx (2)
272-278:onLoadis excluded from the effect deps — stale closure risk.Unlike
onChangewhich is stabilised viaonChangeRef,onLoadcaptures whatever closure existed at the time the effect was created. If a parent passes a newonLoadcallback between renders (common with inline arrows), the stale version fires.Consider applying the same ref pattern you already use for
onChange:+ const onLoadRef = useRef(onLoad); + onLoadRef.current = onLoad; ... useEffect(() => { - if (loaded && onLoad) { - onLoad(items); + if (loaded && onLoadRef.current) { + onLoadRef.current(items); } - // eslint-disable-next-line react-hooks/exhaustive-deps }, [loaded, items]);This eliminates both the stale closure and the eslint suppression.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/console-shared/src/components/dropdown/ResourceDropdown.tsx` around lines 272 - 278, The effect that calls onLoad(items) uses a stale closure because onLoad is omitted from deps; fix by mirroring the onChangeRef pattern: create an onLoadRef (useRef) that is updated whenever onLoad prop changes, then inside the useEffect that watches [loaded, items] call onLoadRef.current(items) instead of onLoad(items), and remove the eslint-disable comment so the hook deps are correct; reference the existing useEffect, the onLoad prop, and implement onLoadRef similar to the existing onChangeRef logic.
75-91:craftResourceKeyreturnsundefinedkey/customKey whenresourceFilterrejects — consider making the contract explicit.When
resourceFilteris provided and returnsfalse,keyremainsundefined. Callers rely on the downstreamindexKeytruthiness check (Line 150) to skip these entries, which works — butcustomResourceKeyis still invoked withundefinedaskey(Line 88). If anycustomResourceKeyimplementation doesn't guard againstundefined, it'll produce unexpected results.Returning early (or returning
null) when the filter rejects would make the intent clearer and avoid callingcustomResourceKeywith a garbage first argument.♻️ Suggested tightening
const craftResourceKey = ( resource: K8sResourceKind, dataSelector: ResourceDropdownProps['dataSelector'], resourceFilter: ResourceDropdownProps['resourceFilter'], customResourceKey: ResourceDropdownProps['customResourceKey'], -): { customKey: string; key: string } => { - let key; - if (resourceFilter && resourceFilter(resource)) { - key = _.get(resource, dataSelector); - } else if (!resourceFilter) { - key = _.get(resource, dataSelector); +): { customKey: string; key: string } | null => { + if (resourceFilter && !resourceFilter(resource)) { + return null; } + const key = _.get(resource, dataSelector); return { customKey: customResourceKey ? customResourceKey(key, resource) : key, key, }; };Then at call sites, guard with:
- const { customKey, key } = craftResourceKey(...); - const indexKey = customKey || key; - if (indexKey) { + const result = craftResourceKey(...); + if (!result) continue; + const indexKey = result.customKey || result.key; + if (indexKey) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/console-shared/src/components/dropdown/ResourceDropdown.tsx` around lines 75 - 91, craftResourceKey currently leaves key undefined when resourceFilter returns false and still calls customResourceKey with that undefined key; change craftResourceKey to return null (or an explicit sentinel) immediately when resourceFilter is present and returns false so customResourceKey is not invoked, e.g. have craftResourceKey return null | { customKey: string; key: string } and update callers that use indexKey truthiness (the consumer that checks indexKey) to skip entries when craftResourceKey returns null; reference symbols: craftResourceKey, resourceFilter, customResourceKey, and the caller that checks indexKey to locate and update the code.frontend/public/components/utils/storage-class-dropdown.tsx (3)
187-187: Accessingitem.props.nameon a React element is fragile.This works today because both
StorageClassDropdownEntryandStorageClassDropdownNoStorageClassOptionreceive anameprop, but it relies on React element internals. If the rendering components are ever refactored (e.g., wrapped in another element), the autocomplete silently breaks. Consider storing a parallelnamemap or using a data-driven approach rather than reaching into JSX props.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/public/components/utils/storage-class-dropdown.tsx` at line 187, The autocompleteFilter currently reaches into React element internals via item.props.name which is fragile; instead attach the display name to the option metadata when you build the dropdown entries (e.g., include a plain data object or a stable key with the name alongside the React node) and change autocompleteFilter to read from that metadata (referencing autocompleteFilter, fuzzy, StorageClassDropdownEntry, and StorageClassDropdownNoStorageClassOption). Concretely: when creating the list of options, produce items like { key, node: <StorageClassDropdownEntry .../>, name: "<displayName>" } or store a nameMap keyed by the item key, then update autocompleteFilter to use item.name or nameMap[item.key] rather than item.props.name so wrapping or refactors of the JSX won’t break filtering.
99-116: Use_.forEach(ordata.forEach) instead of_.mapfor side-effect-only iteration.
_.mapallocates and returns an array that is immediately discarded._.forEachcommunicates intent better and avoids the throwaway allocation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/public/components/utils/storage-class-dropdown.tsx` around lines 99 - 116, Replace the side-effect-only use of _.map with _.forEach (or data.forEach) in the iteration that populates unorderedItems from StorageClassResourceKind items: change the _.map(...) call to _.forEach(...) so we don't allocate a throwaway array, keeping the loop body that sets unorderedItems[resource.metadata.name] with kindLabel, name, description (using _.get), default (isDefaultClass(resource)), accessMode, provisioner, parameters, type, zone and resource unchanged.
140-140:filterin the dependency array may cause excessive recomputation.If any parent passes an inline function (
filter={(item) => ...}), theitemsuseMemo recomputes on every parent render. TheonChangeRefpattern was used to stabiliseonChange— consider the same approach forfilterif callers are unlikely to memoize it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/public/components/utils/storage-class-dropdown.tsx` at line 140, The memo for computing `items` in StorageClassDropdown is unstable because `filter` in the dependency array may be an inline function and cause recomputation; instead, create a stable ref for `filter` (e.g. `filterRef`) and update it inside an effect, then remove `filter` from the `useMemo` deps and use `filterRef.current` inside the memo; follow the same pattern used for `onChangeRef` so callers can pass inline filters without forcing `items` to recompute, keeping other deps (`loaded`, `loadError`, `data`, `t`) intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@frontend/packages/console-shared/src/components/dropdown/ResourceDropdown.tsx`:
- Around line 223-270: The effect can set selectedItem to
allSelectorItem.allSelectorKey even when items is empty, causing name =
items[selectedItem] to be undefined and onChangeRef.current to be called with an
undefined name; update the effect in ResourceDropdown useEffect to guard against
that by resolving a safe title/name before calling setSelectedTitle/onChangeRef:
compute name = items[selectedItem] ?? allSelectorItem?.allSelectorTitle (or a
sensible fallback) and only call onChangeRef.current(selectedItem, name,
resourceMap[selectedItem]) when selectedItem is defined and name is
non-undefined (or else skip the onChange and still setSelectedTitle to the
fallback), referencing selectedItem, allSelectorItem, items, setSelectedTitle,
onChangeRef and prevSelectedKeyRef to locate and change the logic.
In `@frontend/public/components/utils/storage-class-dropdown.tsx`:
- Around line 86-88: The component currently initializes local state with
selectedKeyProp via useState(selectedKeyProp) so later updates to the prop are
ignored; update the component to sync prop changes by adding a useEffect that
watches selectedKeyProp and calls setSelectedKey(selectedKeyProp) (or remove
local state and read selectedKeyProp directly if you intend a fully controlled
component). Keep the existing setSelectedKey and onChangeRef wiring but ensure
the effect updates selectedKey when selectedKeyProp changes to restore
controlled behavior.
- Around line 145-153: The effect in useEffect is calling
onChangeRef.current(selectedItem.resource) even when the sentinel item (created
with { kindLabel:'', name: noStorageClass }) has no resource, which results in
undefined being sent; update the effect in useEffect (which watches loaded,
loadError, effectiveKey, items) to first check that selectedItem exists and has
a defined resource (or that effectiveKey !== '' / not the noStorageClass
sentinel) and only then call onChangeRef.current(selectedItem.resource);
otherwise skip calling the callback to avoid passing undefined to parent
components.
- Around line 191-219: The component currently hides the entire dropdown while
loading or on load error because of the outer guard using loaded &&
itemsAvailableToShow; change the render guard so the UI renders either when
still loading or when there are items to show (e.g., render when !loaded ||
itemsAvailableToShow) and keep using the computed displayTitle useMemo (which
returns LoadingInline or an error string) as the ConsoleSelect title; ensure
ConsoleSelect is rendered with an empty items array and disabled state when
loading/error so users see the LoadingInline or error text (reference: loaded,
itemsAvailableToShow, displayTitle, ConsoleSelect, LoadingInline, loadError,
handleChange).
- Around line 14-26: The prop types in StorageClassDropdownProps use parameter
names instead of types (making onChange and filter effectively any) and the name
prop is declared but unused: change onChange and filter to accept a concrete
type (e.g., define or reuse a StorageClassOption/SelectedStorageClass type and
declare onChange: (selected: StorageClassOption) => void and filter?: (option:
StorageClassOption) => boolean) instead of (object) or (param); also either
remove the unused name prop or actually consume/destructure name in the
StorageClassDropdown component and forward it to the underlying input/select
element (refer to StorageClassDropdownProps, onChange, filter and name to locate
where to update).
- Around line 198-216: The ConsoleSelect instantiation in
storage-class-dropdown.tsx is not forwarding the describedBy prop, so the help
text paragraph isn't linked via aria-describedby; update the ConsoleSelect props
in the render block (the ConsoleSelect component where
className="co-storage-class-dropdown" and props like isFullWidth, items,
selectedKey, onChange, id, dataTest) to include describedBy={describedBy} so the
control references the help text element and improves accessibility.
---
Nitpick comments:
In
`@frontend/packages/console-shared/src/components/dropdown/ResourceDropdown.tsx`:
- Around line 272-278: The effect that calls onLoad(items) uses a stale closure
because onLoad is omitted from deps; fix by mirroring the onChangeRef pattern:
create an onLoadRef (useRef) that is updated whenever onLoad prop changes, then
inside the useEffect that watches [loaded, items] call onLoadRef.current(items)
instead of onLoad(items), and remove the eslint-disable comment so the hook deps
are correct; reference the existing useEffect, the onLoad prop, and implement
onLoadRef similar to the existing onChangeRef logic.
- Around line 75-91: craftResourceKey currently leaves key undefined when
resourceFilter returns false and still calls customResourceKey with that
undefined key; change craftResourceKey to return null (or an explicit sentinel)
immediately when resourceFilter is present and returns false so
customResourceKey is not invoked, e.g. have craftResourceKey return null | {
customKey: string; key: string } and update callers that use indexKey truthiness
(the consumer that checks indexKey) to skip entries when craftResourceKey
returns null; reference symbols: craftResourceKey, resourceFilter,
customResourceKey, and the caller that checks indexKey to locate and update the
code.
In
`@frontend/packages/helm-plugin/src/components/details-page/__tests__/HelmReleaseDetails.spec.tsx`:
- Around line 66-103: Tests repeat renderWithProviders(<LoadedHelmReleaseDetails
{...loadedHelmReleaseDetailsProps} />) in many it blocks; extract a local helper
(e.g., const renderComponent = () =>
renderWithProviders(<LoadedHelmReleaseDetails {...loadedHelmReleaseDetailsProps}
/>)) at the top of the describe or beforeEach and replace direct
renderWithProviders calls with renderComponent to reduce duplication and improve
readability while continuing to mutate loadedHelmReleaseDetailsProps in each
test.
In `@frontend/public/components/utils/storage-class-dropdown.tsx`:
- Line 187: The autocompleteFilter currently reaches into React element
internals via item.props.name which is fragile; instead attach the display name
to the option metadata when you build the dropdown entries (e.g., include a
plain data object or a stable key with the name alongside the React node) and
change autocompleteFilter to read from that metadata (referencing
autocompleteFilter, fuzzy, StorageClassDropdownEntry, and
StorageClassDropdownNoStorageClassOption). Concretely: when creating the list of
options, produce items like { key, node: <StorageClassDropdownEntry .../>, name:
"<displayName>" } or store a nameMap keyed by the item key, then update
autocompleteFilter to use item.name or nameMap[item.key] rather than
item.props.name so wrapping or refactors of the JSX won’t break filtering.
- Around line 99-116: Replace the side-effect-only use of _.map with _.forEach
(or data.forEach) in the iteration that populates unorderedItems from
StorageClassResourceKind items: change the _.map(...) call to _.forEach(...) so
we don't allocate a throwaway array, keeping the loop body that sets
unorderedItems[resource.metadata.name] with kindLabel, name, description (using
_.get), default (isDefaultClass(resource)), accessMode, provisioner, parameters,
type, zone and resource unchanged.
- Line 140: The memo for computing `items` in StorageClassDropdown is unstable
because `filter` in the dependency array may be an inline function and cause
recomputation; instead, create a stable ref for `filter` (e.g. `filterRef`) and
update it inside an effect, then remove `filter` from the `useMemo` deps and use
`filterRef.current` inside the memo; follow the same pattern used for
`onChangeRef` so callers can pass inline filters without forcing `items` to
recompute, keeping other deps (`loaded`, `loadError`, `data`, `t`) intact.
ℹ️ Review info
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 (7)
frontend/packages/console-shared/src/components/dropdown/ResourceDropdown.tsxfrontend/packages/console-shared/src/components/formik-fields/ResourceDropdownField.tsxfrontend/packages/helm-plugin/src/components/details-page/__tests__/HelmReleaseDetails.spec.tsxfrontend/public/components/graphs/health.jsxfrontend/public/components/graphs/index.tsxfrontend/public/components/graphs/status.jsxfrontend/public/components/utils/storage-class-dropdown.tsx
💤 Files with no reviewable changes (3)
- frontend/public/components/graphs/index.tsx
- frontend/public/components/graphs/status.jsx
- frontend/public/components/graphs/health.jsx
| useEffect(() => { | ||
| if (!mountedRef.current) { | ||
| mountedRef.current = true; | ||
| prevSelectedKeyRef.current = selectedKey; | ||
| return; | ||
| } | ||
| if (key !== selectedKey) { | ||
| onChange && onChange(key, name, this.state.resources[key]); | ||
|
|
||
| if (!loaded || loadError) { | ||
| prevSelectedKeyRef.current = selectedKey; | ||
| return; | ||
| } | ||
| }; | ||
|
|
||
| private onChange = (key: string) => { | ||
| this.handleChange(key, this.state.items); | ||
| }; | ||
| let selectedItem = selectedKey; | ||
| if ( | ||
| (_.isEmpty(items) || !items[selectedKey]) && | ||
| allSelectorItem && | ||
| allSelectorItem.allSelectorKey !== selectedKey | ||
| ) { | ||
| selectedItem = allSelectorItem.allSelectorKey; | ||
| } else if (autoSelect && !selectedKey) { | ||
| selectedItem = | ||
| loaded && _.isEmpty(items) && actionItems | ||
| ? actionItems[0].actionKey | ||
| : _.get(_.keys(items), 0); | ||
| } | ||
|
|
||
| render() { | ||
| return ( | ||
| <ConsoleSelect | ||
| id={this.props.id} | ||
| ariaLabel={this.props.ariaLabel} | ||
| className={this.props.className} | ||
| menuClassName={this.props.menuClassName} | ||
| buttonClassName={this.props.buttonClassName} | ||
| titlePrefix={this.props.titlePrefix} | ||
| isFullWidth={this.props.isFullWidth} | ||
| autocompleteFilter={this.props.autocompleteFilter || fuzzy} | ||
| actionItems={this.props.actionItems} | ||
| items={this.state.items} | ||
| onChange={this.onChange} | ||
| selectedKey={this.props.selectedKey} | ||
| title={this.props.title || this.state.title} | ||
| autocompletePlaceholder={this.props.placeholder} | ||
| userSettingsPrefix={this.props.userSettingsPrefix} | ||
| storageKey={this.props.storageKey} | ||
| disabled={this.props.disabled} | ||
| /> | ||
| ); | ||
| } | ||
| } | ||
| if (selectedItem) { | ||
| const name = items[selectedItem]; | ||
| const selectedActionItem = | ||
| actionItems && actionItems.find((ai) => selectedItem === ai.actionKey); | ||
| const title = selectedActionItem ? selectedActionItem.actionTitle : name; | ||
| setSelectedTitle(title); | ||
| if (selectedItem !== prevSelectedKeyRef.current) { | ||
| onChangeRef.current?.(selectedItem, name, resourceMap[selectedItem]); | ||
| } | ||
| } | ||
|
|
||
| prevSelectedKeyRef.current = selectedKey; | ||
| }, [ | ||
| loaded, | ||
| loadError, | ||
| items, | ||
| selectedKey, | ||
| autoSelect, | ||
| allSelectorItem, | ||
| actionItems, | ||
| resourceMap, | ||
| ]); |
There was a problem hiding this comment.
Auto-select effect: selectedItem may reference a key absent from items, causing name to be undefined.
Lines 236–241: when _.isEmpty(items) is true and allSelectorItem is present, selectedItem is set to allSelectorItem.allSelectorKey. However, allSelectorKey is only inserted into items when unsortedList is non-empty (Line 194). So when resources are truly empty, items[selectedItem] on Line 250 evaluates to undefined, and onChange fires with name = undefined.
This may replicate the class component's existing behavior, but it's worth a defensive guard (or at minimum an explicit comment) since consumers receiving an undefined name for a supposedly-selected item is surprising.
if (selectedItem) {
const name = items[selectedItem];
+ // If the selected key isn't actually in items (e.g., allSelectorKey with no resources),
+ // skip the onChange notification to avoid confusing consumers.
+ if (name === undefined && !actionItems?.find((ai) => selectedItem === ai.actionKey)) {
+ prevSelectedKeyRef.current = selectedKey;
+ return;
+ }
const selectedActionItem =🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/packages/console-shared/src/components/dropdown/ResourceDropdown.tsx`
around lines 223 - 270, The effect can set selectedItem to
allSelectorItem.allSelectorKey even when items is empty, causing name =
items[selectedItem] to be undefined and onChangeRef.current to be called with an
undefined name; update the effect in ResourceDropdown useEffect to guard against
that by resolving a safe title/name before calling setSelectedTitle/onChangeRef:
compute name = items[selectedItem] ?? allSelectorItem?.allSelectorTitle (or a
sensible fallback) and only call onChangeRef.current(selectedItem, name,
resourceMap[selectedItem]) when selectedItem is defined and name is
non-undefined (or else skip the onChange and still setSelectedTitle to the
fallback), referencing selectedItem, allSelectorItem, items, setSelectedTitle,
onChangeRef and prevSelectedKeyRef to locate and change the logic.
| export type StorageClassDropdownProps = { | ||
| id?: string; | ||
| name?: string; | ||
| onChange: (object) => void; | ||
| describedBy?: string; | ||
| desc?: string; | ||
| defaultClass?: string; | ||
| required?: boolean; | ||
| hideClassName?: string; | ||
| filter?: (param) => boolean; | ||
| selectedKey?: string; | ||
| 'data-test'?: string; | ||
| }; |
There was a problem hiding this comment.
Prop types are effectively any and name is unused.
onChange: (object) => void—objecthere is a parameter name, not a type. This compiles as(object: any) => void, losing all type safety. Same forfilter?: (param) => boolean.nameis declared in the props but never destructured or consumed by the component.
Proposed fix
export type StorageClassDropdownProps = {
id?: string;
- name?: string;
- onChange: (object) => void;
+ onChange: (resource: StorageClassResourceKind | undefined) => void;
describedBy?: string;
desc?: string;
defaultClass?: string;
required?: boolean;
hideClassName?: string;
- filter?: (param) => boolean;
+ filter?: (item: Record<string, any>) => boolean;
selectedKey?: string;
'data-test'?: string;
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/public/components/utils/storage-class-dropdown.tsx` around lines 14
- 26, The prop types in StorageClassDropdownProps use parameter names instead of
types (making onChange and filter effectively any) and the name prop is declared
but unused: change onChange and filter to accept a concrete type (e.g., define
or reuse a StorageClassOption/SelectedStorageClass type and declare onChange:
(selected: StorageClassOption) => void and filter?: (option: StorageClassOption)
=> boolean) instead of (object) or (param); also either remove the unused name
prop or actually consume/destructure name in the StorageClassDropdown component
and forward it to the underlying input/select element (refer to
StorageClassDropdownProps, onChange, filter and name to locate where to update).
| const [selectedKey, setSelectedKey] = useState<string | undefined>(selectedKeyProp); | ||
| const onChangeRef = useRef(onChangeProp); | ||
| onChangeRef.current = onChangeProp; |
There was a problem hiding this comment.
Controlled selectedKey prop ignored after mount.
useState(selectedKeyProp) only uses the prop as the initial value. If the parent updates selectedKeyProp (e.g., on form reset), the internal state won't sync. The class component likely read from props directly on each render, so this is a behavioral regression.
Either sync via an effect or lift state entirely:
Option A: Sync prop changes into local state
const [selectedKey, setSelectedKey] = useState<string | undefined>(selectedKeyProp);
+
+ useEffect(() => {
+ setSelectedKey(selectedKeyProp);
+ }, [selectedKeyProp]);
+
const onChangeRef = useRef(onChangeProp);
onChangeRef.current = onChangeProp;📝 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 [selectedKey, setSelectedKey] = useState<string | undefined>(selectedKeyProp); | |
| const onChangeRef = useRef(onChangeProp); | |
| onChangeRef.current = onChangeProp; | |
| const [selectedKey, setSelectedKey] = useState<string | undefined>(selectedKeyProp); | |
| useEffect(() => { | |
| setSelectedKey(selectedKeyProp); | |
| }, [selectedKeyProp]); | |
| const onChangeRef = useRef(onChangeProp); | |
| onChangeRef.current = onChangeProp; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/public/components/utils/storage-class-dropdown.tsx` around lines 86
- 88, The component currently initializes local state with selectedKeyProp via
useState(selectedKeyProp) so later updates to the prop are ignored; update the
component to sync prop changes by adding a useEffect that watches
selectedKeyProp and calls setSelectedKey(selectedKeyProp) (or remove local state
and read selectedKeyProp directly if you intend a fully controlled component).
Keep the existing setSelectedKey and onChangeRef wiring but ensure the effect
updates selectedKey when selectedKeyProp changes to restore controlled behavior.
| useEffect(() => { | ||
| if (!loaded || loadError) { | ||
| return; | ||
| } | ||
| const selectedItem = items[effectiveKey]; | ||
| if (selectedItem) { | ||
| onChangeRef.current(selectedItem.resource); | ||
| } | ||
| }, [loaded, loadError, effectiveKey, items]); |
There was a problem hiding this comment.
onChange fires with undefined when the "No default StorageClass" sentinel is selected.
Line 129 creates a sentinel entry { kindLabel: '', name: noStorageClass } with no resource property. When effectiveKey resolves to '' (no default exists and user hasn't picked anything), line 151 calls onChangeRef.current(selectedItem.resource) — which is undefined. If any parent assumes the callback receives a valid StorageClassResourceKind, this will surface as a runtime error (e.g., accessing .metadata of undefined).
Either guard against the sentinel or don't fire onChange for it:
Proposed fix
useEffect(() => {
if (!loaded || loadError) {
return;
}
const selectedItem = items[effectiveKey];
- if (selectedItem) {
+ if (selectedItem?.resource) {
onChangeRef.current(selectedItem.resource);
}
}, [loaded, loadError, effectiveKey, items]);📝 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.
| useEffect(() => { | |
| if (!loaded || loadError) { | |
| return; | |
| } | |
| const selectedItem = items[effectiveKey]; | |
| if (selectedItem) { | |
| onChangeRef.current(selectedItem.resource); | |
| } | |
| }, [loaded, loadError, effectiveKey, items]); | |
| useEffect(() => { | |
| if (!loaded || loadError) { | |
| return; | |
| } | |
| const selectedItem = items[effectiveKey]; | |
| if (selectedItem?.resource) { | |
| onChangeRef.current(selectedItem.resource); | |
| } | |
| }, [loaded, loadError, effectiveKey, items]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/public/components/utils/storage-class-dropdown.tsx` around lines 145
- 153, The effect in useEffect is calling
onChangeRef.current(selectedItem.resource) even when the sentinel item (created
with { kindLabel:'', name: noStorageClass }) has no resource, which results in
undefined being sent; update the effect in useEffect (which watches loaded,
loadError, effectiveKey, items) to first check that selectedItem exists and has
a defined resource (or that effectiveKey !== '' / not the noStorageClass
sentinel) and only then call onChangeRef.current(selectedItem.resource);
otherwise skip calling the callback to avoid passing undefined to parent
components.
| return ( | ||
| <span className="co-resource-item"> | ||
| <ResourceIcon kind={props.kindLabel} /> | ||
| <span className="co-resource-item__resource-name"> | ||
| {props.name} | ||
| <div className="pf-v6-u-font-size-xs pf-v6-u-text-color-subtle"> | ||
| {' '} | ||
| {storageClassDescriptionLine} | ||
| <> | ||
| {loaded && itemsAvailableToShow ? ( | ||
| <div> | ||
| <label className={css(hideClassName, { 'co-required': required })} htmlFor={id}> | ||
| {t('StorageClass')} | ||
| </label> | ||
| <ConsoleSelect | ||
| className="co-storage-class-dropdown" | ||
| isFullWidth | ||
| autocompleteFilter={autocompleteFilter} | ||
| autocompletePlaceholder={t('Select StorageClass')} | ||
| items={dropdownItems} | ||
| selectedKey={effectiveKey} | ||
| title={displayTitle} | ||
| alwaysShowTitle | ||
| onChange={handleChange} | ||
| id={id} | ||
| dataTest={dataTest} | ||
| menuClassName="dropdown-menu--text-wrap" | ||
| /> | ||
| {describedBy ? ( | ||
| <p className="help-block" id={describedBy}> | ||
| {t('StorageClass for the new claim')} | ||
| </p> | ||
| ) : null} | ||
| </div> | ||
| </span> | ||
| </span> | ||
| ); | ||
| }; | ||
|
|
||
| const StorageClassDropdownNoStorageClassOption = (props) => { | ||
| return ( | ||
| <span className="co-resource-item"> | ||
| <span className="co-resource-item__resource-name">{props.name}</span> | ||
| </span> | ||
| ) : null} | ||
| </> |
There was a problem hiding this comment.
Loading and error states are never rendered — UX regression.
The outer condition on line 193 gates on loaded && itemsAvailableToShow, which means:
- While loading (
loaded === false): the component renders nothing — users see a blank space with no loading indicator. - On error (
loadErrortruthy):itemsis empty anddefaultClassis'', soitemsAvailableToShowis also falsy — the error message computed indisplayTitleis never shown.
The displayTitle useMemo correctly computes <LoadingInline /> and an error string, but neither is ever displayed because the ConsoleSelect that would show them is behind the guard. The old class component rendered inline loading/error feedback, so this is a regression.
Proposed fix — show loading/error states outside the guard
return (
<>
- {loaded && itemsAvailableToShow ? (
+ {loadError ? (
+ <div className="cos-error-title">{t('Error loading {{desc}}', { desc })}</div>
+ ) : !loaded ? (
+ <LoadingInline />
+ ) : itemsAvailableToShow ? (
<div>
- <label className={css(hideClassName, { 'co-required': required })} htmlFor={id}>
+ <label className={css(hideClassName, required && 'co-required')} htmlFor={id}>
{t('StorageClass')}
</label>
<ConsoleSelect🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/public/components/utils/storage-class-dropdown.tsx` around lines 191
- 219, The component currently hides the entire dropdown while loading or on
load error because of the outer guard using loaded && itemsAvailableToShow;
change the render guard so the UI renders either when still loading or when
there are items to show (e.g., render when !loaded || itemsAvailableToShow) and
keep using the computed displayTitle useMemo (which returns LoadingInline or an
error string) as the ConsoleSelect title; ensure ConsoleSelect is rendered with
an empty items array and disabled state when loading/error so users see the
LoadingInline or error text (reference: loaded, itemsAvailableToShow,
displayTitle, ConsoleSelect, LoadingInline, loadError, handleChange).
| <ConsoleSelect | ||
| className="co-storage-class-dropdown" | ||
| isFullWidth | ||
| autocompleteFilter={autocompleteFilter} | ||
| autocompletePlaceholder={t('Select StorageClass')} | ||
| items={dropdownItems} | ||
| selectedKey={effectiveKey} | ||
| title={displayTitle} | ||
| alwaysShowTitle | ||
| onChange={handleChange} | ||
| id={id} | ||
| dataTest={dataTest} | ||
| menuClassName="dropdown-menu--text-wrap" | ||
| /> | ||
| {describedBy ? ( | ||
| <p className="help-block" id={describedBy}> | ||
| {t('StorageClass for the new claim')} | ||
| </p> | ||
| ) : null} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the ConsoleSelect component
fd -t f "console-select" --type=tsx --type=tsRepository: openshift/console
Length of output: 231
🏁 Script executed:
# Also search for ConsoleSelect imports and definition
rg -n "export.*ConsoleSelect" --type=tsx --type=ts -B2 -A5Repository: openshift/console
Length of output: 88
🏁 Script executed:
# Check the full storage-class-dropdown component context around lines 195-220
cat -n frontend/public/components/utils/storage-class-dropdown.tsx | sed -n '190,220p'Repository: openshift/console
Length of output: 1212
🏁 Script executed:
# Search for ConsoleSelect component definition
rg -n "export.*ConsoleSelect|const ConsoleSelect" -A 10 | head -50Repository: openshift/console
Length of output: 4480
🏁 Script executed:
# Find the file that exports ConsoleSelect
fd ConsoleSelectRepository: openshift/console
Length of output: 125
🏁 Script executed:
# Also try searching in public/components
rg -l "ConsoleSelect" frontend/public/components --max-count=5Repository: openshift/console
Length of output: 925
🏁 Script executed:
cat -n frontend/public/components/utils/console-select.tsx | head -180Repository: openshift/console
Length of output: 6911
🏁 Script executed:
# Also check lines around describedBy usage
rg -n "describedBy" frontend/public/components/utils/console-select.tsx -B2 -A2Repository: openshift/console
Length of output: 593
🏁 Script executed:
rg -n "describedBy" frontend/public/components/utils/storage-class-dropdown.tsx -B5 -A5Repository: openshift/console
Length of output: 1033
🏁 Script executed:
# Check the component function signature to see where describedBy comes from
cat -n frontend/public/components/utils/storage-class-dropdown.tsx | sed -n '1,50p'Repository: openshift/console
Length of output: 2125
Pass describedBy prop to ConsoleSelect so the help text is linked via aria-describedby.
The describedBy prop is available in the component but not forwarded to the ConsoleSelect component. Add describedBy={describedBy} to the ConsoleSelect props so the help text at line 213-215 is properly associated with the dropdown control.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/public/components/utils/storage-class-dropdown.tsx` around lines 198
- 216, The ConsoleSelect instantiation in storage-class-dropdown.tsx is not
forwarding the describedBy prop, so the help text paragraph isn't linked via
aria-describedby; update the ConsoleSelect props in the render block (the
ConsoleSelect component where className="co-storage-class-dropdown" and props
like isFullWidth, items, selectedKey, onChange, id, dataTest) to include
describedBy={describedBy} so the control references the help text element and
improves accessibility.
|
@logonoff: The following tests 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. |
2 participants
Builds on #15954 for StorageClassDropdown
This PR refactors all
UNSAFE_class components except for Firehose to React.FC. React 18 concurrent rendering doesn't officially support class components that useUNSAFE_methods so we should remove themSummary by CodeRabbit
Bug Fixes
Removals