frontend: helpers: clusterSettings: Guard localStorage reads against corrupt data - #7266
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Kakumanu-Harshitha 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 |
d4976b5 to
64f73af
Compare
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
build: update npm version and package-lock.json— Missingarea: descriptionprefix — e.g.frontend: HomeButton: Fix so it navigates to homeorbackend: config: Add enable-dynamic-clusters flag.build: fix npm ci flag and regenerate package-lock.json— Missingarea: descriptionprefix — e.g.frontend: HomeButton: Fix so it navigates to homeorbackend: config: Add enable-dynamic-clusters flag.build: use npm install in Dockerfile to fix lockfile sync issues— Missingarea: descriptionprefix — e.g.frontend: HomeButton: Fix so it navigates to homeorbackend: config: Add enable-dynamic-clusters flag.build(frontend): pin @emnapi versions via overrides to fix lockfile sync— Missingarea: descriptionprefix — e.g.frontend: HomeButton: Fix so it navigates to homeorbackend: config: Add enable-dynamic-clusters flag.build(frontend): add @emnapi to dependencies to ensure lockfile tracking— 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
Adds defensive handling for cluster-setting writes, alongside broader npm and container-build changes. The read-path fix is already present on the base branch.
Changes:
- Catches and tests
localStorage.setItemfailures. - Regenerates frontend dependencies and adds
@emnapipackages. - Changes Docker npm installation behavior; plugin-example CI currently fails.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
frontend/src/helpers/clusterSettings.ts |
Handles storage write failures. |
frontend/src/helpers/clusterSettings.test.ts |
Tests write failures and improves cleanup. |
frontend/package.json |
Adds and overrides @emnapi dependencies. |
frontend/package-lock.json |
Regenerates dependency resolution. |
Dockerfile |
Updates npm and frontend installation. |
Dockerfile.plugins |
Updates npm for plugin builds. |
Files not reviewed (1)
- frontend/package-lock.json: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| try { | ||
| localStorage.setItem(`cluster_settings.${clusterName}`, JSON.stringify(settings)); | ||
| } catch (error) { |
| "@emnapi/core": "1.10.0", | ||
| "@emnapi/runtime": "1.10.0", |
| COPY frontend/package*.json /headlamp/frontend/ | ||
| WORKDIR /headlamp | ||
| RUN cd ./frontend && npm ci --only=prod | ||
| RUN cd ./frontend && npm install --omit=dev --no-audit --no-fund |
|
|
||
| FROM --platform=${BUILDPLATFORM} node:22@sha256:5647be709086c696ff32edaaf1c70cd26d1da6ab2b39c32f3c7b4c4a31957e37 AS frontend-build | ||
| # Ensure npm >= 11 to satisfy engines in frontend/package.json | ||
| RUN npm install -g npm@^11.0.0 |
| # Build the plugin | ||
| FROM node:22@sha256:5647be709086c696ff32edaaf1c70cd26d1da6ab2b39c32f3c7b4c4a31957e37 as builder | ||
| # Ensure npm >= 11 to satisfy engines in frontend/package.json | ||
| RUN npm install -g npm@^11.0.0 |
1c1e364 to
756c69f
Compare
…orage failures Wrap localStorage.setItem in storeClusterSettings with try/catch to prevent an uncaught QuotaExceededError (or any storage exception) from crashing the application. On failure, the error is logged to console and the call becomes a safe no-op. Add tests for the new error-handling path and harden existing test spy teardown with try/finally blocks. Signed-off-by: Kakumanu-Harshitha <harshithakakumanu2006@gmail.com>
756c69f to
9721b93
Compare
illume
left a comment
There was a problem hiding this comment.
Thanks for this PR.
The open review comments from Copilot still need attention — can you have a look? Once addressed, please mark them as resolved.
f00969f to
9721b93
Compare
|
hey @illume PTAL! when you are free? |
Summary
loadClusterSettingscalledJSON.parsewithout any error handling. CorruptedlocalStorage entries (e.g. from browser extensions, quota truncation, or manual
edits) threw an uncaught
SyntaxErrorthat propagated to all callers(NamespacesAutocomplete, GlobalSearch, NodeShellTerminal, PodDebugTerminal, etc.),
crashing the settings panel. Additionally, valid but non-object JSON payloads
(
123,[],null) passed through silently, breaking downstream property accesses.Related Issue
Fixes #7265
Changes
clusterSettings.ts: WrapJSON.parsein try/catch; add strict type guardrejecting non-object payloads, falling back to
{}with a logged error.clusterSettings.ts: WraplocalStorage.setItemin try/catch so quota errorsare logged instead of thrown.
clusterSettings.test.ts: Add tests for invalid JSON, primitive payloads(string, number, array, null), and storage quota errors; use try/finally for
spy cleanup to prevent mock leakage.
Steps to Test
cluster_settings.<cluster-name>to{not valid json.123or["a"]— verifyloadClusterSettingsreturns{}gracefully.npm run frontend:test -- src/helpers/clusterSettings.test.ts— all tests pass.Notes for the Reviewer
this commit fulfils that TODO.
frontend: pluginConfigSlice: Guard JSON.parse of localStorage with try/catch#6119 (pluginConfigSlicetry/catch).