guard client storage access Fixes [#331]#332
Open
Th-Shivam wants to merge 1 commit intoAOSSIE-Org:mainfrom
Open
guard client storage access Fixes [#331]#332Th-Shivam wants to merge 1 commit intoAOSSIE-Org:mainfrom
Th-Shivam wants to merge 1 commit intoAOSSIE-Org:mainfrom
Conversation
Contributor
WalkthroughThis PR adds a centralized storage utility module with four safe localStorage access functions and updates existing code across multiple components to use these helpers instead of direct localStorage calls, ensuring graceful fallbacks in restricted browser contexts. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Pull request overview
This PR improves client startup resilience by centralizing guarded localStorage access and safe parsing helpers, preventing crashes in restricted browser contexts (private mode, embedded webviews, blocked storage) while defaulting to sane settings.
Changes:
- Added
safeGetItem/safeSetItemplus typed helpersgetStoredNumber/getStoredBooleanfor robust localStorage reads/writes and parsing. - Updated settings, trackpad, theme initialization, and auth token bootstrap to use the new storage helpers instead of direct
localStorageaccess. - Added Vitest coverage for blocked storage access and malformed stored values.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/utils/storage.ts |
Introduces shared safe storage access + parsing helpers. |
src/utils/storage.test.ts |
Adds unit tests for failure/fallback behavior of the storage helpers. |
src/routes/trackpad.tsx |
Uses safe helpers for sensitivity/invert settings bootstrap. |
src/routes/settings.tsx |
Uses safe helpers for settings initialization and persistence (including auth token and theme). |
src/routes/__root.tsx |
Uses safe helper for theme initialization at app root. |
src/contexts/ConnectionProvider.tsx |
Uses safe helpers for auth token read/write during connection bootstrap. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+70
to
+81
| it("supports JSON encoded booleans", () => { | ||
| vi.stubGlobal( | ||
| "window", | ||
| { | ||
| localStorage: { | ||
| getItem: () => "true", | ||
| }, | ||
| } as unknown as Window, | ||
| ) | ||
|
|
||
| expect(getStoredBoolean("rein_invert", false)).toBe(true) | ||
| }) |
There was a problem hiding this comment.
The test named "supports JSON encoded booleans" currently stubs getItem to return the exact string "true", which is handled by the early saved === "true" branch and never exercises the JSON.parse(saved) path. Either rename this test to reflect what it actually covers (boolean strings), or adjust/add a test case that forces the JSON.parse branch (e.g., leading/trailing whitespace) so that code path is covered.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/contexts/ConnectionProvider.tsx`:
- Around line 67-72: The code persists the auth token under the key
"rein_auth_token" using safeGetItem/safeSetItem in ConnectionProvider
(token/urlToken logic), which exposes it to script-accessible storage; remove
the localStorage-backed persistence and switch to a secure storage strategy:
stop calling safeSetItem("rein_auth_token", ...) and stop reading from
safeGetItem for auth tokens, instead keep the token in React state/session
memory or delegate storage to a server-set HttpOnly secure cookie (preferred) or
use a platform secure store (e.g., expo-secure-store) via
SecureStore.getItemAsync/ setItemAsync if client-side secure storage is
required; update any related usages (including the settings route) to read from
the new mechanism and ensure token propagation uses the ConnectionProvider state
or server cookie rather than localStorage.
In `@src/utils/storage.test.ts`:
- Around line 14-81: Add two tests: one that stubs absence of window
(vi.stubGlobal("window", undefined)) to exercise the no-window branch for
safeGetItem/safeSetItem/getStoredNumber/getStoredBoolean and assert they return
their safe defaults (null/false/default number/default boolean); and one that
stubs localStorage.getItem to return a partially malformed numeric string (e.g.,
"42abc") and assert getStoredNumber("rein_sensitivity", 1) falls back or coerces
as intended (match the implementation's expected behavior); reference
safeGetItem, safeSetItem, and getStoredNumber/getStoredBoolean when adding these
test cases.
In `@src/utils/storage.ts`:
- Around line 22-27: The getStoredNumber function currently uses
Number.parseFloat which accepts partial numeric strings (e.g., "1abc") and thus
bypasses the malformed-value fallback; change the parsing to use Number(saved)
to force strict conversion, keep the existing Number.isFinite(parsed) check to
return the fallback on invalid values, and update the return path in
getStoredNumber accordingly so only fully numeric strings are accepted.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 80496704-2566-499d-acc9-0e6e2438c939
📒 Files selected for processing (6)
src/contexts/ConnectionProvider.tsxsrc/routes/__root.tsxsrc/routes/settings.tsxsrc/routes/trackpad.tsxsrc/utils/storage.test.tssrc/utils/storage.ts
Comment on lines
+67
to
+72
| const storedToken = safeGetItem("rein_auth_token") | ||
|
|
||
| const token = urlToken || storedToken | ||
|
|
||
| if (urlToken && urlToken !== storedToken) { | ||
| try { | ||
| localStorage.setItem("rein_auth_token", urlToken) | ||
| } catch (e) { | ||
| // Failed to store | ||
| } | ||
| safeSetItem("rein_auth_token", urlToken) |
Contributor
There was a problem hiding this comment.
Avoid persisting auth tokens in localStorage-backed storage.
rein_auth_token remains script-readable at rest, which increases token theft risk in any XSS scenario (also written in src/routes/settings.tsx, Line 86). Prefer HttpOnly secure cookies (server-managed) or at least session-scoped memory strategy for browser clients.
As per coding guidelines, "No exposed API keys or sensitive data" and "Use expo-secure-store for sensitive storage".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/contexts/ConnectionProvider.tsx` around lines 67 - 72, The code persists
the auth token under the key "rein_auth_token" using safeGetItem/safeSetItem in
ConnectionProvider (token/urlToken logic), which exposes it to script-accessible
storage; remove the localStorage-backed persistence and switch to a secure
storage strategy: stop calling safeSetItem("rein_auth_token", ...) and stop
reading from safeGetItem for auth tokens, instead keep the token in React
state/session memory or delegate storage to a server-set HttpOnly secure cookie
(preferred) or use a platform secure store (e.g., expo-secure-store) via
SecureStore.getItemAsync/ setItemAsync if client-side secure storage is
required; update any related usages (including the settings route) to read from
the new mechanism and ensure token propagation uses the ConnectionProvider state
or server cookie rather than localStorage.
Comment on lines
+14
to
+81
| it("returns null when localStorage access throws on read", () => { | ||
| vi.stubGlobal( | ||
| "window", | ||
| { | ||
| localStorage: { | ||
| getItem: () => { | ||
| throw new Error("blocked") | ||
| }, | ||
| }, | ||
| } as unknown as Window, | ||
| ) | ||
|
|
||
| expect(safeGetItem("rein_auth_token")).toBeNull() | ||
| }) | ||
|
|
||
| it("returns false when localStorage access throws on write", () => { | ||
| vi.stubGlobal( | ||
| "window", | ||
| { | ||
| localStorage: { | ||
| setItem: () => { | ||
| throw new Error("blocked") | ||
| }, | ||
| }, | ||
| } as unknown as Window, | ||
| ) | ||
|
|
||
| expect(safeSetItem("rein_auth_token", "token")).toBe(false) | ||
| }) | ||
|
|
||
| it("falls back when stored number is invalid", () => { | ||
| vi.stubGlobal( | ||
| "window", | ||
| { | ||
| localStorage: { | ||
| getItem: () => "not-a-number", | ||
| }, | ||
| } as unknown as Window, | ||
| ) | ||
|
|
||
| expect(getStoredNumber("rein_sensitivity", 1)).toBe(1) | ||
| }) | ||
|
|
||
| it("falls back when stored boolean is malformed", () => { | ||
| vi.stubGlobal( | ||
| "window", | ||
| { | ||
| localStorage: { | ||
| getItem: () => "{oops", | ||
| }, | ||
| } as unknown as Window, | ||
| ) | ||
|
|
||
| expect(getStoredBoolean("rein_invert", false)).toBe(false) | ||
| }) | ||
|
|
||
| it("supports JSON encoded booleans", () => { | ||
| vi.stubGlobal( | ||
| "window", | ||
| { | ||
| localStorage: { | ||
| getItem: () => "true", | ||
| }, | ||
| } as unknown as Window, | ||
| ) | ||
|
|
||
| expect(getStoredBoolean("rein_invert", false)).toBe(true) | ||
| }) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add tests for no-window branch and partially malformed numeric values.
Current coverage is good, but these two branches are still missing and are directly relevant to this PR’s resilience goal.
Suggested test additions
+ it("returns null when window is unavailable", () => {
+ vi.stubGlobal("window", undefined as unknown as Window)
+ expect(safeGetItem("rein_auth_token")).toBeNull()
+ })
+
+ it("falls back when number has trailing invalid characters", () => {
+ vi.stubGlobal(
+ "window",
+ {
+ localStorage: {
+ getItem: () => "1.2abc",
+ },
+ } as unknown as Window,
+ )
+ expect(getStoredNumber("rein_sensitivity", 1)).toBe(1)
+ })As per coding guidelines, "**/*.test.{ts,tsx,js,jsx}: Review test files for: Comprehensive coverage of component behavior".
📝 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.
Suggested change
| it("returns null when localStorage access throws on read", () => { | |
| vi.stubGlobal( | |
| "window", | |
| { | |
| localStorage: { | |
| getItem: () => { | |
| throw new Error("blocked") | |
| }, | |
| }, | |
| } as unknown as Window, | |
| ) | |
| expect(safeGetItem("rein_auth_token")).toBeNull() | |
| }) | |
| it("returns false when localStorage access throws on write", () => { | |
| vi.stubGlobal( | |
| "window", | |
| { | |
| localStorage: { | |
| setItem: () => { | |
| throw new Error("blocked") | |
| }, | |
| }, | |
| } as unknown as Window, | |
| ) | |
| expect(safeSetItem("rein_auth_token", "token")).toBe(false) | |
| }) | |
| it("falls back when stored number is invalid", () => { | |
| vi.stubGlobal( | |
| "window", | |
| { | |
| localStorage: { | |
| getItem: () => "not-a-number", | |
| }, | |
| } as unknown as Window, | |
| ) | |
| expect(getStoredNumber("rein_sensitivity", 1)).toBe(1) | |
| }) | |
| it("falls back when stored boolean is malformed", () => { | |
| vi.stubGlobal( | |
| "window", | |
| { | |
| localStorage: { | |
| getItem: () => "{oops", | |
| }, | |
| } as unknown as Window, | |
| ) | |
| expect(getStoredBoolean("rein_invert", false)).toBe(false) | |
| }) | |
| it("supports JSON encoded booleans", () => { | |
| vi.stubGlobal( | |
| "window", | |
| { | |
| localStorage: { | |
| getItem: () => "true", | |
| }, | |
| } as unknown as Window, | |
| ) | |
| expect(getStoredBoolean("rein_invert", false)).toBe(true) | |
| }) | |
| it("returns null when localStorage access throws on read", () => { | |
| vi.stubGlobal( | |
| "window", | |
| { | |
| localStorage: { | |
| getItem: () => { | |
| throw new Error("blocked") | |
| }, | |
| }, | |
| } as unknown as Window, | |
| ) | |
| expect(safeGetItem("rein_auth_token")).toBeNull() | |
| }) | |
| it("returns false when localStorage access throws on write", () => { | |
| vi.stubGlobal( | |
| "window", | |
| { | |
| localStorage: { | |
| setItem: () => { | |
| throw new Error("blocked") | |
| }, | |
| }, | |
| } as unknown as Window, | |
| ) | |
| expect(safeSetItem("rein_auth_token", "token")).toBe(false) | |
| }) | |
| it("falls back when stored number is invalid", () => { | |
| vi.stubGlobal( | |
| "window", | |
| { | |
| localStorage: { | |
| getItem: () => "not-a-number", | |
| }, | |
| } as unknown as Window, | |
| ) | |
| expect(getStoredNumber("rein_sensitivity", 1)).toBe(1) | |
| }) | |
| it("falls back when stored boolean is malformed", () => { | |
| vi.stubGlobal( | |
| "window", | |
| { | |
| localStorage: { | |
| getItem: () => "{oops", | |
| }, | |
| } as unknown as Window, | |
| ) | |
| expect(getStoredBoolean("rein_invert", false)).toBe(false) | |
| }) | |
| it("supports JSON encoded booleans", () => { | |
| vi.stubGlobal( | |
| "window", | |
| { | |
| localStorage: { | |
| getItem: () => "true", | |
| }, | |
| } as unknown as Window, | |
| ) | |
| expect(getStoredBoolean("rein_invert", false)).toBe(true) | |
| }) | |
| it("returns null when window is unavailable", () => { | |
| vi.stubGlobal("window", undefined as unknown as Window) | |
| expect(safeGetItem("rein_auth_token")).toBeNull() | |
| }) | |
| it("falls back when number has trailing invalid characters", () => { | |
| vi.stubGlobal( | |
| "window", | |
| { | |
| localStorage: { | |
| getItem: () => "1.2abc", | |
| }, | |
| } as unknown as Window, | |
| ) | |
| expect(getStoredNumber("rein_sensitivity", 1)).toBe(1) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/storage.test.ts` around lines 14 - 81, Add two tests: one that
stubs absence of window (vi.stubGlobal("window", undefined)) to exercise the
no-window branch for safeGetItem/safeSetItem/getStoredNumber/getStoredBoolean
and assert they return their safe defaults (null/false/default number/default
boolean); and one that stubs localStorage.getItem to return a partially
malformed numeric string (e.g., "42abc") and assert
getStoredNumber("rein_sensitivity", 1) falls back or coerces as intended (match
the implementation's expected behavior); reference safeGetItem, safeSetItem, and
getStoredNumber/getStoredBoolean when adding these test cases.
Comment on lines
+22
to
+27
| export function getStoredNumber(key: string, fallback: number): number { | ||
| const saved = safeGetItem(key) | ||
| if (saved === null) return fallback | ||
|
|
||
| const parsed = Number.parseFloat(saved) | ||
| return Number.isFinite(parsed) ? parsed : fallback |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In JavaScript, what do Number.parseFloat("1abc") and Number("1abc") return, and which approach rejects trailing non-numeric characters?
💡 Result:
Number.parseFloat("1abc") returns 1, while Number("1abc") returns NaN. The Number approach rejects trailing non-numeric characters, whereas Number.parseFloat parses the leading numeric portion and ignores the rest.
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/parseFloat
- 2: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number
- 3: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/Number
- 4: http://developer.mozilla.org/en/Core_JavaScript_1.5_Reference/Global_Objects/Number
- 5: https://stackoverflow.com/questions/11988547/what-is-the-difference-between-number-and-parsefloat
- 6: https://stackoverflow.com/questions/46621185/what-is-the-critical-difference-between-number-parseint-number-parsefloat
Use strict number parsing to preserve malformed-value fallback.
Line 26 uses Number.parseFloat, which accepts partial values (e.g., "1abc" → 1). This conflicts with the malformed-value fallback requirement. Use Number() instead, which rejects strings with trailing non-numeric characters.
Proposed fix
export function getStoredNumber(key: string, fallback: number): number {
const saved = safeGetItem(key)
if (saved === null) return fallback
- const parsed = Number.parseFloat(saved)
+ const normalized = saved.trim()
+ if (normalized === "") return fallback
+ const parsed = Number(normalized)
return Number.isFinite(parsed) ? parsed : fallback
}📝 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.
Suggested change
| export function getStoredNumber(key: string, fallback: number): number { | |
| const saved = safeGetItem(key) | |
| if (saved === null) return fallback | |
| const parsed = Number.parseFloat(saved) | |
| return Number.isFinite(parsed) ? parsed : fallback | |
| export function getStoredNumber(key: string, fallback: number): number { | |
| const saved = safeGetItem(key) | |
| if (saved === null) return fallback | |
| const normalized = saved.trim() | |
| if (normalized === "") return fallback | |
| const parsed = Number(normalized) | |
| return Number.isFinite(parsed) ? parsed : fallback | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/storage.ts` around lines 22 - 27, The getStoredNumber function
currently uses Number.parseFloat which accepts partial numeric strings (e.g.,
"1abc") and thus bypasses the malformed-value fallback; change the parsing to
use Number(saved) to force strict conversion, keep the existing
Number.isFinite(parsed) check to return the fallback on invalid values, and
update the return path in getStoredNumber accordingly so only fully numeric
strings are accepted.
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.
Addressed Issues:
Fixes #331
Description
This PR fixes a client-side storage resilience issue in the app.
Previously, the app assumed
localStoragewas always available and always contained valid values. In restricted browser contexts such as private browsing, embedded webviews, or storage-blocked environments, these reads/writes could fail and break app initialization. There was also inconsistent handling of malformed stored values, especially for settings likerein_invertandrein_sensitivity.This PR introduces shared safe storage helpers and uses them across the affected client entry points so the app falls back to defaults instead of crashing.
Changes included:
localStorageaccess in settings initializationlocalStorageaccess in trackpad settings bootstraplocalStorageaccess in theme initializationlocalStorageaccess in auth token bootstrapScreenshots/Recordings:
Not applicable. This PR does not introduce a UI change.
Functional Verification
Screen Mirror
Authentication
Basic Gestures
One-finger tap: Verified as Left Click.
Two-finger tap: Verified as Right Click.
Click and drag: Verified selection behavior.
Pinch to zoom: Verified zoom functionality (if applicable).
Modes & Settings
Cursor mode: Cursor moves smoothly and accurately.
Scroll mode: Page scrolls as expected.
Sensitivity: Verified changes in cursor speed/sensitivity settings.
Copy and Paste: Verified both Copy and Paste functionality.
Invert Scrolling: Verified scroll direction toggles correctly.
Advanced Input
Key combinations: Verified "hold" behavior for modifiers (e.g., Ctrl+C) and held keys are shown in buffer.
Keyboard input: Verified Space, Backspace, and Enter keys work correctly.
Glide typing: Verified path drawing and text output.
Voice input: Verified speech-to-text functionality for full sentences.
Backspace doesn't send the previous input.
Any other gesture or input behavior introduced:
Verified safe fallback behavior for:
localStoragereadslocalStoragewritesAdditional Notes:
npm testfailed to start due to a missingwinstondependency in the current setupnpm run checkfailed becausebiomewas not available in the current setupChecklist
My PR addresses a single issue, fixes a single bug or makes a single improvement.
My code follows the project's code style and conventions
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
If applicable, I have made corresponding changes or additions to the documentation
If applicable, I have made corresponding changes or additions to tests
My changes generate no new warnings or errors
I have joined the and I will share a link to this PR with the project maintainers there
I have read the
Once I submit my PR, CodeRabbit AI will automatically review it and I will address CodeRabbit's comments.
Incase of UI change I've added a demo video.
Summary by CodeRabbit
Release Notes
Improvements
Tests