feat(KFLUXUI-1007): add versions tab to component page #714
Open
marcin-michal wants to merge 6 commits intokonflux-ci:mainfrom
Open
feat(KFLUXUI-1007): add versions tab to component page #714marcin-michal wants to merge 6 commits intokonflux-ci:mainfrom
marcin-michal wants to merge 6 commits intokonflux-ci:mainfrom
Conversation
Assisted-by: Cursor
marcin-michal
requested review from
JoaoPedroPP,
StanislavJochman,
abhinandan13jan,
janaki29,
milantaky,
rakshett,
rrosatti,
sahil143 and
testcara
Contributor
✅ 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. |
Contributor
|
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 |
|---|---|
Component Version UI src/components/ComponentVersion/ComponentVersionListView/ComponentVersionListHeader.ts, src/components/ComponentVersion/ComponentVersionListView/ComponentVersionListRow.tsx, src/components/ComponentVersion/ComponentVersionListView/ComponentVersionListView.tsx |
Adds table header config, row renderer (links, branch URL creation, pipeline name resolution), and list view with filtering, sorting state, and toolbar. |
Component Version Tests src/components/ComponentVersion/ComponentVersionListView/__tests__/ComponentVersionListRow.spec.tsx, src/components/ComponentVersion/ComponentVersionListView/__tests__/ComponentVersionListView.spec.tsx |
Unit tests covering row rendering, branch link behavior, pipeline fallback logic, filtering (debounce, case-insensitive), and error/empty states. |
Components Page Integration src/components/ComponentsPage/ComponentDetails/ComponentDetailsView.tsx, src/components/ComponentsPage/tabs/ComponentVersionsTab.tsx, src/components/ComponentsPage/tabs/__tests__/ComponentVersionsTab.spec.tsx |
Use metadata.name for display, add ComponentVersionsTab that mounts the versions list within a FilterContextProvider, and tests verifying rendering and prop passing. |
Routing & Params src/routes/paths.ts, src/routes/utils.ts, src/routes/page-routes/components-page.tsx, src/routes/page-routes/__tests__/components-page.spec.tsx |
Add COMPONENT_VERSIONS_PATH and COMPONENT_VERSION_DETAILS_PATH, add versionRevision router param, and register ComponentVersionsTab in component routes (tests updated). |
Types src/types/component.ts |
Introduce types for versions/pipelines/statuses (e.g., ComponentVersion, PipelineDefinition, ComponentStatus), extend ComponentSource and ComponentSpecs with new fields and deprecation notices for legacy fields. |
Git Utilities src/utils/git-utils.tsx, src/utils/__tests__/git-utils.spec.tsx |
Add createBranchUrl(repoUrl, branch) with provider-specific branch path builders (GitHub, GitLab, Forgejo, Codeberg) and comprehensive tests for edge cases and provider handling. |
Build Config webpack.dev.config.js |
Change devServer.historyApiFallback to { disableDotRule: true } to allow SPA routes containing dots. |
Sequence Diagram
sequenceDiagram
participant User as User
participant Tab as ComponentVersionsTab
participant Filter as FilterContext
participant View as ComponentVersionListView
participant API as ComponentData
participant Table as Table
User->>Tab: Navigate to Versions tab
Tab->>Filter: Initialize filterParams ['name']
Tab->>View: Render(componentName)
View->>API: useComponent(namespace, componentName)
API-->>View: component (with versions)
View->>Filter: Read name filter
Filter-->>View: Filter text
View->>View: Filter & sort versions
View->>Table: Render headers & rows (pass customData)
Table->>View: Render ComponentVersionListRow per version
Table-->>User: Display table and toolbar
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
- chore(KFLUXUI-1014): forgejo added as git provider #675 — Overlaps in git-utils provider/path logic and related tests.
- feat(KFLUXUI-690): Adds a feature flag and new Components page #577 — Related changes to component page routing and registrations.
Suggested labels
ok-to-test
Suggested reviewers
- sahil143
- testcara
- rrosatti
- StanislavJochman
Poem
A versions tab now lights the way,
Branch links follow repo and sway.
Rows that sort and filters hum,
Types and tests ensure it’s done.
Cheers to routes and features won! 🎉
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title accurately describes the main feature added: a versions tab to the component page, with clear reference to the associated issue KFLUXUI-1007. |
| 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 docstrings (stacked PR)
- 📝 Generate docstrings (commit on current branch)
🧪 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.
Assisted-by: Cursor
marcin-michal
force-pushed
the
KFLUXUI-1007/versions-tab
branch
from
286b53d to
f31463a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
src/utils/git-utils.tsx (1)
70-72:cleanUrl.includes(host)can produce false matches for non-host segments.A repository URL such as
https://mirrors.github.com.example.com/org/repoorhttps://evil.com/github.com-mirror/repowould match'github.com'and emit an incorrect branch URL. Consider comparing against the parsed hostname instead.♻️ Proposed refactor
- const cleanUrl = repoUrl.replace(/\.git$/, ''); - const match = providerBranchPaths.find(([host]) => cleanUrl.includes(host)); + const cleanUrl = repoUrl.replace(/\.git$/, ''); + let hostname: string; + try { + hostname = new URL(cleanUrl).hostname; + } catch { + return undefined; + } + const match = providerBranchPaths.find( + ([host]) => hostname === host || hostname.endsWith(`.${host}`), + );The
endsWith('.'+host)clause retains the existing support for arbitrary subdomains offorgejo.org.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/git-utils.tsx` around lines 70 - 72, The current comparison using cleanUrl.includes(host) can yield false positives; instead parse repoUrl with the URL constructor and compare the parsed hostname to host using equality or endsWith('.' + host) to allow subdomains (e.g., support arbitrary subdomains of forgejo.org). Update the logic around cleanUrl, repoUrl, providerBranchPaths and match so you derive const hostname = new URL(repoUrl).hostname (handle invalid URLs if necessary) and use hostname === host || hostname.endsWith('.' + host) when finding the matching providerBranchPaths entry.src/components/ComponentsPage/tabs/ComponentVersionsTab.tsx (1)
11-20: New feature component is not wrapped with a feature flag.The coding guideline requires wrapping new features with
IfFeature(oruseIsOnFeatureFlag) during development.ComponentVersionsTabhas no such guard.💡 Suggested approach
+import { IfFeature } from '~/feature-flags/hooks'; export const ComponentVersionsTab: React.FC = () => { ... return ( - <> + <IfFeature flag="component-versions"> <Title ...>Versions</Title> <FilterContextProvider filterParams={['name']}> <ComponentVersionListView componentName={componentName} /> </FilterContextProvider> - </> + </IfFeature> ); };As per coding guidelines: "Wrap new features with client-side feature flags during development" (
src/components/**/*.{ts,tsx}).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ComponentsPage/tabs/ComponentVersionsTab.tsx` around lines 11 - 20, ComponentVersionsTab renders the new feature ComponentVersionListView without a feature-flag guard; wrap the new UI with the client-side feature flag (use IfFeature or useIsOnFeatureFlag) so it only shows when enabled. Modify ComponentVersionsTab to check the flag (e.g., useIsOnFeatureFlag('yourFeatureFlag') or wrap the JSX in <IfFeature name="yourFeatureFlag">) around ComponentVersionListView (and its FilterContextProvider) so the existing Title remains or is also gated per design, referencing the ComponentVersionsTab and ComponentVersionListView symbols when locating the change.src/components/ComponentVersion/ComponentVersionListView/__tests__/ComponentVersionListView.spec.tsx (1)
128-143:fireEventused instead ofuserEvent— violates coding guideline.Lines 133, 150, and 165 use synchronous
fireEvent.change. The coding guideline explicitly requiresuserEvent.setup()with async interactions. The same applies to the case-insensitive and empty-state filter tests.♻️ Proposed refactor for the filtering tests
-import { act, fireEvent, screen } from '@testing-library/react'; +import { act, screen } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; it('should filter versions by name', async () => { + const user = userEvent.setup({ advanceTimers: jest.advanceTimersByTime }); const view = renderWithQueryClientAndRouter(<VersionListView />); const filter = screen.getByPlaceholderText<HTMLInputElement>('Filter by name...'); - act(() => { - fireEvent.change(filter, { target: { value: 'Version' } }); - }); - act(() => { - jest.advanceTimersByTime(700); - }); + await user.clear(filter); + await user.type(filter, 'Version'); + act(() => { jest.advanceTimersByTime(700); }); view.rerender(<VersionListView />); ... });Apply the same pattern to the case-insensitive and empty-filtered-state tests.
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/ComponentVersion/ComponentVersionListView/__tests__/ComponentVersionListView.spec.tsx` around lines 128 - 143, Tests use synchronous fireEvent.change on the VersionListView filter which violates the guideline; switch to asynchronous userEvent: import userEvent, create const user = userEvent.setup(), replace fireEvent.change(filter, { target: { value: 'Version' } }) with await user.type(filter, 'Version') (or await user.clear(filter) then await user.type(...) for empty/start cases), keep the jest.advanceTimersByTime(700) to trigger debounce but ensure the test uses async/await (e.g., await jest.advanceTimersByTimeAsync or wrap in act with await) and await any necessary re-renders before assertions; apply the same replacement pattern to the case-insensitive and empty-filtered-state tests referencing VersionListView and the filter input placeholder "Filter by name...".
🤖 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/tabs/__tests__/ComponentVersionsTab.spec.tsx`:
- Around line 6-9: Replace the manual jest.mock of useParams in
ComponentVersionsTab.spec.tsx with the shared factory: remove the jest.mock(...)
block and the cast on line 21, import createUseParamsMock from
'~/unit-test-utils/mock-react-router', and call createUseParamsMock({
componentName: 'my-component' }) inside the test file's beforeEach() so
useParams is mocked consistently across tests.
In `@src/components/ComponentsPage/tabs/ComponentVersionsTab.tsx`:
- Around line 8-9: useParams<RouterParams>() returns a partial so componentName
is string|undefined and passing it directly to ComponentVersionListView (prop
componentName: string) causes a TS error; fix by guarding or asserting non-null:
retrieve componentName via useParams, then either add a runtime guard (e.g., if
(!componentName) return null or render a fallback/redirect) before rendering
<ComponentVersionListView componentName={componentName} /> or, if you can
guarantee it's present, use a non-null assertion (<ComponentVersionListView
componentName={componentName!} />); update code around useParams, componentName
and the ComponentVersionListView render to ensure the prop is always a string.
- Line 7: Add a top-level React import so the React.FC type resolves (e.g.,
import React from 'react'), then fix the potentially undefined componentName
returned by useParams<RouterParams>() before passing it into
ComponentVersionListView: use the symbol useParams to extract componentName and
guard it (e.g., if (!componentName) return null or render a fallback) or
alternatively update ComponentVersionListView's componentName prop to accept
string | undefined; also consider whether this tab should be wrapped in
IfFeature and adjust accordingly.
In
`@src/components/ComponentVersion/ComponentVersionListView/__tests__/ComponentVersionListRow.spec.tsx`:
- Around line 57-62: The test ComponentVersionListRow.spec.tsx uses a weak
assertion for dashes; update the assertion in the 'should render "-" when
context is not set' test so it expects at least two '-' characters (e.g., change
expect(...length).toBeGreaterThanOrEqual(1) to toBeGreaterThanOrEqual(2)) to
ensure both the context and pipeline cells render a dash for
mockVersionNoPipeline; target the same renderWithQueryClient(...) invocation and
the same screen.getAllByText('-') call in that test.
In
`@src/components/ComponentVersion/ComponentVersionListView/__tests__/ComponentVersionListView.spec.tsx`:
- Line 1: The test imports the legacy table API via "Table as PfTable" and
"TableHeader" from "@patternfly/react-table/deprecated"; update the test to use
the non-deprecated PatternFly table API by changing the import to the current
package and adjust the test's TableComponent mock accordingly (replace uses of
PfTable and TableHeader with their modern equivalents and update any prop/shim
differences in the ComponentVersionListView mock to match the new TableComponent
API).
In
`@src/components/ComponentVersion/ComponentVersionListView/ComponentVersionListView.tsx`:
- Around line 19-23: The sort key 'build-pipeline' in sortPaths is pointing at a
ComponentBuildPipeline object, so _.sortBy(resources, 'build-pipeline') yields
meaningless order; change the flow to sort by a derived string (pipeline name)
instead: add a pipelineName field to the items passed into useSortedResources
(e.g., map resources to {...resource, pipelineName:
getPipelineName(resource['build-pipeline'])}) and update sortPaths to reference
that pipelineName key, or if you prefer to disable pipeline sorting remove
pipeline from SortableHeaders and from sortPaths; export or move getPipelineName
from ComponentVersionListRow.tsx into a shared util so you can compute the
pipelineName before calling useSortedResources.
- Line 73: The EmptyMsg function is recreated on every render inside
ComponentVersionListView, causing React to remount FilteredEmptyState each time;
move the EmptyMsg definition out of the render/component body and into module
scope (or memoize it) so it remains stable across renders, and ensure it still
calls the onClearFilters handler passed from ComponentVersionListView (e.g.,
accept onClearFilters as a prop or wrap a stable callback via useCallback) so
Table can render <EmptyMsg /> without remounting.
- Around line 84-91: The BaseTextFilterToolbar invocation is passing a
kebab-case HTML attribute data-test which doesn't match the component's expected
prop dataTest, causing the inner Toolbar to receive undefined; update the prop
name on the BaseTextFilterToolbar element from data-test to dataTest so the
component receives and forwards the value correctly (see BaseTextFilterToolbar
usage where text={nameFilter}, setText and onClearFilters are set).
---
Nitpick comments:
In `@src/components/ComponentsPage/tabs/ComponentVersionsTab.tsx`:
- Around line 11-20: ComponentVersionsTab renders the new feature
ComponentVersionListView without a feature-flag guard; wrap the new UI with the
client-side feature flag (use IfFeature or useIsOnFeatureFlag) so it only shows
when enabled. Modify ComponentVersionsTab to check the flag (e.g.,
useIsOnFeatureFlag('yourFeatureFlag') or wrap the JSX in <IfFeature
name="yourFeatureFlag">) around ComponentVersionListView (and its
FilterContextProvider) so the existing Title remains or is also gated per
design, referencing the ComponentVersionsTab and ComponentVersionListView
symbols when locating the change.
In
`@src/components/ComponentVersion/ComponentVersionListView/__tests__/ComponentVersionListView.spec.tsx`:
- Around line 128-143: Tests use synchronous fireEvent.change on the
VersionListView filter which violates the guideline; switch to asynchronous
userEvent: import userEvent, create const user = userEvent.setup(), replace
fireEvent.change(filter, { target: { value: 'Version' } }) with await
user.type(filter, 'Version') (or await user.clear(filter) then await
user.type(...) for empty/start cases), keep the jest.advanceTimersByTime(700) to
trigger debounce but ensure the test uses async/await (e.g., await
jest.advanceTimersByTimeAsync or wrap in act with await) and await any necessary
re-renders before assertions; apply the same replacement pattern to the
case-insensitive and empty-filtered-state tests referencing VersionListView and
the filter input placeholder "Filter by name...".
In `@src/utils/git-utils.tsx`:
- Around line 70-72: The current comparison using cleanUrl.includes(host) can
yield false positives; instead parse repoUrl with the URL constructor and
compare the parsed hostname to host using equality or endsWith('.' + host) to
allow subdomains (e.g., support arbitrary subdomains of forgejo.org). Update the
logic around cleanUrl, repoUrl, providerBranchPaths and match so you derive
const hostname = new URL(repoUrl).hostname (handle invalid URLs if necessary)
and use hostname === host || hostname.endsWith('.' + host) when finding the
matching providerBranchPaths entry.
src/components/ComponentsPage/tabs/__tests__/ComponentVersionsTab.spec.tsx
Outdated
Show resolved
Hide resolved
...ponents/ComponentVersion/ComponentVersionListView/__tests__/ComponentVersionListRow.spec.tsx
Outdated
Show resolved
Hide resolved
...onents/ComponentVersion/ComponentVersionListView/__tests__/ComponentVersionListView.spec.tsx
Outdated
Show resolved
Hide resolved
src/components/ComponentVersion/ComponentVersionListView/ComponentVersionListView.tsx
Show resolved
Hide resolved
src/components/ComponentVersion/ComponentVersionListView/ComponentVersionListView.tsx
Outdated
Show resolved
Hide resolved
src/components/ComponentVersion/ComponentVersionListView/ComponentVersionListView.tsx
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/components/ComponentVersion/ComponentVersionListView/ComponentVersionListView.tsx (1)
25-27:React.PropsWithChildrenis unnecessary —childrenis never consumed.The component neither destructures nor renders
children, so wrapping props withPropsWithChildrenonly widens the public surface area with an unused optional prop.♻️ Proposed simplification
const ComponentVersionListView: React.FC< - React.PropsWithChildren<ComponentVersionListViewProps> + ComponentVersionListViewProps > = ({ componentName }) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ComponentVersion/ComponentVersionListView/ComponentVersionListView.tsx` around lines 25 - 27, ComponentVersionListView currently types its props as React.PropsWithChildren<ComponentVersionListViewProps> even though the component never uses children; remove the unnecessary PropsWithChildren wrapper and type the component with ComponentVersionListViewProps directly (e.g., change the generic on ComponentVersionListView from React.PropsWithChildren<ComponentVersionListViewProps> to ComponentVersionListViewProps or adjust the function signature to accept ComponentVersionListViewProps), and update any imports/types if needed to keep types consistent for the ComponentVersionListView symbol and its props type ComponentVersionListViewProps.src/utils/__tests__/git-utils.spec.tsx (1)
121-123: LGTM — one coverage gap to address once the implementation is fixed.The suite is comprehensive. However, there is no test guarding against the false-positive host-substring match described in the
git-utils.tsxcomment (e.g.createBranchUrl('https://notgithub.com/org/repo', 'main')should returnundefinedafter the hostname comparison fix is applied). Add that case to prevent regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/__tests__/git-utils.spec.tsx` around lines 121 - 123, Add a guard test to the existing git utils test suite to prevent false-positive host-substring matches: add an it(...) case that calls createBranchUrl('https://notgithub.com/org/repo', 'main') and asserts it returns undefined so only exact hostnames (e.g., github.com) are accepted; place the new test alongside the other provider checks in the git-utils.spec.tsx suite and mirror the style of the existing unknown-provider test to prevent regressions.
🤖 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/tabs/ComponentVersionsTab.tsx`:
- Around line 7-21: Wrap the new ComponentVersionsTab UI in the IfFeature
component so the feature is gated; specifically, update the ComponentVersionsTab
component to render its JSX inside <IfFeature
flag="component-versions">...</IfFeature> (import IfFeature from
~/feature-flags/hooks) and ensure it still passes componentName into
ComponentVersionListView; then add a new flag entry to the FLAGS object in
src/feature-flags/flags.ts with key: 'component-versions', description:
'Component versions tab on the component details page', defaultEnabled: false,
and status: 'in-development'.
In `@src/utils/git-utils.tsx`:
- Around line 71-73: The provider matching currently uses
cleanUrl.includes(host) which yields false positives; update the logic in
createBranchUrl (and where providerBranchPaths is searched) to parse the URL
hostname (new URL(cleanUrl).hostname) and match exactly or with subdomain
support using hostname === host || hostname.endsWith(`.${host}`), mirroring
getGitPath’s endsWith handling (e.g., for forgejo.org); after changing
providerBranchPaths.find to use this hostname check, add a regression test in
git-utils.spec.tsx asserting createBranchUrl('https://notgithub.com/org/repo',
'main') returns undefined.
---
Duplicate comments:
In `@src/components/ComponentsPage/tabs/ComponentVersionsTab.tsx`:
- Around line 7-9: Import the React namespace for type usage and ensure the
param type matches the non-optional prop: add "import React from 'react'" (or
import type React) so React.FC resolves, then narrow or assert the params'
componentName before passing to ComponentVersionListView (e.g., read
componentName from useParams<RouterParams>(), throw or return early if
componentName is undefined, or use a non-null assertion/cast) so
ComponentVersionListView receives a plain string; update ComponentVersionsTab to
use the validated/asserted componentName when calling ComponentVersionListView.
In
`@src/components/ComponentVersion/ComponentVersionListView/__tests__/ComponentVersionListRow.spec.tsx`:
- Around line 57-62: The test's assertion in the ComponentVersionListRow spec is
too weak: when rendering ComponentVersionListRow with mockVersionNoPipeline via
renderWithQueryClient the description and pipeline cells both render '-' but the
test only checks for at least 1 dash; update the assertion to require both
dashes (change the expect on screen.getAllByText('-') to assert length >= 2) so
it matches the intended behavior for mockVersionNoPipeline and aligns with the
later test's check.
In
`@src/components/ComponentVersion/ComponentVersionListView/ComponentVersionListView.tsx`:
- Line 73: EmptyMsg is defined inside ComponentVersionListView causing a new
component type each render and remounting FilteredEmptyState; move the EmptyMsg
definition to module scope (outside the ComponentVersionListView function) or
export/lift it so it becomes a stable reference, and ensure it receives the
onClearFilters callback via props (pass onClearFilters directly to the
module-scoped EmptyMsg or wrap onClearFilters in a stable useCallback in
ComponentVersionListView if needed).
- Around line 84-91: The toolbar is receiving a kebab-case JSX prop instead of
the camelCase prop the component expects, so BaseTextFilterToolbar's dataTest
prop is undefined; update the call site in ComponentVersionListView to pass
dataTest="version-list-toolbar" (not data-test) when rendering
<BaseTextFilterToolbar ...>, ensuring BaseTextFilterToolbar's dataTest prop is
populated and forwarded to the inner Toolbar.
- Around line 19-23: The sort key 'build-pipeline' in sortPaths (mapping for
SortableHeaders.pipeline) points to a ComponentBuildPipeline object so
useSortedResources's _.sortBy(resources, 'build-pipeline') sorts by object
reference; update the implementation to sort by a string pipeline name instead
by either changing sortPaths to map SortableHeaders.pipeline to a derived key
like 'build-pipeline.name' or add a flat property (e.g. pipelineName) to each
resource before sorting and have useSortedResources sort by that; alternatively
remove SortableHeaders.pipeline and its column if pipeline sorting is not
required—make the change around sortPaths, SortableHeaders, and
useSortedResources to ensure lodash receives a string path to sort
alphabetically.
---
Nitpick comments:
In
`@src/components/ComponentVersion/ComponentVersionListView/ComponentVersionListView.tsx`:
- Around line 25-27: ComponentVersionListView currently types its props as
React.PropsWithChildren<ComponentVersionListViewProps> even though the component
never uses children; remove the unnecessary PropsWithChildren wrapper and type
the component with ComponentVersionListViewProps directly (e.g., change the
generic on ComponentVersionListView from
React.PropsWithChildren<ComponentVersionListViewProps> to
ComponentVersionListViewProps or adjust the function signature to accept
ComponentVersionListViewProps), and update any imports/types if needed to keep
types consistent for the ComponentVersionListView symbol and its props type
ComponentVersionListViewProps.
In `@src/utils/__tests__/git-utils.spec.tsx`:
- Around line 121-123: Add a guard test to the existing git utils test suite to
prevent false-positive host-substring matches: add an it(...) case that calls
createBranchUrl('https://notgithub.com/org/repo', 'main') and asserts it returns
undefined so only exact hostnames (e.g., github.com) are accepted; place the new
test alongside the other provider checks in the git-utils.spec.tsx suite and
mirror the style of the existing unknown-provider test to prevent regressions.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #714 +/- ##
==========================================
- Coverage 87.23% 86.72% -0.52%
==========================================
Files 764 768 +4
Lines 58225 58567 +342
Branches 5658 6915 +1257
==========================================
- Hits 50792 50790 -2
- Misses 7376 7569 +193
- Partials 57 208 +151
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 128 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Assisted-by: Cursor
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (6)
src/components/ComponentVersion/ComponentVersionListView/__tests__/ComponentVersionListRow.spec.tsx (3)
38-129: Missing edge-case / error-path coverage.The suite covers the primary happy paths but is missing tests for edge cases the component explicitly handles (e.g.,
obj.revisionbeing an empty string — the row renders'-'viaobj.revision || '-'when there is nobranchUrl). Per coding guidelines: "Test both success and error scenarios for all components and features".Consider adding:
- A test for an empty
obj.revision(verifies the'-'fallback in the revision cell).- A test for a
mockVersionwithobj.revision = ''and norepoUrlto cover theobj.revision || '-'branch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ComponentVersion/ComponentVersionListView/__tests__/ComponentVersionListRow.spec.tsx` around lines 38 - 129, Add tests to cover the edge case where obj.revision is an empty string so the component falls back to '-' (the obj.revision || '-' branch). Specifically, create a variant of mockVersion with revision set to '' and render ComponentVersionListRow via renderWithQueryClient (optionally with custom VersionListRowCustomData that omits repoUrl) and assert that the revision cell contains '-' (and that no link is rendered). Name the tests clearly (e.g., "renders '-' when revision is empty" and "renders '-' when revision is empty and repoUrl is not set") and reuse existing helpers/mocks (mockVersion, renderWithQueryClient, VersionListRowCustomData) for consistency.
61-76: Plain-text revision tests don't verify the absence of an anchor element.The tests confirm
'ver-1.0'is rendered, but a bug increateBranchUrlthat still generates a URL for unknown providers would be invisible because the assertions don't check that no<a>wraps the text.♻️ Proposed additions
it('should render revision as plain text when repo URL is unknown provider', () => { ... renderWithQueryClient(<ComponentVersionListRow obj={mockVersion} customData={customData} />); - expect(screen.getByText('ver-1.0')).toBeInTheDocument(); + const text = screen.getByText('ver-1.0'); + expect(text).toBeInTheDocument(); + expect(text.closest('a')).toBeNull(); }); it('should render revision as plain text when repo URL is not set', () => { ... renderWithQueryClient(<ComponentVersionListRow obj={mockVersion} customData={customData} />); - expect(screen.getByText('ver-1.0')).toBeInTheDocument(); + const text = screen.getByText('ver-1.0'); + expect(text).toBeInTheDocument(); + expect(text.closest('a')).toBeNull(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ComponentVersion/ComponentVersionListView/__tests__/ComponentVersionListRow.spec.tsx` around lines 61 - 76, Update the two tests in ComponentVersionListRow.spec.tsx to not only assert that "ver-1.0" is rendered but also assert it is not wrapped in an anchor: locate the rendered text for 'ver-1.0' from ComponentVersionListRow and add an assertion that there is no link element for that label (e.g., ensure queryByRole('link', { name: /ver-1.0/ }) returns null or that the element's tagName !== 'A') so failures in createBranchUrl producing unexpected anchors are caught.
4-9: UserenderWithQueryClientAndRouterinstead of mockingreact-router-dom.The component uses
Linkfromreact-router-domand should be tested with a real Router context viarenderWithQueryClientAndRouter. While the current mock works forLinkalone, it's not idiomatic to this codebase and doesn't follow the established testing pattern. UsingrenderWithQueryClientAndRouterfrom~/unit-test-utils/rendering-utilsprovides proper Router context without requiring manual mocks.Refactor
-import { renderWithQueryClient } from '~/unit-test-utils/mock-react-query'; +import { renderWithQueryClientAndRouter } from '~/unit-test-utils/rendering-utils'; -jest.mock('react-router-dom', () => ({ - Link: (props) => <a href={props.to}>{props.children}</a>, -})); // ...in each test -renderWithQueryClient(<ComponentVersionListRow ... />); +renderWithQueryClientAndRouter(<ComponentVersionListRow ... />);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ComponentVersion/ComponentVersionListView/__tests__/ComponentVersionListRow.spec.tsx` around lines 4 - 9, Replace the manual react-router-dom mock in the test by using the shared helper that provides a real Router context: import and use renderWithQueryClientAndRouter (from '~/unit-test-utils/rendering-utils') instead of renderWithQueryClient, and remove the jest.mock('react-router-dom') block; render ComponentVersionListRow (and any tests that rely on VersionListRowCustomData) with renderWithQueryClientAndRouter so Link behavior is exercised with a proper Router.src/components/ComponentVersion/ComponentVersionListView/ComponentVersionListView.tsx (3)
24-26:React.PropsWithChildrenis unnecessary —childrenis never used.♻️ Proposed fix
-const ComponentVersionListView: React.FC< - React.PropsWithChildren<ComponentVersionListViewProps> -> = ({ componentName }) => { +const ComponentVersionListView: React.FC<ComponentVersionListViewProps> = ({ componentName }) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ComponentVersion/ComponentVersionListView/ComponentVersionListView.tsx` around lines 24 - 26, The component type incorrectly wraps ComponentVersionListViewProps with React.PropsWithChildren even though children isn't used; update the component declaration for ComponentVersionListView to use ComponentVersionListViewProps directly (e.g., change the type annotation to React.FC<ComponentVersionListViewProps> or a plain function signature taking ComponentVersionListViewProps) and remove any reference to children so the props type matches actual usage.
97-99:Rowinline arrow function creates a new reference on every render.For lists with many versions this can cause unnecessary child re-renders. Consider stabilizing with
React.useCallback:♻️ Proposed refactor
+ const Row = React.useCallback( + (props) => ( + <ComponentVersionListRow obj={props.obj as ComponentVersion} customData={customData} /> + ), + [customData], + ); return ( <> ... <Table ... - Row={(props) => ( - <ComponentVersionListRow obj={props.obj as ComponentVersion} customData={customData} /> - )} + Row={Row} ... />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ComponentVersion/ComponentVersionListView/ComponentVersionListView.tsx` around lines 97 - 99, The inline arrow passed as the Row prop in ComponentVersionListView creates a new function each render and causes unnecessary re-renders of ComponentVersionListRow; wrap that arrow in React.useCallback inside the ComponentVersionListView component (e.g., const renderRow = React.useCallback((props) => <ComponentVersionListRow obj={props.obj as ComponentVersion} customData={customData} />, [customData])) and pass renderRow to the Row prop, ensuring React is imported and the dependency array includes any values used (like customData) so the callback is stable across renders.
24-26: New feature component is not wrapped with a client-side feature flag.Per coding guidelines, new features should be gated with
IfFeature(from~/feature-flags/hooks) during development to allow progressive rollout and safe rollback. As per coding guidelines: "Wrap new features with client-side feature flags during development".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ComponentVersion/ComponentVersionListView/ComponentVersionListView.tsx` around lines 24 - 26, Wrap the new ComponentVersionListView output in the client-side feature gate: import IfFeature from '~/feature-flags/hooks' and wrap the component's returned JSX (the body of ComponentVersionListView) with <IfFeature flag="componentVersionList">...</IfFeature> (or the agreed feature-flag key), providing a safe fallback (e.g., null or placeholder) so the feature can be toggled during rollout; ensure the import and flag name reference IfFeature and ComponentVersionListView so reviewers can find the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@src/components/ComponentVersion/ComponentVersionListView/ComponentVersionListView.tsx`:
- Line 88: The Toolbar prop name is passed as kebab-case `data-test` but
`BaseTextFilterToolbar` expects camelCase `dataTest`, so update the prop on the
`BaseTextFilterToolbar`/`ComponentVersionListView` usage to use `dataTest`
instead of `data-test` so the inner `Toolbar` receives the correct string (fix
the prop passed to BaseTextFilterToolbar and any other occurrences in
ComponentVersionListView where `data-test` is used).
- Line 72: ComponentVersionListView currently defines EmptyMsg inline which
forces FilteredEmptyState to remount on every render; move the EmptyMsg
component definition out of ComponentVersionListView (define it at module scope
as EmptyMsg or FilteredEmptyStateWrapper) and have ComponentVersionListView pass
a stable callback to it by wrapping onClearFilters with React.useCallback (or
pass the bound prop directly if already stable); update usages in
ComponentVersionListView to use the top-level EmptyMsg and ensure the prop name
matches FilteredEmptyState's onClearFilters.
---
Nitpick comments:
In
`@src/components/ComponentVersion/ComponentVersionListView/__tests__/ComponentVersionListRow.spec.tsx`:
- Around line 38-129: Add tests to cover the edge case where obj.revision is an
empty string so the component falls back to '-' (the obj.revision || '-'
branch). Specifically, create a variant of mockVersion with revision set to ''
and render ComponentVersionListRow via renderWithQueryClient (optionally with
custom VersionListRowCustomData that omits repoUrl) and assert that the revision
cell contains '-' (and that no link is rendered). Name the tests clearly (e.g.,
"renders '-' when revision is empty" and "renders '-' when revision is empty and
repoUrl is not set") and reuse existing helpers/mocks (mockVersion,
renderWithQueryClient, VersionListRowCustomData) for consistency.
- Around line 61-76: Update the two tests in ComponentVersionListRow.spec.tsx to
not only assert that "ver-1.0" is rendered but also assert it is not wrapped in
an anchor: locate the rendered text for 'ver-1.0' from ComponentVersionListRow
and add an assertion that there is no link element for that label (e.g., ensure
queryByRole('link', { name: /ver-1.0/ }) returns null or that the element's
tagName !== 'A') so failures in createBranchUrl producing unexpected anchors are
caught.
- Around line 4-9: Replace the manual react-router-dom mock in the test by using
the shared helper that provides a real Router context: import and use
renderWithQueryClientAndRouter (from '~/unit-test-utils/rendering-utils')
instead of renderWithQueryClient, and remove the jest.mock('react-router-dom')
block; render ComponentVersionListRow (and any tests that rely on
VersionListRowCustomData) with renderWithQueryClientAndRouter so Link behavior
is exercised with a proper Router.
In
`@src/components/ComponentVersion/ComponentVersionListView/ComponentVersionListView.tsx`:
- Around line 24-26: The component type incorrectly wraps
ComponentVersionListViewProps with React.PropsWithChildren even though children
isn't used; update the component declaration for ComponentVersionListView to use
ComponentVersionListViewProps directly (e.g., change the type annotation to
React.FC<ComponentVersionListViewProps> or a plain function signature taking
ComponentVersionListViewProps) and remove any reference to children so the props
type matches actual usage.
- Around line 97-99: The inline arrow passed as the Row prop in
ComponentVersionListView creates a new function each render and causes
unnecessary re-renders of ComponentVersionListRow; wrap that arrow in
React.useCallback inside the ComponentVersionListView component (e.g., const
renderRow = React.useCallback((props) => <ComponentVersionListRow obj={props.obj
as ComponentVersion} customData={customData} />, [customData])) and pass
renderRow to the Row prop, ensuring React is imported and the dependency array
includes any values used (like customData) so the callback is stable across
renders.
- Around line 24-26: Wrap the new ComponentVersionListView output in the
client-side feature gate: import IfFeature from '~/feature-flags/hooks' and wrap
the component's returned JSX (the body of ComponentVersionListView) with
<IfFeature flag="componentVersionList">...</IfFeature> (or the agreed
feature-flag key), providing a safe fallback (e.g., null or placeholder) so the
feature can be toggled during rollout; ensure the import and flag name reference
IfFeature and ComponentVersionListView so reviewers can find the change.
Assisted-by: Cursor
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/components/ComponentVersion/ComponentVersionListView/__tests__/ComponentVersionListView.spec.tsx (2)
12-16: AddafterEachto reset shared filter params between tests.
mockUseSearchParamBatch()factory closures all share the same module-levelparamsvariable inmock-useSearchParam.ts. Filter state written by one test leaks into the next because there is no reset between runs. The current order happens to avoid failures, but it is fragile.♻️ Proposed fix
+import { mockUseSearchParamBatch, mockUseSearchParamBatchUnset } from '~/unit-test-utils/mock-useSearchParam';If
unsetis not already exported from the mock utility, expose it and call it inafterEach:describe('ComponentVersionListView', () => { mockUseNamespaceHook('test-ns'); beforeEach(() => { useComponentMock.mockReturnValue([mockComponent, true, undefined]); }); + + afterEach(() => { + mockUseSearchParamBatchUnset(); + });Alternatively, verify whether the existing
unset()returned frommockUseSearchParamBatch()can be called directly via a module-level reference captured once inbeforeAll.Also applies to: 92-97
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ComponentVersion/ComponentVersionListView/__tests__/ComponentVersionListView.spec.tsx` around lines 12 - 16, Tests leak shared filter state because the mockUseSearchParamBatch factory shares module-level params; add an afterEach that resets that state by calling the mock reset function (either export and call unset from mock-useSearchParam.ts or capture the unset returned by mockUseSearchParamBatch in a beforeAll and call it in afterEach). Locate usages of mockUseSearchParamBatch and ensure an afterEach invokes the reset (unset) so each test starts with clean params; if unset isn't exported, modify mock-useSearchParam.ts to expose a reset/unset function and call it in afterEach.
2-2: ReplacefireEventwithuserEvent.setup()per coding guidelines.The coding guidelines mandate
userEvent.setup()and async interactions over synchronousfireEvent. Withjest.useFakeTimers()active, the standard approach is to configureuserEventto use the fake timer:♻️ Proposed refactor
-import { act, fireEvent, screen } from '@testing-library/react'; +import { act, screen } from '@testing-library/react'; +import userEvent from '@testing-library/user-event';- it('should filter versions by name', () => { - const view = renderWithQueryClientAndRouter(<VersionListView />); - - const filter = screen.getByPlaceholderText<HTMLInputElement>('Filter by name...'); - act(() => { - fireEvent.change(filter, { target: { value: 'Version' } }); - }); - act(() => { - jest.advanceTimersByTime(700); - }); - view.rerender(<VersionListView />); - - expect(screen.getByText('Version 1.0')).toBeInTheDocument(); - expect(screen.queryByText('Test')).not.toBeInTheDocument(); - expect(screen.queryByText('Different pipeline')).not.toBeInTheDocument(); - }); + it('should filter versions by name', async () => { + const user = userEvent.setup({ advanceTimers: jest.advanceTimersByTime }); + const view = renderWithQueryClientAndRouter(<VersionListView />); + + const filter = screen.getByPlaceholderText<HTMLInputElement>('Filter by name...'); + await user.type(filter, 'Version'); + act(() => { jest.advanceTimersByTime(700); }); + view.rerender(<VersionListView />); + + expect(screen.getByText('Version 1.0')).toBeInTheDocument(); + expect(screen.queryByText('Test')).not.toBeInTheDocument(); + expect(screen.queryByText('Different pipeline')).not.toBeInTheDocument(); + });Apply the same pattern to the case-insensitive and empty-state filter tests.
Also applies to: 123-169
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ComponentVersion/ComponentVersionListView/__tests__/ComponentVersionListView.spec.tsx` at line 2, Replace synchronous fireEvent usage with async userEvent in the ComponentVersionListView tests: remove fireEvent import, import userEvent from '@testing-library/user-event', create a user instance via userEvent.setup({ advanceTimers: (ms) => jest.advanceTimersByTime(ms) }) where tests use jest.useFakeTimers(), and update interactions (click/type) to await user.method(...) in the general tests and the case-insensitive and empty-state filter tests so they use async userEvent calls instead of fireEvent.
🤖 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/utils/__tests__/git-utils.spec.tsx`:
- Around line 70-147: Add a unit test that covers the createBranchUrl try/catch
path by passing a non-parseable repoUrl (e.g. 'not-a-url') and asserting it
returns undefined; update the existing test suite for createBranchUrl to include
a case like expect(createBranchUrl('not-a-url', 'main')).toBeUndefined() so the
new catch branch in createBranchUrl is exercised.
---
Nitpick comments:
In
`@src/components/ComponentVersion/ComponentVersionListView/__tests__/ComponentVersionListView.spec.tsx`:
- Around line 12-16: Tests leak shared filter state because the
mockUseSearchParamBatch factory shares module-level params; add an afterEach
that resets that state by calling the mock reset function (either export and
call unset from mock-useSearchParam.ts or capture the unset returned by
mockUseSearchParamBatch in a beforeAll and call it in afterEach). Locate usages
of mockUseSearchParamBatch and ensure an afterEach invokes the reset (unset) so
each test starts with clean params; if unset isn't exported, modify
mock-useSearchParam.ts to expose a reset/unset function and call it in
afterEach.
- Line 2: Replace synchronous fireEvent usage with async userEvent in the
ComponentVersionListView tests: remove fireEvent import, import userEvent from
'@testing-library/user-event', create a user instance via userEvent.setup({
advanceTimers: (ms) => jest.advanceTimersByTime(ms) }) where tests use
jest.useFakeTimers(), and update interactions (click/type) to await
user.method(...) in the general tests and the case-insensitive and empty-state
filter tests so they use async userEvent calls instead of fireEvent.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/components/ComponentVersion/ComponentVersionListView/ComponentVersionListView.tsxsrc/components/ComponentVersion/ComponentVersionListView/__tests__/ComponentVersionListView.spec.tsxsrc/components/ComponentsPage/tabs/__tests__/ComponentVersionsTab.spec.tsxsrc/utils/__tests__/git-utils.spec.tsxsrc/utils/git-utils.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/ComponentVersion/ComponentVersionListView/ComponentVersionListView.tsx
Assisted-by: Cursor
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/utils/__tests__/git-utils.spec.tsx (1)
70-151: LGTM — comprehensive coverage forcreateBranchUrl.The new suite covers all meaningful branches of the implementation: guard clauses (
undefined/empty inputs), all mapped providers (GitHub, GitLab, Forgejo, Codeberg), thecatchpath for non-parseable URLs (line 125-127 — previously flagged, now addressed), the hostname exact-match-vs-substring boundary (lines 129-132), subdomain matching (lines 134-138), and branch name normalization (dots/slashes). The.gitsuffix stripping is tested only for GitLab, but since all providers share the samecleanUrlderivation, that single case is sufficient to verify the stripping behavior.One minor observation: line 129-132 places two
expect()calls in a singleitblock. Splitting them into two separate test cases would give more targeted failure messages, but it is a nitpick.♻️ Split dual-assertion test into two focused cases
- it('should not match hostnames that contain a provider name as a substring', () => { - expect(createBranchUrl('https://notgithub.com/org/repo', 'main')).toBeUndefined(); - expect(createBranchUrl('https://mygitlab.com/org/repo', 'main')).toBeUndefined(); - }); + it('should not match hostnames that contain a provider name as a leading substring', () => { + expect(createBranchUrl('https://notgithub.com/org/repo', 'main')).toBeUndefined(); + }); + + it('should not match hostnames that contain a provider name as a trailing substring', () => { + expect(createBranchUrl('https://mygitlab.com/org/repo', 'main')).toBeUndefined(); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/__tests__/git-utils.spec.tsx` around lines 70 - 151, The test "should not match hostnames that contain a provider name as a substring" in src/utils/__tests__/git-utils.spec.tsx currently contains two expect() assertions; split this into two separate it() cases (e.g., "should not match hostname notgithub.com" and "should not match hostname mygitlab.com") so each test contains a single expect calling createBranchUrl(...), preserving the existing input strings and expected undefined result; keep the same describe('createBranchUrl') context and test helper usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/utils/__tests__/git-utils.spec.tsx`:
- Around line 70-151: The test "should not match hostnames that contain a
provider name as a substring" in src/utils/__tests__/git-utils.spec.tsx
currently contains two expect() assertions; split this into two separate it()
cases (e.g., "should not match hostname notgithub.com" and "should not match
hostname mygitlab.com") so each test contains a single expect calling
createBranchUrl(...), preserving the existing input strings and expected
undefined result; keep the same describe('createBranchUrl') context and test
helper usage.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes
KFLUXUI-1007
Description
Added the version list page to the component page.
Builds upon #712 which should be merged first.
Type of change
Screen shots / Gifs for design review
2026-02-19.23-24-41.mp4
How to test or reproduce?
yarn run create-mock-componentson one of the precreated namespaces in the local cluster.https://localhost:8080/ns/<ns>/components/<component>/versionsSummary by CodeRabbit
New Features
Bug Fixes
Tests
Chores