feat(frontend): add label selector and resource type filtering in resource map - #6821
feat(frontend): add label selector and resource type filtering in resource map#6821Rucha0901 wants to merge 8 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Rucha0901 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/assign |
illume
left a comment
There was a problem hiding this comment.
Thanks for this PR.
A few of the commits don't quite follow the project guidelines. We use Linux kernel style for git commits — have a look at the contributing guide and previous commits with git log.
Commits that need attention
Address review feedback on plugin keyboard shortcuts types— Missingarea: descriptionprefix — e.g.frontend: HomeButton: Fix so it navigates to homeorbackend: config: Add enable-dynamic-clusters flag.refactor(frontend): standardize theme breakpoints and expose hook to plugins— Missingarea: descriptionprefix — e.g.frontend: HomeButton: Fix so it navigates to homeorbackend: config: Add enable-dynamic-clusters flag.test(frontend): normalize storybook css hashes and update snapshots— Missingarea: descriptionprefix — e.g.frontend: HomeButton: Fix so it navigates to homeorbackend: config: Add enable-dynamic-clusters flag.feat(frontend): add label selector and resource type filtering in resource map— Missingarea: descriptionprefix — e.g.frontend: HomeButton: Fix so it navigates to homeorbackend: config: Add enable-dynamic-clusters flag.test(frontend): update storybook snapshots for GraphView filtering UI controls— Missingarea: descriptionprefix — e.g.frontend: HomeButton: Fix so it navigates to homeorbackend: config: Add enable-dynamic-clusters flag.fix(frontend): add optional chaining for metadata resourceVersion in streamingApi— Missingarea: descriptionprefix — e.g.frontend: HomeButton: Fix so it navigates to homeorbackend: config: Add enable-dynamic-clusters flag.i18n(frontend): update translation catalogs for resource map filtering controls— Missingarea: descriptionprefix — e.g.frontend: HomeButton: Fix so it navigates to homeorbackend: config: Add enable-dynamic-clusters flag.
Commit guidelines
- Use atomic commits focused on a single change.
- Use the title format
<area>: <Description of changes>— description must start with a capital letter. - Keep the title under 72 characters (soft requirement).
- Explain the intention and why the change is needed.
- Make commit titles meaningful and describe what changed.
- Do not add code that a later commit rewrites; squash or reorder commits instead.
- Do not include
Fixes #NNin commit messages.
Good examples:
frontend: HomeButton: Fix so it navigates to homebackend: config: Add enable-dynamic-clusters flag
illume
left a comment
There was a problem hiding this comment.
Thanks for this PR.
the PR has a merge-main commit; please rebase against main to keep the history clean.
Why this matters
Merge commits from main make the PR history harder to review. Please rebase your branch on top of the latest main instead, then update the PR with the rebased commits.
A few of the commits don't quite follow the project guidelines. We use Linux kernel style for git commits — have a look at the contributing guide and previous commits with git log.
Commits that need attention
Address review feedback on plugin keyboard shortcuts types— Missingarea: descriptionprefix — e.g.frontend: HomeButton: Fix so it navigates to homeorbackend: config: Add enable-dynamic-clusters flag.refactor(frontend): standardize theme breakpoints and expose hook to plugins— Missingarea: descriptionprefix — e.g.frontend: HomeButton: Fix so it navigates to homeorbackend: config: Add enable-dynamic-clusters flag.test(frontend): normalize storybook css hashes and update snapshots— Missingarea: descriptionprefix — e.g.frontend: HomeButton: Fix so it navigates to homeorbackend: config: Add enable-dynamic-clusters flag.feat(frontend): add label selector and resource type filtering in resource map— Missingarea: descriptionprefix — e.g.frontend: HomeButton: Fix so it navigates to homeorbackend: config: Add enable-dynamic-clusters flag.test(frontend): update storybook snapshots for GraphView filtering UI controls— Missingarea: descriptionprefix — e.g.frontend: HomeButton: Fix so it navigates to homeorbackend: config: Add enable-dynamic-clusters flag.fix(frontend): add optional chaining for metadata resourceVersion in streamingApi— Missingarea: descriptionprefix — e.g.frontend: HomeButton: Fix so it navigates to homeorbackend: config: Add enable-dynamic-clusters flag.i18n(frontend): update translation catalogs for resource map filtering controls— Missingarea: descriptionprefix — e.g.frontend: HomeButton: Fix so it navigates to homeorbackend: config: Add enable-dynamic-clusters flag.fix(frontend): resolve syntax error in plugin registry— Missingarea: descriptionprefix — e.g.frontend: HomeButton: Fix so it navigates to homeorbackend: config: Add enable-dynamic-clusters flag.chore(frontend): normalize emotion snapshots— Missingarea: descriptionprefix — e.g.frontend: HomeButton: Fix so it navigates to homeorbackend: config: Add enable-dynamic-clusters flag.
Commit guidelines
- Use atomic commits focused on a single change.
- Use the title format
<area>: <Description of changes>— description must start with a capital letter. - Keep the title under 72 characters (soft requirement).
- Explain the intention and why the change is needed.
- Make commit titles meaningful and describe what changed.
- Do not add code that a later commit rewrites; squash or reorder commits instead.
- Do not include
Fixes #NNin commit messages.
Good examples:
frontend: HomeButton: Fix so it navigates to homebackend: config: Add enable-dynamic-clusters flag
|
Important Review skippedToo many files! This PR contains 862 files, which is 562 over the limit of 300. To get a review, reduce the PR to 300 files or fewer by splitting it into smaller PRs or changing its base branch. Usage-priced reviews support at most 300 files. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (862)
You can disable this status message by setting the |
illume
left a comment
There was a problem hiding this comment.
Thanks for the contribution.
it looks like there's a merge-main commit in this PR — could you rebase onto main instead?
Why this matters
Merge commits from main make the PR history harder to review. Please rebase your branch on top of the latest main instead, then update the PR with the rebased commits.
Could you take a look at the commit messages in this PR? We follow a Linux kernel style for git commits — see the contributing guide and git log for examples.
Commits that need attention
Address review feedback on plugin keyboard shortcuts types— Missingarea: descriptionprefix — e.g.frontend: HomeButton: Fix so it navigates to homeorbackend: config: Add enable-dynamic-clusters flag.refactor(frontend): standardize theme breakpoints and expose hook to plugins— Missingarea: descriptionprefix — e.g.frontend: HomeButton: Fix so it navigates to homeorbackend: config: Add enable-dynamic-clusters flag.test(frontend): normalize storybook css hashes and update snapshots— Missingarea: descriptionprefix — e.g.frontend: HomeButton: Fix so it navigates to homeorbackend: config: Add enable-dynamic-clusters flag.feat(frontend): add label selector and resource type filtering in resource map— Missingarea: descriptionprefix — e.g.frontend: HomeButton: Fix so it navigates to homeorbackend: config: Add enable-dynamic-clusters flag.test(frontend): update storybook snapshots for GraphView filtering UI controls— Missingarea: descriptionprefix — e.g.frontend: HomeButton: Fix so it navigates to homeorbackend: config: Add enable-dynamic-clusters flag.fix(frontend): add optional chaining for metadata resourceVersion in streamingApi— Missingarea: descriptionprefix — e.g.frontend: HomeButton: Fix so it navigates to homeorbackend: config: Add enable-dynamic-clusters flag.i18n(frontend): update translation catalogs for resource map filtering controls— Missingarea: descriptionprefix — e.g.frontend: HomeButton: Fix so it navigates to homeorbackend: config: Add enable-dynamic-clusters flag.fix(frontend): resolve syntax error in plugin registry— Missingarea: descriptionprefix — e.g.frontend: HomeButton: Fix so it navigates to homeorbackend: config: Add enable-dynamic-clusters flag.chore(frontend): normalize emotion snapshots— Missingarea: descriptionprefix — e.g.frontend: HomeButton: Fix so it navigates to homeorbackend: config: Add enable-dynamic-clusters flag.ci: exclude plugin-only dependency '@emotion/jest' from being copied to frontend— Description must start with a capital letter — e.g.frontend: HomeButton: Fix the buttonnotfrontend: HomeButton: fix the button.
Commit guidelines
- Use atomic commits focused on a single change.
- Use the title format
<area>: <Description of changes>— description must start with a capital letter. - Keep the title under 72 characters (soft requirement).
- Explain the intention and why the change is needed.
- Make commit titles meaningful and describe what changed.
- Do not add code that a later commit rewrites; squash or reorder commits instead.
- Do not include
Fixes #NNin commit messages.
Good examples:
frontend: HomeButton: Fix so it navigates to homebackend: config: Add enable-dynamic-clusters flag
The GitHub CI test job has snapshot failures. Run cd frontend && npm run test -- -u to regenerate the snapshots.
How to update snapshots
Run cd frontend && npm run test -- -u to regenerate all snapshots. Review the diff to make sure the visual changes are intentional, then commit the updated snapshot files.
2129d11 to
a782509
Compare
|
Hi @illume , |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 44 out of 46 changed files in this pull request and generated no new comments.
Suppressed comments (5)
frontend/src/components/resourceMap/graph/graphFiltering.ts:63
- Use an own-property check here.
inalso matches inherited names such as the valid label keytoString, so an existence selector for that key accepts every normally deserialized labels object even when the label is absent.
return key in nodeLabels;
frontend/src/components/resourceMap/graph/graphFiltering.ts:62
- An empty string cannot represent both a bare-key existence selector and an equality selector with an empty value. Kubernetes permits empty label values, but input
env=is parsed as''and this branch then matches every object that merely hasenv, includingenv=prod. Preserve whether=was present so empty-value equality remains exact.
if (!value) {
frontend/src/components/resourceMap/GraphView.tsx:577
- Every keystroke changes the filter signature, forcing full graph filtering, regrouping, and a new asynchronous layout. On the supported 100,000-node graphs, typing one selector launches several expensive passes and can make the input unresponsive. Debounce applying the parsed selector or commit it on Enter/apply rather than filtering on each character.
onChange={e => setLabelInput(e.target.value)}
frontend/src/components/resourceMap/GraphView.tsx:216
- The new text-to-selector parser and controls have no behavioral tests; the added tests construct
GraphFilterobjects directly, so parsing comma/space-separated input and wiring selected kinds into filtering can regress while they still pass. Add GraphView interaction tests that enter selectors/select kinds and assert the resulting visible nodes, including invalid and empty-value input.
// Parsed label selector entries from search input
const parsedLabels = useMemo(() => {
frontend/src/plugin/registry.tsx:1251
- This blank-line edit is unrelated to Resource Map filtering. The PR also contains no-op DryRun snapshot rewrites and glossary-only
Service Namereordering; please revert this unrelated churn so the change remains focused and generated snapshots only record this feature's UI changes.
a806d96 to
a782509
Compare
illume
left a comment
There was a problem hiding this comment.
Thanks for these changes.
Can you please have a look at the git commits to see if they meet the contribution guidelines? We use a Linux kernel style of git commits. See the contributing guide for general context, and please see previous git commits with git log for examples.
Commits that need attention
fix(resourceMap): resolve label filtering issues and add interaction tests— Missingarea: descriptionprefix — e.g.frontend: HomeButton: Fix so it navigates to homeorbackend: config: Add enable-dynamic-clusters flag.
Commit guidelines
- Use atomic commits focused on a single change.
- Use the title format
<area>: <Description of changes>— description must start with a capital letter. - Keep the title under 72 characters (soft requirement).
- Explain the intention and why the change is needed.
- Make commit titles meaningful and describe what changed.
- Do not add code that a later commit rewrites; squash or reorder commits instead.
- Do not include
Fixes #NNin commit messages.
Good examples:
frontend: HomeButton: Fix so it navigates to homebackend: config: Add enable-dynamic-clusters flag
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 45 out of 47 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
frontend/src/components/resourceMap/GraphView.tsx:574
- The new interaction test exercises only the label field; it never selects an option from this multi-select. Add a GraphView interaction test that selects a kind and verifies
filterGraphreceives aresourceTypefilter with the selectedSet, so this UI wiring is covered.
onChange={(_, newValue) => setSelectedKinds(newValue)}
frontend/src/plugin/registry.tsx:1251
- This blank-line edit is unrelated to Resource Map filtering. The PR also contains no-op DryRun snapshot rewrites and
Service Nameglossary reordering; revert this unrelated generated churn so the diff remains focused on the feature and its GraphView snapshots.
frontend/src/components/resourceMap/GraphView.test.tsx:98 - The final
fix(resourceMap): ...commit adds this test while also correcting label semantics introduced by the first feature commit, leaving one logical change split across a corrective pass. Please squash/reorder these changes into the feature commit and rename the commit to the repository's<area>: <description>format (for example,frontend: resourceMap: Add resource and label filtering).
it('filters nodes based on label input', async () => {
illume
left a comment
There was a problem hiding this comment.
Thanks for these changes.
Can you please address the open review comments? Once you've resolved each one, please mark it as resolved.
illume
left a comment
There was a problem hiding this comment.
Thanks for the contribution.
Could you take a look at the commit messages in this PR? We follow a Linux kernel style for git commits — see the contributing guide and git log for examples.
Commits that need attention
fix(resourceMap): resolve label filtering issues and add interaction tests— Missingarea: descriptionprefix — e.g.frontend: HomeButton: Fix so it navigates to homeorbackend: config: Add enable-dynamic-clusters flag.test(graphFiltering): use null instead of empty string for label existence check— Missingarea: descriptionprefix — e.g.frontend: HomeButton: Fix so it navigates to homeorbackend: config: Add enable-dynamic-clusters flag.
Commit guidelines
- Use atomic commits focused on a single change.
- Use the title format
<area>: <Description of changes>— description must start with a capital letter. - Keep the title under 72 characters (soft requirement).
- Explain the intention and why the change is needed.
- Make commit titles meaningful and describe what changed.
- Do not add code that a later commit rewrites; squash or reorder commits instead.
- Do not include
Fixes #NNin commit messages.
Good examples:
frontend: HomeButton: Fix so it navigates to homebackend: config: Add enable-dynamic-clusters flag
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 45 out of 47 changed files in this pull request and generated no new comments.
Suppressed comments (7)
frontend/src/i18n/locales/zh/translation.json:1
- Adding empty-string translations for new UI keys can cause those labels/placeholders to render blank (and potentially block fallback behavior depending on i18n configuration). Prefer omitting untranslated keys so fallback can apply, or temporarily set them to the English value until proper translations are provided.
frontend/src/plugin/registry.tsx:1 - The added blank line directly after
/**is unusual for JSDoc/TSDoc formatting and adds noise without adding documentation value. Consider removing the extra empty line to keep comment style consistent.
frontend/src/components/resourceMap/graph/graphFiltering.test.ts:1587 - These tests construct
Service/Deploymentnodes using thePodclass with an overriddenkind. This is brittle because a future change to thePodconstructor (or type definitions) could normalize/overridekindback toPod, silently breaking the intent of the test. Prefer using a minimalkubeObjectshape ({ kind, metadata }) or the correct resource classes for Service/Deployment.
describe('resourceType filter', () => {
const podNode: GraphNode = {
id: 'pod1',
kubeObject: new Pod({
kind: 'Pod',
metadata: { namespace: 'ns1', name: 'pod1' },
} as any),
};
const svcNode: GraphNode = {
id: 'svc1',
kubeObject: new Pod({
kind: 'Service',
metadata: { namespace: 'ns1', name: 'svc1' },
} as any),
};
frontend/src/components/resourceMap/GraphView.tsx:221
- When
nodeschange,availableKindscan drop previously-selected kinds, butselectedKindsis not reconciled. This can leave stale selections that still apply filtering (potentially hiding all nodes) while no longer being present in the dropdown options. Consider pruningselectedKindsin an effect whenavailableKindschanges (e.g., keep only selections still present inavailableKinds).
// Available resource kinds from current graph nodes
const availableKinds = useMemo(() => {
const kinds = new Set<string>();
nodes.forEach(node => {
if (node.kubeObject?.kind) {
kinds.add(node.kubeObject.kind);
}
});
return Array.from(kinds).sort();
}, [nodes]);
frontend/src/components/resourceMap/graph/graphFiltering.ts:67
- This path allocates new arrays for
Object.keys(...)andObject.entries(...)for every node evaluation. On large graphs (which this component explicitly targets), that can add measurable overhead. Consider computing the entries once per filter evaluation (e.g., cacheObject.entries(filter.labels)or store a precomputed entries array alongside the filter when buildingfilters).
if (filter.type === 'labelSelector' && Object.keys(filter.labels).length > 0) {
const nodeLabels = node.kubeObject?.metadata?.labels;
if (!nodeLabels) return false;
return Object.entries(filter.labels).every(([key, value]) => {
if (value === null) {
return Object.prototype.hasOwnProperty.call(nodeLabels, key);
}
return nodeLabels[key] === value;
});
}
frontend/src/components/resourceMap/GraphView.test.tsx:127
- This test mixes fake timers with
waitFor, which internally relies on timers for polling; that can lead to flaky/hanging tests depending on the test runner configuration. Prefer asserting synchronously after flushing the debounce (or explicitly advancing timers forwaitFor’s polling), and ensurevi.useRealTimers()runs in afinally/afterEachso timers are always restored even if an assertion fails.
vi.useFakeTimers();
render(
<TestContext>
<GraphView />
</TestContext>
);
const input = await screen.findByPlaceholderText(/Labels/);
// Type an empty value equality selector
fireEvent.change(input, { target: { value: 'env=' } });
act(() => {
vi.advanceTimersByTime(600);
});
await waitFor(() => {
expect(mocks.filterGraphSpy).toHaveBeenCalledWith(
expect.any(Array),
expect.any(Array),
expect.arrayContaining([
expect.objectContaining({
type: 'labelSelector',
labels: { env: '' },
}),
])
);
});
frontend/src/components/resourceMap/GraphView.test.tsx:148
- This test mixes fake timers with
waitFor, which internally relies on timers for polling; that can lead to flaky/hanging tests depending on the test runner configuration. Prefer asserting synchronously after flushing the debounce (or explicitly advancing timers forwaitFor’s polling), and ensurevi.useRealTimers()runs in afinally/afterEachso timers are always restored even if an assertion fails.
vi.useRealTimers();
…p-label-kind-filtering
illume
left a comment
There was a problem hiding this comment.
Thanks for this PR.
the PR has a merge-main commit; please rebase against main to keep the history clean.
Why this matters
Merge commits from main make the PR history harder to review. Please rebase your branch on top of the latest main instead, then update the PR with the rebased commits.
A few of the commits don't quite follow the project guidelines. We use Linux kernel style for git commits — have a look at the contributing guide and previous commits with git log.
Commits that need attention
fix(resourceMap): resolve label filtering issues and add interaction tests— Missingarea: descriptionprefix — e.g.frontend: HomeButton: Fix so it navigates to homeorbackend: config: Add enable-dynamic-clusters flag.test(graphFiltering): use null instead of empty string for label existence check— Missingarea: descriptionprefix — e.g.frontend: HomeButton: Fix so it navigates to homeorbackend: config: Add enable-dynamic-clusters flag.chore: fix missing i18n strings in pt-br translation— Missingarea: descriptionprefix — e.g.frontend: HomeButton: Fix so it navigates to homeorbackend: config: Add enable-dynamic-clusters flag.
Commit guidelines
- Use atomic commits focused on a single change.
- Use the title format
<area>: <Description of changes>— description must start with a capital letter. - Keep the title under 72 characters (soft requirement).
- Explain the intention and why the change is needed.
- Make commit titles meaningful and describe what changed.
- Do not add code that a later commit rewrites; squash or reorder commits instead.
- Do not include
Fixes #NNin commit messages.
Good examples:
frontend: HomeButton: Fix so it navigates to homebackend: config: Add enable-dynamic-clusters flag
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 31 changed files in this pull request and generated no new comments.
Suppressed comments (6)
frontend/src/components/resourceMap/GraphView.tsx:226
- Duplicate keys are silently overwritten here. For example,
env=prod,env=devbecomes only{ env: 'dev' }and matches dev nodes, even though label requirements are combined with AND and this selector should match nothing (or be rejected as invalid). Please reject duplicate keys with validation feedback or preserve requirements as an array so none are discarded.
const k = part.substring(0, eqIdx).trim();
const v = part.substring(eqIdx + 1).trim();
if (k) {
labels[k] = v;
frontend/src/components/resourceMap/GraphView.tsx:268
- This filter still does not meet issue #6820's example of hiding Pods when only Service/Deployment are selected.
filterGraphexpands each direct match through every incoming and outgoing edge (graphFiltering.ts:112-155), so connected non-selected kinds—and potentially the whole connected component—remain visible. The PR test plan says to retain related nodes, which conflicts with the linked issue; please resolve that contract and either make resource-type filtering strict or explicitly revise the acceptance criteria.
if (selectedKinds.length > 0) {
filters.push({ type: 'resourceType', kinds: new Set(selectedKinds) });
frontend/src/components/resourceMap/GraphView.tsx:573
- The new resource-type control has no interaction coverage:
GraphView.test.tsxexercises only the label field, while the lower-level predicate tests cannot catch broken Autocomplete wiring. Add a test that selects one or more kinds and verifiesfilterGraphreceives the expectedresourceTypefilter andSet.
onChange={(_, newValue) => setSelectedKinds(newValue)}
frontend/src/components/resourceMap/GraphView.test.tsx:98
- The PR range still contains merge commit
805ef45cand later corrective commits (86549621,cde5c9a9) that rewrite the feature's label semantics/tests. Repository policy requires a linear, coherent history: rebase onto the base branch, squash/reorder those corrections into the feature commit, and rename the remainingfix(...),test(...), andchore:titles to the documented<area>: <description>format.
it('filters nodes based on label input', async () => {
frontend/src/plugin/registry.tsx:1298
- This added blank JSDoc line is unrelated to Resource Map filtering and leaves an empty line immediately after
/**. Revert it to keep this feature diff focused.
frontend/src/components/resourceMap/GraphView.tsx:568 - This is a visible toolbar change, but the PR marks screenshots as N/A. Repository frontend guidance requires screenshots for UI changes; please add a Resource Map screenshot showing both controls (and ideally an active filter state) so reviewers can verify layout and visual impact.
<Autocomplete
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Summary
This PR adds support for filtering Kubernetes resource types and label selectors in the Resource Map component.
Related Issue
Fixes #6820
Changes
GraphFilterunion type ingraphFiltering.tswithresourceTypeandlabelSelectorvariants.matchesAllFilterslogic ingraphFiltering.tsto filter nodes matching selected kinds and key-value label selectors.GraphView.tsxfor multi-select resource type dropdown and label search text field.computeFilterSigandbuildFiltersto serialize and merge resource type and label filters properly.graphFiltering.test.tsto test resource kind and label selector filter matching.Steps to Test
app=nginx,env=prod) in the Labels search field.Screenshots (if applicable)
N/A
Notes for the Reviewer
vitest graphFiltering.test.ts).npm run ci-lint).