fix: add defensive null checks for storage cleanup operations #4830
Conversation
- Created utils/storage.ts with safe storage access utilities - Added initializeStorage() to clean up null values on app load - Updated NavBar to use safe setStorageItem() function - Prevents 'Cannot read properties of null (reading length)' error Fixes asyncapi#4787
debasish5452v
requested review from
Mayaleeeee,
akshatnema,
anshgoyalevil,
asyncapi-bot-eve,
derberg,
princerajpoot20 and
sambhavgupta0705
as code owners
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughAdds a browser-safe storage utility (getStorageItem, setStorageItem, initializeStorage), calls initializeStorage once on app mount, replaces a direct localStorage write in NavBar.changeLanguage with setStorageItem, and narrows a ToolsCard useEffect dependency to run only on initial mount. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Browser
participant MyApp
participant NavBar
participant StorageUtil as StorageUtil
participant LocalStorage as localStorage
rect rgb(240,248,255)
Note over Browser,MyApp: App startup
Browser->>MyApp: load / mount
MyApp->>StorageUtil: initializeStorage()
StorageUtil->>LocalStorage: get keys & getItem(key)
alt value === null
StorageUtil->>LocalStorage: removeItem(key)
end
end
rect rgb(245,255,240)
Note over Browser,NavBar: User selects language
Browser->>NavBar: select language
NavBar->>StorageUtil: setStorageItem('lang', value)
StorageUtil->>LocalStorage: setItem('lang', value)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
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 |
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-4830--asyncapi-website.netlify.app/ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/navigation/NavBar.tsxpages/_app.tsxutils/storage.ts
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-29T14:21:43.887Z
Learnt from: sammy200-ui
Repo: asyncapi/website PR: 4804
File: components/navigation/Filter.tsx:32-41
Timestamp: 2025-12-29T14:21:43.887Z
Learning: In the Filter component (components/navigation/Filter.tsx), the useEffect hooks are intentionally designed to react only to route/URL changes, not to prop changes (data, checks, onFilter). The omitted dependencies are by design.
Applied to files:
pages/_app.tsx
📚 Learning: 2024-11-11T21:30:32.478Z
Learnt from: amanbhoria
Repo: asyncapi/website PR: 3373
File: components/AlgoliaSearch.tsx:313-313
Timestamp: 2024-11-11T21:30:32.478Z
Learning: In the `SearchButton` component within `components/AlgoliaSearch.tsx`, if the component re-renders on every button click and the `useEffect` runs accordingly, adding dependencies to the dependency array might not be necessary.
Applied to files:
pages/_app.tsxcomponents/navigation/NavBar.tsx
📚 Learning: 2025-12-29T14:21:28.216Z
Learnt from: sammy200-ui
Repo: asyncapi/website PR: 4804
File: components/navigation/Filter.tsx:32-41
Timestamp: 2025-12-29T14:21:28.216Z
Learning: In the asyncapi/website repository, when you intentionally omit dependencies in React hooks (e.g., useEffect, useMemo), add an eslint-disable comment with an explanatory note above the line to justify the design choice. For example: // eslint-disable-next-line react-hooks/exhaustive-deps: intentionally omitting dependencies to avoid unnecessary re-runs. This helps reviewers understand the rationale and keeps lint guidance informative.
Applied to files:
pages/_app.tsxcomponents/navigation/NavBar.tsx
🧬 Code graph analysis (2)
pages/_app.tsx (1)
utils/storage.ts (1)
initializeStorage(40-59)
components/navigation/NavBar.tsx (1)
utils/storage.ts (1)
setStorageItem(26-34)
🪛 GitHub Actions: PR testing - if Node project
pages/_app.tsx
[error] 26-26: Expected blank line before this statement. padding-line-between-statements
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Redirect rules - asyncapi-website
- GitHub Check: Header rules - asyncapi-website
- GitHub Check: Pages changed - asyncapi-website
- GitHub Check: Lighthouse CI
🔇 Additional comments (4)
utils/storage.ts (3)
7-19: LGTM!The function correctly handles browser vs. SSR environments, null values, and errors. The use of nullish coalescing (
??) is appropriate for returning the default value whengetItemreturnsnull.
26-34: LGTM!The function provides a clean wrapper around
localStorage.setItemwith appropriate error handling and browser environment checks.
40-59: This cleanup is defensive but may be unnecessary under normal localStorage behavior.The function removes keys where
getItem(key)returnsnull. However, under standard localStorage behavior, if a key exists inObject.keys(localStorage),getItem(key)should always return a string value—nullis only returned for keys that don't exist. The condition on line 52 should theoretically never be true.The code comment mentions preventing errors when "third-party code tries to access
.lengthon storage values," which suggests this is addressing external/environmental edge cases (possibly corrupted storage or browser extensions). However, without concrete evidence of this issue or a reference to issue #4787, this appears to be defensive programming that may not actually prevent the reported error.If third-party code is the source, consider whether the real fix should be:
- Validating/sanitizing all storage values before use (already done in
getStorageItemwith?? defaultValue)- Preventing direct access to raw localStorage outside the utility functions
- Providing more specific guards where
.lengthis accessedcomponents/navigation/NavBar.tsx (1)
8-8: LGTM! The migration to the safe storage wrapper is comprehensive.The replacement of direct
localStorage.setItemwith thesetStorageItemwrapper is correct. The wrapper properly handles SSR safety withtypeof windowchecks and includes error handling with logging.A codebase audit confirms no remaining direct localStorage usage exists outside the wrapper—the migration is complete and consistent across the codebase.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4830 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 22 22
Lines 798 798
Branches 146 146
=========================================
Hits 798 798 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/tools/ToolsCard.tsx
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-29T14:21:43.887Z
Learnt from: sammy200-ui
Repo: asyncapi/website PR: 4804
File: components/navigation/Filter.tsx:32-41
Timestamp: 2025-12-29T14:21:43.887Z
Learning: In the Filter component (components/navigation/Filter.tsx), the useEffect hooks are intentionally designed to react only to route/URL changes, not to prop changes (data, checks, onFilter). The omitted dependencies are by design.
Applied to files:
components/tools/ToolsCard.tsx
📚 Learning: 2024-11-11T21:30:32.478Z
Learnt from: amanbhoria
Repo: asyncapi/website PR: 3373
File: components/AlgoliaSearch.tsx:313-313
Timestamp: 2024-11-11T21:30:32.478Z
Learning: In the `SearchButton` component within `components/AlgoliaSearch.tsx`, if the component re-renders on every button click and the `useEffect` runs accordingly, adding dependencies to the dependency array might not be necessary.
Applied to files:
components/tools/ToolsCard.tsx
📚 Learning: 2025-12-29T14:21:28.216Z
Learnt from: sammy200-ui
Repo: asyncapi/website PR: 4804
File: components/navigation/Filter.tsx:32-41
Timestamp: 2025-12-29T14:21:28.216Z
Learning: In the asyncapi/website repository, when you intentionally omit dependencies in React hooks (e.g., useEffect, useMemo), add an eslint-disable comment with an explanatory note above the line to justify the design choice. For example: // eslint-disable-next-line react-hooks/exhaustive-deps: intentionally omitting dependencies to avoid unnecessary re-runs. This helps reviewers understand the rationale and keeps lint guidance informative.
Applied to files:
components/tools/ToolsCard.tsx
🪛 GitHub Actions: PR testing - if Node project
components/tools/ToolsCard.tsx
[error] 94-94: Prettier formatting issue detected. Replace the multi-line expression with a single-line form as required by prettier/prettier.
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Redirect rules - asyncapi-website
- GitHub Check: Header rules - asyncapi-website
- GitHub Check: Pages changed - asyncapi-website
🔇 Additional comments (1)
components/tools/ToolsCard.tsx (1)
31-35: LGTM! Correct fix for the dependency array.The empty dependency array is appropriate here. The truncation check measures DOM layout once after initial mount, and
descriptionRef.currentis mutable (doesn't trigger re-runs anyway). This resolves potential eslint warnings.
2 participants
Description
Fixes #4787
Added defensive null checks to prevent runtime errors when browser storage contains null values.
Changes
utils/storage.tswith safe storage access utilitiesinitializeStorage()function to clean up null values on app loadsetStorageItem()functionTesting
Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.