Conversation
Assisted-by: Cursor
…tip, loading test Co-authored-by: Cursor <cursoragent@cursor.com> Assisted-by: Cursor
…d activity tabs Co-authored-by: Cursor <cursoragent@cursor.com> Assisted-by: Cursor
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Note
|
| Cohort / File(s) | Summary |
|---|---|
Branch Filtering Infrastructure src/hooks/usePipelineRunsV2.ts, src/hooks/useLatestPushBuildPipeline.ts, src/utils/kubearchive-filter-transform.ts |
Added filterByTargetBranch support to pipeline runs queries across etcd, KubeArchive, and merged results. Updated selectors and memoization dependencies to conditionally apply branch filtering when branchName is provided. |
Pipeline Runs Components src/components/Activity/PipelineRunsTab.tsx, src/components/PipelineRun/PipelineRunListView/PipelineRunsListView.tsx |
Added optional branchName prop to enable branch-scoped filtering and column persistence. Integrated with updated usePipelineRunsV2 selector and extended dependency arrays. |
Commits Filtering src/components/Commits/CommitsListPage/CommitsListView.tsx |
Added optional branchName prop to CommitsListView with conditional filterByTargetBranch selector application and updated memoization dependencies. |
Component Activity Tab (V2) src/components/ComponentsPage/ComponentDetails/tabs/ComponentActivityTab.tsx, src/components/ComponentsPage/ComponentDetails/__tests__/ComponentActivityTab.spec.tsx, src/components/ComponentsPage/ComponentDetails/index.ts |
New Activity section component with Commits and Pipeline runs tabs, route-based tab navigation, and feature flag gating. Includes comprehensive unit tests validating rendering, navigation, and error states. |
Component Versions UI src/components/ComponentsPage/Versions/ComponentVersionsList/*, src/components/ComponentsPage/Versions/ComponentVersionDetails/* |
New components for displaying and filtering component versions (list view with search/pagination) and detailed version information including latest build, commits, and pipeline runs. Includes header/row table definitions and comprehensive test coverage. |
Component Version Hooks & Types src/hooks/useComponentVersions.ts |
New hook to fetch and transform pipeline runs into component version rows grouped by branch, including branch name, description, and latest pipeline run details. |
Routing & Path Constants src/routes/page-routes/components-page.tsx, src/routes/paths.ts, src/routes/utils.ts |
Added nested routes for Activity (activity/:activityTab) and Versions (versions/:versionName) under component details. New path constants for activity and version detail views with dynamic tab/version parameters. |
Route Tests src/routes/page-routes/__tests__/components-page.spec.tsx |
Updated tests to verify new route structure including activity and versions paths, route element presence, and loader configuration. |
Test Infrastructure src/hooks/__tests__/useLatestPushBuildPipeline.spec.ts, src/components/NamespaceList/__tests__/useNamespaceActions.spec.tsx |
New comprehensive test suite for latest build pipeline hooks with branch filtering validation. Updated async test utility to use waitFor for proper async handling. |
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
- feat(KFLUXUI-668): Created hook to fetch PipelineRuns from KubeArchive & PipelineRunsListView component migration #403: Extends branch-based filtering through PipelineRunsTab and updates hooks to support filterByTargetBranch in the same components modified by this PR.
- feat: add usePipelineRunsV2 hook and update pipeline run components #432: Modifies usePipelineRunsV2 and pipeline-runs components; this PR extends that work with branch filtering and threading through additional components.
- feat(KFLUXUI-690): Adds a feature flag and new Components page #577: Both PRs expand the "components-page" feature surface with routes, paths, and feature-flagged UI components.
Suggested labels
ok-to-test
Suggested reviewers
- sahil143
- JoaoPedroPP
- abhinandan13jan
- StanislavJochman
- janaki29
- testcara
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 inconclusive)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Title check | ❓ Inconclusive | The PR title is vague and non-descriptive, using only a Jira ticket reference without indicating the actual feature being implemented. | Revise the title to clearly describe the main change, such as 'Add component versions tab with branch filtering' or 'Implement Version details view with activity tracking'. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In
`@src/components/ComponentsPage/ComponentDetails/tabs/ComponentActivityTab.tsx`:
- Around line 21-25: ComponentActivityTab currently passes the
possibly-undefined param componentName from useParams<RouterParams>() directly
into useComponent, causing a type/ runtime mismatch; update the call to
useComponent to pass a non-undefined string (e.g. componentName ?? '') and add
an early-return guard that handles the missing param case (render null or a
placeholder) so the hook never receives undefined; reference
ComponentActivityTab, useParams, RouterParams, componentName and useComponent
when making this change.
In
`@src/components/ComponentsPage/ComponentDetails/tabs/ComponentVersionsTab.tsx`:
- Around line 49-51: The branches error is currently returned as an inline Alert
by calling getErrorState(branchesError, branchesLoaded, 'branches', true)
outside of the DetailsSection, causing it to render without surrounding context;
either remove the final true flag so getErrorState(branchesError,
branchesLoaded, 'branches') returns the full ErrorEmptyState (matching the
component error handling) or move the current return so the getErrorState(...,
true) invocation is rendered inside the DetailsSection wrapper; update the logic
near the branchesError check in ComponentVersionsTab.tsx to use one of these two
approaches and keep the handling consistent with the earlier component error
path.
- Around line 12-82: ComponentVersionsTab should explicitly guard its rendered
UI with the same feature flag as ComponentActivityTab: import IfFeature from
'~/feature-flags/hooks' and wrap the JSX returned by ComponentVersionsTab (the
DetailsSection/List block returned at the end of the ComponentVersionsTab
function) inside <IfFeature flag="components-page">...</IfFeature>; keep the
early returns for missing componentName, loading spinners, and error states
as-is so only the final successful render is feature-gated.
In
`@src/components/ComponentsPage/ComponentVersionDetails/ComponentVersionDetailsView.tsx`:
- Around line 37-54: ComponentVersionDetailsView accesses
component.spec.componentName (used in the DetailsPage headTitle) without
guarding for a null/undefined component returned by useComponent; add the same
null-check used in ComponentVersionOverviewTab (e.g., if (!component) return
null;) at the start of ComponentVersionDetailsView after the componentError
check so any subsequent uses of component (component.spec.componentName, verName
in headTitle, and other accesses) are safe.
In
`@src/components/ComponentsPage/ComponentVersionDetails/tabs/ComponentVersionLatestBuild.tsx`:
- Around line 63-65: The expression assigning imageUrlFromPlr can throw if
getPipelineRunStatusResults(pipelineRun) returns undefined; update the code that
computes imageUrlFromPlr (referencing pipelineRun and
getPipelineRunStatusResults) to guard the results before calling .find — e.g.
obtain the results into a variable and default to an empty array or use optional
chaining, then safely access the `.value` of the found entry; ensure you still
check pipelineRun?.status as before.
- Around line 20-29: The imports in this file mix relative '../../../../' paths
with alias imports; update all relative imports (symbols: COMMIT_DETAILS_PATH,
Timestamp, useNamespace, ComponentKind, ImageRepositoryVisibility,
getCommitsFromPLRs, getImageUrlForVisibility, getPipelineRunStatusResults,
CommitLabel, useBuildLogViewerModal, ScanDescriptionListGroup) to use the
project's configured absolute path aliases (e.g. use ~/routes, ~/shared,
~/types, ~/utils, ~/components as appropriate) so all imports follow the alias
style consistently.
In `@src/hooks/useComponentBranches.ts`:
- Around line 9-44: The hook useComponentBranches currently passes a hard limit:
100 into usePipelineRunsV2 which can omit less-active branches; instead
implement pagination: call usePipelineRunsV2 without a fixed low limit (or
remove the limit from the selector) and if usePipelineRunsV2 exposes
getNextPage, iterate/getNextPage until no more pages, merging pipelineRuns
across pages before computing branches (still dedupe by
PipelineRunLabel.COMMIT_BRANCH_ANNOTATION); update the selector creation in the
React.useMemo and the code that consumes usePipelineRunsV2
(useComponentBranches) to accumulate pages into a single pipelineRuns array so
all branches are discovered.
🧹 Nitpick comments (14)
src/hooks/useLatestPushBuildPipeline.ts (1)
71-107: Consider consolidating with the existing branch-less variant to reduce duplication.This hook is nearly identical to
useLatestSuccessfulBuildPipelineRunForComponentV2(lines 35–69). The only material difference is the optionalbranchNameparameter and the conditionalfilterByTargetBranchin the selector. You could unify both into a single hook with an optionalbranchNameparameter and have the existing call sites passundefined.Additionally,
latestSuccess(line 92) evaluates tofalse | PipelineRunKind | undefinedbecauseloaded && !error && pipelines?.find(...)short-circuits tofalsewhenloadedis falsy. The declared return typePipelineRunKind | undefineddoesn't account for thefalsecase. This is inherited from the existing hook but worth correcting in both.♻️ Suggested consolidation (optional)
-export const useLatestSuccessfulBuildPipelineRunForComponentV2 = ( - namespace: string, - componentName: string, -): [PipelineRunKind, boolean, unknown] => { - const [pipelines, loaded, error, getNextPage] = usePipelineRunsV2( - namespace, - React.useMemo( - () => ({ - selector: { - matchLabels: { - [PipelineRunLabel.PIPELINE_TYPE]: PipelineRunType.BUILD, - [PipelineRunLabel.COMPONENT]: componentName, - }, - }, - }), - [componentName], - ), - ); - - const latestSuccess = React.useMemo( - () => - loaded && - !error && - pipelines?.find((pipeline) => pipelineRunStatus(pipeline) === runStatus.Succeeded), - [error, loaded, pipelines], - ); - - React.useEffect(() => { - if (loaded && !error && !latestSuccess && getNextPage) { - getNextPage(); - } - }, [loaded, error, getNextPage, latestSuccess]); - - return [latestSuccess, loaded, error]; -}; - -export const useLatestSuccessfulBuildPipelineRunForComponentAndBranchV2 = ( +export const useLatestSuccessfulBuildPipelineRunForComponentV2 = ( namespace: string, componentName: string, branchName?: string | undefined, ): [PipelineRunKind | undefined, boolean, unknown] => { const [pipelines, loaded, error, getNextPage] = usePipelineRunsV2( namespace, React.useMemo( () => ({ selector: { matchLabels: { [PipelineRunLabel.PIPELINE_TYPE]: PipelineRunType.BUILD, [PipelineRunLabel.COMPONENT]: componentName, }, ...(branchName ? { filterByTargetBranch: branchName } : {}), }, }), [componentName, branchName], ), ); const latestSuccess = React.useMemo( () => { if (!loaded || error) return undefined; return pipelines?.find((pipeline) => pipelineRunStatus(pipeline) === runStatus.Succeeded); }, [error, loaded, pipelines], ); React.useEffect(() => { if (loaded && !error && !latestSuccess && getNextPage) { getNextPage(); } }, [loaded, error, getNextPage, latestSuccess]); return [latestSuccess, loaded, error]; };src/hooks/usePipelineRunsV2.ts (1)
218-246: Branch filter incombinedDatais redundant for etcd/kubearchive sources.Both
etcdRuns(line 90) andkubearchiveData(line 197) already filter by branch before reachingcombinedData. The filter here only adds value fortrResources(Tekton Results) which lack server-side branch filtering. The comment on line 218 documents this intent, but the redundant re-filtering of already-filtered data adds minor overhead on every memo evaluation.Consider applying the branch filter only to
trResourcesbefore merging, rather than post-merge on the full set. That said, this is a small optimization and not blocking.src/components/ComponentsPage/ComponentVersionDetails/tabs/ComponentVersionLatestBuild.tsx (1)
86-89: Raw<div>elements used for layout instead of PatternFly components.Lines 86 and 113–116 use raw
<div>tags with custom CSS classes. Per coding guidelines, PatternFly layout components should be used instead.♻️ Suggested replacement for the spinner wrapper (line 86-89)
- return ( - <div className="pf-u-m-lg"> - <Spinner /> - </div> - ); + return ( + <Bullseye className="pf-u-m-lg"> + <Spinner /> + </Bullseye> + );For lines 113–117, consider using PatternFly
Stack/StackItemorFlex/FlexItemcomponents to replace the nesteddivlayout for the build completion section.As per coding guidelines: "NEVER use raw HTML layout tags (
<div>,<section>,<header>) or custom CSS layouts. Always use PatternFly layout components."Also applies to: 113-117
src/components/ComponentsPage/ComponentVersionDetails/tabs/ComponentVersionOverviewTab.tsx (2)
42-42: Raw<div>used for layout wrapper.Line 42 uses
<div className="component-version-details">which violates the coding guideline to use PatternFly layout components. Consider usingStackfrom PatternFly.As per coding guidelines: "NEVER use raw HTML layout tags (
<div>,<section>,<header>) or custom CSS layouts."
19-22: RedundantuseComponentfetch — parent already loads the same data.
ComponentVersionDetailsView(the parent) already callsuseComponent(namespace, componentName)and handles loading/error states. This tab re-fetches the same resource. While the K8s watch layer likely caches the result, this is unnecessary duplication. Consider passingcomponentas a prop or via an outlet context from the parent layout.src/routes/utils.ts (1)
22-22: Consider usingversionNameinstead ofverNamefor clarity.Other params use full words (e.g.,
applicationName,componentName,releaseName). The abbreviatedverNameis inconsistent. This would also require updatingpaths.tsand all consumers.src/components/ComponentsPage/ComponentDetails/__tests__/ComponentActivityTab.spec.tsx (2)
3-3: PreferuserEvent.setup()overfireEventfor user interactions.Line 89 uses
fireEvent.click(plrTab)wrapped inact(). The testing guidelines recommenduserEvent.setup()for more realistic event simulation.Suggested refactor
-import { screen, fireEvent, act } from '@testing-library/react'; +import { screen } from '@testing-library/react'; +import userEvent from '@testing-library/user-event';it('should render two tabs under component activity', async () => { + const user = userEvent.setup(); componentMock.mockReturnValue([MockComponents[0], true]); renderWithQueryClientAndRouter(<ComponentActivityTab />); screen.getByTestId('comp__activity__tabItem commits'); const plrTab = screen.getByTestId('comp__activity__tabItem pipelineruns'); - await act(() => fireEvent.click(plrTab)); + await user.click(plrTab); // V2 route uses /ns/:workspaceName/components/:componentName/activity/:activityTab expect(navigateMock).toHaveBeenCalledWith( '/ns/test-ns/components/test-component/activity/pipelineruns', ); });As per coding guidelines: "Use
userEvent.setup()and async user interactions instead of synchronousfireEventfor testing user actions."Also applies to: 83-94
49-112: Consider adding an error scenario test.The suite covers loading and success states but does not test the component error path (when
useComponentreturns an error). Adding a test for error rendering would improve coverage of thegetErrorStatebranch.As per coding guidelines: "Test both success and error scenarios for all components and features."
src/components/ComponentsPage/ComponentDetails/tabs/ComponentActivityTab.tsx (2)
79-81:nonTestSnapShotFilteris re-created on every render.This function is declared inline in the render body, producing a new reference each render. Since it's passed as
customFiltertoPipelineRunsTab→PipelineRunsListView, it can trigger unnecessary re-computations in downstreamuseMemohooks that includecustomFilterin their dependency arrays (e.g.,statusFilterObj,typeFilterObj,filteredPLRs).Suggested fix
+const nonTestSnapShotFilter = (plr: PipelineRunKind) => + plr.metadata.labels?.[PipelineRunLabel.PIPELINE_TYPE] !== 'test' || + !plr.spec.params?.find((p) => p.name === 'SNAPSHOT'); + export const ComponentActivityTab: React.FC = () => {Or, if keeping it inside the component, wrap with
useCallback:- const nonTestSnapShotFilter = (plr: PipelineRunKind) => - plr.metadata.labels?.[PipelineRunLabel.PIPELINE_TYPE] !== 'test' || - !plr.spec.params?.find((p) => p.name === 'SNAPSHOT'); + const nonTestSnapShotFilter = React.useCallback( + (plr: PipelineRunKind) => + plr.metadata.labels?.[PipelineRunLabel.PIPELINE_TYPE] !== 'test' || + !plr.spec.params?.find((p) => p.name === 'SNAPSHOT'), + [], + );
83-135: Raw<div>wrapper should use a PatternFly layout component.Line 85 uses a bare
<div>as a wrapper. Per the project's coding guidelines, PatternFly layout components should be used instead of raw HTML elements.Suggested fix
- <div> - <DetailsSection + <Stack> + <DetailsSection ... - </div> + </Stack>(Import
Stackfrom@patternfly/react-core.)As per coding guidelines: "NEVER use raw HTML layout tags (
<div>,<section>,<header>) or custom CSS layouts. Always use PatternFly layout components."src/routes/page-routes/__tests__/components-page.spec.tsx (1)
69-85: Consider asserting the nested version-details children structure.The test verifies that
versionDetailsRoute.childrenis defined (Line 84) but doesn't assert the structure of those nested children (e.g., the index/overview route, theactivityroute, andactivity/:activityTabroute). This would improve confidence that the full version-details route tree is correctly wired.src/components/ComponentsPage/ComponentVersionDetails/tabs/ComponentVersionActivityTab.tsx (3)
82-136: Replace raw<div>with a PatternFly layout component.Line 84 wraps the
DetailsSectionin a raw<div>. The coding guidelines state to never use raw HTML layout tags and to always use PatternFly layout components instead.Proposed fix
- <div> - <DetailsSection + <Stack> + <DetailsSectionand the closing tag:
- </div> + </Stack>Add
Stackto the PatternFly import on Line 3.As per coding guidelines: "NEVER use raw HTML layout tags (
,, ) or custom CSS layouts. Always use PatternFly layout components."
92-96: Inline styles onTabs— prefer PatternFly spacing utilities.The inline
styleprop withwidth: 'fit-content'and a PF spacing variable could be replaced with PatternFly class names or layout props to stay consistent with the no-custom-CSS guideline.
76-80:nonTestSnapShotFilteris re-created every render.This inline filter function is passed as a prop to
PipelineRunsTab. SinceTabsusesunmountOnExit, the impact is minimal, but wrapping it inReact.useCallback(with an empty dependency array, since it captures no reactive values) would be slightly cleaner.
src/components/ComponentsPage/ComponentDetails/tabs/ComponentActivityTab.tsx
Outdated
Show resolved
Hide resolved
src/components/ComponentsPage/ComponentDetails/tabs/ComponentVersionsTab.tsx
Outdated
Show resolved
Hide resolved
src/components/ComponentsPage/ComponentDetails/tabs/ComponentVersionsTab.tsx
Outdated
Show resolved
Hide resolved
src/components/ComponentsPage/ComponentVersionDetails/ComponentVersionDetailsView.tsx
Outdated
Show resolved
Hide resolved
src/components/ComponentsPage/ComponentVersionDetails/tabs/ComponentVersionLatestBuild.tsx
Outdated
Show resolved
Hide resolved
src/components/ComponentsPage/ComponentVersionDetails/tabs/ComponentVersionLatestBuild.tsx
Outdated
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #702 +/- ##
==========================================
- Coverage 87.12% 86.72% -0.40%
==========================================
Files 764 776 +12
Lines 58225 59420 +1195
Branches 5658 5803 +145
==========================================
+ Hits 50727 51532 +805
- Misses 7430 7713 +283
- Partials 68 175 +107
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 88 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
rakshett
requested review from
JoaoPedroPP,
StanislavJochman,
abhinandan13jan,
janaki29,
marcin-michal,
rrosatti and
sahil143
Assisted-by: Cursor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
`@src/components/ComponentsPage/ComponentVersionDetails/__tests__/ComponentVersionDetailsView.spec.tsx`:
- Around line 4-8: Replace the deep relative imports in this test with the
project path aliases; specifically change the imports for FLAGS,
useIsOnFeatureFlag, useComponent, renderWithQueryClientAndRouter, and
mockUseNamespaceHook to use their configured aliases (e.g. import { FLAGS } from
'~/feature-flags/flags', useIsOnFeatureFlag from '~/feature-flags/hooks',
useComponent from '~/hooks/useComponents', renderWithQueryClientAndRouter from
'~/unit-test-utils', and mockUseNamespaceHook from
'~/unit-test-utils/mock-namespace') so the file uses absolute aliased paths
instead of '../../../../' style relative paths.
In
`@src/components/ComponentsPage/ComponentVersionDetails/__tests__/ComponentVersionLatestBuild.spec.tsx`:
- Around line 76-82: In the test "should show spinner while pipeline run or task
runs are loading" replace the fragile document.querySelector('.pf-v5-c-spinner')
lookup with a semantic query for the spinner role (e.g., use getByRole or
screen.getByRole with role="progressbar") so the assertion checks for the
spinner via its ARIA role; update the assertion in the
ComponentVersionLatestBuild spec to assert presence using
getByRole('progressbar') instead of document.querySelector to avoid relying on
PatternFly internal class names.
- Around line 1-10: The test file ComponentVersionLatestBuild.spec.tsx uses deep
relative imports; replace them with the project's absolute path aliases (e.g.
import PipelineRunLabel from '~/consts/pipelinerun', useImageProxy from
'~/hooks/useImageProxy', useImageRepository from '~/hooks/useImageRepository',
useLatestSuccessfulBuildPipelineRunForComponentAndBranchV2 from
'~/hooks/useLatestPushBuildPipeline', useTaskRunsForPipelineRuns from
'~/hooks/useTaskRunsV2', renderWithQueryClientAndRouter and mockUseNamespaceHook
from '~/unit-test-utils', and ComponentVersionLatestBuild from
'~/components/ComponentsPage/ComponentVersionDetails/tabs/ComponentVersionLatestBuild')
so the file uses the configured aliases; update all import statements
accordingly and run the tests to ensure module resolution still works.
In `@src/hooks/__tests__/useLatestPushBuildPipeline.spec.ts`:
- Around line 125-144: The test for useLatestPushBuildPipelineRunForComponentV2
is missing an assertion that the selector includes the COMMIT_EVENT_TYPE_LABEL
with value 'push'; update the expectation in the spec that calls
mockUsePipelineRunsV2 to include expect.objectContaining({
'pipelines.appstudio.openshift.io/type': 'build',
'appstudio.openshift.io/component': 'my-component', [COMMIT_EVENT_TYPE_LABEL]:
'push' }) (or add a separate expect.objectContaining for the
COMMIT_EVENT_TYPE_LABEL) so the mockUsePipelineRunsV2 call is asserted to
include the push event type label.
🧹 Nitpick comments (16)
src/components/NamespaceList/__tests__/useNamespaceActions.spec.tsx (1)
78-81: Good fix: usingwaitForinstead of zero-delaysetTimeout.This correctly awaits the async re-render before asserting. However, the same fragile
new Promise((resolve) => setTimeout(resolve, 0))pattern remains in three other tests (lines 65, 99, 110). Consider applyingwaitForconsistently to at least lines 65 and 110 to avoid intermittent failures. Line 99 is more nuanced (checking intermediate loading state) but could also benefit fromwaitFor.Example: applying `waitFor` to the test at line 65
// Wait for the permission check to complete - await new Promise((resolve) => setTimeout(resolve, 0)); - - expect(mockCheckReviewAccesses).toHaveBeenCalledTimes(1); + await waitFor(() => { + expect(mockCheckReviewAccesses).toHaveBeenCalledTimes(1); + });Example: applying `waitFor` to the test at line 110
// Trigger permission check first result.current[2](true); - await new Promise((resolve) => setTimeout(resolve, 0)); + await waitFor(() => { + expect(mockCheckReviewAccesses).toHaveBeenCalled(); + });src/hooks/__tests__/useComponentBranches.spec.ts (1)
1-80: Good test coverage overall.Tests cover null/undefined inputs, loading, error, deduplication, sorting, and selector verification. Well-structured with clear Arrange-Act-Assert patterns.
One minor gap: there's no test for pipeline runs that lack the
COMMIT_BRANCH_ANNOTATIONentirely (e.g., a run withannotations: {}). The hook handles this via theif (branch)guard, but an explicit test would document that contract.src/components/ComponentsPage/ComponentVersionDetails/__tests__/ComponentVersionActivityTab.spec.tsx (1)
84-94: PreferuserEvent.setup()overfireEventfor user interactions.The coding guidelines specify using
userEvent.setup()with async interactions instead of synchronousfireEvent. This also eliminates the need for the manualact()wrapper.Proposed fix
+import userEvent from '@testing-library/user-event'; + it('should navigate to version activity path when Pipeline runs tab is clicked', async () => { + const user = userEvent.setup(); componentMock.mockReturnValue([MockComponents[0], true]); renderWithQueryClientAndRouter(<ComponentVersionActivityTab />); screen.getByTestId('comp-version-activity-tab-commits'); const plrTab = screen.getByTestId('comp-version-activity-tab-pipelineruns'); - await act(() => fireEvent.click(plrTab)); + await user.click(plrTab); expect(navigateMock).toHaveBeenCalledWith( '/ns/test-ns/components/test-component/vers/main/activity/pipelineruns', ); });As per coding guidelines: "Use
userEvent.setup()and async user interactions instead of synchronousfireEventfor testing user actions."src/components/ComponentsPage/ComponentDetails/tabs/ComponentActivityTab.tsx (2)
79-81:nonTestSnapShotFilteris recreated on every render.This filter function is defined inline in the component body and passed as a prop to
PipelineRunsTab. It will create a new reference on each render, potentially triggering unnecessary re-renders or re-computations downstream. Consider extracting it as a module-level constant since it has no dependencies on component state or props.Proposed fix
+// Module-level constant — no closure over component state needed +const nonTestSnapShotFilter = (plr: PipelineRunKind) => + plr.metadata.labels?.[PipelineRunLabel.PIPELINE_TYPE] !== 'test' || + !plr.spec.params?.find((p) => p.name === 'SNAPSHOT'); + export const ComponentActivityTab: React.FC = () => { ... - const nonTestSnapShotFilter = (plr: PipelineRunKind) => - plr.metadata.labels?.[PipelineRunLabel.PIPELINE_TYPE] !== 'test' || - !plr.spec.params?.find((p) => p.name === 'SNAPSHOT'); - return (
92-100: Inline style object creates a new reference each render.Minor: the
styleobject onTabsis recreated every render. Consider extracting it as a constant.const tabsStyle = { width: 'fit-content', marginBottom: 'var(--pf-v5-global--spacer--md)' } as const;src/components/ComponentsPage/ComponentVersionDetails/__tests__/ComponentVersionDetailsView.spec.tsx (1)
70-71: Prefer semantic queries overgetByTestIdwhere possible.Lines 70 and 81 use
getByTestId('spinner')andgetByTestId('details'). The coding guidelines recommend prioritizinggetByRoleorgetByLabelText. For example, a spinner typically renders withrole="status"orrole="progressbar"in PatternFly, which could be queried semantically.If the underlying components don't expose accessible roles, the current approach is acceptable—but worth checking.
Also applies to: 81-81
src/components/ComponentsPage/ComponentVersionDetails/__tests__/ComponentVersionLatestBuild.spec.tsx (3)
12-26: Jest mock module paths should also use path aliases if supported by the Jest config.The
jest.mock(...)calls use relative paths. If the project's JestmoduleNameMappersupports~/aliases, these should be updated consistently. Otherwise, this is a known limitation.
94-107: Consider using a semantic query for the build logs element if possible.Line 105 uses
getByTestIdwhich the coding guidelines recommend as a last resort. If this element has an accessible role or label, prefergetByRoleorgetByLabelText. If not, the current approach is acceptable but consider adding an accessible label to the component.As per coding guidelines: "Prioritize semantic queries (
getByRole,getByLabelText) over implementation-detail queries (getByTestId,getByClassName)."
68-107: Consider adding error/edge-case tests for other hooks.Tests cover the main pipeline run error, loading, empty, and success states well. However, there are no tests for error states from
useTaskRunsForPipelineRuns,useImageProxy, oruseImageRepository. If the component handles those errors (e.g., shows fallback UI), they should be covered.Based on learnings: "Test both success and error scenarios for all components and features."
src/components/ComponentsPage/ComponentVersionDetails/__tests__/ComponentVersionOverviewTab.spec.tsx (3)
4-7: Use absolute path aliases instead of relative imports.The coding guidelines require absolute imports using configured path aliases (
~/hooks,~/unit-test-utils, etc.) instead of deep relative paths like../../../../hooks/useComponents.Suggested fix
-import { useComponent } from '../../../../hooks/useComponents'; -import { renderWithQueryClientAndRouter } from '../../../../unit-test-utils'; -import { mockUseNamespaceHook } from '../../../../unit-test-utils/mock-namespace'; -import ComponentVersionOverviewTab from '../tabs/ComponentVersionOverviewTab'; +import { useComponent } from '~/hooks/useComponents'; +import { renderWithQueryClientAndRouter } from '~/unit-test-utils'; +import { mockUseNamespaceHook } from '~/unit-test-utils/mock-namespace'; +import ComponentVersionOverviewTab from '../tabs/ComponentVersionOverviewTab';Note: the relative import for the sibling component under test (
../tabs/...) is acceptable since it's within the same feature directory.As per coding guidelines: "Use absolute imports with configured path aliases:
~/components,~/types,~/k8s,~/utils,~/models,@routes".
69-82: Consider asserting the repository link href.The success-path test verifies labels and text but doesn't assert that the repository URL (
https://example.com/repo) is actually rendered as a link. Adding a check likeexpect(screen.getByRole('link', { name: /example.com\/repo/ })).toHaveAttribute('href', 'https://example.com/repo')would catch regressions in the link wiring.
14-16: Use absolute import alias in jest.mock path for consistency with project guidelines.The
jest.mockpath should use the configured alias (~/hooks/useComponents) to match the project's standard for absolute imports. While the relative path works, several test files already follow the absolute alias pattern in jest.mock calls, ensuring consistency across the codebase.src/hooks/__tests__/useLatestPushBuildPipeline.spec.ts (4)
2-2: Use absolute import alias for types.Per project conventions, prefer
~/typesover relative../../types.-import { PipelineRunKind } from '../../types'; +import { PipelineRunKind } from '~/types';As per coding guidelines,
src/**/*.{ts,tsx}: "Use absolute imports with configured path aliases:~/components,~/types,~/k8s,~/utils,~/models,@routes".
25-48: Missing error scenario test.This
describeblock covers the happy path and empty-results case but omits the error state (e.g., whenusePipelineRunsV2returns an error). Adding a test likeit('should propagate error', ...)with a mock returning[[], false, new Error('fail')]would improve coverage.Based on learnings: "Test both success and error scenarios for all components and features."
50-74: Also missing error scenario here.Same gap — no test for when
usePipelineRunsV2returns an error. Particularly relevant for this hook since it has thegetNextPageeffect that should not fire on error (per the!errorguard in the hook source).
76-118: Good branch-filter coverage; error scenario still missing.The three tests cover the key branch-filtering logic well. Consider adding a test that verifies behavior when
usePipelineRunsV2returns an error — the hook should not callgetNextPageand should propagate the error.
...onents/ComponentsPage/ComponentVersionDetails/__tests__/ComponentVersionDetailsView.spec.tsx
Outdated
Show resolved
Hide resolved
...onents/ComponentsPage/ComponentVersionDetails/__tests__/ComponentVersionLatestBuild.spec.tsx
Outdated
Show resolved
Hide resolved
...mponentsPage/Versions/ComponentVersionDetails/__tests__/ComponentVersionLatestBuild.spec.tsx
Show resolved
Hide resolved
marcin-michal
left a comment
marcin-michal
left a comment
There was a problem hiding this comment.
I think it's better to create a separate folder for versions, same as it's for commits, pipeline runs etc.
| [currentTab, getActivityTabRoute, navigate], | ||
| ); | ||
|
|
||
| React.useEffect(() => { |
There was a problem hiding this comment.
why not reuse the same logic as in ActivityTab.tsx?
There was a problem hiding this comment.
Done – added Versions/ with ComponentVersionsList/ and ComponentVersionDetails/, same idea as Commits/PipelineRun.
| // We will not include any test pipelines that were run against the snapshot that contained the image. | ||
| // If there is such a test pipeline directly run on the image itself, and not on the snapshot, then we want to include it | ||
| const nonTestSnapShotFilter = (plr: PipelineRunKind) => | ||
| plr.metadata.labels?.[PipelineRunLabel.PIPELINE_TYPE] !== 'test' || |
There was a problem hiding this comment.
why exclude test snapshot pipeline runs, when it's not the case in the current activity tab?
There was a problem hiding this comment.
Removed the custom filter so component Activity matches the main Activity tab and shows all pipeline runs.
| componentName ?? '', | ||
| true, | ||
| ); | ||
| const [branches, branchesLoaded, branchesError] = useComponentBranches( |
There was a problem hiding this comment.
Versions have been added to the component model https://github.com/konflux-ci/application-api/blob/main/api/v1alpha1/component_types.go#L76-L79 . Use this instead.
Take a look at https://drive.google.com/file/d/1NNS98eJjKOl7jhRRhhyO-iydgA_xKD6n/view, more specifically the part where Sahil mentions how to add the new component model to the cluster.
| ) : ( | ||
| <List isPlain isBordered data-test="component-versions-list"> | ||
| {branches.map((branch) => ( | ||
| <ListItem key={branch}> |
There was a problem hiding this comment.
this list should list more than just name of the version
There was a problem hiding this comment.
+1
The way it is current implemented it is not what is expected in design
There was a problem hiding this comment.
Implemented a table with Name, Description, Git branch/tag, and Pipeline (with links).
| import { useComponent, useComponents } from '../../../../hooks/useComponents'; | ||
| import { usePipelineRunsV2 } from '../../../../hooks/usePipelineRunsV2'; | ||
| import { renderWithQueryClientAndRouter } from '../../../../unit-test-utils'; | ||
| import { mockUseNamespaceHook } from '../../../../unit-test-utils/mock-namespace'; |
There was a problem hiding this comment.
Spec already uses ~ for hooks/components; no change needed.
src/hooks/useComponentBranches.ts
Outdated
| * Note: Uses a limit of 100 pipeline runs; branches whose runs fall outside | ||
| * the first 100 (by creation time) will not appear in the list. | ||
| */ | ||
| export const useComponentBranches = ( |
There was a problem hiding this comment.
Removed useComponentBranches; the UI uses useComponentVersions only.
| const matchExpressions: MatchExpression[] = [...(filterBy.matchExpressions ?? [])]; | ||
|
|
||
| // Build the final selector (excluding custom filter fields) | ||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars |
There was a problem hiding this comment.
leave the no-unused-vars comment, this does essentially the same thing but is more verbose and confusing
There was a problem hiding this comment.
Simplified selector to only matchLabels and matchExpressions and updated the test to match.
|
@coderabbitai how can I test this? |
|
Tip For best results, initiate chat on the files or code changes.
Prerequisites
Testing Steps1. Access Component Versions List
2. Test Version Details - Overview Tab
3. Test Version Details - Activity Tab
4. Test Component Activity Tab (at component level)
Edge Cases to Test
Feature Flag Test
Let me know if you need any clarification on these steps! |
There was a problem hiding this comment.
The views are different from what is expected from design. Please adjust your components following design for this.
One more thing, I navigate to /ns/<my-namespace>/components/<my-component>/versions to access the list of versions(which should be a table) and I navigate to /ns/<my-namespace>/components/<my-component>/vers/<my-version> to access version details. Why for one is version and for other is vers? I think the should be the same.
| [currentTab, getActivityTabRoute, navigate], | ||
| ); | ||
|
|
||
| React.useEffect(() => { |
| ) : ( | ||
| <List isPlain isBordered data-test="component-versions-list"> | ||
| {branches.map((branch) => ( | ||
| <ListItem key={branch}> |
There was a problem hiding this comment.
+1
The way it is current implemented it is not what is expected in design
| return null; | ||
| } | ||
|
|
||
| if (!componentLoaded) { |
There was a problem hiding this comment.
nit: you can put both checks for componentLoaded and branchesLoaded together here:
if (!componentLoaded || !branchesLoaded) {
return (
<Bullseye>
<Spinner data-test="spinner" />
</Bullseye>
);
}There was a problem hiding this comment.
We now use versionsLoaded and have a single !componentLoaded || !versionsLoaded check.
| } | ||
|
|
||
| return ( | ||
| <IfFeature flag="components-page" fallback={null}> |
There was a problem hiding this comment.
no need to set fallback prop here
There was a problem hiding this comment.
Removed the fallback on the Versions tab.
| @@ -0,0 +1,108 @@ | |||
| import React from 'react'; | |||
There was a problem hiding this comment.
Removed the unused React import
| it('should show error state when pipeline run fails to load', () => { | ||
| mockUseLatestBuild.mockReturnValue([undefined, true, new Error('Failed')]); | ||
| renderWithQueryClientAndRouter( | ||
| <ComponentVersionLatestBuild component={mockComponent} branchName="main" />, |
There was a problem hiding this comment.
it seems all the below check failures make sense, you can check locally by running yarn tsc
There was a problem hiding this comment.
Type issues in that spec are fixed.
| @@ -0,0 +1,83 @@ | |||
| import React from 'react'; | |||
There was a problem hiding this comment.
Removed the unused React import
| }} | ||
| > | ||
| <DescriptionListGroup> | ||
| <DescriptionListTerm>Branch</DescriptionListTerm> |
There was a problem hiding this comment.
the list items here doesn't match the design, unless I'm seeing an not up to date version of the Figma file :)
There was a problem hiding this comment.
This is been updated with the Figma with matching the table. Thanks @rrosatti
| {verName} <FeatureFlagIndicator flags={['components-page']} fullLabel /> | ||
| </Text> | ||
| } | ||
| breadcrumbs={[ |
There was a problem hiding this comment.
I believe we should not have breadcrumbs here
There was a problem hiding this comment.
The breadcrumbs has been removed
| kind: 'PipelineRun', | ||
| metadata: { name: 'build-1', namespace: 'test-ns', creationTimestamp: '2025-01-01T00:00:00Z' }, | ||
| spec: {}, | ||
| status: { conditions: [{ type: 'Succeeded', status: 'True' }] }, |
There was a problem hiding this comment.
addressed the type issues in that spec.
| return [latestSuccess, loaded, error]; | ||
| }; | ||
|
|
||
| export const useLatestSuccessfulBuildPipelineRunForComponentAndBranchV2 = ( |
There was a problem hiding this comment.
as I mentioned in a previous comment, I'd try to reuse useLatestSuccessfulBuildPipelineRunForComponentV2
There was a problem hiding this comment.
Confirmed – the branch hook just calls that one with the branch param.
src/routes/paths.ts
Outdated
| `:${RouterParams.activityTab}`, | ||
| ); | ||
|
|
||
| export const COMPONENT_VERSION_DETAILS_PATH = COMPONENT_DETAILS_V2_PATH.extend('vers').extend( |
There was a problem hiding this comment.
Updated – routes use versions everywhere.
Assisted-by: Cursor
Assisted-by: Cursor
Assisted-by: Cursor
|
/review |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@src/components/ComponentsPage/ComponentVersionDetails/__tests__/ComponentVersionDetailsView.spec.tsx`:
- Around line 20-21: The mock declaration for jest.mock('~/feature-flags/hooks')
is using a deep relative path inside jest.requireActual which is inconsistent
and fragile; change the requireActual call to use the same alias
(jest.requireActual('~/feature-flags/hooks')) so both sides reference the
identical module path; update the test file where jest.mock and
jest.requireActual are used to import the real implementation via the
'~/feature-flags/hooks' alias (preserving the rest of the spread behavior) to
match project import conventions and avoid brittle relative paths.
In
`@src/components/ComponentsPage/ComponentVersionDetails/tabs/ComponentVersionOverviewTab.tsx`:
- Line 42: The wrapper div with className "component-version-details" inside
ComponentVersionOverviewTab should be replaced with a PatternFly Stack: import
Stack (and StackItem if needed) from '@patternfly/react-core', replace the <div
className="component-version-details"> container with <Stack hasGutter> and wrap
each DetailsSection block in a <StackItem> (or rely on Stack children if
acceptable); remove the raw div and any custom layout CSS tied to that class or
move necessary styling to a class on the Stack, preserving existing content and
spacing.
🧹 Nitpick comments (8)
src/components/ComponentsPage/ComponentVersionDetails/__tests__/ComponentVersionDetailsView.spec.tsx (2)
67-71: Prefer semantic queries where possible overgetByTestId.Lines 70 and 81 use
getByTestId('spinner')andgetByTestId('details'). The coding guidelines recommend prioritizing semantic queries (getByRole,getByLabelText) overgetByTestId. If the spinner uses a PatternFly<Spinner>(which renders withrole="progressbar"),getByRole('progressbar')would be more resilient and accessible-aligned.For
getByTestId('details'), check if there's a suitable heading or landmark role available.As per coding guidelines: "Prioritize semantic queries (
getByRole,getByLabelText) over implementation-detail queries (getByTestId,getByClassName) when selecting elements in tests."Also applies to: 79-84
46-53: MovemockUseNamespaceHookintobeforeEachfor test isolation consistency.The
mockUseNamespaceHookcall at describe scope (line 47) usesjest.spyOn()which persists across all tests without reset. Other mocks (useParamsMock,useComponentMock,mockUseIsOnFeatureFlag) are properly reset inbeforeEach, creating an inconsistency that risks test pollution. Move it intobeforeEachto ensure proper isolation and match the pattern used for other mocks.src/components/ComponentsPage/ComponentVersionDetails/tabs/ComponentVersionOverviewTab.tsx (1)
51-51: Redundant null coalescing after early return guard.
versionNameis guaranteed non-null after the guard on line 24. The?? '-'on line 51 and?? ''on line 65 are dead code paths. Not a bug, but slightly misleading.src/routes/page-routes/components-page.tsx (1)
9-14: Consider using the~/componentspath alias for consistency.The new import block on lines 9–14 uses a relative path (
../../components/ComponentsPage/ComponentVersionDetails). The project's coding guidelines recommend absolute imports with configured path aliases. Line 1 already uses@routes, so aligning new imports with~/components/ComponentsPage/ComponentVersionDetailswould be more consistent.That said, line 8 (pre-existing) also uses a relative path, so this is a broader file-level concern rather than something introduced solely by this PR.
As per coding guidelines, "Use absolute imports with configured path aliases:
~/components,~/types,~/k8s,~/utils,~/models,@routes".src/routes/page-routes/__tests__/components-page.spec.tsx (1)
69-85: Consider adding assertions for the nested children ofversionDetailsRoute.The test verifies that
versionDetailsRoute.childrenis defined but doesn't assert on the structure of those nested children (overview index route, activity route, activity/:activityTab route). Adding a few assertions would improve coverage of the new route hierarchy.Suggested additions
expect(versionDetailsRoute.path).toBe(`versions/:versionName`); expect(versionDetailsRoute.children).toBeDefined(); + + const versionChildren = versionDetailsRoute.children ?? []; + expect(versionChildren).toHaveLength(3); + expect(versionChildren[0].index).toBe(true); + expect(versionChildren[1].path).toBe('activity'); + expect(versionChildren[2].path).toBe('activity/:activityTab'); });src/hooks/useLatestPushBuildPipeline.ts (2)
35-71: Branch filtering integration looks correct.The optional
branchNameis properly threaded into the selector viafilterByTargetBranchand included in theuseMemodependency array. The conditional spread on Line 49 ensures no stale keys leak whenbranchNameisundefined.One minor note:
latestSuccesson Line 56 can evaluate tofalse(not justPipelineRunKind | undefined) whenloadedisfalse, since the&&short-circuits. The return type on Line 39 declaresPipelineRunKind | undefined, which doesn't coverfalse. This is a pre-existing issue, but worth noting since you're already touching this function.Optional: tighten the expression to match the declared return type
const latestSuccess = React.useMemo( () => - loaded && - !error && - pipelines?.find((pipeline) => pipelineRunStatus(pipeline) === runStatus.Succeeded), + loaded && !error + ? pipelines?.find((pipeline) => pipelineRunStatus(pipeline) === runStatus.Succeeded) + : undefined, [error, loaded, pipelines], );
73-78: Wrapper hook is a thin alias — consider whether it adds value.
useLatestSuccessfulBuildPipelineRunForComponentAndBranchV2has an identical signature and body to calling the underlying hook directly. If the intent is to provide a semantically distinct name for branch-scoped usage, that's fine — just confirming this is deliberate and not an intermediate refactoring artifact.src/hooks/__tests__/useLatestPushBuildPipeline.spec.ts (1)
29-52: Consider adding an error-state test case.The
useLatestBuildPipelineRunForComponentV2describe block tests loaded and empty states but not the error path (e.g.,mockUsePipelineRunsV2.mockReturnValue([[], false, new Error('fail')])). Same applies to the other describe blocks. Error scenarios help catch regressions in error propagation.
...onents/ComponentsPage/ComponentVersionDetails/__tests__/ComponentVersionDetailsView.spec.tsx
Outdated
Show resolved
Hide resolved
src/components/ComponentsPage/ComponentVersionDetails/tabs/ComponentVersionOverviewTab.tsx
Outdated
Show resolved
Hide resolved
Assisted-by: Cursor
Assisted-by: Cursor
Assisted-by: Cursor
Assisted-by: Cursor
Assisted-by: Cursor
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai resolve fixed comments |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 980
The resolved threads covered:
All your fixed comments have been resolved! ✅ 🧠 Learnings used |
Assisted-by: Cursor
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (10)
src/components/ComponentsPage/Versions/ComponentVersionDetails/__tests__/ComponentVersionActivityTab.spec.tsx (1)
83-93: PreferuserEventoverfireEventfor click interactions.The coding guidelines require
userEvent.setup()for user interactions.♻️ Proposed refactor
+import userEvent from '@testing-library/user-event'; ... - it('should navigate to version activity path when Pipeline runs tab is clicked', async () => { + it('should navigate to version activity path when Pipeline runs tab is clicked', async () => { + const user = userEvent.setup(); componentMock.mockReturnValue([MockComponents[0], true]); renderWithQueryClientAndRouter(<ComponentVersionActivityTab />); screen.getByTestId('comp-version-activity-tab-commits'); const plrTab = screen.getByTestId('comp-version-activity-tab-pipelineruns'); - await act(() => fireEvent.click(plrTab)); + await user.click(plrTab); expect(navigateMock).toHaveBeenCalledWith( '/ns/test-ns/components/test-component/versions/main/activity/pipelineruns', ); });As per coding guidelines: "Use
userEvent.setup()and async user interactions instead of synchronousfireEventfor testing user actions."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ComponentsPage/Versions/ComponentVersionDetails/__tests__/ComponentVersionActivityTab.spec.tsx` around lines 83 - 93, Replace the synchronous fireEvent click with an async user interaction using userEvent.setup() in the test for ComponentVersionActivityTab: import userEvent from '@testing-library/user-event', create a user via userEvent.setup(), then await user.click(the element found by getByTestId('comp-version-activity-tab-pipelineruns')) instead of await act(() => fireEvent.click(plrTab)); keep the existing assertion against navigateMock unchanged.src/components/ComponentsPage/Versions/ComponentVersionDetails/tabs/ComponentVersionLatestBuild.tsx (2)
36-38:React.PropsWithChildrenis unnecessary — the component doesn't accept or renderchildren.♻️ Proposed refactor
-const ComponentVersionLatestBuild: React.FC< - React.PropsWithChildren<ComponentVersionLatestBuildProps> -> = ({ component, branchName }) => { +const ComponentVersionLatestBuild: React.FC<ComponentVersionLatestBuildProps> = ({ + component, + branchName, +}) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ComponentsPage/Versions/ComponentVersionDetails/tabs/ComponentVersionLatestBuild.tsx` around lines 36 - 38, The component type unnecessarily wraps props with React.PropsWithChildren even though ComponentVersionLatestBuild does not accept or render children; update the declaration to use the plain props type (e.g., React.FC<ComponentVersionLatestBuildProps> or a direct function signature with ComponentVersionLatestBuildProps) and remove React.PropsWithChildren to avoid implying children are supported; adjust the ComponentVersionLatestBuild definition accordingly.
86-89: Raw<div>elements used for layout, violating the PatternFly layout guideline.Line 86 wraps a
Spinnerin a<div className="pf-u-m-lg">. Lines 113–126 use nested<div>elements for the build-completion layout. PatternFly layout components should be used instead.♻️ Proposed refactor for the spinner wrapper (lines 86–89)
- <div className="pf-u-m-lg"> - <Spinner /> - </div> + <Bullseye> + <Spinner /> + </Bullseye>(
Bullseyeis already imported in sibling components; add to imports if not already present.)♻️ Proposed refactor for build-completion layout (lines 113–126)
- <div className="component-details__build-completion"> - <div className="component-details__build-completion--time"> - <div>Completed at</div> - <Timestamp timestamp={pipelineRun?.status?.completionTime ?? '-'} /> - </div> - <Button ...>View build logs</Button> - </div> + <Flex direction={{ default: 'column' }} spaceItems={{ default: 'spaceItemsSm' }}> + <FlexItem> + <Flex spaceItems={{ default: 'spaceItemsSm' }}> + <FlexItem>Completed at</FlexItem> + <FlexItem><Timestamp timestamp={pipelineRun?.status?.completionTime ?? '-'} /></FlexItem> + </Flex> + </FlexItem> + <FlexItem> + <Button ...>View build logs</Button> + </FlexItem> + </Flex>As per coding guidelines: "NEVER use raw HTML layout tags (
<div>,<section>,<header>). Always use PatternFly layout components."Also applies to: 113-127
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ComponentsPage/Versions/ComponentVersionDetails/tabs/ComponentVersionLatestBuild.tsx` around lines 86 - 89, Replace raw <div> layout wrappers in ComponentVersionLatestBuild with PatternFly layout components: wrap the loading Spinner in a Bullseye instead of <div className="pf-u-m-lg"> (import Bullseye if missing) and replace the nested layout used for the build-completion block with a Flex + FlexItem vertical-centered layout (use Flex direction="column" and alignItems="center" and appropriate spacing) rather than raw <div>s; keep existing Spinner and any Text/Title/Icon components but move them into the new Bullseye/Flex structure and update imports to include Bullseye, Flex, and FlexItem.src/hooks/useComponentVersions.ts (1)
52-56: Prefer a locale-independent comparison for ISO 8601 timestamp sorting.
bTime.localeCompare(aTime)is locale-dependent. While it happens to produce correct lexicographic results for zero-padded ISO timestamps in most environments, locale-independent comparison is more robust and is the established pattern in the rest of the codebase (e.g.,getCommitsFromPLRsusesnew Date().getTime()).♻️ Proposed refactor
- const aTime = a.status?.startTime ?? a.metadata?.creationTimestamp ?? ''; - const bTime = b.status?.startTime ?? b.metadata?.creationTimestamp ?? ''; - return bTime.localeCompare(aTime); + const aTime = new Date(a.status?.startTime ?? a.metadata?.creationTimestamp ?? 0).getTime(); + const bTime = new Date(b.status?.startTime ?? b.metadata?.creationTimestamp ?? 0).getTime(); + return bTime - aTime;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useComponentVersions.ts` around lines 52 - 56, The sort currently uses localeCompare on ISO strings in useComponentVersions (variable sorted from runs), which is locale-dependent; change the comparator to perform a numeric timestamp comparison (e.g., Date.parse or new Date(...).getTime()) on each run's timestamp (status.startTime or metadata.creationTimestamp) and return the numeric difference (bTimeMs - aTimeMs) so sorting is locale-independent and consistent with patterns like getCommitsFromPLRs.src/components/ComponentsPage/Versions/ComponentVersionDetails/__tests__/ComponentVersionLatestBuild.spec.tsx (1)
55-108: Loading spinner fortaskRunsLoaded=falseis not covered, andafterEachcleanup is absent.The component's loading guard checks
!taskRunsLoadedindependently of!pipelineRunLoaded, but the test suite only exercises the pipeline run loading path. ThetaskRunsLoaded=falsebranch is untested.♻️ Proposed additions
+ afterEach(() => { + jest.clearAllMocks(); + }); + + it('should show spinner when task runs are loading', () => { + mockUseTaskRuns.mockReturnValue([[], false, undefined]); + renderWithQueryClientAndRouter( + <ComponentVersionLatestBuild component={mockComponent} branchName="main" />, + ); + expect(screen.getByRole('progressbar')).toBeInTheDocument(); + });As per coding guidelines: "Test both success and error scenarios for all components and features."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ComponentsPage/Versions/ComponentVersionDetails/__tests__/ComponentVersionLatestBuild.spec.tsx` around lines 55 - 108, The test suite misses the branch where taskRunsLoaded is false and lacks teardown; add a new test for ComponentVersionLatestBuild that sets mockUseLatestBuild to a loaded state (e.g., [mockPipelineRun, true, undefined]) and mockUseTaskRuns to a loading state (e.g., [[], false, undefined]) then render and assert a spinner (getByRole('progressbar')); also add an afterEach block that calls jest.resetAllMocks() and testing-library cleanup (or equivalent) to reset mocks and DOM between tests, referencing mockUseTaskRuns, mockUseLatestBuild, and ComponentVersionLatestBuild to locate where to change behavior.src/components/ComponentsPage/Versions/ComponentVersionDetails/__tests__/ComponentVersionOverviewTab.spec.tsx (1)
36-92: Consider addingafterEach(() => jest.clearAllMocks())and a test case with populated pipeline run data.
afterEach(() => jest.clearAllMocks())is absent. WhilebeforeEachre-sets return values before each test, clearing mock call history between tests prevents false positives in tests that inspect call counts or arguments.The success test (line 75) sets
mockUseLatestBuildto[null, true, undefined](no pipeline run). There's no test that exercises the "Latest commit" and "Latest pipelinerun" links when pipeline run data is present — those branches of the render logic are untested.♻️ Proposed additions
+ afterEach(() => { + jest.clearAllMocks(); + }); + + it('should render pipeline run and commit links when data is loaded', () => { + mockUseLatestBuild.mockReturnValue([ + { + metadata: { name: 'plr-1', namespace: 'test-ns' }, + spec: { pipelineRef: { name: 'my-pipeline' }, params: [] }, + status: { startTime: '2025-01-01T00:00:00Z' }, + }, + true, + undefined, + ]); + renderWithQueryClientAndRouter(<ComponentVersionOverviewTab />); + expect(screen.getByText('my-pipeline')).toBeInTheDocument(); + expect(screen.getByText('plr-1')).toBeInTheDocument(); + });As per coding guidelines: "Test both success and error scenarios for all components and features."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ComponentsPage/Versions/ComponentVersionDetails/__tests__/ComponentVersionOverviewTab.spec.tsx` around lines 36 - 92, Add an afterEach hook to clear mock state by adding afterEach(() => jest.clearAllMocks()) so mock call history is reset between tests, and add a new test (e.g., extending "should render overview with version details and latest build" or a separate "should render links when pipeline run present") that sets mockUseLatestBuild to return a populated pipeline run object (not [null, true, undefined]) and asserts the "Latest commit" and "Latest pipelinerun" links render and point to the expected values; update useComponentMock/useParamsMock as needed in that test to simulate the loaded state so the render path exercising pipeline run links is covered.src/components/ComponentsPage/ComponentDetails/tabs/ComponentActivityTab.tsx (1)
70-74: Replace inlinestyleprop onTabswith a PatternFly spacing utility or className.Inline styles with raw CSS variable references (
var(--pf-v5-global--spacer--md)) are a minor violation of the guideline to avoid custom CSS layouts in favor of PatternFly components.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ComponentsPage/ComponentDetails/tabs/ComponentActivityTab.tsx` around lines 70 - 74, Replace the inline style prop on the Tabs component by removing the style={{...}} and using PatternFly utility classes via className instead; for example, apply the PatternFly spacing utility (e.g., "pf-v5-u-mb-md") for the bottom margin and a PF display/width utility (e.g., "pf-u-display-inline-block" or a width-fit utility) to achieve the same "fit-content" layout—update the Tabs element (the Tabs component instance) to use these className utilities rather than inline CSS variables.src/components/ComponentsPage/Versions/ComponentVersionDetails/tabs/ComponentVersionActivityTab.tsx (1)
78-80: MovenonTestSnapShotFilteroutside the component.The function captures no component state or props — it depends only on the module-level
PipelineRunLabelconstant. Redefining it on every render creates a new function reference each time, which can causePipelineRunsTabto re-render if it uses the filter in a dependency array or memo.♻️ Proposed fix
+const nonTestSnapShotFilter = (plr: PipelineRunKind) => + plr.metadata.labels?.[PipelineRunLabel.PIPELINE_TYPE] !== 'test' || + !plr.spec.params?.find((p) => p.name === 'SNAPSHOT'); + const ComponentVersionActivityTab: React.FC = () => { ... - const nonTestSnapShotFilter = (plr: PipelineRunKind) => - plr.metadata.labels?.[PipelineRunLabel.PIPELINE_TYPE] !== 'test' || - !plr.spec.params?.find((p) => p.name === 'SNAPSHOT');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ComponentsPage/Versions/ComponentVersionDetails/tabs/ComponentVersionActivityTab.tsx` around lines 78 - 80, nonTestSnapShotFilter is recreated on every render of ComponentVersionActivityTab although it only depends on the module-level PipelineRunLabel and PipelineRunKind; move the function to module scope (outside the ComponentVersionActivityTab function) so it has a stable reference (or export it if needed by other modules) and ensure it still uses the same predicate logic checking plr.metadata.labels?.[PipelineRunLabel.PIPELINE_TYPE] and plr.spec.params?.find((p) => p.name === 'SNAPSHOT') so PipelineRunsTab/memoized consumers won't re-render due to a changing function identity.src/components/ComponentsPage/Versions/ComponentVersionsList/ComponentVersionsTab.tsx (1)
29-29:useParams()missingRouterParamstype parameter — inconsistent withComponentVersionsListRow.
ComponentVersionsListRowusesuseParams<RouterParams>()for the same route context. Using the typed overload here provides compile-time safety and consistency.♻️ Proposed fix
-import { useParams } from 'react-router-dom'; +import { useParams } from 'react-router-dom'; +import { RouterParams } from '~/routes/utils'; ... - const { componentName } = useParams(); + const { componentName } = useParams<RouterParams>();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ComponentsPage/Versions/ComponentVersionsList/ComponentVersionsTab.tsx` at line 29, ComponentVersionsTab is calling useParams without the RouterParams type parameter which is inconsistent with ComponentVersionsListRow; update the hook usage in ComponentVersionsTab (the line with useParams()) to call useParams<RouterParams>() so it matches ComponentVersionsListRow and provides compile-time typing for route params.src/components/ComponentsPage/Versions/ComponentVersionsList/ComponentVersionsListRow.tsx (1)
23-26:componentNamefromuseParamsis redundant —customData.componentNamecarries the same value.
ComponentVersionsTab(the parent) already readscomponentNamefromuseParams()and passes it down incustomData. CallinguseParams<RouterParams>()here again adds a coupling to the route context that's unnecessary.♻️ Proposed refactor
-import { Link, useParams } from 'react-router-dom'; +import { Link } from 'react-router-dom'; ... -import { RouterParams } from '~/routes/utils'; ... const ComponentVersionsListRow: React.FC<...> = ({ obj, customData }) => { - const { componentName } = useParams<RouterParams>(); - const namespace = customData?.namespace ?? ''; + const namespace = customData?.namespace ?? ''; + const componentName = customData?.componentName; ... actions: deps → [namespace, componentName, obj.name] // unchanged🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ComponentsPage/Versions/ComponentVersionsList/ComponentVersionsListRow.tsx` around lines 23 - 26, Remove the redundant route coupling by deleting the useParams<RouterParams>() call and instead read componentName from the passed-in customData; replace any local references to the imported componentName with customData?.componentName (e.g., set const componentName = customData?.componentName ?? ''), update any usages in ComponentVersionsListRow to reference that variable, and remove the unused import/type references related to useParams/RouterParams so the component relies only on the parent-provided data (ComponentVersionsTab -> customData.componentName).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/components/ComponentsPage/ComponentDetails/tabs/ComponentActivityTab.tsx`:
- Line 68: The description in ComponentActivityTab.tsx is incorrect for the
component-scoped Activity tab; update the text used in the ComponentActivityTab
component (the description prop/string) to refer to activity for this specific
component (e.g., "Monitor your commits and their pipeline progression for this
component across its branches.") so it no longer implies "across all
components."
In
`@src/components/ComponentsPage/Versions/ComponentVersionDetails/__tests__/ComponentVersionActivityTab.spec.tsx`:
- Around line 34-38: The jest.mock block in ComponentVersionActivityTab.spec.tsx
references React.ReactNode but React is not imported; import the ReactNode type
from 'react' (or replace React.ReactNode with the named ReactNode) and update
the file's imports so the mock's IfFeature prop typing uses the imported
ReactNode type to fix the TypeScript namespace error.
In
`@src/components/ComponentsPage/Versions/ComponentVersionDetails/tabs/ComponentVersionActivityTab.tsx`:
- Around line 21-36: ComponentVersionActivityTab lacks the null-guard for
componentName and null-coalescing when building routes; add the same defensive
check used in ComponentActivityTab (i.e., after hooks, if (!componentName)
return null) to avoid rendering with undefined, and update the
getActivityTabRoute (and any createPath calls) to pass componentName:
componentName ?? '' and versionName: versionName ?? '' so generated paths never
contain the literal "undefined".
- Line 84: The outer raw <div> wrapping DetailsSection in the
ComponentVersionActivityTab component violates the layout guidelines; remove the
unnecessary <div> (or replace it with a React.Fragment or a PatternFly
PageSection) so that DetailsSection is rendered directly inside the
ComponentVersionActivityTab render/return, ensuring you update the JSX in
ComponentVersionActivityTab to no longer include the raw HTML wrapper around
DetailsSection.
In
`@src/components/ComponentsPage/Versions/ComponentVersionDetails/tabs/ComponentVersionOverviewTab.tsx`:
- Around line 28-33: The hook call to
useLatestSuccessfulBuildPipelineRunForComponentAndBranchV2 is using redundant
and problematic args: replace the current arguments (namespace,
component?.metadata?.name ?? '', versionName ?? undefined) with (namespace,
componentName, versionName) so you pass the route param componentName (avoiding
the empty-string while the component loads) and the existing versionName
directly (no ?? undefined since versionName is already string | undefined);
update the call site in ComponentVersionOverviewTab.tsx accordingly.
In
`@src/components/ComponentsPage/Versions/ComponentVersionsList/__tests__/ComponentVersionsTab.spec.tsx`:
- Line 25: The test uses the IfFeature mock component which types its children
as React.ReactNode but React is not imported; add the missing top-level import
"import React from 'react';" so the React namespace is in scope for the
React.ReactNode type used in IfFeature (and keep the existing JSX fragment
<>...</> unchanged).
- Around line 64-76: Add the missing React import to the test file so TypeScript
recognizes React.ReactNode (e.g., import React from 'react') to fix the
strict-mode type error; keep the existing getByTestId('spinner') checks
(jest.setup.js sets testIdAttribute to "data-test") but replace the
implementation-detail queries getByTestId('version-name-main') and
getByTestId('version-name-develop') with semantic queries (e.g., getByText(...)
or getByRole(...) scoped to the table/row) inside ComponentVersionsTab.spec.tsx
so the tests assert visible text/roles instead of test ids.
In
`@src/components/ComponentsPage/Versions/ComponentVersionsList/ComponentVersionsTab.tsx`:
- Line 117: The ToolbarItem using inline style (style={{ marginLeft: 'auto' }})
should be changed to use PatternFly's align prop: remove the style prop from the
ToolbarItem and add align="alignEnd" (or a responsive align value if needed) so
the element is right-aligned; update the JSX in ComponentVersionsTab where
<ToolbarItem ...> is defined to use align="alignEnd" instead of the inline
marginLeft hack.
- Around line 71-73: The useEffect block that watches nameFilter and calls
setPage(1) is redundant because the debounced callback onNameFilter already
calls setPage(1); remove the React.useEffect(...) that references nameFilter to
avoid the extra no-op render and rely on onNameFilter to reset page, ensuring
you only keep the existing onNameFilter, setPage, and nameFilter logic.
- Around line 29-34: The controlled SearchInput is bound to the debounced
nameFilter while useComponent(..., true) causes re-renders from Kubernetes watch
events, which drops keystrokes; fix by introducing a separate immediate input
state (e.g., searchInput or nameFilterInput) for the SearchInput value and keep
the existing debounced nameFilter for querying/filtering; update the immediate
state on every onChange so the input is responsive, run your existing debounce
effect to sync that immediate state into the debounced nameFilter used by
useComponent/filters, and leave useComponent(namespace, componentName ?? '',
true) unchanged so watch behavior continues but no longer resets the live input
during debounces.
In `@src/hooks/useComponentVersions.ts`:
- Around line 21-37: The hard-coded limit of 100 in the usePipelineRunsV2 call
can drop older branch runs; update the call in useComponentVersions.ts (the
usePipelineRunsV2 invocation that uses PipelineRunLabel.COMPONENT and the
componentName selector) to use a larger limit (e.g., 500) or replace the literal
with a clearly named constant (e.g., PIPELINE_RUNS_LIMIT) so it’s easy to
adjust, and add a brief comment noting the trade-off (why the higher limit or
that trimming may omit older branches) so reviewers/users are aware of the
limitation.
In `@src/routes/page-routes/__tests__/components-page.spec.tsx`:
- Around line 72-84: The test destructures children into indexRoute,
activityWithTabRoute, activityRoute, versionsRoute, versionDetailsRoute without
asserting children length, and uses weak toBeDefined() assertions for
versionsRoute.element and versionDetailsRoute.children; add an explicit
expect(children).toHaveLength(5) (or the exact expected length) before
destructuring to prevent mis-assignment, and replace the toBeDefined() checks
with precise assertions that match the expected structure for
versionsRoute.element and versionDetailsRoute.children (e.g.,
toEqual/toStrictEqual against the expected component element or child routes or
use a snapshot) while keeping the existing explicit check for
indexRoute.element.
---
Nitpick comments:
In
`@src/components/ComponentsPage/ComponentDetails/tabs/ComponentActivityTab.tsx`:
- Around line 70-74: Replace the inline style prop on the Tabs component by
removing the style={{...}} and using PatternFly utility classes via className
instead; for example, apply the PatternFly spacing utility (e.g.,
"pf-v5-u-mb-md") for the bottom margin and a PF display/width utility (e.g.,
"pf-u-display-inline-block" or a width-fit utility) to achieve the same
"fit-content" layout—update the Tabs element (the Tabs component instance) to
use these className utilities rather than inline CSS variables.
In
`@src/components/ComponentsPage/Versions/ComponentVersionDetails/__tests__/ComponentVersionActivityTab.spec.tsx`:
- Around line 83-93: Replace the synchronous fireEvent click with an async user
interaction using userEvent.setup() in the test for ComponentVersionActivityTab:
import userEvent from '@testing-library/user-event', create a user via
userEvent.setup(), then await user.click(the element found by
getByTestId('comp-version-activity-tab-pipelineruns')) instead of await act(()
=> fireEvent.click(plrTab)); keep the existing assertion against navigateMock
unchanged.
In
`@src/components/ComponentsPage/Versions/ComponentVersionDetails/__tests__/ComponentVersionLatestBuild.spec.tsx`:
- Around line 55-108: The test suite misses the branch where taskRunsLoaded is
false and lacks teardown; add a new test for ComponentVersionLatestBuild that
sets mockUseLatestBuild to a loaded state (e.g., [mockPipelineRun, true,
undefined]) and mockUseTaskRuns to a loading state (e.g., [[], false,
undefined]) then render and assert a spinner (getByRole('progressbar')); also
add an afterEach block that calls jest.resetAllMocks() and testing-library
cleanup (or equivalent) to reset mocks and DOM between tests, referencing
mockUseTaskRuns, mockUseLatestBuild, and ComponentVersionLatestBuild to locate
where to change behavior.
In
`@src/components/ComponentsPage/Versions/ComponentVersionDetails/__tests__/ComponentVersionOverviewTab.spec.tsx`:
- Around line 36-92: Add an afterEach hook to clear mock state by adding
afterEach(() => jest.clearAllMocks()) so mock call history is reset between
tests, and add a new test (e.g., extending "should render overview with version
details and latest build" or a separate "should render links when pipeline run
present") that sets mockUseLatestBuild to return a populated pipeline run object
(not [null, true, undefined]) and asserts the "Latest commit" and "Latest
pipelinerun" links render and point to the expected values; update
useComponentMock/useParamsMock as needed in that test to simulate the loaded
state so the render path exercising pipeline run links is covered.
In
`@src/components/ComponentsPage/Versions/ComponentVersionDetails/tabs/ComponentVersionActivityTab.tsx`:
- Around line 78-80: nonTestSnapShotFilter is recreated on every render of
ComponentVersionActivityTab although it only depends on the module-level
PipelineRunLabel and PipelineRunKind; move the function to module scope (outside
the ComponentVersionActivityTab function) so it has a stable reference (or
export it if needed by other modules) and ensure it still uses the same
predicate logic checking plr.metadata.labels?.[PipelineRunLabel.PIPELINE_TYPE]
and plr.spec.params?.find((p) => p.name === 'SNAPSHOT') so
PipelineRunsTab/memoized consumers won't re-render due to a changing function
identity.
In
`@src/components/ComponentsPage/Versions/ComponentVersionDetails/tabs/ComponentVersionLatestBuild.tsx`:
- Around line 36-38: The component type unnecessarily wraps props with
React.PropsWithChildren even though ComponentVersionLatestBuild does not accept
or render children; update the declaration to use the plain props type (e.g.,
React.FC<ComponentVersionLatestBuildProps> or a direct function signature with
ComponentVersionLatestBuildProps) and remove React.PropsWithChildren to avoid
implying children are supported; adjust the ComponentVersionLatestBuild
definition accordingly.
- Around line 86-89: Replace raw <div> layout wrappers in
ComponentVersionLatestBuild with PatternFly layout components: wrap the loading
Spinner in a Bullseye instead of <div className="pf-u-m-lg"> (import Bullseye if
missing) and replace the nested layout used for the build-completion block with
a Flex + FlexItem vertical-centered layout (use Flex direction="column" and
alignItems="center" and appropriate spacing) rather than raw <div>s; keep
existing Spinner and any Text/Title/Icon components but move them into the new
Bullseye/Flex structure and update imports to include Bullseye, Flex, and
FlexItem.
In
`@src/components/ComponentsPage/Versions/ComponentVersionsList/ComponentVersionsListRow.tsx`:
- Around line 23-26: Remove the redundant route coupling by deleting the
useParams<RouterParams>() call and instead read componentName from the passed-in
customData; replace any local references to the imported componentName with
customData?.componentName (e.g., set const componentName =
customData?.componentName ?? ''), update any usages in ComponentVersionsListRow
to reference that variable, and remove the unused import/type references related
to useParams/RouterParams so the component relies only on the parent-provided
data (ComponentVersionsTab -> customData.componentName).
In
`@src/components/ComponentsPage/Versions/ComponentVersionsList/ComponentVersionsTab.tsx`:
- Line 29: ComponentVersionsTab is calling useParams without the RouterParams
type parameter which is inconsistent with ComponentVersionsListRow; update the
hook usage in ComponentVersionsTab (the line with useParams()) to call
useParams<RouterParams>() so it matches ComponentVersionsListRow and provides
compile-time typing for route params.
In `@src/hooks/useComponentVersions.ts`:
- Around line 52-56: The sort currently uses localeCompare on ISO strings in
useComponentVersions (variable sorted from runs), which is locale-dependent;
change the comparator to perform a numeric timestamp comparison (e.g.,
Date.parse or new Date(...).getTime()) on each run's timestamp (status.startTime
or metadata.creationTimestamp) and return the numeric difference (bTimeMs -
aTimeMs) so sorting is locale-independent and consistent with patterns like
getCommitsFromPLRs.
src/components/ComponentsPage/ComponentDetails/tabs/ComponentActivityTab.tsx
Outdated
Show resolved
Hide resolved
...mponentsPage/Versions/ComponentVersionDetails/__tests__/ComponentVersionActivityTab.spec.tsx
Show resolved
Hide resolved
...ponents/ComponentsPage/Versions/ComponentVersionDetails/tabs/ComponentVersionActivityTab.tsx
Outdated
Show resolved
Hide resolved
...ponents/ComponentsPage/Versions/ComponentVersionDetails/tabs/ComponentVersionActivityTab.tsx
Outdated
Show resolved
Hide resolved
...ponents/ComponentsPage/Versions/ComponentVersionDetails/tabs/ComponentVersionOverviewTab.tsx
Show resolved
Hide resolved
src/components/ComponentsPage/Versions/ComponentVersionsList/ComponentVersionsTab.tsx
Show resolved
Hide resolved
src/components/ComponentsPage/Versions/ComponentVersionsList/ComponentVersionsTab.tsx
Outdated
Show resolved
Hide resolved
src/components/ComponentsPage/Versions/ComponentVersionsList/ComponentVersionsTab.tsx
Outdated
Show resolved
Hide resolved
Assisted-by: Cursor
Assisted-by: Cursor
|
@coderabbitai Resolve your review comments if fixed |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 5168
The resolved comments covered:
All your fixes have been acknowledged and the review threads are now resolved. Great work addressing the feedback! 🎉 🧠 Learnings used |
4 participants
Fixes
https://issues.redhat.com/browse/KFLUXUI-1007
Description
Implements the Version tab which contain list page for versions of the component.
The Version tab displays:
-Name
-Description
-Git branch : link to git source branch
-Pipeline
Route: /ns//components//vers
Type of change
Screen shots / Gifs for design review
How to test or reproduce?
Browser conformance:
Summary by CodeRabbit
New Features
Tests