feat(groups): Add Groups list page#888
Draft
CryptoRodeo wants to merge 41 commits intoguacsec:mainfrom
Draft
Conversation
Work In Progress. Signed-off-by: Bryan Ramos <bramos@redhat.com>
Contributor
Reviewer's GuideAdds a new Groups list page wired into routing, sidebar navigation, and the shared table-controls system, backed by a dummy groups query that builds a hierarchical tree and renders an expandable, bulk-selectable table with filtering and pagination. Sequence diagram for loading and displaying the Groups listsequenceDiagram
actor User
participant Sidebar
participant Router
participant Groups
participant GroupsProvider
participant TableControls as TableControls_hooks
participant GroupsQuery as useFetchGroups
participant ReactQuery as ReactQuery_useQuery
User->>Sidebar: Click_Groups_link
Sidebar->>Router: Navigate_to_Paths.groups
Router->>Groups: Render_Groups_component
Groups->>GroupsProvider: Wrap_children_in_GroupsProvider
GroupsProvider->>TableControls: useTableControlState(config_for_groups)
TableControls-->>GroupsProvider: tableControlState
GroupsProvider->>GroupsQuery: useFetchGroups(hubRequestParams)
GroupsQuery->>ReactQuery: useQuery(queryKey_groups, queryFn)
ReactQuery-->>GroupsQuery: dummyData(total,items)
GroupsQuery->>GroupsQuery: buildGroupTree(items)
GroupsQuery-->>GroupsProvider: roots,total,isFetching,fetchError
GroupsProvider->>TableControls: useTableControlProps(state,roots,total)
TableControls-->>GroupsProvider: tableControls
GroupsProvider-->>Groups: Provide_GroupsContext(tableControls,bulkSelection,...)
Groups->>GroupsToolbar: Render_toolbar_with_GroupsContext
Groups->>GroupsTable: Render_table_with_GroupsContext
GroupsTable->>GroupsContext: Read_tableControls_and_bulkSelection
GroupsTable->>GroupsTable: flattenVisibleRows(currentPageItems,isCellExpanded)
GroupsTable-->>User: Display_expandable_tree_table_with_pagination
Class diagram for new Groups types and componentsclassDiagram
class TGroupDD {
+string id
+string parent
+string name
+Record~string,string~ labels
}
class TGroupTreeNode {
+string id
+string parent
+string name
+Record~string,string~ labels
+TGroupTreeNode[] children
}
TGroupDD <|-- TGroupTreeNode
class UseFetchGroupsHook {
+useFetchGroups(params, disableQuery)
+buildGroupTree(items)
}
TGroupDD --> UseFetchGroupsHook : items
TGroupTreeNode --> UseFetchGroupsHook : roots
class IGroupsContext {
+ITableControls tableControls
+BulkSelectionValues bulkSelection_controls
+boolean bulkSelection_isEnabled
+number totalItemCount
+boolean isFetching
+AxiosError fetchError
}
class GroupsContext {
+IGroupsContext value
}
class GroupsProvider {
+boolean isBulkSelectionEnabled
+ReactNode children
}
class GroupsToolbar {
+render()
}
class GroupsTable {
+render()
+flattenVisibleRows(nodes, isExpanded, depth)
}
class GroupTableData {
+TGroupTreeNode item
+render()
}
class GroupLabels {
+Record~string,string~ labels
+render()
}
class GroupsPage {
+render()
}
class TableControlsHooks {
+useTableControlState(config)
+useTableControlProps(args)
}
class BulkSelectionHook {
+useBulkSelection(config)
}
class TablePersistenceKeyPrefixes {
+string groups
}
GroupsContext ..> IGroupsContext
GroupsProvider --> GroupsContext : provides
GroupsProvider --> TableControlsHooks : uses
GroupsProvider --> BulkSelectionHook : uses
GroupsProvider --> UseFetchGroupsHook : uses
GroupsPage --> GroupsProvider
GroupsPage --> GroupsToolbar
GroupsPage --> GroupsTable
GroupsToolbar --> GroupsContext : consumes
GroupsTable --> GroupsContext : consumes
GroupsTable --> GroupTableData
GroupTableData --> GroupLabels
TablePersistenceKeyPrefixes <.. GroupsProvider : uses_prefix_groups
UseFetchGroupsHook ..> ReactQuery : uses
class ReactQuery {
+useQuery(options)
}
Flow diagram for navigation and routing to the Groups pageflowchart LR
subgraph SidebarNavigation
SBOMsLink["SBOMs_nav_link"]
GroupsLink["Groups_nav_link"]
end
PathsGroups["Paths.groups_/groups"]
RouterConfig["AppRoutes_route_for_groups"]
GroupsPage["Groups_page_component"]
Provider["GroupsProvider_context"]
Toolbar["GroupsToolbar"]
Table["GroupsTable"]
GroupsLink --> PathsGroups
PathsGroups --> RouterConfig
RouterConfig --> GroupsPage
GroupsPage --> Provider
Provider --> Toolbar
Provider --> Table
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 7 issues, and left some high level feedback:
- In
useFetchGroups, the query currently resolvesdummyDataandbuildGroupTreeis called ondummyData.itemsinstead of the querieddata?.items; this will make it easy to forget to switch to real data—consider wiring the tree-building to the actual response (and memoizing it) so the hook’s output always reflects the query result. - The
GroupsTablerenders multiple<Tbody>elements and uses fragments and some<Tbody>tags without stable keys; this can confuse React and PatternFly table expectations—consider using a single<Tbody>with PatternFly’s built‑in expandable row pattern (or at least ensure each logical row/section has a stable key on the immediate mapped element). - There are a few type loosenings that could cause issues later (e.g.,
GroupTableData’sitemis typed asanyandgetTdPropsnow allowscolumnKeyto be optional); tightening these toTGroupTreeNodeand requiringcolumnKeywhere appropriate will help catch misuse of table helpers at compile time.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `useFetchGroups`, the query currently resolves `dummyData` and `buildGroupTree` is called on `dummyData.items` instead of the queried `data?.items`; this will make it easy to forget to switch to real data—consider wiring the tree-building to the actual response (and memoizing it) so the hook’s output always reflects the query result.
- The `GroupsTable` renders multiple `<Tbody>` elements and uses fragments and some `<Tbody>` tags without stable keys; this can confuse React and PatternFly table expectations—consider using a single `<Tbody>` with PatternFly’s built‑in expandable row pattern (or at least ensure each logical row/section has a stable key on the immediate mapped element).
- There are a few type loosenings that could cause issues later (e.g., `GroupTableData`’s `item` is typed as `any` and `getTdProps` now allows `columnKey` to be optional); tightening these to `TGroupTreeNode` and requiring `columnKey` where appropriate will help catch misuse of table helpers at compile time.
## Individual Comments
### Comment 1
<location> `client/src/app/queries/groups.ts:17-26` </location>
<code_context>
+const dummyData: { total: number, items: TGroupDD[], tree: Record<string, any> } = {
</code_context>
<issue_to_address>
**issue (bug_risk):** `dummyData.total` does not match the number of items defined.
`total` is set to 10 but there are only 9 entries in `items`, so pagination will indicate more rows than exist. Please keep `total` consistent with `items.length`, even for stub data, to avoid confusing UI behavior and hiding real pagination issues.
</issue_to_address>
### Comment 2
<location> `client/src/app/pages/groups/groups-table.tsx:52-61` </location>
<code_context>
+ <Table {...tableProps} aria-label="sbom-table">
</code_context>
<issue_to_address>
**nitpick (bug_risk):** The table ARIA label and pagination ID still reference `sbom-table` instead of `groups`.
`Table`'s `aria-label` and `SimplePagination`'s `idPrefix` still use `"sbom-table"`, likely copied from the SBOM view. Please rename them (e.g., to `"groups-table"`) so the DOM/ARIA metadata correctly reflects this page’s content and remains clear for accessibility and automation.
</issue_to_address>
### Comment 3
<location> `client/src/app/pages/groups/groups-table.tsx:63-51` </location>
<code_context>
+ isNoData={totalItemCount === 0}
+ numRenderedColumns={numRenderedColumns}
+ >
+ {currentPageItems.map((item, rowIndex) => {
+ return (
+ <>
+ <Tbody key={item.id} isExpanded={isCellExpanded(item)}>
+ <Tr {...getTrProps({ item })}>
</code_context>
<issue_to_address>
**issue (bug_risk):** Using a bare fragment inside `.map` without a key will trigger React key warnings.
In `currentPageItems.map`, the fragment (`<>...</>`) is the top-level returned element, but the `key` is on the nested `<Tbody>`. React expects the `key` on the top-level element, which can cause key warnings and reconciliation issues. Either move the `key` to the fragment (e.g., `<React.Fragment key={item.id}>`) or remove the fragment and wrap the rows so that the keyed element is the one directly returned from `map`.
</issue_to_address>
### Comment 4
<location> `client/src/app/pages/groups/groups-table.tsx:90-92` </location>
<code_context>
+
+ </Tbody>
+ {
+ item.children.map((childNode) => {
+ return (
+ <Tbody>
+ <Tr
+ key={childNode.id}
</code_context>
<issue_to_address>
**issue (bug_risk):** Child `Tbody` elements are missing a `key` and will also hit React’s key requirements.
For each `item.children.map` iteration, add a `key` prop to the returned `<Tbody>` (for example, `key={childNode.id}`) to avoid React warnings and potential instability when rows are added, removed, or reordered.
</issue_to_address>
### Comment 5
<location> `client/src/app/pages/groups/groups-context.tsx:89-93` </location>
<code_context>
+ idProperty: "id",
+ currentPageItems: groups,
+ totalItemCount,
+ isLoading: false,
+ hasActionsColumn: true,
+ });
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Table controls are always told `isLoading: false`, ignoring the actual fetch status.
`useFetchGroups` already provides `isFetching`, but the table controls ignore it and always show `isLoading: false`. Wire `isLoading` to `isFetching` so loading indicators and disabled states correctly reflect re-fetches and avoid inconsistent UX.
```suggestion
const tableControls = useTableControlProps({
...tableControlState,
idProperty: "id",
currentPageItems: groups,
totalItemCount,
isLoading: isFetching,
```
</issue_to_address>
### Comment 6
<location> `client/src/app/pages/groups/group-table-data.tsx:4` </location>
<code_context>
+import { Stack, StackItem, Flex, FlexItem, Label, LabelGroup } from "@patternfly/react-core"
+import { NavLink } from "react-router-dom"
+
+export const GroupTableData = ({ item }: { item: any }) => {
+
+ return (
</code_context>
<issue_to_address>
**suggestion:** The `item` prop is typed as `any`, losing the structural guarantees you have elsewhere.
`GroupTableData` takes `{ item: any }` even though the groups domain already defines `TGroupTreeNode`. Typing `item` as `TGroupTreeNode` (or a shared interface) would give you compile-time checks for `item.name` and any future fields rendered here, making changes safer and catching shape mismatches earlier.
Suggested implementation:
```typescript
import { Stack, StackItem, Flex, FlexItem, Label, LabelGroup } from "@patternfly/react-core"
import { NavLink } from "react-router-dom"
import type { TGroupTreeNode } from "PATH/TO/GROUPS/DOMAIN/TGroupTreeNode"
export const GroupTableData = ({ item }: { item: TGroupTreeNode }) => {
```
You’ll need to adjust the import path for `TGroupTreeNode` so it matches your existing groups domain type export. For example, it might be something like:
- `import type { TGroupTreeNode } from "@/app/api/groups/types"` or
- `import type { TGroupTreeNode } from "../../api/groups/types"`
Also make sure that `TGroupTreeNode` exposes at least the fields you are using in this component (e.g. `name`, `id`, etc.), or adjust the type if `GroupTableData` needs a more specific subset.
</issue_to_address>
### Comment 7
<location> `client/src/app/pages/groups/group-table-data.tsx:14-16` </location>
<code_context>
</NavLink>
</li>
+ <li className={nav.navItem}>
+ <NavLink
+ to={Paths.groups}
+ className={({ isActive }) => {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Linking the group name to an external placeholder URL is likely not the intended navigation behavior.
The `NavLink` for the group name still points to `'https://example.com'`, which looks like a temporary stub. If this should go to a group details view, please update it to the correct internal route, or remove the link until that route exists to avoid sending users off-site unexpectedly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
14
to
16
| <NavLink | ||
| className="pf-v6-c-button pf-m-link pf-m-inline" | ||
| to={'https://example.com'} |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Linking the group name to an external placeholder URL is likely not the intended navigation behavior.
The NavLink for the group name still points to 'https://example.com', which looks like a temporary stub. If this should go to a group details view, please update it to the correct internal route, or remove the link until that route exists to avoid sending users off-site unexpectedly.
Signed-off-by: Bryan Ramos <bramos@redhat.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #888 +/- ##
==========================================
- Coverage 63.60% 63.58% -0.03%
==========================================
Files 184 184
Lines 3264 3262 -2
Branches 742 743 +1
==========================================
- Hits 2076 2074 -2
+ Misses 898 895 -3
- Partials 290 293 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Bryan Ramos <bramos@redhat.com>
Signed-off-by: Bryan Ramos <bramos@redhat.com>
Signed-off-by: Bryan Ramos <bramos@redhat.com>
The structure should be what the ADR defines Signed-off-by: Bryan Ramos <bramos@redhat.com>
Signed-off-by: Bryan Ramos <bramos@redhat.com>
Go with a flattening strategy. Signed-off-by: Bryan Ramos <bramos@redhat.com>
Signed-off-by: Bryan Ramos <bramos@redhat.com>
Collaborator
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- In
GroupLabels, the emptiness checkObject.keys(labels) === 0is always false; useObject.keys(labels).length === 0to avoid rendering an emptyLabelGroup. - In
GroupsTable, the bottomSimplePaginationusesidPrefix="sbom-table"while the rest of the page is groups-specific (and the toolbar usesgroups-table), which looks like a copy-paste mismatch that could complicate DOM IDs or test selectors. - In
GroupTableData, theNavLinkpoints to a hard-coded external URL (https://example.com); consider wiring this to the appropriate groups route or making the placeholder explicit so it doesn’t ship as-is.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `GroupLabels`, the emptiness check `Object.keys(labels) === 0` is always false; use `Object.keys(labels).length === 0` to avoid rendering an empty `LabelGroup`.
- In `GroupsTable`, the bottom `SimplePagination` uses `idPrefix="sbom-table"` while the rest of the page is groups-specific (and the toolbar uses `groups-table`), which looks like a copy-paste mismatch that could complicate DOM IDs or test selectors.
- In `GroupTableData`, the `NavLink` points to a hard-coded external URL (`https://example.com`); consider wiring this to the appropriate groups route or making the placeholder explicit so it doesn’t ship as-is.
## Individual Comments
### Comment 1
<location> `client/src/app/hooks/table-controls/types.ts:426` </location>
<code_context>
*/
getTdProps: (
- args: { columnKey: TColumnKey } & DiscriminatedArgs<
+ args: { columnKey?: TColumnKey } & DiscriminatedArgs<
"isCompoundExpandToggle",
{ item: TItem; rowIndex: number }
</code_context>
<issue_to_address>
**issue (bug_risk):** Making `columnKey` optional in `getTdProps` may break assumptions in existing call sites and implementations.
Since `columnKey` was previously required, making it optional can break existing `getTdProps` implementations that assume it’s always defined (e.g., using it as a map key or for metrics), leading to runtime errors. Consider either a separate helper for the compound expand toggle case or a discriminated union where only the `isCompoundExpandToggle` branch omits `columnKey`, while keeping it required elsewhere.
</issue_to_address>
### Comment 2
<location> `client/src/app/pages/groups/group-labels.tsx:8` </location>
<code_context>
+
+// TODO: Change color based on label type
+export const GroupLabels = ({ labels }: Props) => {
+ if (!labels || Object.keys(labels) === 0) {
+ return null;
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** The empty-labels check is incorrect and will never treat an empty object as empty.
`Object.keys(labels)` returns an array, so comparing it directly to `0` is always false. As a result, an empty `labels` object still renders `LabelGroup`. Use `Object.keys(labels).length === 0` (or `!Object.keys(labels).length`) instead.
</issue_to_address>
### Comment 3
<location> `client/src/app/pages/groups/groups-table.tsx:129-133` </location>
<code_context>
+ })}
+ </ConditionalTableBody >
+ </Table >
+ <SimplePagination
+ idPrefix="sbom-table"
+ isTop={false}
+ paginationProps={paginationProps}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The pagination `idPrefix` is still `sbom-table`, which is confusing and may cause DOM id collisions.
This appears copied from the SBOM table. Reusing `idPrefix="sbom-table"` here complicates debugging and test selectors, and risks duplicate DOM IDs when both views are rendered. Please use a more specific prefix, e.g. `"groups-table"`.
```suggestion
<SimplePagination
idPrefix="groups-table"
isTop={false}
paginationProps={paginationProps}
/>
```
</issue_to_address>
### Comment 4
<location> `client/src/app/queries/groups.ts:293-295` </location>
<code_context>
+) => {
+ // const { q, ...rest } = requestParamsQuery((params));
+
+ const { data, isLoading, error, refetch } = useQuery({
+ queryKey: [GroupQueryKey, params],
+ queryFn: () => Promise.resolve(dummyData),
+ enabled: !disableQuery
+ });
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The hook ignores the backend `total` and derives `total` from roots only, which may misrepresent pagination once real data is used.
`useFetchGroups` currently sets `total` to `roots.length` even though the API response includes its own `total`. If the real API returns only a page of items plus a larger `total`, this will under-report the count and can break pagination controls. Please use `data?.total` for the overall count (optionally still exposing `roots.length` separately) so this hook aligns with other list hooks and behaves correctly once wired to the real backend.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Signed-off-by: Bryan Ramos <bramos@redhat.com>
Signed-off-by: Bryan Ramos <bramos@redhat.com>
Signed-off-by: Bryan Ramos <bramos@redhat.com>
Signed-off-by: Bryan Ramos <bramos@redhat.com>
Collaborator
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- There is a type mismatch between
labelsinTGroupDD(Record<string, any>) and inGroupLabels(Record<string, string>); aligning these will prevent subtle runtime issues and unnecessary casting. - The
NavLinkinGroupTableDatais hard-coded tohttps://example.com; consider wiring this to a real route or an ID-based detail path so the link behavior matches the data model. - The
useFetchGroupshook still relies ondummyDataand aPromise.resolvequery function; adding a clear TODO and centralizing this mock behind a flag or test helper will reduce the risk of it persisting into production unintentionally.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There is a type mismatch between `labels` in `TGroupDD` (`Record<string, any>`) and in `GroupLabels` (`Record<string, string>`); aligning these will prevent subtle runtime issues and unnecessary casting.
- The `NavLink` in `GroupTableData` is hard-coded to `https://example.com`; consider wiring this to a real route or an ID-based detail path so the link behavior matches the data model.
- The `useFetchGroups` hook still relies on `dummyData` and a `Promise.resolve` query function; adding a clear TODO and centralizing this mock behind a flag or test helper will reduce the risk of it persisting into production unintentionally.
## Individual Comments
### Comment 1
<location> `client/src/app/pages/groups/groups-table.tsx:85-86` </location>
<code_context>
+ return (
+ <>
+ <Table {...tableProps} aria-label="sbom-groups-table">
+ <Thead>
+ <Tr>
+ </Tr>
+ </Thead>
</code_context>
<issue_to_address>
**issue:** The table header row is empty, which produces a column-less header in the UI.
Since `<Thead>` contains an empty `<Tr>`, the table renders with no visible column headers. Consider adding at least one meaningful header cell (e.g., a placeholder like "Name") or omitting the header until real columns are available to avoid a confusing header row.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Signed-off-by: Bryan Ramos <bramos@redhat.com>
Signed-off-by: Bryan Ramos <bramos@redhat.com>
Signed-off-by: Bryan Ramos <bramos@redhat.com>
Collaborator
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
GroupTableDataandGroupLabels, you're wrapping labels inLabelGrouptwice, which will render nestedLabelGroupcomponents; consider letting onlyGroupLabelsown theLabelGroupor having it render justLabels. - The
NavLinkinGroupTableDatais hard-coded tohttps://example.com; if this is a temporary placeholder, consider wiring it to an internal path (e.g., usingPaths) or at least marking it with a clear TODO so it’s not forgotten. - In
GroupsContext,isSortEnabledis set tofalsebuthubSortFieldKeysis still configured ingetHubRequestParams; either enable sorting or remove the unused sort mapping to avoid confusion.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `GroupTableData` and `GroupLabels`, you're wrapping labels in `LabelGroup` twice, which will render nested `LabelGroup` components; consider letting only `GroupLabels` own the `LabelGroup` or having it render just `Label`s.
- The `NavLink` in `GroupTableData` is hard-coded to `https://example.com`; if this is a temporary placeholder, consider wiring it to an internal path (e.g., using `Paths`) or at least marking it with a clear TODO so it’s not forgotten.
- In `GroupsContext`, `isSortEnabled` is set to `false` but `hubSortFieldKeys` is still configured in `getHubRequestParams`; either enable sorting or remove the unused sort mapping to avoid confusion.
## Individual Comments
### Comment 1
<location> `client/src/app/pages/groups/group-table-data.tsx:14-23` </location>
<code_context>
+ alignItems={{ default: 'alignItemsCenter' }}
+ gap={{ default: 'gapSm' }}
+ flexWrap={{ default: "wrap" }}>
+ <FlexItem>
+ <NavLink
+ className="pf-v6-c-button pf-m-link pf-m-inline"
+ to={'https://example.com'}
+ >
+ {item.name}
+ </NavLink>
+ </FlexItem>
+ <FlexItem>
+ <LabelGroup>
+ <GroupLabels labels={item.labels} />
+ </LabelGroup>
+ </FlexItem>
</code_context>
<issue_to_address>
**issue:** Avoid nesting `LabelGroup` inside another `LabelGroup` when rendering labels.
`GroupLabels` already wraps its contents in a `LabelGroup`, so this creates nested `LabelGroup`s that can affect spacing and styling. Either drop the outer `LabelGroup` and let `GroupLabels` handle layout, or change `GroupLabels` to render plain `Label`s and keep the `LabelGroup` only here.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Signed-off-by: Bryan Ramos <bramos@redhat.com>
Signed-off-by: Bryan Ramos <bramos@redhat.com>
Signed-off-by: Bryan Ramos <bramos@redhat.com>
Collaborator
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
useFetchGroupshook importsdummyDatafrom the page layer, which inverts normal layering; consider moving the dummy data into the queries layer (or a shared fixtures module) so the data layer doesn’t depend on UI code. - In
GroupTableData, the group name links to a hardcodedhttps://example.com; even as a placeholder, it would be safer to either omit the link or route to an internal path to avoid shipping an external URL by mistake.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `useFetchGroups` hook imports `dummyData` from the page layer, which inverts normal layering; consider moving the dummy data into the queries layer (or a shared fixtures module) so the data layer doesn’t depend on UI code.
- In `GroupTableData`, the group name links to a hardcoded `https://example.com`; even as a placeholder, it would be safer to either omit the link or route to an internal path to avoid shipping an external URL by mistake.
## Individual Comments
### Comment 1
<location> `client/src/app/pages/groups/group-table-data.tsx:15-17` </location>
<code_context>
</NavLink>
</li>
+ <li className={nav.navItem}>
+ <NavLink
+ to={Paths.groups}
+ className={({ isActive }) => {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Using `NavLink` with an external URL is likely unintended and may break routing expectations.
`NavLink` is designed for internal routes and SPA history, so targeting a hardcoded external URL is confusing and may bypass the router. Use a plain `<a href="https://example.com">` for external navigation, or better, point `NavLink` at the appropriate internal route (e.g., a group details path derived from `item.id`).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
15
to
17
| <NavLink | ||
| className="pf-v6-c-button pf-m-link pf-m-inline" | ||
| to={'https://example.com'} |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Using NavLink with an external URL is likely unintended and may break routing expectations.
NavLink is designed for internal routes and SPA history, so targeting a hardcoded external URL is confusing and may bypass the router. Use a plain <a href="https://example.com"> for external navigation, or better, point NavLink at the appropriate internal route (e.g., a group details path derived from item.id).
Signed-off-by: Bryan Ramos <bramos@redhat.com>
Signed-off-by: Bryan Ramos <bramos@redhat.com>
Also format Signed-off-by: Bryan Ramos <bramos@redhat.com>
Signed-off-by: Bryan Ramos <bramos@redhat.com>
Signed-off-by: Bryan Ramos <bramos@redhat.com>
No reason to flatten twice Signed-off-by: Bryan Ramos <bramos@redhat.com>
Also update comment. Signed-off-by: Bryan Ramos <bramos@redhat.com>
Signed-off-by: Bryan Ramos <bramos@redhat.com>
Signed-off-by: Bryan Ramos <bramos@redhat.com>
I guess PatternFly has that. I should read the docs more. Signed-off-by: Bryan Ramos <bramos@redhat.com>
Signed-off-by: Bryan Ramos <bramos@redhat.com>
- Cleanup unused code - Go back to single node selection - Add action column with dummy actions Signed-off-by: Bryan Ramos <bramos@redhat.com>
Signed-off-by: Bryan Ramos <bramos@redhat.com>
- remove comments - update return value names Signed-off-by: Bryan Ramos <bramos@redhat.com>
Signed-off-by: Bryan Ramos <bramos@redhat.com>
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.
🚧 Work In Progress. 🚧
Demo of what it currently looks like
GroupsTreeTableDemoWIP.mp4
Summary by Sourcery
Add a new Groups listing page wired into navigation and routing, backed by a table controls context and dummy query data.
New Features:
/groupsroute and sidebar navigation link to access the Groups list.Enhancements:
Summary by Sourcery
Add a new Groups listing page with navigation, routing, and table controls based on dummy group data.
New Features:
/groupsroute and sidebar navigation entry to access the Groups list.Enhancements: