ui: route partial-response localStorage through a shared accessor - #8992
Open
donaldraph wants to merge 1 commit into
Open
donaldraph wants to merge 1 commit into
donaldraph wants to merge 1 commit into
Conversation
Signed-off-by: DonaldRaph <62525712+donaldraph@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8992 +/- ##
==========================================
- Coverage 64.53% 64.52% -0.02%
==========================================
Files 289 289
Lines 37366 37366
==========================================
- Hits 24113 24109 -4
- Misses 11156 11161 +5
+ Partials 2097 2096 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
Every localStorage read/write in the React app goes through the
useLocalStoragehook, except theusePartialResponsekey inPanel.tsxandPanelList.tsx, which touchlocalStoragedirectly.Panelis a class component and can't call the hook, which is why it bypassed it.I pulled the raw operations out into three plain functions in
useLocalStorage.tsx—getStorageItem,setStorageItem,hasStorageItem— so both hook and non-hook callers go through the same accessor.useLocalStoragebecomes a thin wrapper over them, and I routed the three bypass sites through them instead of touchinglocalStoragedirectly. After this, no rawlocalStorage.access remains in the app outside that one module.I intended this as a read/write refactor with no behavior change, so I checked the full git history of the key: it has only ever been written as
JSON.stringify(<boolean>)since it was introduced, so under normal use the stored value is always"true","false", or absent.One thing I want to flag: routing reads through
JSON.parse(which the old raw-string comparison didn't do) would throw on a non-JSON value. I confirmed that can't happen through app code, only via manual localStorage tampering or corruption — but to keep the old "never crashes on a bad value" behavior, I added a try/catch ingetStorageItemthat falls back to the initial value on a parse error, the same approach jaeger-ui uses for this. I usedhasStorageItem(no parsing) for thecomponentDidMountexistence check to keep exact parity there.One residual I'll disclose:
handleChangePartialResponsenow parses via the typed accessor, so a whitespace-padded value like" true "would parse totruewhere the old=== 'true'didn't match. No app code has ever produced such a value, and closing it fully would mean abandoning the typed accessor for that one read, which didn't seem worth it — happy to change that if you'd prefer.Verification
I verified behavior parity empirically across every value the key could hold before settling on this shape.
make react-app-lintandmake react-app-testboth pass (260 tests).