frontend: globalSearch: Improve useLocalStorageState and fix sync bugs - #5936
Conversation
k8s-ci-robot
added
the
size/S
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: vishnukothakapu 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 |
k8s-ci-robot
added
the
cncf-cla: yes
85c6b95 to
429b362
Compare
illume
left a comment
illume
left a comment
There was a problem hiding this comment.
Thanks for this PR.
There are snapshot mismatches in CI. You can fix them locally by running cd frontend && npm run test -- -u.
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.
There was a problem hiding this comment.
Pull request overview
This PR addresses a stale-closure bug in useLocalStorageState that could cause cross-component updates triggered via useLocalStorageState.update() to apply using an outdated setter/state snapshot, leading to missed or incorrect synchronization across mounted components.
Changes:
- Makes the hook’s
setfunction stable across renders viauseCallback([key]). - Switches to the functional
setState(oldState => ...)form to avoid closing over a stalestatevalue. - Fixes the
useEffectdependency list by depending onset(removing the prior exhaustive-deps suppression).
Notes (review constraints):
- CI/check status and PR commit history (merge commits / commit-message coherence) were not available in the provided context.
illume
left a comment
illume
left a comment
There was a problem hiding this comment.
Thanks for the contribution.
There are some open Copilot review comments — could you take a look at them? Please mark each one as resolved once you've addressed it.
429b362 to
bc24145
Compare
k8s-ci-robot
added
needs-rebase
bc24145 to
866d3c8
Compare
k8s-ci-robot
added
size/S
5d5713c to
07887c4
Compare
07887c4 to
ec81afd
Compare
k8s-ci-robot
added
size/M
illume
left a comment
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.
illume
left a comment
illume
left a comment
There was a problem hiding this comment.
Thanks for working on this.
Would you mind addressing the open Copilot review comments? Please mark each comment as resolved after addressing it.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
frontend/src/components/globalSearch/useLocalStorageState.test.tsx:52
- This test exercises the JSON-parse failure path, which logs via
console.warn, but it doesn't stubconsole.warn. Stubbing it avoids noisy test output and aligns with other tests that treat expected warnings as part of the assertion surface.
it('returns the default value when the stored JSON is malformed', () => {
localStorage.setItem(TEST_KEY, 'not-valid-json{{{');
const { result } = renderHook(() => useLocalStorageState(TEST_KEY, 0));
expect(result.current[0]).toBe(0);
});
frontend/src/components/globalSearch/useLocalStorageState.test.tsx:46
- This test intentionally triggers the hook's
console.warnpath, but it doesn't stubconsole.warn. This can make unit test output noisy and inconsistent with other tests in the repo that silence/inspect expected warnings. Consider stubbingconsole.warnwithin the test (and restoring it) when exercising expected error paths.
This issue also appears on line 48 of the same file.
it('returns the default value when localStorage.getItem throws', () => {
const spy = vi.spyOn(localStorage, 'getItem').mockImplementation(() => {
throw new Error('Storage disabled');
});
try {
frontend/src/components/globalSearch/useLocalStorageState.test.tsx:123
- This test intentionally triggers
useLocalStorageState.update'sconsole.errorpath, but it doesn't stubconsole.error, which can produce noisy CI output. Consider stubbingconsole.errorand optionally asserting it was called.
it('does not notify listeners if useLocalStorageState.update fails to persist', () => {
const spy = vi.spyOn(localStorage, 'setItem').mockImplementation(() => {
throw new Error('Storage write disabled');
});
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
frontend/src/components/globalSearch/useLocalStorageState.tsx:92
- Same issue here:
setState(newValue)will invokenewValueif it’s a function (React treats it as an updater). UsesetState(() => newValue)so cross-component updates always set the value literally.
setState(newValue);
frontend/src/components/globalSearch/useLocalStorageState.tsx:80
setState(newValue)will treat a functionnewValueas a state-updater callback, which can cause unintended behavior ifT(or a mistakenupdate()call) ever passes a function value. Use the functional form to always set the value literally.
This issue also appears on line 92 of the same file.
setState(newValue);
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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:
📝 WalkthroughWalkthroughThe pull request adds pod and workload diagnostics, CRD validation, plugin resource relations, StatefulSet creation, local-storage synchronization, and supporting UI, documentation, testing, and localization updates. ChangesDiagnostics and resource UI
CRD resilience and resource relations
Supporting application and documentation updates
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ResourceDetails
participant useObjectEvents
participant OwnedPodsSection
participant Diagnostics
ResourceDetails->>useObjectEvents: fetch object events
ResourceDetails->>OwnedPodsSection: request owned pods
useObjectEvents-->>Diagnostics: provide events
OwnedPodsSection-->>Diagnostics: provide pods and errors
Diagnostics-->>ResourceDetails: render findings and log actions
sequenceDiagram
participant Plugin
participant registerResourceRelationProvider
participant ReduxGraphState
participant GraphSources
Plugin->>registerResourceRelationProvider: submit relation
registerResourceRelationProvider->>ReduxGraphState: store validated relation
GraphSources->>ReduxGraphState: read relations
GraphSources-->>GraphSources: create labeled relation edges
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
frontend/src/components/globalSearch/useLocalStorageState.tsx:127
Object.assign(useLocalStorageStateBase, { update() { ... } })can cause TypeScript to lose or degrade the generic call signature for consumers (depending on TS/version/config). To keep the public API strongly typed, add an explicit type annotation for the exported value (e.g., an intersection oftypeof useLocalStorageStateBasewith an{ update(...) }shape) when assigning the Object.assign result.
export const useLocalStorageState = Object.assign(useLocalStorageStateBase, {
/**
* Update the value in local storage and notify all `useLocalStorageState` hooks.
*
* @param key - local storage key
* @param value - local storage value
*/
update(key: string, value: any): void {
try {
localStorage.setItem(key, JSON.stringify(value));
} catch (e) {
console.error(`Failed to save "${key}" to localStorage`, e);
return;
}
updateListeners[key]?.forEach(fn => fn(value));
},
});
frontend/src/components/globalSearch/useLocalStorageState.test.tsx:34
- The suite no longer clears
localStoragein abeforeEach, so tests can become order-dependent and leak state between cases within this file (e.g., a test writingTEST_KEYcan affect later tests expecting no entry). Add abeforeEach(() => localStorage.clear())(or at least clearTEST_KEY) in thisdescribeblock to ensure isolation and stability.
describe('useLocalStorageState', () => {
it('returns the default value when localStorage has no entry', () => {
const { result } = renderHook(() => useLocalStorageState(TEST_KEY, 0));
expect(result.current[0]).toBe(0);
});
it('returns the persisted value when localStorage already has an entry', () => {
localStorage.setItem(TEST_KEY, JSON.stringify(42));
const { result } = renderHook(() => useLocalStorageState(TEST_KEY, 0));
expect(result.current[0]).toBe(42);
});
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/src/components/crd/List.tsx (1)
53-65: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPartial
specguarding in the CRD views. Both files now treatspecas possibly undefined forspec.names, but they still dereference otherspecfields directly. A CRD delivered withoutspectherefore still breaks these screens, so the stated fix is incomplete.
frontend/src/components/crd/List.tsx#L53-L65: apply optional handling to theGroupcolumn at Line 87 and theScopecolumn at Line 92.frontend/src/components/crd/Details.tsx#L69-L87: apply optional handling toitem.spec.groupat Line 44,item.spec.versionat Line 48,item.spec.scopeat Line 52,item.spec.subresourcesat Lines 56-57, and passitem.spec?.versions ?? []at Line 116.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/crd/List.tsx` around lines 53 - 65, Complete the partial spec guarding in frontend/src/components/crd/List.tsx lines 53-65 by applying optional handling to the Group and Scope column accessors. In frontend/src/components/crd/Details.tsx lines 69-87, guard item.spec.group, item.spec.version, item.spec.scope, and item.spec.subresources, and pass an empty array when item.spec.versions is unavailable. Preserve existing rendering behavior when spec is present.frontend/src/components/resourceMap/sources/definitions/relations.tsx (1)
90-114: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInclude the relation ID in indexed owner-edge IDs.
The generic relation path appends
relation.idto each edge ID. Both indexed owner-relation builders still emit onlyfrom.id + '-' + to.id. A matching edge from another relation can then be removed during edge deduplication.
frontend/src/components/resourceMap/sources/definitions/relations.tsx#L90-L114: derive the owner relation ID once and append it whenbuildEdgesWithIndexcreates an edge.frontend/src/components/resourceMap/sources/definitions/relations.tsx#L118-L156: derive the reversed-owner relation ID once and append it whenbuildEdgesWithIndexcreates an edge.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/resourceMap/sources/definitions/relations.tsx` around lines 90 - 114, Update makeOwnerRelation in frontend/src/components/resourceMap/sources/definitions/relations.tsx lines 90-114 to derive the owner relation ID once and append it to indexed edge IDs created by buildEdgesWithIndex. Apply the same change to the reversed-owner relation builder at lines 118-156, ensuring both builders include their relation ID alongside from.id and to.id.
🧹 Nitpick comments (11)
frontend/src/components/statefulset/CreateStatefulSetForm.tsx (1)
55-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
_.cloneDeepfor draft cloning.The shared form and
useSelectorPodTemplateclone drafts with_.cloneDeep.structuredClonethrowsDataCloneErrorfor non-cloneable values, such as functions or class instances. Use_.cloneDeephere for consistency and for safety with unexpected draft content.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/statefulset/CreateStatefulSetForm.tsx` at line 55, Replace structuredClone in the draft update flow with the shared _.cloneDeep utility, keeping the existing resource cloning behavior and update logic unchanged.frontend/src/components/statefulset/CreateStatefulSetForm.stories.tsx (1)
117-139: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a story that exercises the OnDelete cleanup effect.
OnDeleteStrategy.argsprovidesupdateStrategy.type: 'OnDelete'with norollingUpdate, so the cleanup effect in CreateStatefulSetForm.tsx Lines 53-59 never runs. Add a story withupdateStrategy: { type: 'OnDelete', rollingUpdate: { partition: 3 } }to cover the removal path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/statefulset/CreateStatefulSetForm.stories.tsx` around lines 117 - 139, Update the StatefulSet story using the OnDelete strategy to include a rollingUpdate object with partition set to 3, so CreateStatefulSetForm’s cleanup effect executes the partition-removal path. Preserve the existing OnDelete resource configuration and story purpose.frontend/src/components/statefulset/__snapshots__/CreateStatefulSetForm.Empty.stories.storyshot (1)
341-404: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueThe
Partitionfield appears without a selected update strategy.In the
Emptystory,spec.updateStrategy.typeis undefined. The condition at CreateStatefulSetForm.tsx Line 83 tests only!== 'OnDelete', so the form rendersPartitionfor an unset strategy. Consider renderingPartitiononly when the type equalsRollingUpdate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/statefulset/__snapshots__/CreateStatefulSetForm.Empty.stories.storyshot` around lines 341 - 404, Update the conditional rendering around the Partition field in CreateStatefulSetForm so it renders only when spec.updateStrategy.type equals "RollingUpdate"; keep it hidden for undefined and "OnDelete" strategies.frontend/src/components/crd/CustomResourceInstancesList.test.tsx (1)
189-213: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the all-unusable empty state.
This test covers the mixed case (one unusable CRD, one usable CRD). The new branch in
CustomResourceInstancesList.tsxat Lines 204-208 rendersNo CustomResourceDefinitions with usable specs were found.when every CRD is unusable. That branch has no test. Add a case with only unusable CRDs and assert the message instead of a loader.💚 Proposed additional test
+ it('shows the empty state when every CRD has an unusable spec (`#4824`)', () => { + const incompleteCrd = { + cluster: 'test-cluster', + metadata: { name: 'incomplete.example.com', namespace: 'default' }, + jsonData: { status: { acceptedNames: { categories: [] } } }, + makeCRClassOrNull: () => null, + } as unknown as ReturnType<typeof makeMockCrd>; + + setOuterCrdsList([incompleteCrd]); + + renderList(); + + expect( + screen.getByText('No CustomResourceDefinitions with usable specs were found.') + ).toBeInTheDocument(); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/crd/CustomResourceInstancesList.test.tsx` around lines 189 - 213, Add a test alongside the existing makeCRClassOrNull null case in CustomResourceInstancesList.test.tsx using only CRDs whose makeCRClassOrNull returns null. Render the list and assert that “No CustomResourceDefinitions with usable specs were found.” is displayed, while the loading indicator is absent.frontend/src/components/crd/CustomResourceInstancesList.tsx (1)
38-48: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConfirm the remount key protects the per-entry hook calls.
CrInstancesViewcallscrdClass.useList(...)once per entry at Line 43. The hook count therefore depends onclassified.length. The design relies onremountKeychanging whenever membership changes, so React never renders a different hook count in the same component instance.One case needs confirmation: two different CRDs that produce the same
crdSortKey.sortKeyreturns${cluster}/${uid || name}. Ifmetadata.uidis absent for one CRD and another CRD'snameequals that uid value in the same cluster, the keys collide and the fingerprint stays stable while the count changes. This is unlikely in practice. If you want the guarantee to be structural instead of probabilistic, derivekeyfromclassified.lengthas well.♻️ Optional hardening
- const remountKey = useMemo(() => fingerprint(classified.map(it => it.crdSortKey)), [classified]); + const remountKey = useMemo( + () => `${classified.length}|${fingerprint(classified.map(it => it.crdSortKey))}`, + [classified] + );Also applies to: 169-184
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/crd/CustomResourceInstancesList.tsx` around lines 38 - 48, Harden the remount key used by CrInstancesView so it changes whenever the number of classified entries changes, preventing a different count of per-entry crdClass.useList hooks from rendering in the same component instance. Include classified.length in the key/fingerprint derivation alongside the existing CRD identity values, preserving the current remount behavior for membership changes.frontend/src/components/crd/CustomResourceDetails.test.tsx (1)
73-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the real
Loadercomponent in this behavior test.
Loaderis a local implementation. The test can assert that the real loading title is absent. The mock makes the assertion depend on the mock output instead of the rendered application behavior.Proposed change
-vi.mock('../common/Loader', () => ({ - default: ({ title }: { title: string }) => <div data-testid="mock-loader">{title}</div>, -}));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/crd/CustomResourceDetails.test.tsx` around lines 73 - 75, Remove the vi.mock for Loader from CustomResourceDetails tests and render the real Loader component so the behavior assertion verifies the actual loading title is absent from the application output.Source: Coding guidelines
frontend/src/components/diagnostics/Diagnostics.tsx (1)
407-418: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the misplaced JSDoc blocks to the functions they describe.
Two doc comments sit above the wrong function:
- Lines 407-411 describe
getPodDiagnosticsbut precedediagnosisHint.- Lines 811-814 describe
PodDiagnosticsSectionbut precedegetFailingContainerName.Move each block directly above its function. Both affected functions are exported, so editors show the wrong description.
Also applies to: 811-820
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/diagnostics/Diagnostics.tsx` around lines 407 - 418, Move the JSDoc describing aggregated pod findings so it directly precedes the exported getPodDiagnostics function, and move the JSDoc describing the diagnostics section so it directly precedes the exported PodDiagnosticsSection function. Remove both misplaced comment blocks from above diagnosisHint and getFailingContainerName without changing their text or function behavior.frontend/src/components/diagnostics/Diagnostics.stories.tsx (1)
46-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd pod stories for the loading and error states.
WorkloadDiagnosticsSectionhasWorkloadLoadingandWorkloadPodsErrorstories.PodDiagnosticsSectionhas only healthy and failing stories. Add a story that exercises the log action (onViewLogswith a failing non-default container) and one that covers an empty/unavailable events result.As per coding guidelines: "Add Storybook stories with error and loading states for new React components."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/diagnostics/Diagnostics.stories.tsx` around lines 46 - 60, Extend the PodTemplate stories with loading and unavailable-events cases for PodDiagnosticsSection, matching the existing WorkloadDiagnosticsSection state coverage. Add a story that provides a failing pod with a non-default container and exercises the onViewLogs action, plus a story representing an empty or unavailable events result; reuse existing pod/event fixtures and story conventions.Source: Coding guidelines
frontend/src/components/workload/Details.test.tsx (1)
32-48: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert event propagation in this wiring test.
The mocked
DetailsGridsuppliescontext.events, but the assertions only checkpodsanderrors. A regression that dropscontext.eventsfromWorkloadDiagnosticsSectionwould pass. Add a non-empty event fixture and assert that it reacheslastDiagnosticsProps().events.As per coding guidelines, test files should prefer real implementations and integration tests, and mock dependencies only when necessary.
Also applies to: 162-180
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/workload/Details.test.tsx` around lines 32 - 48, Update the DetailsGrid wiring test and its fixtures to include a non-empty events collection in the mocked context, then assert that lastDiagnosticsProps().events matches that fixture. Preserve the existing pods and errors assertions, and keep the mock limited to the necessary DetailsGrid boundary.Source: Coding guidelines
frontend/src/components/pod/Details.test.tsx (1)
91-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the diagnostics integration observable in tests.
This mock replaces
PodDiagnosticsSectionwith a no-op. ThePodDetailstests cannot detect a brokeneventsoronViewLogsprop, or verify container-specific log launching. Capture these props and assert them, or add a focused test that renders the real component. Keep the mock only if real event fetching makes it necessary.As per coding guidelines, test files should prefer real implementations and integration tests, and mock dependencies only when necessary.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/pod/Details.test.tsx` around lines 91 - 94, Remove the no-op PodDiagnosticsSection mock in the PodDetails tests unless real event fetching requires it, preferring the real diagnostics integration. If mocking remains necessary, capture the events and onViewLogs props and add assertions or a focused test covering their propagation and container-specific log launching.Source: Coding guidelines
frontend/src/components/common/index.test.ts (1)
73-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winScope
internalExportsto the source module.
internalExports.includes(key)ignoresuseObjectEventswhereverkeyis evaluated. If the export check covers multiple source modules, a future same-named export can bypass validation. Scope the exception toObjectEventList, or use a per-module allowlist.Also applies to: 109-112
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/common/index.test.ts` around lines 73 - 74, Scope the useObjectEvents exception in internalExports to the ObjectEventList source module instead of applying it to every key evaluated by the export check. Update the validation logic around internalExports so same-named exports from other modules remain subject to normal validation, using a per-module allowlist if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/scripts/generate-release-issue-body.js:
- Line 72: Update the release issue body generated by the relevant template in
generate-release-issue-body.js to remove the literal ellipsis from the static
server command’s code span, using ./frontend/build as the path while preserving
any continuation punctuation outside the command.
In `@docs/development/plugins/functionality/index.md`:
- Around line 245-265: Update both relation predicates in the plugin
documentation to scope matches by namespace as well as metadata.name, ensuring
Deployment, Secret, and custom-source resources from different namespaces cannot
be related accidentally. Keep the existing name-matching behavior and add
corresponding metadata.namespace comparisons for each endpoint.
In `@docs/development/telemetry.md`:
- Around line 131-161: Update the Prometheus scrape target in
kubernetes-headlamp-monitoring.yaml to use the Headlamp Service port 80, such as
headlamp.kube-system.svc.cluster.local:80 or headlamp:80, and remove the stale
:4466 target. Keep the existing /metrics scrape path and metrics configuration
unchanged.
In `@frontend/src/components/crd/crInstancesKey.ts`:
- Around line 71-83: Update frontend/src/components/crd/crInstancesKey.ts lines
71-83 in the fingerprint/sortKey serialization path to frame every key entry
with its length in both the joined and hashed representations, removing reliance
on an unescaped separator for entry boundaries. Update lines 101-103 to
serialize cluster and id as an unambiguous tuple, such as
JSON.stringify([cluster, id]). Add regression tests covering embedded separators
and delimiter-containing cluster or identifier values.
In `@frontend/src/components/crd/CustomResourceInstancesList.tsx`:
- Around line 200-210: Add translation entries for every new translation|...
string used by CustomResourceInstancesList, CustomResourceList, and
CustomResourceDetails to the locale files under the existing locale structure.
Reuse the exact keys and provide localized labels consistent with each locale’s
conventions so the UI does not display raw key strings.
In `@frontend/src/components/crd/Details.tsx`:
- Line 69: Guard every remaining direct spec access in the Details render path,
including group, version, scope, subresources, and versions, by consistently
handling an absent item.spec. Ensure the SimpleTable at the versions usage
receives a safe fallback collection when versions is undefined, while preserving
the existing fallback behavior for kind and name.
In `@frontend/src/components/diagnostics/Diagnostics.test.ts`:
- Around line 150-152: Update the duplicate-hint assertion in the Diagnostics
test to check for the emitted id `pod-scheduling-event` rather than
`pod-scheduling-condition`, so it verifies that getPendingHints does not produce
the duplicate scheduling hint.
In `@frontend/src/components/pod/Details.tsx`:
- Around line 894-905: Update the diagnostics integration around
PodDiagnosticsSection so its failing-container log action uses the same
AuthVisible get permission gate for the pod log subresource as the header log
action. Pass an authorization-aware onViewLogs callback or apply the gate inside
PodDiagnosticsSection, ensuring users without permission cannot see or open
PodLogViewer while authorized behavior remains unchanged.
In `@frontend/src/components/resourceMap/sources/definitions/relations.test.tsx`:
- Around line 243-249: Add a test fixture in the relations test setup where the
CRD’s makeCRClassOrNull returns null, then invoke useGetAllRelations and assert
it does not throw and produces no CRD owner relation. Keep the existing
complete-CRD coverage unchanged and target the resilience path through
useGetCRToOwnerRelations.
In `@frontend/src/components/resourceMap/sources/definitions/relations.tsx`:
- Around line 404-420: The plugin relation handling must contain indexed
edge-builder failures: in
frontend/src/components/resourceMap/sources/definitions/relations.tsx lines
404-420, wrap plugin buildEdgesWithIndex invocation in try/catch and return an
empty edge list on errors; in frontend/src/plugin/registry.tsx lines 1271-1325,
validate that a defined buildEdgesWithIndex value is a function and reject
registrations that provide any other type.
In `@frontend/src/components/statefulset/CreateStatefulSetForm.stories.tsx`:
- Around line 84-115: The Filled story in
frontend/src/components/statefulset/CreateStatefulSetForm.stories.tsx:84-115
must include a valid serviceName under Filled.args.spec so the required form
field is populated and the story remains valid. Regenerate
frontend/src/components/statefulset/__snapshots__/CreateStatefulSetForm.Filled.stories.storyshot:288-336
so the Service Name input reflects the new value.
In `@frontend/src/i18n/locales/hi/translation.json`:
- Line 568: Update the CRD empty-state translation consistently: change the
CustomResourceList translation call to the new key and add localized entries for
it in frontend/src/i18n/locales/hi/translation.json:568,
it/translation.json:573, ja/translation.json:563, ko/translation.json:563,
pt/translation.json:573, ru/translation.json:578, ta/translation.json:568,
ur/translation.json:568, and zh-tw/translation.json:563, then remove the old key
only after every consumer and locale entry has been migrated.
---
Outside diff comments:
In `@frontend/src/components/crd/List.tsx`:
- Around line 53-65: Complete the partial spec guarding in
frontend/src/components/crd/List.tsx lines 53-65 by applying optional handling
to the Group and Scope column accessors. In
frontend/src/components/crd/Details.tsx lines 69-87, guard item.spec.group,
item.spec.version, item.spec.scope, and item.spec.subresources, and pass an
empty array when item.spec.versions is unavailable. Preserve existing rendering
behavior when spec is present.
In `@frontend/src/components/resourceMap/sources/definitions/relations.tsx`:
- Around line 90-114: Update makeOwnerRelation in
frontend/src/components/resourceMap/sources/definitions/relations.tsx lines
90-114 to derive the owner relation ID once and append it to indexed edge IDs
created by buildEdgesWithIndex. Apply the same change to the reversed-owner
relation builder at lines 118-156, ensuring both builders include their relation
ID alongside from.id and to.id.
---
Nitpick comments:
In `@frontend/src/components/common/index.test.ts`:
- Around line 73-74: Scope the useObjectEvents exception in internalExports to
the ObjectEventList source module instead of applying it to every key evaluated
by the export check. Update the validation logic around internalExports so
same-named exports from other modules remain subject to normal validation, using
a per-module allowlist if needed.
In `@frontend/src/components/crd/CustomResourceDetails.test.tsx`:
- Around line 73-75: Remove the vi.mock for Loader from CustomResourceDetails
tests and render the real Loader component so the behavior assertion verifies
the actual loading title is absent from the application output.
In `@frontend/src/components/crd/CustomResourceInstancesList.test.tsx`:
- Around line 189-213: Add a test alongside the existing makeCRClassOrNull null
case in CustomResourceInstancesList.test.tsx using only CRDs whose
makeCRClassOrNull returns null. Render the list and assert that “No
CustomResourceDefinitions with usable specs were found.” is displayed, while the
loading indicator is absent.
In `@frontend/src/components/crd/CustomResourceInstancesList.tsx`:
- Around line 38-48: Harden the remount key used by CrInstancesView so it
changes whenever the number of classified entries changes, preventing a
different count of per-entry crdClass.useList hooks from rendering in the same
component instance. Include classified.length in the key/fingerprint derivation
alongside the existing CRD identity values, preserving the current remount
behavior for membership changes.
In `@frontend/src/components/diagnostics/Diagnostics.stories.tsx`:
- Around line 46-60: Extend the PodTemplate stories with loading and
unavailable-events cases for PodDiagnosticsSection, matching the existing
WorkloadDiagnosticsSection state coverage. Add a story that provides a failing
pod with a non-default container and exercises the onViewLogs action, plus a
story representing an empty or unavailable events result; reuse existing
pod/event fixtures and story conventions.
In `@frontend/src/components/diagnostics/Diagnostics.tsx`:
- Around line 407-418: Move the JSDoc describing aggregated pod findings so it
directly precedes the exported getPodDiagnostics function, and move the JSDoc
describing the diagnostics section so it directly precedes the exported
PodDiagnosticsSection function. Remove both misplaced comment blocks from above
diagnosisHint and getFailingContainerName without changing their text or
function behavior.
In `@frontend/src/components/pod/Details.test.tsx`:
- Around line 91-94: Remove the no-op PodDiagnosticsSection mock in the
PodDetails tests unless real event fetching requires it, preferring the real
diagnostics integration. If mocking remains necessary, capture the events and
onViewLogs props and add assertions or a focused test covering their propagation
and container-specific log launching.
In
`@frontend/src/components/statefulset/__snapshots__/CreateStatefulSetForm.Empty.stories.storyshot`:
- Around line 341-404: Update the conditional rendering around the Partition
field in CreateStatefulSetForm so it renders only when spec.updateStrategy.type
equals "RollingUpdate"; keep it hidden for undefined and "OnDelete" strategies.
In `@frontend/src/components/statefulset/CreateStatefulSetForm.stories.tsx`:
- Around line 117-139: Update the StatefulSet story using the OnDelete strategy
to include a rollingUpdate object with partition set to 3, so
CreateStatefulSetForm’s cleanup effect executes the partition-removal path.
Preserve the existing OnDelete resource configuration and story purpose.
In `@frontend/src/components/statefulset/CreateStatefulSetForm.tsx`:
- Line 55: Replace structuredClone in the draft update flow with the shared
_.cloneDeep utility, keeping the existing resource cloning behavior and update
logic unchanged.
In `@frontend/src/components/workload/Details.test.tsx`:
- Around line 32-48: Update the DetailsGrid wiring test and its fixtures to
include a non-empty events collection in the mocked context, then assert that
lastDiagnosticsProps().events matches that fixture. Preserve the existing pods
and errors assertions, and keep the mock limited to the necessary DetailsGrid
boundary.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4786ef9e-b906-499e-b2af-80cd78143e98
⛔ Files ignored due to path filters (1)
docs/development/plugins/images/resource-relation-provider.pngis excluded by!**/*.png
📒 Files selected for processing (90)
.github/scripts/generate-release-issue-body.jsapp/electron/tray.tsbackend/pkg/clusterinventory/clusterinventory_fuzz_test.godocs/development/backend.mddocs/development/index.mddocs/development/plugins/functionality/index.mddocs/development/telemetry.mdfrontend/src/components/App/icons.tsfrontend/src/components/Sidebar/useSidebarItems.tsxfrontend/src/components/common/ObjectEventList.tsxfrontend/src/components/common/Resource/CreateButton.tsxfrontend/src/components/common/Resource/Resource.tsxfrontend/src/components/common/Resource/index.tsxfrontend/src/components/common/index.test.tsfrontend/src/components/common/index.tsfrontend/src/components/crd/CustomResourceDetails.test.tsxfrontend/src/components/crd/CustomResourceDetails.tsxfrontend/src/components/crd/CustomResourceInstancesList.test.tsxfrontend/src/components/crd/CustomResourceInstancesList.tsxfrontend/src/components/crd/CustomResourceList.tsxfrontend/src/components/crd/Details.tsxfrontend/src/components/crd/List.tsxfrontend/src/components/crd/crInstancesKey.test.tsfrontend/src/components/crd/crInstancesKey.tsfrontend/src/components/diagnostics/Diagnostics.stories.tsxfrontend/src/components/diagnostics/Diagnostics.test.tsfrontend/src/components/diagnostics/Diagnostics.tsxfrontend/src/components/diagnostics/__snapshots__/Diagnostics.PodHealthy.stories.storyshotfrontend/src/components/diagnostics/__snapshots__/Diagnostics.PodWithIssues.stories.storyshotfrontend/src/components/diagnostics/__snapshots__/Diagnostics.WorkloadHealthy.stories.storyshotfrontend/src/components/diagnostics/__snapshots__/Diagnostics.WorkloadLoading.stories.storyshotfrontend/src/components/diagnostics/__snapshots__/Diagnostics.WorkloadPodsError.stories.storyshotfrontend/src/components/diagnostics/__snapshots__/Diagnostics.WorkloadWithIssues.stories.storyshotfrontend/src/components/diagnostics/storyHelper.tsfrontend/src/components/globalSearch/useLocalStorageState.test.tsxfrontend/src/components/globalSearch/useLocalStorageState.tsxfrontend/src/components/pod/Details.test.tsxfrontend/src/components/pod/Details.tsxfrontend/src/components/pod/__snapshots__/PodDetails.DebugDisabled.stories.storyshotfrontend/src/components/pod/__snapshots__/PodDetails.Error.stories.storyshotfrontend/src/components/pod/__snapshots__/PodDetails.Initializing.stories.storyshotfrontend/src/components/pod/__snapshots__/PodDetails.LivenessFailed.stories.storyshotfrontend/src/components/pod/__snapshots__/PodDetails.PullBackOff.stories.storyshotfrontend/src/components/pod/__snapshots__/PodDetails.Running.stories.storyshotfrontend/src/components/pod/__snapshots__/PodDetails.Successful.stories.storyshotfrontend/src/components/resourceMap/edges/GraphEdgeComponent.tsxfrontend/src/components/resourceMap/graph/graphModel.tsxfrontend/src/components/resourceMap/graphViewSlice.test.tsxfrontend/src/components/resourceMap/graphViewSlice.tsxfrontend/src/components/resourceMap/sources/GraphSources.test.tsxfrontend/src/components/resourceMap/sources/GraphSources.tsxfrontend/src/components/resourceMap/sources/definitions/relationIds.tsfrontend/src/components/resourceMap/sources/definitions/relations.test.tsxfrontend/src/components/resourceMap/sources/definitions/relations.tsxfrontend/src/components/resourceMap/sources/definitions/sources.test.tsxfrontend/src/components/resourceMap/sources/definitions/sources.tsxfrontend/src/components/statefulset/CreateStatefulSetForm.stories.tsxfrontend/src/components/statefulset/CreateStatefulSetForm.tsxfrontend/src/components/statefulset/__snapshots__/CreateStatefulSetForm.Default.stories.storyshotfrontend/src/components/statefulset/__snapshots__/CreateStatefulSetForm.Empty.stories.storyshotfrontend/src/components/statefulset/__snapshots__/CreateStatefulSetForm.Filled.stories.storyshotfrontend/src/components/statefulset/__snapshots__/CreateStatefulSetForm.OnDeleteStrategy.stories.storyshotfrontend/src/components/workload/Details.test.tsxfrontend/src/components/workload/Details.tsxfrontend/src/i18n/locales/ar/translation.jsonfrontend/src/i18n/locales/bn/translation.jsonfrontend/src/i18n/locales/de/translation.jsonfrontend/src/i18n/locales/en/translation.jsonfrontend/src/i18n/locales/es/translation.jsonfrontend/src/i18n/locales/fr/translation.jsonfrontend/src/i18n/locales/he/translation.jsonfrontend/src/i18n/locales/hi/translation.jsonfrontend/src/i18n/locales/it/translation.jsonfrontend/src/i18n/locales/ja/translation.jsonfrontend/src/i18n/locales/ko/translation.jsonfrontend/src/i18n/locales/pt/translation.jsonfrontend/src/i18n/locales/ru/translation.jsonfrontend/src/i18n/locales/ta/translation.jsonfrontend/src/i18n/locales/ur/translation.jsonfrontend/src/i18n/locales/zh-tw/translation.jsonfrontend/src/i18n/locales/zh/translation.jsonfrontend/src/lib/k8s/KubeObject.test.tsfrontend/src/lib/k8s/crd.test.tsfrontend/src/lib/k8s/crd.tsfrontend/src/lib/k8s/crdSpec.tsfrontend/src/plugin/__snapshots__/pluginLib.snapshotfrontend/src/plugin/registry.test.tsfrontend/src/plugin/registry.tsxplugins/examples/customizing-map/src/index.tsxplugins/headlamp-plugin/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/components/globalSearch/useLocalStorageState.tsx
- frontend/src/components/globalSearch/useLocalStorageState.test.tsx
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
🛑 Comments failed to post (12)
.github/scripts/generate-release-issue-body.js (1)
72-72: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Remove the literal ellipsis from the command path.
The code span contains
./frontend/build.... A release tester who copies the command passes an invalid or unintended path. Use./frontend/build, or place the ellipsis outside the code span.Proposed documentation fix
- - Run the app in static server mode (`./backend/headlamp-server -html-static-dir ./frontend/build...`) + - Run the app in static server mode (`./backend/headlamp-server -html-static-dir ./frontend/build`)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.- Run the app in static server mode (\`./backend/headlamp-server -html-static-dir ./frontend/build\`)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/scripts/generate-release-issue-body.js at line 72, Update the release issue body generated by the relevant template in generate-release-issue-body.js to remove the literal ellipsis from the static server command’s code span, using ./frontend/build as the path while preserving any continuation punctuation outside the command.docs/development/plugins/functionality/index.md (1)
245-265: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Scope relation predicates to the namespace.
The examples match names only.
metadata.nameis not unique for namespaced Deployments and Secrets. The predicate can connect unrelated objects with the same names in different namespaces. Comparemetadata.namespacetoo, or use stable resource IDs.Proposed fix for the Deployment and Secret example
return ( from.kubeObject?.jsonData.metadata.name === "my-deployment" && - to.kubeObject?.jsonData.metadata.name === "my-secret" + to.kubeObject?.jsonData.metadata.name === "my-secret" && + from.kubeObject?.jsonData.metadata.namespace === + to.kubeObject?.jsonData.metadata.namespace );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/development/plugins/functionality/index.md` around lines 245 - 265, Update both relation predicates in the plugin documentation to scope matches by namespace as well as metadata.name, ensuring Deployment, Secret, and custom-source resources from different namespaces cannot be related accidentally. Keep the existing name-matching behavior and add corresponding metadata.namespace comparisons for each endpoint.docs/development/telemetry.md (1)
131-161: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail fd -a -e yaml -e yml 'kubernetes-headlamp|prometheus' . rg -n -C 4 'headlamp.*(:80|:4466)|targetPort|/metrics' \ --glob '*.yaml' --glob '*.yml' .Repository: kubernetes-sigs/headlamp
Length of output: 41890
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '--- manifests namespace/header ---\n' sed -n '1,60p' kubernetes-headlamp.yaml kubernetes-headlamp-monitoring.yaml 2>/dev/null || true printf '\n--- Prometheus scrape target section ---\n' sed -n '140,160p' kubernetes-headlamp-monitoring.yaml 2>/dev/null || true printf '\n--- Service selectors in manifests ---\n' sed -n '1,20p' kubernetes-headlamp.yaml 2>/dev/null || true printf '\n--- Deployments / metrics listener config ---\n' rg -n -C 2 'kind: Deployment|HEADLAMP_CONFIG_|listen|port: 4466|4466' kubernetes-headlamp.yaml kubernetes-headlamp-monitoring.yaml 2>/dev/null || trueRepository: kubernetes-sigs/headlamp
Length of output: 253
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- manifests namespace/header ---' sed -n '1,60p' kubernetes-headlamp.yaml sed -n '1,60p' kubernetes-headlamp-monitoring.yaml printf '%s\n' '' printf '%s\n' '--- Prometheus scrape target section ---' sed -n '140,160p' kubernetes-headlamp-monitoring.yaml printf '%s\n' '' printf '%s\n' '--- Service selectors in manifests ---' sed -n '1,20p' kubernetes-headlamp.yaml printf '%s\n' '' printf '%s\n' '--- Deployments / metrics listener config ---' rg -n -C 2 'kind: Deployment|HEADLAMP_CONFIG_|listen|port: 4466|4466' kubernetes-headlamp.yaml kubernetes-headlamp-monitoring.yamlRepository: kubernetes-sigs/headlamp
Length of output: 6938
🌐 Web query:
Headlamp source HEADLAMP_CONFIG_METRICS_ENABLED metrics endpoint💡 Result:
The HEADLAMP_CONFIG_METRICS_ENABLED environment variable is a configuration flag used in Headlamp to control the exposure of an internal Prometheus metrics endpoint for the Headlamp backend [1][2]. When this variable is set to true, Headlamp exposes metrics in Prometheus format at the /metrics endpoint [1][3][2]. Key details regarding this configuration include: Integration: Enabling this endpoint allows monitoring systems (like Prometheus) to scrape metrics from the Headlamp backend [2][4]. This is commonly achieved in Kubernetes environments by using a ServiceMonitor or similar discovery mechanism to target the metrics port (typically 9090) on the Headlamp service [5][2][6]. Configuration: It is set as an environment variable in the Headlamp deployment [5][6]. For example: - name: HEADLAMP_CONFIG_METRICS_ENABLED value: "true" Distinction from Resource Metrics: It is important to distinguish this backend metrics endpoint from the cluster resource metrics (CPU/memory usage for pods and nodes) displayed within the Headlamp UI [7]. Headlamp's UI resource metrics rely on the Kubernetes metrics-server being installed in the cluster, whereas HEADLAMP_CONFIG_METRICS_ENABLED specifically enables telemetry about the Headlamp application itself [1][7]. Do not confuse this with the Prometheus plugin, which is a separate extension that allows Headlamp to query and visualize data from an existing Prometheus instance within the cluster [8].
Citations:
- 1: https://github.com/kubernetes-sigs/headlamp/blob/main/backend/cmd/headlamp.go
- 2: https://docs.sudhanva.me/how-to/headlamp/
- 3: https://techdocs.broadcom.com/us/en/vmware-tanzu/bitnami-secure-images/bitnami-secure-images/services/bsi-app-doc/apps-charts-headlamp-index.html
- 4: https://docs.sudhanva.me/how-to/monitoring/
- 5: https://github.com/kubernetes-sigs/headlamp/blob/main/kubernetes-headlamp.yaml
- 6: #5915
- 7: https://kubernetes.io/blog/2026/07/13/kubernetes-dashboard-to-headlamp/
- 8: https://deepwiki.com/headlamp-k8s/plugins/2.3-prometheus-plugin
🌐 Web query:
Headlamp kubernetes-headlamp.yaml HEADLAMP_CONFIG_METRICS_ENABLED metrics endpoint port💡 Result:
When enabling metrics in Headlamp by setting the HEADLAMP_CONFIG_METRICS_ENABLED environment variable to true, the metrics are exposed on port 9090 [1][2][3]. In the official Headlamp example kubernetes-headlamp.yaml file, the container is explicitly configured with two ports: 1. Port 4466, which serves the primary Headlamp application [1][3]. 2. Port 9090, which is named metrics and corresponds to the Prometheus metrics endpoint [1][3]. The application code further confirms this behavior, as setting HEADLAMP_CONFIG_METRICS_ENABLED to true registers a handler on the /metrics endpoint [2][4].
Citations:
- 1: https://github.com/kubernetes-sigs/headlamp/blob/main/kubernetes-headlamp.yaml
- 2: https://github.com/kubernetes-sigs/headlamp/blob/main/backend/cmd/headlamp.go
- 3: https://github.com/kubernetes-sigs/headlamp/blob/eccb660b/kubernetes-headlamp.yaml
- 4: https://github.com/kubernetes-sigs/headlamp/blob/eccb660b/backend/cmd/headlamp.go
Correct the Headlamp metrics scrape target in
kubernetes-headlamp-monitoring.yaml.
HEADLAMP_CONFIG_METRICS_ENABLEDroutes/metricsover the main HTTP port4466in these manifests, not theHEADLAMP_CONFIG_METRICS_PORTpath. Keep Prometheus scrapingheadlamp.kube-system.svc.cluster.local:80orheadlamp:80, and remove the stale:4466target.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/development/telemetry.md` around lines 131 - 161, Update the Prometheus scrape target in kubernetes-headlamp-monitoring.yaml to use the Headlamp Service port 80, such as headlamp.kube-system.svc.cluster.local:80 or headlamp:80, and remove the stale :4466 target. Keep the existing /metrics scrape path and metrics configuration unchanged.frontend/src/components/crd/crInstancesKey.ts (1)
71-83: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use unambiguous serialization for CRD identity keys.
Both paths concatenate strings with delimiters that the typed inputs can contain. For example,
fingerprint(['a\x1fb', 'c'])andfingerprint(['a', 'b\x1fc'])produce the same joined and hashed stream. AsortKey()collision can also make a changed CRD set retain the same remount key. This can preserve hook state for the wrong CRD entry.
frontend/src/components/crd/crInstancesKey.ts#L71-L83: Frame every entry with its length in both the join and hash paths. Do not use an unescaped separator as entry framing.frontend/src/components/crd/crInstancesKey.ts#L101-L103: Serializeclusterandidas an unambiguous tuple, such asJSON.stringify([cluster, id]).- Add regression tests with embedded separators and delimiter-containing cluster or identifier values.
📍 Affects 1 file
frontend/src/components/crd/crInstancesKey.ts#L71-L83(this comment)frontend/src/components/crd/crInstancesKey.ts#L101-L103🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/crd/crInstancesKey.ts` around lines 71 - 83, Update frontend/src/components/crd/crInstancesKey.ts lines 71-83 in the fingerprint/sortKey serialization path to frame every key entry with its length in both the joined and hashed representations, removing reliance on an unescaped separator for entry boundaries. Update lines 101-103 to serialize cluster and id as an unambiguous tuple, such as JSON.stringify([cluster, id]). Add regression tests covering embedded separators and delimiter-containing cluster or identifier values.frontend/src/components/crd/CustomResourceInstancesList.tsx (1)
200-210: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Verify the new i18n keys are registered in the locale files. fd -g 'translation.json' frontend/src/i18n/locales | while IFS= read -r f; do echo "== $f" rg -n 'usable specs|incomplete spec' "$f" || echo " (missing)" doneRepository: kubernetes-sigs/headlamp
Length of output: 1202
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== candidate files ==" fd -a 'CustomResource(InstancesList|List|Details)\.tsx$|translation\.json$' frontend/src | sed 's#^\./##' echo echo "== i18n usages in PR files ==" for f in \ frontend/src/components/crd/CustomResourceInstancesList.tsx \ frontend/src/components/crd/CustomResourceList.tsx \ frontend/src/components/crd/CustomResourceDetails.tsx do if [ -f "$f" ]; then echo "-- $f --" rg -n "t\\('translation\\|" "$f" || true fi done echo echo "== translation key definitions across translated locale files ==" python3 - <<'PY' import json from pathlib import Path for root in sorted(Path('frontend/src/i18n/locales').glob('*')): path = root / 'translation.json' if not path.exists(): continue try: data = json.loads(path.read_text()) except Exception as e: print(f"== {path} parse error: {e}") continue needles = [ 'No CustomResourceDefinitions with usable specs were found.', 'This CustomResourceDefinition has an incomplete spec.', ] hits = [k for k in data if k in needles] print(f"== {path} ==") if hits: print(json.dumps({"translation_keys": {k: data[k] for k in hits}}, ensure_ascii=False, indent=2)) else: print("(missing)") PYRepository: kubernetes-sigs/headlamp
Length of output: 4066
Add the new translation keys to the locale files.
CustomResourceInstancesList.tsx,CustomResourceList.tsx, andCustomResourceDetails.tsxusetranslation|...strings, but the locale files underfrontend/src/i18n/localesdo not define these keys. Add the missing entries, or UI text will render the key string instead of the localized label.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/crd/CustomResourceInstancesList.tsx` around lines 200 - 210, Add translation entries for every new translation|... string used by CustomResourceInstancesList, CustomResourceList, and CustomResourceDetails to the locale files under the existing locale structure. Reuse the exact keys and provide localized labels consistent with each locale’s conventions so the UI does not display raw key strings.frontend/src/components/crd/Details.tsx (1)
69-69: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard the remaining
specreads in this file.Lines 69 and 87 now treat
item.specas possibly undefined. Other reads in the same render path still dereference it directly:
- Line 44:
item.spec.group- Line 48:
item.spec.version- Line 52:
item.spec.scope- Lines 56-57:
item.spec.subresources- Line 116:
data={item.spec.versions}If
specis absent, this component still throws, so the new optional chaining does not prevent the crash. Line 116 also breaks whenversionsis absent, becauseSimpleTablereceivesundefined. Apply the same optional handling to those reads.🛡️ Proposed fix
- value: item.spec.group, + value: item.spec?.group,- value: item.spec.version, + value: item.spec?.version,- value: item.spec.scope, + value: item.spec?.scope,- value: item.spec.subresources && Object.keys(item.spec.subresources).join(' & '), - hide: !item.spec.subresources, + value: item.spec?.subresources && Object.keys(item.spec.subresources).join(' & '), + hide: !item.spec?.subresources,- data={item.spec.versions} + data={item.spec?.versions ?? []}Also applies to: 87-87
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/crd/Details.tsx` at line 69, Guard every remaining direct spec access in the Details render path, including group, version, scope, subresources, and versions, by consistently handling an absent item.spec. Ensure the SimpleTable at the versions usage receives a safe fallback collection when versions is undefined, while preserving the existing fallback behavior for kind and name.frontend/src/components/diagnostics/Diagnostics.test.ts (1)
150-152: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert on the id that the implementation can emit.
getPendingHintsemits the idpod-scheduling-event(Diagnostics.tsx line 326). No code path emitspod-scheduling-condition, so this assertion passes unconditionally and does not detect a duplicate scheduling hint.💚 Proposed fix
// The failing PodScheduled condition must not also produce a duplicate // scheduling hint from getPendingHints. - expect(diagnostics.some(item => item.id === 'pod-scheduling-condition')).toBe(false); + expect(diagnostics.some(item => item.id === 'pod-scheduling-event')).toBe(false);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// The failing PodScheduled condition must not also produce a duplicate // scheduling hint from getPendingHints. expect(diagnostics.some(item => item.id === 'pod-scheduling-event')).toBe(false);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/diagnostics/Diagnostics.test.ts` around lines 150 - 152, Update the duplicate-hint assertion in the Diagnostics test to check for the emitted id `pod-scheduling-event` rather than `pod-scheduling-condition`, so it verifies that getPendingHints does not produce the duplicate scheduling hint.frontend/src/components/pod/Details.tsx (1)
894-905: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Apply the existing log permission gate to diagnostics.
The header log action uses
AuthVisibleforgeton thelogsubresource. This callback is passed toPodDiagnosticsSectionfor every pod, so a user without that permission can still see the failing-container log button and openPodLogViewer. The backend should still reject the request, so this is not an authorization bypass. It is a broken unauthorized-user flow. Pass an authorization-aware callback or gate the action insidePodDiagnosticsSection.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/pod/Details.tsx` around lines 894 - 905, Update the diagnostics integration around PodDiagnosticsSection so its failing-container log action uses the same AuthVisible get permission gate for the pod log subresource as the header log action. Pass an authorization-aware onViewLogs callback or apply the gate inside PodDiagnosticsSection, ensuring users without permission cannot see or open PodLogViewer while authorized behavior remains unchanged.frontend/src/components/resourceMap/sources/definitions/relations.test.tsx (1)
243-249: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add coverage for an incomplete CRD.
Add a fixture where
makeCRClassOrNull()returnsnull. Assert thatuseGetAllRelations()does not throw and does not add a CRD owner relation. This covers the new resilience path inuseGetCRToOwnerRelations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/resourceMap/sources/definitions/relations.test.tsx` around lines 243 - 249, Add a test fixture in the relations test setup where the CRD’s makeCRClassOrNull returns null, then invoke useGetAllRelations and assert it does not throw and produces no CRD owner relation. Keep the existing complete-CRD coverage unchanged and target the resilience path through useGetCRToOwnerRelations.Source: Coding guidelines
frontend/src/components/resourceMap/sources/definitions/relations.tsx (1)
404-420: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Contain plugin indexed edge-builder failures.
safePluginRelationscatches errors frompredicate, butGraphSourceManagerinvokesbuildEdgesWithIndexdirectly. A plugin can register a throwing edge builder, or a truthy non-function value, and crash Resource Map rendering.
frontend/src/components/resourceMap/sources/definitions/relations.tsx#L404-L420: wrap pluginbuildEdgesWithIndexintry/catchand return an empty edge list on failure.frontend/src/plugin/registry.tsx#L1271-L1325: rejectbuildEdgesWithIndexwhen it is defined but not a function.📍 Affects 2 files
frontend/src/components/resourceMap/sources/definitions/relations.tsx#L404-L420(this comment)frontend/src/plugin/registry.tsx#L1271-L1325🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/resourceMap/sources/definitions/relations.tsx` around lines 404 - 420, The plugin relation handling must contain indexed edge-builder failures: in frontend/src/components/resourceMap/sources/definitions/relations.tsx lines 404-420, wrap plugin buildEdgesWithIndex invocation in try/catch and return an empty edge list on errors; in frontend/src/plugin/registry.tsx lines 1271-1325, validate that a defined buildEdgesWithIndex value is a function and reject registrations that provide any other type.frontend/src/components/statefulset/CreateStatefulSetForm.stories.tsx (1)
84-115: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The
Filledstory omits the requiredspec.serviceName. The form marksspec.serviceNameas required, so the story renders an empty required field and reportsvalid === false, which contradicts its stated purpose.
frontend/src/components/statefulset/CreateStatefulSetForm.stories.tsx#L84-L115: addserviceNametoFilled.args.spec.frontend/src/components/statefulset/__snapshots__/CreateStatefulSetForm.Filled.stories.storyshot#L288-L336: regenerate this snapshot so theService Nameinput holds the new value.📍 Affects 2 files
frontend/src/components/statefulset/CreateStatefulSetForm.stories.tsx#L84-L115(this comment)frontend/src/components/statefulset/__snapshots__/CreateStatefulSetForm.Filled.stories.storyshot#L288-L336🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/statefulset/CreateStatefulSetForm.stories.tsx` around lines 84 - 115, The Filled story in frontend/src/components/statefulset/CreateStatefulSetForm.stories.tsx:84-115 must include a valid serviceName under Filled.args.spec so the required form field is populated and the story remains valid. Regenerate frontend/src/components/statefulset/__snapshots__/CreateStatefulSetForm.Filled.stories.storyshot:288-336 so the Service Name input reflects the new value.frontend/src/i18n/locales/hi/translation.json (1)
568-568: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail old_key='No custom resources found' new_key='No CustomResourceDefinitions with usable specs were found.' printf '%s\n' "Old-key usages outside locale files:" rg -n -F "$old_key" frontend --glob '!frontend/src/i18n/locales/**' || true printf '%s\n' "New-key usages outside locale files:" rg -n -F "$new_key" frontend --glob '!frontend/src/i18n/locales/**' || trueRepository: kubernetes-sigs/headlamp
Length of output: 363
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' "Relevant source around CustomResourceList:" sed -n '150,190p' frontend/src/components/crd/CustomResourceList.tsx printf '%s\n' "Locale entries for both keys:" for f in frontend/src/i18n/locales/*/translation.json; do echo "--- $f" rg -n 'No custom.resources.found|No CustomResourceDefinitions with usable specs were found' "$f" || true doneRepository: kubernetes-sigs/headlamp
Length of output: 3286
🏁 Script executed:
#!/bin/bash set -euo pipefail # Extract translation keys referenced and present in each affected locale outside locale files. python3 - <<'PY' from pathlib import Path import json, re root = Path("frontend/src/i18n/locales") files = sorted(root.glob("*/translation.json")) patterns = [ r"translation\|\s*(['\"])([^\1]+?)\1", r"i18next\.t\(\s*(['\"])translation\|([^\1]+?)\1", ] for file in files: data = json.loads(file.read_text()) text = file.read_text() missing = [] for pat in patterns: for m in re.finditer(pat, text): key = m.group(2) if key in data and key.startswith("No CustomResourceDefinitions"): print(f"{file}:{key}:present={key in data}") break else: continue break else: # No translation| No Custom..., report missing if old-like key in data as context. keys = set(data) if "translation|No CustomResourceDefinitions with usable specs were found." not in keys: print(f"{file}:no-new-key:missing=True") PYRepository: kubernetes-sigs/headlamp
Length of output: 1355
Update the current CRD translation call before replacing the key.
frontend/src/components/crd/CustomResourceList.tsx:175still usestranslation|No custom resources foundas the only app-side consumer, and the new key is not present in the locale files. Either keep the existing key with localized values, or update the source call plus every locale entry with the new key before removing the old one.📍 Affects 9 files
frontend/src/i18n/locales/hi/translation.json#L568-L568(this comment)frontend/src/i18n/locales/it/translation.json#L573-L573frontend/src/i18n/locales/ja/translation.json#L563-L563frontend/src/i18n/locales/ko/translation.json#L563-L563frontend/src/i18n/locales/pt/translation.json#L573-L573frontend/src/i18n/locales/ru/translation.json#L578-L578frontend/src/i18n/locales/ta/translation.json#L568-L568frontend/src/i18n/locales/ur/translation.json#L568-L568frontend/src/i18n/locales/zh-tw/translation.json#L563-L563🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/i18n/locales/hi/translation.json` at line 568, Update the CRD empty-state translation consistently: change the CustomResourceList translation call to the new key and add localized entries for it in frontend/src/i18n/locales/hi/translation.json:568, it/translation.json:573, ja/translation.json:563, ko/translation.json:563, pt/translation.json:573, ru/translation.json:578, ta/translation.json:568, ur/translation.json:568, and zh-tw/translation.json:563, then remove the old key only after every consumer and locale entry has been migrated.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
frontend/src/components/globalSearch/useLocalStorageState.tsx:115
- The PR description includes an auto-generated “Summary by CodeRabbit” section listing unrelated features (pods/workload diagnostics, StatefulSet creation support, telemetry docs, etc.) that are not part of this diff. This makes the PR description misleading for reviewers/release notes; please remove or replace that section so it reflects only this hook fix.
export const useLocalStorageState = Object.assign(useLocalStorageStateBase, {
/**
* Update the value in local storage and notify all `useLocalStorageState` hooks.
*
* @param key - local storage key
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
frontend/src/components/globalSearch/useLocalStorageState.test.tsx:106
- The cross-component regression claimed by the PR is not demonstrated here: in the pre-change implementation the listener called
set(() => newValue), so its updater ignored the capturedstateand this test would also produce42after the forced rerender. The actual failing test is the consecutive functional-setter case above, while the Global Search caller also benefits fromupdate()now persisting when no subscriber is mounted. Please align the title/linked-issue explanation with those concrete bugs, or add a listener-path test that fails against the old implementation before claiming the stale listener is fixed.
it('propagates updates via useLocalStorageState.update across hook instances', () => {
const { result: a } = renderHook(() => useLocalStorageState(TEST_KEY, 0));
const { result: b } = renderHook(() => useLocalStorageState(TEST_KEY, 0));
// Force a re-render of one hook instance before update() fires.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
frontend/src/components/globalSearch/useLocalStorageState.tsx:139
- The current diff no longer implements the stale-closure fix described in the PR: the base hook already uses a shared
StorageEntrywithuseSyncExternalStore, and production callers now use the synchronized setter. This change instead reintroduces a new.updatepublic member with no non-test call sites, while the PR body still claimsstateRef/useEffectchanges and “no public API” change. Please either retitle and rewrite the PR to justify restoring this API, or remove the now-obsolete addition.
export const useLocalStorageState = Object.assign(useLocalStorageStateBase, {
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: vishnukothakapu 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 |
|
Hi @illume , I wanted to leave a quick note explaining the recent force-push and rewrite of this PR's description. As you might have noticed, the original stale-closure bug that this PR was opened to fix 2 months ago was recently resolved in Since I had put a lot of work and gone through many review cycles on this PR over the last two months, I didn't want all that effort to go to waste! Instead of closing this, I pivoted the PR. I kept the The PR is now fully updated, tested, and ready for review under its new purpose! |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (3)
frontend/src/components/common/Resource/LogsButton.tsx:574
- This only broadcasts changes made by
LogsButton; itsselectedSeveritiesis still auseStateinitialized once from storage, so an update from another tab or hook never updates an already-open LogsButton. Cross-tab severity synchronization therefore remains one-way. Back this state withuseLocalStorageState(while preserving validation) or subscribe it to external updates.
setSelectedSeverities(value);
useLocalStorageState.update('headlamp.logs.severityFilter', value);
frontend/src/components/globalSearch/useLocalStorageState.tsx:36
StorageEvent.newValueisnullforremoveItem, andkeyisnullforclear; both cases are ignored here, so mounted hooks keep stale values after another tab resets storage. Please retain each entry's default value and notify affected subscribers with that default for removals and clears.
if (event.key === null) {
return;
}
const entry = storageEntries.get(event.key);
if (entry && event.newValue !== null) {
frontend/src/components/globalSearch/useLocalStorageState.test.tsx:283
- These tests call
.update()directly, so they do not cover the production regression inLogsButton; reverting that component to rawlocalStorage.setItemwould leave every added test passing. Add aLogsButtonregression test that changes the severity control and verifies a hook consumer receives the update.
it('propagates updates via useLocalStorageState.update across hook instances', () => {
const { result: a } = renderHook(() => useLocalStorageState(storageKey, 0));
const { result: b } = renderHook(() => useLocalStorageState(storageKey, 0));
act(() => {
illume
left a comment
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
Address Copilot review feedback— 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 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (3)
frontend/src/components/common/Resource/LogsButton.tsx:117
- The second commit,
Address Copilot review feedback, significantly rewrites the first commit's LogsButton integration and adds omitted remove/clear handling, while its title does not describe those changes. Please squash it into the first commit or rewrite the commits into coherent<area>: <description>units so the PR history presents the final design directly.
const [storedSeverities, setStoredSeverities] = useLocalStorageState<LogSeverity[]>(
'headlamp.logs.severityFilter',
[...ALL_SEVERITIES]
);
frontend/src/components/common/Resource/LogsButton.tsx:569
- The PR description says LogsButton was refactored to use the new
useLocalStorageState.update()utility, but this path now uses the normal hook setter, and the utility has no production caller. Please align the description and scope with the final implementation—either remove the unused public utility and its dedicated tests, or document the separate use case that requires it.
setStoredSeverities(value);
frontend/src/components/globalSearch/useLocalStorageState.tsx:35
- The global
storagelistener also receivessessionStorageevents. Without checkingevent.storageArea, a same-named session-storage update can overwrite this hook's local-storage state, andsessionStorage.clear()(key === null) resets every active entry. Ignore events whose storage area is notlocalStorage, and cover that case in the synchronization tests.
window.addEventListener('storage', (event: StorageEvent) => {
if (event.key === null) {
// localStorage.clear() was called. Reset all entries to their default values.
for (const entry of storageEntries.values()) {
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
frontend/src/components/globalSearch/useLocalStorageState.tsx:180
- The PR says
LogsButtonwas refactored to use this new.update()API, but the component actually uses the hook setter, and repository search finds no caller of.update(). This leaves an untested public API that is unnecessary for the synchronization fix. Please either remove this export and correct the description, or integrate and test it where it is genuinely needed.
export const useLocalStorageState = Object.assign(useLocalStorageStateBase, {
/**
* Update the value in local storage and notify all `useLocalStorageState` hooks.
*
* @param key - local storage key
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
frontend/src/components/globalSearch/useLocalStorageState.tsx:45
- After
useSyncExternalStorereplays a subscription (as React StrictMode does), the existing cleanup removes this hook's entry fromstorageEntries, but the nextsubscribeonly re-adds the listener to the captured entry and never restores the map entry. This lookup then returnsundefined, so mounted hooks under StrictMode—such asuseGraphViewportinsideGraphView's StrictMode boundary—will not receive the new cross-tab events. Please makesubscriberestore the map entry (and make cleanup delete it only when the map still points to that same entry), with a StrictMode regression test.
const entry = storageEntries.get(event.key);
|
@illume Just an update: the latest Copilot reviews have generated no new comments. The PR is fully up-to-date and ready for your review whenever you have the time! |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
frontend/src/components/globalSearch/useLocalStorageState.tsx:49
- Please add a regression test for the new
removeItempath. The cross-tab tests cover updates andclear(), but not a matching storage event withnewValue === null, so the default-value fallback in this branch can regress unnoticed.
if (event.newValue === null) {
// localStorage.removeItem() was called for this key.
entry.value = JSON.parse(entry.serializedDefaultValue);
This commit brings several necessary improvements to useLocalStorageState and uses them to fix a bug in LogsButton. 1. Added cross-tab synchronization: The hook now listens to the browser's 'storage' event, ensuring that changes made in one tab immediately update the state in all other tabs, including correctly falling back to default values on removeItem or clear. 2. Added direct-value setters: The setState function now accepts both updater functions and direct values (T | (oldValue: T) => T) to improve DX. 3. Fixed LogsButton sync bug: Refactored LogsButton to fully use the useLocalStorageState hook instead of raw localStorage, adding robust two-way sync for the log severity filter so that it stays perfectly in sync across the entire application and multiple tabs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (2)
.github/workflows/app.yml:87
- This installs the app dependencies twice:
npm ci --forceruns here, thenmake app-buildinvokes the newnpm cicommand inMakefile:86. Becausenpm ciremoves the existingnode_modulesbefore reinstalling, the first install is discarded for every Windows matrix job. Letapp-buildperform the single clean install.
cd app
npm ci --force
cd ..
make app-build
Makefile:86
- This app build-policy change is unrelated to the local-storage/log synchronization described by the PR, and the accompanying workflow modification touches a manual-review-only area without any rationale in the description. Please revert these two app-build changes or move them to a focused PR that explains and validates the intended CI change.
cd app && npm ci && node ./scripts/setup-plugins.js && npm run build
Summary
While the original stale-closure bug was resolved in main via
useSyncExternalStore(commit 676ef52), that refactor unfortunately missed some critical features of standard local storage hooks. Since I've spent two months iterating on this PR, I decided to pivot this branch to add those missing features, drastically improving the hook and fixing a real bug!The Improvements
1. Cross-Tab Synchronization (Missing in main)
The current implementation in main only synchronizes state within the same window because it doesn't listen to the browser's storage event. I added a global
window.addEventListener('storage')listener. Now, if a user has Headlamp open in two tabs and changes a setting (like log severity), all tabs immediately update in sync.2. Direct-Value Setters (Missing in main)
Standard React
useStateaccepts both updater functions and direct values. The implementation in main forced developers to use updater functions (e.g.,setZoomMode(() => '100%')). I updated the signature to acceptT | ((oldValue: T) => T)for a much better developer experience.3. Fixed LogsButton Synchronization Bug
Previously,
LogsButton.tsxupdatedheadlamp.logs.severityFilterdirectly vialocalStorage.setItem. Because it bypassed the hook, other components (like Pod Details) did not re-render when it changed. I refactoredLogsButtonto fully consume theuseLocalStorageStatehook instead of raw local storage, instantly fixing this cross-component sync bug!Steps to Test