CONSOLE-5034: Migrate tile-view-page and value-from-pair to TypeScript#16412
CONSOLE-5034: Migrate tile-view-page and value-from-pair to TypeScript#16412logonoff wants to merge 3 commits intoopenshift:mainfrom
Conversation
|
@logonoff: This pull request references CONSOLE-5034 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 "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. |
openshift-ci-robot
added
the
jira/valid-reference
|
[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
the
approved
logonoff
force-pushed
the
CONSOLE-5034-proptypes
branch
from
793dc6f to
199fb2a
Compare
|
/label px-approved |
openshift-ci
Bot
added
px-approved
📝 WalkthroughWalkthroughThis PR adds and tightens TypeScript typings across several utility components and updates imports/tests for compatibility. value-from-pair.tsx is rewritten from a class to a typed functional component with new exported types (e.g., ValueFromPairProps, RefChangeValue, PairValue) and typed internal subcomponents. name-value-editor.tsx changes pair change-handler types to accept a union 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
logonoff
force-pushed
the
CONSOLE-5034-proptypes
branch
from
2a75676 to
8ecf6e8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
frontend/public/components/utils/tile-view-page.tsx (1)
991-991: 💤 Low valueMinor: Redundant type assertion on ref.
The
filterByKeywordInputref is already typed asRefObject<HTMLInputElement>at declaration (line 659). This cast is unnecessary.♻️ Suggested simplification
<SearchInput data-test="search-operatorhub" - ref={filterByKeywordInput as RefObject<HTMLInputElement>} + ref={filterByKeywordInput} placeholder={t('public~Filter by keyword...')}🤖 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 `@frontend/public/components/utils/tile-view-page.tsx` at line 991, Remove the redundant type assertion on the ref prop: replace the usage of filterByKeywordInput as RefObject<HTMLInputElement> in the JSX ref attribute with the already-typed filterByKeywordInput; ensure the declared ref (filterByKeywordInput) remains typed as RefObject<HTMLInputElement> (from its declaration around line 659) so no further changes are needed to types.frontend/public/components/utils/value-from-pair.tsx (1)
92-101: ⚡ Quick winNarrow
pairtype to match the object structure the component actually requires.The current
PairValueunion (string | number | Record<string, unknown>) is too broad. Line 415 callsObject.keys(pair)[0], and line 424 performs indexed access—both expecting a plain object. Primitives passed here would silently fail (returningnullat line 419) instead of being caught at the type level.The proposed type
Record<string, FieldRefData | ResourceFieldRefData | RefValue>aligns perfectly with the component's actual contract: each call site extracts a value (e.g.,pair[EnvFromPair.Resource]) that maps to the expected data types forRefComponentProps. This tightening improves early error detection without affecting existing safe callers.Proposed fix
export type PairValue = string | number | Record<string, unknown>; + +type ValueFromData = Record<string, FieldRefData | ResourceFieldRefData | RefValue>; export type ValueFromPairProps = { - pair: PairValue; + pair: ValueFromData; configMaps?: K8sItemList; secrets?: K8sItemList; serviceAccounts?: K8sItemList; onChange?: (e: { target: { value: RefChangeValue } }) => void; disabled?: boolean; };🤖 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 `@frontend/public/components/utils/value-from-pair.tsx` around lines 92 - 101, The PairValue union is too permissive for ValueFromPairProps: the component uses Object.keys(pair)[0] and indexed accesses (e.g., pair[EnvFromPair.Resource]) so primitives can slip through; change the PairValue alias to a Record<string, FieldRefData | ResourceFieldRefData | RefValue> to reflect the actual object shape the component expects, update ValueFromPairProps to use this narrowed PairValue, and ensure any call sites constructing pair conform to those FieldRefData/ResourceFieldRefData/RefValue types used by RefComponentProps so TypeScript will catch invalid primitives at compile time.
🤖 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 `@frontend/public/components/utils/name-value-editor.tsx`:
- Around line 58-67: The parent handlers currently declare the value parameter
as string but child callbacks (handleChangeValueFromPair and
handleChangeResource) pass RefChangeValue, so update the type contract to accept
the full union PairChangeValue; specifically change the onChange handler
signatures used in the parent components (the functions referenced where
PairElementProps.onChange is passed) to accept value: PairChangeValue (string |
RefChangeValue) instead of string, and ensure any downstream uses narrow/handle
the RefChangeValue case (type guards or discriminants) where appropriate; keep
PairElementProps.onChange and the PairChangeValue type as the single source of
truth.
In `@frontend/public/components/utils/tile-view-page.tsx`:
- Around line 533-538: defaultFilters is a module-level object that is being
referenced directly in the component state and then mutated by functions like
updateActiveFilters and clearActiveFilters via _.set, causing state pollution;
fix this by cloning defaultFilters when initializing state (e.g., pass a deep
clone into useState) and ensure updateActiveFilters and clearActiveFilters make
and mutate a deep clone of the current filters (not the original/default object)
before calling setState; also review getAvailableFilters to ensure it returns
clones rather than references to defaultFilters or nested objects so no
mutations leak back to the module-level constant.
- Around line 691-726: getUpdatedState is defined with an empty dependency array
but closes over items, itemsSorter, keywordCompare, and filterGroups causing
stale behavior; update the useCallback deps to include items, itemsSorter,
keywordCompare, and filterGroups so recategorizeItems and getFilterGroupCounts
see current values, and if that introduces re-render loops ensure itemsSorter
and filterGroups are memoized/stable in the parent (e.g., useMemo/useCallback)
to avoid churn.
---
Nitpick comments:
In `@frontend/public/components/utils/tile-view-page.tsx`:
- Line 991: Remove the redundant type assertion on the ref prop: replace the
usage of filterByKeywordInput as RefObject<HTMLInputElement> in the JSX ref
attribute with the already-typed filterByKeywordInput; ensure the declared ref
(filterByKeywordInput) remains typed as RefObject<HTMLInputElement> (from its
declaration around line 659) so no further changes are needed to types.
In `@frontend/public/components/utils/value-from-pair.tsx`:
- Around line 92-101: The PairValue union is too permissive for
ValueFromPairProps: the component uses Object.keys(pair)[0] and indexed accesses
(e.g., pair[EnvFromPair.Resource]) so primitives can slip through; change the
PairValue alias to a Record<string, FieldRefData | ResourceFieldRefData |
RefValue> to reflect the actual object shape the component expects, update
ValueFromPairProps to use this narrowed PairValue, and ensure any call sites
constructing pair conform to those FieldRefData/ResourceFieldRefData/RefValue
types used by RefComponentProps so TypeScript will catch invalid primitives at
compile time.
🪄 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: 998a0bf1-5ef3-438b-a8d4-fa688c5217a7
📒 Files selected for processing (4)
frontend/public/components/utils/__tests__/name-value-editor.spec.tsxfrontend/public/components/utils/name-value-editor.tsxfrontend/public/components/utils/tile-view-page.tsxfrontend/public/components/utils/value-from-pair.tsx
📜 Review details
🔇 Additional comments (6)
frontend/public/components/utils/tile-view-page.tsx (5)
45-125: Type definitions look solid for a generic catalog page component.The flexible
TileItem = Record<string, unknown>trades compile-time safety for runtime flexibility, which is a reasonable choice given this component handles various item shapes from different consumers. The function types with optional properties (useScoring,name) are a pragmatic approach for optional behavior.
24-26: Good: Barrel imports replaced with direct file paths.Moving from
@console/sharedto direct paths like@console/shared/src/hooks/useDebounceCallbackimproves tree-shaking and can reduce bundle size by avoiding pulling in the entire barrel.
40-43: Nice use ofas constfor FilterTypes.This enables TypeScript to infer literal types (
'category' | 'keyword') rather than widening tostring, providing better type safety for filter type checks throughout the file.
644-656: Proper React 19 migration: Default parameter values replacedefaultProps.Using inline default values in the destructuring pattern (
filterGroupNameMap = {},emptyStateTitle = '...') is the correct approach for React 19, wheredefaultPropson function components is removed.
907-909: Standard pattern for typed checkbox event handling.The
(e.target as HTMLInputElement).checkedassertion is the typical approach when the click handler's event type doesn't narrow to the input element. Acceptable given PatternFly's FilterSidePanelCategoryItem API.frontend/public/components/utils/__tests__/name-value-editor.spec.tsx (1)
5-6: Import split looks correct and keeps test typing aligned with the migrated modules.
Replace PropTypes and defaultProps with TypeScript interfaces and default parameter values. Fix barrel imports for useDebounceCallback and getURLWithParams to use direct file paths, and remove unused `shown` prop on VerticalTabs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Convert ValueFromPair from a class component to a function component, replace PropTypes with TypeScript interfaces, and export ValueFromPairProps type for consumers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
logonoff
force-pushed
the
CONSOLE-5034-proptypes
branch
from
8ecf6e8 to
53bad89
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/public/components/utils/value-from-pair.tsx (1)
92-92: ⚡ Quick win
PairValuetype is too permissive—reduces TypeScript's value here.The
PairValue = string | number | Record<string, unknown>union accepts primitives that KubernetesvalueFromfields will never actually be. When you indexpair[valueFromKey]at line 424, the result type isunknown, which then gets passed toComponentexpectingFieldRefData | ResourceFieldRefData | RefValue. This compiles but offers no real type safety.Consider narrowing to the actual shape:
-export type PairValue = string | number | Record<string, unknown>; +export type PairValue = Record<string, FieldRefData | ResourceFieldRefData | RefValue>;This would:
- Eliminate impossible primitive cases
- Provide accurate typing for
pair[valueFromKey]without needing type assertions- Catch misuse at compile time rather than relying on the runtime null guard
If backward compatibility requires accepting primitives from upstream callers, at minimum add a type guard before line 415 to narrow the type explicitly.
Also applies to: 415-424
🤖 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 `@frontend/public/components/utils/value-from-pair.tsx` at line 92, PairValue is too permissive (string | number | Record<string, unknown>) so indexing pair[valueFromKey] yields unknown while Component expects FieldRefData | ResourceFieldRefData | RefValue; update the PairValue alias to the concrete expected shapes (e.g. the specific interfaces used by Component: FieldRefData, ResourceFieldRefData, RefValue) instead of allowing primitives, or if you must keep the broader type, add a narrowing type guard immediately before using pair[valueFromKey] that asserts and refines it to FieldRefData | ResourceFieldRefData | RefValue so no unsafe type assertions are needed when passing the value into Component.
🤖 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.
Nitpick comments:
In `@frontend/public/components/utils/value-from-pair.tsx`:
- Line 92: PairValue is too permissive (string | number | Record<string,
unknown>) so indexing pair[valueFromKey] yields unknown while Component expects
FieldRefData | ResourceFieldRefData | RefValue; update the PairValue alias to
the concrete expected shapes (e.g. the specific interfaces used by Component:
FieldRefData, ResourceFieldRefData, RefValue) instead of allowing primitives, or
if you must keep the broader type, add a narrowing type guard immediately before
using pair[valueFromKey] that asserts and refines it to FieldRefData |
ResourceFieldRefData | RefValue so no unsafe type assertions are needed when
passing the value into Component.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 169a7e3a-ab05-4e82-9bd5-606167c8bb00
⛔ Files ignored due to path filters (1)
frontend/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
frontend/.eslintrc.jsfrontend/package.jsonfrontend/public/components/utils/__tests__/name-value-editor.spec.tsxfrontend/public/components/utils/name-value-editor.tsxfrontend/public/components/utils/tile-view-page.tsxfrontend/public/components/utils/value-from-pair.tsx
💤 Files with no reviewable changes (1)
- frontend/package.json
✅ Files skipped from review due to trivial changes (2)
- frontend/.eslintrc.js
- frontend/public/components/utils/tests/name-value-editor.spec.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/public/components/utils/tile-view-page.tsx
- frontend/public/components/utils/name-value-editor.tsx
📜 Review details
🔇 Additional comments (4)
frontend/public/components/utils/value-from-pair.tsx (4)
119-236: Solid TypeScript migration of NameKeyDropdownPair.The conversion to a typed FC with explicit
NameKeyDropdownPairPropsis well done. Good use ofuseTranslationfor i18n, and the default parameterisKeyRef = truecleanly replaces the old defaultProps pattern.
238-370: Clean typed presentational components.The internal components (
FieldRef,ConfigMapSecretKeyRef,ConfigMapSecretRef,ResourceFieldRef) are properly typed with their respective data shapes. PatternFly v6 classes and disabled state modifiers are correctly applied. Good i18n coverage with the translation hook.
372-405: Good component routing map with proper typing.The
keyStringToComponentrecord cleanly maps Kubernetes valueFrom reference types to their corresponding React components. The routing logic ensures the correct component receives the appropriate data shape.
407-433: Well-structured functional component with proper null safety.The conversion from class to FC is clean. The optional chaining on
onChange?.()at line 429 and the earlynullreturn whencomponentInfois missing are good defensive patterns. The component correctly delegates to the appropriate sub-component based on the valueFrom key.
|
@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
Solution description:
useDebounceCallback,getURLWithParams) to use direct file paths.ValueFromPairclass component to a function component, replace PropTypes with TypeScript interfaces.onChangevalue type toPairChangeValueto correctly reflect thatValueFromPairpassesRefChangeValueobjects.Test cases:
Summary by CodeRabbit