feat: add sort on port forward list columns #13119
Conversation
Signed-off-by: Simon Rey <sfbrey+eqqe@gmail.com>
Signed-off-by: Simon Rey <sfbrey+eqqe@gmail.com>
📝 WalkthroughWalkthroughThe changes introduce sorting functionality to the PortForwardingList table by adding comparator functions to the column definitions for "Name," "Type," "Local Port," and "Remote Port" in the Svelte component. The test suite for PortForwardingList was also expanded to verify the presence and correctness of sorting, as well as to cover rendering scenarios with various port forward entries. No changes were made to exported or public entities outside of the test file and the addition of comparator functions. Assessment against linked issues
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/renderer/src/lib/kubernetes-port-forward/PortForwardingList.spec.tsOops! Something went wrong! :( ESLint: 9.30.1 Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'svelte-eslint-parser' imported from /eslint.config.mjs packages/renderer/src/lib/kubernetes-port-forward/PortForwardingList.svelteOops! Something went wrong! :( ESLint: 9.30.1 Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'svelte-eslint-parser' imported from /eslint.config.mjs 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
🔇 Additional comments (10)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Signed-off-by: Simon Rey <sfbrey+eqqe@gmail.com>
Signed-off-by: Simon Rey <sfbrey+eqqe@gmail.com>
There was a problem hiding this comment.
Hey @eqqe - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `packages/renderer/src/lib/kubernetes-port-forward/PortForwardingList.spec.ts:36` </location>
<code_context>
- vi.mocked(kubeContextStore).kubernetesCurrentContextPortForwards = readable([]);
});
test('empty kubernetesCurrentContextPortForwards store should display empty screen', async () => {
+ vi.mocked(kubeContextStore).kubernetesCurrentContextPortForwards = readable([]);
const { getByText } = render(PortForwardList);
</code_context>
<issue_to_address>
Consider extracting shared setup and assertion logic into helper functions to reduce repetition in your tests.
```markdown
You can DRY up the repetitive setup/assertion in your spec by extracting two small helpers at the top of the file:
```js
// --- at top of PortForwardingList.spec.ts ---
function renderList(forwards: ForwardConfig[]) {
vi.mocked(kubeContextStore).kubernetesCurrentContextPortForwards = readable(forwards);
const utils = render(PortForwardList);
const rowgroups = utils.getAllByRole('rowgroup');
return {
...utils,
headerRows: rowgroups[0],
bodyRows: within(rowgroups[1]).getAllByRole('row'),
};
}
function getColumnData(rows: HTMLElement[], colIdx: number): string[] {
return rows.map(row => {
const cell = within(row).getAllByRole('cell')[colIdx];
return cell.textContent?.trim() ?? '';
});
}
```
Then each test becomes much smaller:
```js
test('empty state', () => {
const { getByText } = renderList([]);
expect(
getByText(
'To forward ports, open the Summary tab on the relevant resource (Pod, Service, or Deployment)',
),
).toBeInTheDocument();
expect(getByText('No port forwarding configured')).toBeInTheDocument();
});
test('one forward shows correct cells', () => {
const cfg: ForwardConfig = { /*…*/ };
const { bodyRows } = renderList([cfg]);
const cells = within(bodyRows[0]).getAllByRole('cell');
expect(cells[2]).toHaveTextContent('my-pod');
expect(cells[4]).toHaveTextContent('80');
// …
});
test('renders N rows', () => {
const arr = Array.from({ length: 10 }, (_, i) => ({ /*…*/ }));
const { bodyRows } = renderList(arr);
expect(bodyRows).toHaveLength(10);
});
test('sorting works', async () => {
const configs = [ /*…unsorted…*/ ];
const { bodyRows, getByRole } = renderList(configs);
// default by name
expect(getColumnData(bodyRows, 2)).toEqual(['alpha-svc', 'middle-dep', 'zebra-pod']);
// click Local Port
const lpHead = getByRole('columnheader', { name: 'Local Port' });
await fireEvent.click(lpHead);
expect(getColumnData(bodyRows, 4)).toEqual(['5000', '7000', '9000']);
await fireEvent.click(lpHead);
expect(getColumnData(bodyRows, 4)).toEqual(['9000', '7000', '5000']);
});
```
This removes all the copy-pasted `render`, `getAllByRole('rowgroup')`, and manual mapping code, while keeping each test focused on its unique assertions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| vi.mocked(kubeContextStore).kubernetesCurrentContextPortForwards = readable([]); | ||
| }); | ||
|
|
||
| test('empty kubernetesCurrentContextPortForwards store should display empty screen', async () => { |
There was a problem hiding this comment.
issue (complexity): Consider extracting shared setup and assertion logic into helper functions to reduce repetition in your tests.
You can DRY up the repetitive setup/assertion in your spec by extracting two small helpers at the top of the file:
```js
// --- at top of PortForwardingList.spec.ts ---
function renderList(forwards: ForwardConfig[]) {
vi.mocked(kubeContextStore).kubernetesCurrentContextPortForwards = readable(forwards);
const utils = render(PortForwardList);
const rowgroups = utils.getAllByRole('rowgroup');
return {
...utils,
headerRows: rowgroups[0],
bodyRows: within(rowgroups[1]).getAllByRole('row'),
};
}
function getColumnData(rows: HTMLElement[], colIdx: number): string[] {
return rows.map(row => {
const cell = within(row).getAllByRole('cell')[colIdx];
return cell.textContent?.trim() ?? '';
});
}Then each test becomes much smaller:
test('empty state', () => {
const { getByText } = renderList([]);
expect(
getByText(
'To forward ports, open the Summary tab on the relevant resource (Pod, Service, or Deployment)',
),
).toBeInTheDocument();
expect(getByText('No port forwarding configured')).toBeInTheDocument();
});
test('one forward shows correct cells', () => {
const cfg: ForwardConfig = { /*…*/ };
const { bodyRows } = renderList([cfg]);
const cells = within(bodyRows[0]).getAllByRole('cell');
expect(cells[2]).toHaveTextContent('my-pod');
expect(cells[4]).toHaveTextContent('80');
// …
});
test('renders N rows', () => {
const arr = Array.from({ length: 10 }, (_, i) => ({ /*…*/ }));
const { bodyRows } = renderList(arr);
expect(bodyRows).toHaveLength(10);
});
test('sorting works', async () => {
const configs = [ /*…unsorted…*/ ];
const { bodyRows, getByRole } = renderList(configs);
// default by name
expect(getColumnData(bodyRows, 2)).toEqual(['alpha-svc', 'middle-dep', 'zebra-pod']);
// click Local Port
const lpHead = getByRole('columnheader', { name: 'Local Port' });
await fireEvent.click(lpHead);
expect(getColumnData(bodyRows, 4)).toEqual(['5000', '7000', '9000']);
await fireEvent.click(lpHead);
expect(getColumnData(bodyRows, 4)).toEqual(['9000', '7000', '5000']);
});This removes all the copy-pasted render, getAllByRole('rowgroup'), and manual mapping code, while keeping each test focused on its unique assertions.
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
What does this PR do?
Add sorting on columns for Kubernetes port forwarding:
I did not add sorting on status because I don't find the corresponding attribute in the comparison function.
Screenshot / video of UI
What issues does this PR fix or reference?
Fixes #11019
How to test this PR?