-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: resolve k8s pods list local storage issue #6775
Conversation
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
1 similar comment
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Reviewed everything up to efbd637 in 34 seconds
More details
- Looked at
13
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. frontend/src/container/InfraMonitoringK8s/Pods/K8sPodLists.tsx:108
- Draft comment:
Consider adding error handling for JSON.parse to prevent potential runtime errors if the local storage value is not a valid JSON. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The code already has a fallback of '[]' when the localStorage value is not present. If the JSON is malformed, it would throw an error, but this is a UI feature for column preferences - not critical functionality. The worst case is the columns reset to default. The code also has a checkif (addedColumns && addedColumns.length > 0)
which would handle null/undefined cases gracefully.
The comment raises a valid point about potential runtime errors. Malformed JSON in localStorage could cause the component to crash.
While true, this is a low-impact UI preference feature. The worst case is the columns reset to default, and the existing null checks provide some safety. Adding try-catch would make the code more complex for minimal benefit.
Delete the comment. The potential issue is low-impact, the code has some safety checks, and adding error handling would increase complexity for minimal benefit.
Workflow ID: wflow_XRX2Xeld0pmermVV
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
efbd637
to
019174a
Compare
@@ -105,7 +105,7 @@ function K8sPodsList({ | |||
); | |||
|
|||
useEffect(() => { | |||
const addedColumns = JSON.parse(get('k8sPodsAddedColumns') ?? ''); | |||
const addedColumns = JSON.parse(get('k8sPodsAddedColumns') ?? '[]'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should always try to add json.parse
in try catch blocks
Summary
Pods List UI was breaking when opened for the 1st time as it couldn't find the column config key in local storage.
Updated the logic to handle it.
Related Issues / PR's
Screenshots
NA
Affected Areas and Manually Tested Areas
Infra Monitoring section
Important
Fixes UI breakage in
K8sPodsList
by defaulting missing local storage key to an empty array.K8sPodsList
where the UI breaks ifk8sPodsAddedColumns
key is missing in local storage by defaulting to an empty array.useEffect
inK8sPodsList.tsx
to useJSON.parse(get('k8sPodsAddedColumns') ?? '[]')
instead ofJSON.parse(get('k8sPodsAddedColumns') ?? '')
.This description was created by for efbd637. It will automatically update as commits are pushed.