feat(RHINENG-25194): CVE Table migration to new library#2587
Closed
Siasurai wants to merge 7 commits into
Closed
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
useCvesPage, there is aconsole.login auseEffectthat will fire in production; consider removing it or routing through whatever logging/telemetry mechanism you normally use to avoid noisy console output.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `useCvesPage`, there is a `console.log` in a `useEffect` that will fire in production; consider removing it or routing through whatever logging/telemetry mechanism you normally use to avoid noisy console output.
## Individual Comments
### Comment 1
<location path="src/Helpers/tableToolsSerialisers.js" line_range="9-10" />
<code_context>
+ }
+};
+
+const textFilterSerialiser = (filterConfigItem, value) =>
+ ƒ`regex(.${filterConfigItem.filterAttribute}, "${value}", "i")`;
+const checkboxFilterSerialiser = (filterConfigItem, values) =>
+ `.${filterConfigItem.filterAttribute} in [${values
</code_context>
<issue_to_address>
**issue (bug_risk):** The text filter serialiser contains an invalid character and will not compile.
Please replace the `ƒ` usage with a valid function or template-literal syntax. For example:
```js
const textFilterSerialiser = (filterConfigItem, value) =>
`regex(.${filterConfigItem.filterAttribute}, "${value}", "i")`;
```
or adjust the string format to whatever the table tools query engine expects.
</issue_to_address>
### Comment 2
<location path="src/Components/SmartComponents/CVEs/useCveTableTableToolsState.js" line_range="29-44" />
<code_context>
+
+ const isEmpty = cves.data.length === 0;
+
+ const emptyRows = useMemo(
+ () => [
+ {
+ heightAuto: true,
+ cells: [
+ {
+ props: { colSpan: header?.length },
+ title: <EmptyStateNoCVEs secondParagraph={emptyMessage} />,
+ },
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Empty-state row colSpan may not cover the selection/expand columns.
`emptyRows` uses `colSpan: header?.length`, but the table can render extra columns for expand/selection (`isExpandable`, `canCollapseAll`, `canSelectAll`). This can leave the empty-state cell too narrow. Please derive `colSpan` from the full rendered column count (including expand/selection), e.g. by computing it from the same header structure as `buildCveExpandableSortHeader` rather than `header.length` alone.
```suggestion
const isEmpty = cves.data.length === 0;
const emptyRows = useMemo(
() => [
{
heightAuto: true,
cells: [
{
props: { colSpan: sortHeaderWithCollapse?.length },
title: <EmptyStateNoCVEs secondParagraph={emptyMessage} />,
},
],
},
],
[emptyMessage, sortHeaderWithCollapse?.length],
);
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
bastilian
reviewed
Apr 30, 2026
| import { SkeletonTable } from '@patternfly/react-component-groups'; | ||
| import { useCveTableTableToolsState } from './useCveTableTableToolsState'; | ||
|
|
||
| const CVEsTableTableToolsInner = ({ header }) => { |
Member
There was a problem hiding this comment.
What was the reason for building a custom table? As far as i can see this seems to only replicate https://github.com/bastilian/tabletools/blob/main/src/components/TableToolsTable/TableToolsTable.js#L20
Contributor
Author
There was a problem hiding this comment.
😶🌫️ To be honest, there wasn’t a deeper reason. I ran into issues while wiring everything up, so I debugged it step by step and then I forgot to switch the manual Table over to TableToolsTable. Sorry, that’s on me (and my short memory)
I’ll take this back as a follow-up refactor to use the proper component.
Contributor
Author
There was a problem hiding this comment.
cleaned it up, it should be better overall now
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
useCveTableToolsQueryhook uses a fixedqueryKey: ['cveListByAccount', 'tableTools']while passingparamsseparately; ifuseQueryWithUtilitieskeys only byqueryKeythis will cause different parameter combinations to share cache, so consider incorporatingparams(or a stable hash of them) into the query key to avoid stale or mixed results.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `useCveTableToolsQuery` hook uses a fixed `queryKey: ['cveListByAccount', 'tableTools']` while passing `params` separately; if `useQueryWithUtilities` keys only by `queryKey` this will cause different parameter combinations to share cache, so consider incorporating `params` (or a stable hash of them) into the query key to avoid stale or mixed results.
## Individual Comments
### Comment 1
<location path="src/Helpers/tableToolsSerialisers.js" line_range="9-10" />
<code_context>
+ }
+};
+
+const textFilterSerialiser = (filterConfigItem, value) =>
+ ƒ`regex(.${filterConfigItem.filterAttribute}, "${value}", "i")`;
+const checkboxFilterSerialiser = (filterConfigItem, values) =>
+ `.${filterConfigItem.filterAttribute} in [${values
</code_context>
<issue_to_address>
**issue (bug_risk):** The text filter serialiser uses an undefined template tag `ƒ`, which will cause a runtime error.
In `textFilterSerialiser`, ``ƒ`regex(...)` `` is parsed as a tagged template using an identifier `ƒ` that is never defined, so this will throw at runtime when invoked. If this should just be a normal template string, remove the tag:
```js
const textFilterSerialiser = (filterConfigItem, value) =>
`regex(.${filterConfigItem.filterAttribute}, "${value}", "i")`;
```
If a tagged template was intended, please add and import the `ƒ` tag implementation.
</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
+9
to
+10
| const textFilterSerialiser = (filterConfigItem, value) => | ||
| ƒ`regex(.${filterConfigItem.filterAttribute}, "${value}", "i")`; |
There was a problem hiding this comment.
issue (bug_risk): The text filter serialiser uses an undefined template tag ƒ, which will cause a runtime error.
In textFilterSerialiser, ƒ`regex(...)` is parsed as a tagged template using an identifier ƒ that is never defined, so this will throw at runtime when invoked. If this should just be a normal template string, remove the tag:
const textFilterSerialiser = (filterConfigItem, value) =>
`regex(.${filterConfigItem.filterAttribute}, "${value}", "i")`;If a tagged template was intended, please add and import the ƒ tag implementation.
Contributor
Author
|
#2642 WIP |
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
2 participants
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.
What
vulnerability.table-tools: switch between the old and the new table safely.How to test
vulnerability.table-toolsReload the page and check:.../vulnerabilities/cvesrequest after load.Summary by Sourcery
Introduce a new feature-flagged CVE table implementation using bastilian-tabletools while centralizing CVEs page logic into a dedicated hook and updating data-fetching and state sync behavior.
New Features:
Bug Fixes:
Enhancements:
Build:
Tests: