feat(KFLUXUI-1006): add Activity tab for component details page#695
feat(KFLUXUI-1006): add Activity tab for component details page#695rakshett wants to merge 8 commits intokonflux-ci:mainfrom
Conversation
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 |
|---|---|
ComponentActivityTab implementation src/components/ComponentsPage/ComponentDetails/tabs/ComponentActivityTab.tsx, src/components/ComponentsPage/ComponentDetails/index.ts |
New React component and export (ACTIVITY_SECONDARY_TAB_KEY, ComponentActivityTab). Reads route params, persists/restores selected tab in localStorage, syncs URL on tab change, feature-flag gated, renders Activity section with Commits and Pipeline runs tabs and related data handling. |
Tests for Activity tab src/components/ComponentsPage/ComponentDetails/__tests__/ComponentActivityTab.spec.tsx |
New test suite mocking router and data hooks (useComponent, useComponents, usePipelineRunsV2, useCommitStatus, useTektonResults) to verify rendering, tab items, navigation to pipelineruns child route, text content, and loading spinner. |
Routing: add nested activity child route src/routes/page-routes/components-page.tsx, src/routes/page-routes/__tests__/components-page.spec.tsx |
Adds route import and a nested child route with :activityTab param; updates route children expectations in tests and ensures activity route element is defined. |
Path exports src/routes/paths.ts |
Adds COMPONENT_ACTIVITY_V2_PATH and COMPONENT_ACTIVITY_V2_CHILD_TAB_PATH for activity and activity child-tab routes. |
Sequence Diagram(s)
sequenceDiagram
participant Browser as Browser
participant Router as Router
participant Component as ComponentActivityTab
participant Commits as CommitsListView
participant Pipelines as PipelineRunsTab
participant Storage as LocalStorage
participant API as useComponent/usePipelineRunsV2
Browser->>Router: Navigate to /ns/:ns/components/:name/activity/:tab?
Router->>Component: render with params (componentName, activityTab)
Component->>Storage: read ACTIVITY_SECONDARY_TAB_KEY for component
alt activityTab present
Component->>Storage: write selected tab
Component->>Router: replace/push URL if needed
end
Component->>API: fetch component & pipeline/commit data
API-->>Component: return data or loading state
alt selected = latest-commits
Component->>Commits: render CommitsListView with filters
else selected = pipelineruns
Component->>Pipelines: render PipelineRunsTab with custom filter
end
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
- [New Component Model] add Component Details page #660: Continues routing work for ComponentDetails activity; this PR implements the activity child-tab route and component.
- feat(KFLUXUI-668): Created hook to fetch PipelineRuns from KubeArchive & PipelineRunsListView component migration #403: Introduces/migrates
usePipelineRunsV2, whichComponentActivityTabconsumes for pipeline runs data. - Revert "feat(KFLUXUI-684): create usePipelineRunsForCommitV2 function… #451: Other changes touching ComponentActivityTab (feature-flag handling), likely to conflict or require reconciliation.
Suggested reviewers
- rrosatti
- janaki29
- JoaoPedroPP
- StanislavJochman
- sahil143
"Activity wakes with tabs anew,
Commits and pipelines scan the view.
Routes remembered, tests confirmed,
A little tab where details churned. 🎉"
🚥 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 pull request title accurately summarizes the main change: adding an Activity tab for the component details page (V2 route), which aligns with the changeset's implementation of the new ComponentActivityTab component and related route changes. |
| 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: 2
🤖 Fix all issues with AI agents
In
`@src/components/ComponentsPage/ComponentDetails/tabs/ComponentActivityTab.tsx`:
- Around line 23-24: The code assumes component is always populated; change the
useComponent destructuring to capture the loading/error state (e.g., const
[component, loading, error] = useComponent(namespace, componentName)) and guard
access to component.spec.application before reading it (set applicationName only
when component is non-null, e.g., compute applicationName = component ?
component.spec.application : undefined or derive it inside a conditional
render), so accessing component.spec.application never happens while component
is null.
In `@src/components/PipelineRun/PipelineRunListView/pipelinerun-actions.tsx`:
- Line 101: Update the inconsistent tooltip text for the disabled rerun action
by removing the accidental " !!" suffix so the disabledTooltip property reads
"You don't have access to rerun" to match the lazy variant; locate the
disabledTooltip entry in pipelinerun-actions.tsx and replace the string
accordingly to ensure consistency across variants.
🧹 Nitpick comments (5)
src/components/ComponentsPage/ComponentDetails/tabs/ComponentActivityTab.tsx (3)
69-69: Replace raw<div>with a PatternFly layout component.Per coding guidelines, raw HTML layout tags should not be used. Consider using a PatternFly
StackorFlexcomponent instead. As per coding guidelines: "NEVER use raw HTML layout tags (<div>,<section>,<header>) or custom CSS layouts."Proposed fix
- <div> + <> <DetailsSection ... </DetailsSection> - </div> + </>If a wrapper element is truly needed for layout, use
<Stack>or<Flex>from PatternFly.
6-12: Mixed import styles: relative paths alongside path aliases.Lines 1–5 use path aliases (
~/components,~/feature-flags), but Lines 6–12 use deep relative paths (../../../../...). For consistency, prefer the configured aliases. As per coding guidelines: "Use absolute imports with configured path aliases:~/components,~/types,~/k8s,~/utils,~/models,@routes."
50-60: Two competinguseEffecthooks may cause an unnecessary render cycle on initial load.When
activityTabis undefined andlastSelectedTabis set:
- Effect at Line 50 writes
currentTab(which equalslastSelectedTab) to localStorage — a no-op in terms of value.- Effect at Line 56 triggers a
navigate(..., { replace: true }), causing a re-render withactivityTabnow set.- The first effect runs again, writing the same value.
This works but is fragile. Consider combining the localStorage persistence into the
setActiveTabcallback and the initial redirect effect, rather than having a separate sync effect.src/components/ComponentsPage/ComponentDetails/__tests__/ComponentActivityTab.spec.tsx (2)
82-82: UseuserEventinstead offireEventfor click interactions.Per testing guidelines, prefer
userEvent.setup()with async interactions over synchronousfireEvent. As per coding guidelines: "UseuserEvent.setup()and async user interactions instead of synchronousfireEventfor testing user actions."Proposed fix
+import userEvent from '@testing-library/user-event'; import { screen, fireEvent, act } from '@testing-library/react';- await act(() => fireEvent.click(plrTab)); + const user = userEvent.setup(); + await user.click(plrTab);
42-98: Missing test coverage for loading and error states.The tests only cover the happy path where
useComponentreturns a loaded component. Given the critical issue flagged inComponentActivityTab.tsx(crash when component is null during loading), adding tests for loading ([null, false, undefined]) and error ([null, true, { code: 404 }]) states would be valuable. As per coding guidelines: "Test both success and error scenarios for all components and features."
src/components/ComponentsPage/ComponentDetails/tabs/ComponentActivityTab.tsx
Outdated
Show resolved
Hide resolved
src/components/PipelineRun/PipelineRunListView/pipelinerun-actions.tsx
Outdated
Show resolved
Hide resolved
src/components/ComponentsPage/ComponentDetails/tabs/ComponentActivityTab.tsx
Outdated
Show resolved
Hide resolved
src/components/ComponentsPage/ComponentDetails/tabs/ComponentActivityTab.tsx
Outdated
Show resolved
Hide resolved
src/components/ComponentsPage/ComponentDetails/tabs/ComponentActivityTab.tsx
Outdated
Show resolved
Hide resolved
src/components/ComponentsPage/ComponentDetails/tabs/ComponentActivityTab.tsx
Outdated
Show resolved
Hide resolved
src/components/PipelineRun/PipelineRunListView/pipelinerun-actions.tsx
Outdated
Show resolved
Hide resolved
src/components/PipelineRun/PipelineRunListView/pipelinerun-actions.tsx
Outdated
Show resolved
Hide resolved
…tip, loading test Co-authored-by: Cursor <cursoragent@cursor.com> Assisted-by: Cursor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@src/components/ComponentsPage/ComponentDetails/tabs/ComponentActivityTab.tsx`:
- Around line 24-27: The localStorage key for useLocalStorage currently depends
on component?.spec.componentName which changes after load, causing stale writes
from the useEffect; update the key to use the stable route param componentName
(from useParams) instead of component?.spec.componentName so the key passed to
useLocalStorage (and variables lastSelectedTab/setLocalStorageItem) is stable
across renders, and ensure the useEffect that compares activityTab/currentTab
and lastSelectedTab uses that stable key (or alternately gate the effect to run
only after loaded === true).
- Line 20: Wrap the ComponentActivityTab export in a feature-flag guard so it
only renders when the "components-page" flag is enabled: either wrap the
component JSX with IfFeature (imported from ~/feature-flags/hooks) or check
useIsOnFeatureFlag("components-page") at the top of the ComponentActivityTab
function and return null when false; keep the existing FeatureFlagIndicator for
display only, but ensure ComponentActivityTab (the exported React.FC) is not
rendered when the flag is off.
🧹 Nitpick comments (7)
src/components/ComponentsPage/ComponentDetails/tabs/ComponentActivityTab.tsx (4)
7-15: Mixed import style: relative paths alongside path aliases.Lines 4–6 use absolute path aliases (
~/components/...,~/feature-flags/...,~/shared/...), but Lines 7–15 use deep relative paths (../../../../...). Per project guidelines, prefer the configured aliases (~/consts,~/hooks,~/routes,~/shared,~/types) for consistency and readability.♻️ Suggested diff
-import { PipelineRunLabel } from '../../../../consts/pipelinerun'; -import { useComponent } from '../../../../hooks/useComponents'; -import { COMPONENT_ACTIVITY_V2_CHILD_TAB_PATH } from '../../../../routes/paths'; -import { RouterParams } from '../../../../routes/utils'; -import { useLocalStorage } from '../../../../shared/hooks/useLocalStorage'; -import { useNamespace } from '../../../../shared/providers/Namespace/useNamespaceInfo'; -import { PipelineRunKind } from '../../../../types'; -import PipelineRunsTab from '../../../Activity/PipelineRunsTab'; -import CommitsListView from '../../../Commits/CommitsListPage/CommitsListView'; +import { PipelineRunLabel } from '~/consts/pipelinerun'; +import { useComponent } from '~/hooks/useComponents'; +import { COMPONENT_ACTIVITY_V2_CHILD_TAB_PATH } from '~/routes/paths'; +import { RouterParams } from '~/routes/utils'; +import { useLocalStorage } from '~/shared/hooks/useLocalStorage'; +import { useNamespace } from '~/shared/providers/Namespace/useNamespaceInfo'; +import { PipelineRunKind } from '~/types'; +import PipelineRunsTab from '~/components/Activity/PipelineRunsTab'; +import CommitsListView from '~/components/Commits/CommitsListPage/CommitsListView';As per coding guidelines: "Use absolute imports with configured path aliases:
~/components,~/types,~/k8s,~/utils,~/models,@routes."
56-60: URL not updated when bothactivityTabandlastSelectedTabare absent.If the route has no
:activityTabparam and nothing is stored in localStorage, this effect is skipped (thelastSelectedTabguard is falsy). The component correctly defaultscurrentTabto'latest-commits'on Line 28, so the UI is fine, but the URL remains without the tab segment—making it non-shareable for the default state.Consider also redirecting when both are absent:
♻️ Suggested fix
React.useEffect(() => { - if (!activityTab && lastSelectedTab) { - navigate(getActivityTabRoute(lastSelectedTab), { replace: true }); + if (!activityTab) { + navigate(getActivityTabRoute(lastSelectedTab || 'latest-commits'), { replace: true }); } }, [activityTab, getActivityTabRoute, lastSelectedTab, navigate]);
82-83: Raw<div>wrapper violates PatternFly layout guideline.The outer
<div>on Line 83 can be replaced with a PatternFly layout component (e.g.,StackorStackItem) for consistency with the project's "no raw HTML layout tags" rule.♻️ Suggested fix
-import { Bullseye, Spinner, Tab, Tabs, TabTitleText } from '@patternfly/react-core'; +import { Bullseye, Spinner, Stack, StackItem, Tab, Tabs, TabTitleText } from '@patternfly/react-core'; ... - <div> + <Stack> + <StackItem> <DetailsSection ...> ... </DetailsSection> - </div> + </StackItem> + </Stack>As per coding guidelines: "NEVER use raw HTML layout tags (
<div>,<section>,<header>) … Always use PatternFly layout components."Also applies to: 132-132
91-95: InlinestyleonTabs— consider using PatternFly spacing utilities or a CSS class.The inline
stylewithwidth: 'fit-content'and avar(--pf-v5-global--spacer--md)margin is functional, but deviates slightly from the PatternFly-first approach. If there's a PatternFly-supported prop or utility class for this, it would be more maintainable.src/components/ComponentsPage/ComponentDetails/__tests__/ComponentActivityTab.spec.tsx (3)
2-2: UseuserEvent.setup()instead offireEventfor user interactions.The guideline specifies using
userEvent.setup()with async interactions rather than synchronousfireEvent. This provides more realistic browser event simulation.♻️ Suggested fix
-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); 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: 76-87
42-104: Missing test for the error scenario.The suite covers rendering, tab navigation, content, and the loading state, but does not test the error path (i.e., when
useComponentreturns an error). Guidelines require testing both success and error scenarios.♻️ Suggested addition
+ it('should render error state when component fails to load', () => { + componentMock.mockReturnValue([null, true, { code: 404 }]); + renderWithQueryClientAndRouter(<ComponentActivityTab />); + // Assert that error UI is rendered (adjust selector based on getErrorState output) + expect(screen.queryByTestId('spinner')).not.toBeInTheDocument(); + expect(screen.queryByTestId('comp__activity__tabItem commits')).not.toBeInTheDocument(); + });As per coding guidelines: "Test both success and error scenarios for all components and features."
69-74: Consider using explicitexpectassertions instead of baregetByTestIdcalls.Lines 72–73 (and similarly 79) rely on
getByTestIdthrowing to fail the test. While functional, usingexpect(...).toBeInTheDocument()(as done in Lines 92–97) is more explicit and consistent.Also applies to: 89-98
src/components/ComponentsPage/ComponentDetails/tabs/ComponentActivityTab.tsx
Show resolved
Hide resolved
src/components/ComponentsPage/ComponentDetails/tabs/ComponentActivityTab.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routes/page-routes/__tests__/components-page.spec.tsx (1)
33-37:⚠️ Potential issue | 🟡 MinorAdd
ComponentActivityTabto the mock factory function.The route file imports
ComponentActivityTabfrom~/components/ComponentsPage/ComponentDetails, which is the same module mocked on line 33. Since the mock factory doesn't includeComponentActivityTab, it resolves toundefined, causing the route elements that render it to havetype: undefined. This would fail at runtime despite thetoBeDefined()test passing.Add to mock:
jest.mock('~/components/ComponentsPage/ComponentDetails', () => ({ ComponentDetailsTab: () => <div data-testid="component-details-tab">Details tab</div>, ComponentDetailsViewLayout: () => <div data-testid="component-details-layout">Layout</div>, ComponentActivityTab: () => <div data-testid="component-activity-tab">Activity tab</div>, componentDetailsViewLoader: jest.fn(() => ({ data: 'test-data' })), }));
🧹 Nitpick comments (1)
src/routes/page-routes/__tests__/components-page.spec.tsx (1)
65-69: Assertions on new route elements are weak.
toBeDefined()only confirms the element is notundefined/null—it doesn't verify the correct component is rendered (e.g.,ComponentActivityTab). Consider usingtoEqual(<ComponentActivityTab />)(once the mock is in place) for bothactivityWithTabRouteandactivityRouteto catch accidental misrouting.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@src/components/ComponentsPage/ComponentDetails/tabs/ComponentActivityTab.tsx`:
- Line 85: The raw <div> wrapper in ComponentActivityTab.tsx (inside the
ComponentActivityTab render/return where a DetailsSection is used) violates
PatternFly layout guidelines; replace that <div> with an appropriate PatternFly
layout component (e.g., Stack or StackItem) or remove the wrapper entirely if
DetailsSection already provides the needed layout—locate the <div> in the
ComponentActivityTab component and swap it for Stack/StackItem from
'@patternfly/react-core' (or remove it) and ensure any children remain
unchanged.
🧹 Nitpick comments (4)
src/components/ComponentsPage/ComponentDetails/tabs/ComponentActivityTab.tsx (4)
8-17: Inconsistent import paths: mix of~/aliases and deep relative paths.Lines 4–7 use the
~/path alias, but lines 8–17 switch to../../../../relative imports. The coding guidelines specify using absolute imports with configured path aliases (~/components,~/types,@routes, etc.).For example:
-import { PipelineRunLabel } from '../../../../consts/pipelinerun'; -import { useComponent } from '../../../../hooks/useComponents'; -import { COMPONENT_ACTIVITY_V2_CHILD_TAB_PATH } from '../../../../routes/paths'; -import { RouterParams } from '../../../../routes/utils'; -import { useLocalStorage } from '../../../../shared/hooks/useLocalStorage'; -import { useNamespace } from '../../../../shared/providers/Namespace/useNamespaceInfo'; -import { PipelineRunKind } from '../../../../types'; -import PipelineRunsTab from '../../../Activity/PipelineRunsTab'; -import CommitsListView from '../../../Commits/CommitsListPage/CommitsListView'; -import { DetailsSection } from '../../../DetailsPage'; +import { PipelineRunLabel } from '~/consts/pipelinerun'; +import { useComponent } from '~/hooks/useComponents'; +import { COMPONENT_ACTIVITY_V2_CHILD_TAB_PATH } from '@routes/paths'; +import { RouterParams } from '@routes/utils'; +import { useLocalStorage } from '~/shared/hooks/useLocalStorage'; +import { useNamespace } from '~/shared/providers/Namespace/useNamespaceInfo'; +import { PipelineRunKind } from '~/types'; +import PipelineRunsTab from '~/components/Activity/PipelineRunsTab'; +import CommitsListView from '~/components/Commits/CommitsListPage/CommitsListView'; +import { DetailsSection } from '~/components/DetailsPage';Verify actual alias mappings before applying — adjust prefixes to match the project's
tsconfigpaths. As per coding guidelines: "Use absolute imports with configured path aliases."#!/bin/bash # Verify which path aliases are configured fd -g 'tsconfig*.json' --exec cat {} | jq '.compilerOptions.paths // empty' 2>/dev/null
93-97: Inline styles onTabs— consider using a CSS class or PatternFly spacing utilities.Inline
styleobjects bypass theming and are harder to maintain. Thewidth: 'fit-content'and spacing could be handled via a CSS module class or PatternFly utility classes (e.g.,pf-v5-u-mb-md).
113-116: Minor inconsistency:componentNamesource differs between localStorage key and child props.Line 27 uses
componentNamefrom route params for the localStorage key, but line 115 passescomponent?.spec?.componentNametoCommitsListView(and line 128 toPipelineRunsTab). These should hold the same value, but using the route param consistently would be simpler and avoid the optional chaining.
79-81:nonTestSnapShotFilteris re-created on every render.Since this is a pure function with no closure over mutable state, wrapping it in
useCallbackor hoisting it outside the component would provide a stable reference, avoiding unnecessary work ifPipelineRunsTabcompares props by reference.Proposed fix — hoist outside component
+// We will not include any test pipelines that were run against the snapshot that contained the image. +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');
src/components/ComponentsPage/ComponentDetails/tabs/ComponentActivityTab.tsx
Outdated
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #695 +/- ##
==========================================
+ Coverage 85.80% 86.66% +0.86%
==========================================
Files 764 765 +1
Lines 58225 58375 +150
Branches 4723 6896 +2173
==========================================
+ Hits 49958 50591 +633
+ Misses 8216 7569 -647
- Partials 51 215 +164
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 120 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
rrosatti
left a comment
rrosatti
left a comment
There was a problem hiding this comment.
I left a minor comment, otherwise code looks good to me :)
|
|
||
| return ( | ||
| <IfFeature flag="components-page" fallback={null}> | ||
| <div> |
There was a problem hiding this comment.
nit: wrapping with <div> here is unnecessary, since it's wrapping only one child and we're not passing any props to it :)
There was a problem hiding this comment.
This is been uodated. Removed Div
Assisted-by: Cursor
rakshett
dismissed stale reviews from StanislavJochman and rrosatti
via
257acbe
|
/review |
| return getErrorState(componentError, loaded, 'component'); | ||
| } | ||
|
|
||
| const applicationName = component?.spec?.application; |
There was a problem hiding this comment.
application will be removed from the component, so it should not be used
https://github.com/konflux-ci/application-api/blob/main/api/v1alpha1/component_types.go#L250-L251
5 participants
Fixes
https://issues.redhat.com/browse/KFLUXUI-1006
Description
Implements the Activity tab for the new Component details page (V2 route).
The Activity tab displays:
Commits sub-tab: Shows commit history for the component using CommitsListView
Pipeline runs sub-tab: Shows pipeline runs for the component using PipelineRunsTab
Type of change
Screen shots / Gifs for design review
How to test or reproduce?
Navigate to Components page
Click on any component to open Component details
Click on the "Activity" tab
Verify both "Commits" and "Pipeline runs" sub-tabs work
Verify tab selection persists in localStorage
Browser conformance:
Summary by CodeRabbit
New Features
Tests