feat: centralize Zustand persistence middleware + add ErrorBoundary components#1321
Conversation
- Create shared persistence config (createPersistConfig) in store/persistence.js with centralized localStorage key map, quota error handling, and graceful degradation when storage is unavailable - Migrate all stores (auth, admin, ticket) to use createPersistConfig instead of inline persist() options - Add ErrorBoundary component with dark-themed fallback UI and diagnostic copy-to-clipboard utility for rapid bug reporting - Wrap root App in ErrorBoundary for global error catching
|
@zhengxing888 is attempting to deploy a commit to the ritesh Team on Vercel. A member of the Team first needs to authorize it. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces centralized persistence configuration for Zustand stores and adds a React error boundary with diagnostic export. A new shared ChangesCentralized Persistence and Error Handling
Sequence Diagram(s)sequenceDiagram
participant React
participant ErrorBoundary
participant Clipboard
React->>ErrorBoundary: render error occurs
ErrorBoundary->>ErrorBoundary: getDerivedStateFromError / componentDidCatch
ErrorBoundary->>ErrorBoundary: update state (hasError, error, errorInfo)
ErrorBoundary->>ErrorBoundary: invoke onError callback (optional)
ErrorBoundary->>ErrorBoundary: render fallback UI
Note over ErrorBoundary: User clicks "Copy Diagnostics"
ErrorBoundary->>ErrorBoundary: buildDiagnosticPayload (time, URL, error details, stack)
ErrorBoundary->>Clipboard: navigator.clipboard.writeText (JSON)
Clipboard-->>ErrorBoundary: success / fallback textarea method
ErrorBoundary->>ErrorBoundary: set copied flag, reset after timeout
Note over ErrorBoundary: User clicks "Try Again"
ErrorBoundary->>ErrorBoundary: handleReset clears error state
ErrorBoundary->>React: render props.children
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
Frontend/src/admin/store/adminStore.js (1)
33-35: 💤 Low valueRemove redundant
nameoverride foradmin-settingspersistence.
PERSISTENCE_KEYS['admin-settings']already maps to'admin-storage-settings', so the explicitname: 'admin-storage-settings'override is redundant and doesn’t change the persisted key when removed.♻️ Drop the redundant override
- createPersistConfig('admin-settings', { - name: 'admin-storage-settings', - }) + createPersistConfig('admin-settings')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Frontend/src/admin/store/adminStore.js` around lines 33 - 35, The createPersistConfig call for 'admin-settings' is redundantly passing name: 'admin-storage-settings' even though PERSISTENCE_KEYS['admin-settings'] already resolves to 'admin-storage-settings'; remove the explicit name override from the createPersistConfig('admin-settings', { ... }) invocation and let the mapping in PERSISTENCE_KEYS supply the persisted name (locate the call to createPersistConfig in adminStore.js and adjust the options object to drop the name property).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Frontend/src/components/ErrorBoundary.jsx`:
- Around line 80-86: The fallback prop is receiving an unbound reference to
buildDiagnosticPayload which loses `this` and throws; fix by converting the
instance method buildDiagnosticPayload into an arrow class field (so it retains
`this`) or, alternatively, pass a bound function at the call site (e.g., provide
() => this.buildDiagnosticPayload()) when calling this.props.fallback in the
ErrorBoundary render path alongside handleReset to ensure the third argument is
always invoked with the correct instance context; update the ErrorBoundary class
method definition (buildDiagnosticPayload) or the this.props.fallback invocation
accordingly.
- Line 103: The component ErrorBoundary.jsx currently uses process.env.NODE_ENV
in the JSX conditional; replace that with Vite's build-time env flag by changing
the dev-only gate to use import.meta.env.DEV (e.g., change the condition around
the JSX fragment that reads process.env.NODE_ENV !== 'production' to
import.meta.env.DEV) so the check is statically replaced by Vite; update any
related imports/usages in ErrorBoundary.jsx to reference import.meta.env.DEV and
remove reliance on process.env.NODE_ENV.
In `@Frontend/src/store/authStore.js`:
- Around line 293-301: The persist() call is given a malformed options argument:
instead of passing the object returned by createPersistConfig('auth', {
partialize: (state) => ({ user: state.user, profile: state.profile }) }), the
code wraps that call incorrectly causing unbalanced parentheses and a syntax
error; fix by passing createPersistConfig(...) directly as the second argument
to persist (i.e., call persist(store, createPersistConfig('auth', { partialize:
(state) => ({ user: state.user, profile: state.profile }) }))) so the partialize
and auth key are preserved and parentheses are balanced.
In `@Frontend/src/store/persistence.js`:
- Around line 72-78: The loop that purges non-critical localStorage entries uses
forward iteration and calls localStorage.removeItem inside the loop, which
causes skipped keys; change the loop in the quota cleanup section to iterate
backward (decrementing i) or call the existing helper clearNonCriticalStorage
instead so removals don't shift remaining keys and all non-critical entries
(based on PERSISTENCE_KEYS) are reliably removed; update the loop surrounding
PERSISTENCE_KEYS and localStorage.key/localStorage.removeItem to use a reverse
for loop or delegate to clearNonCriticalStorage to fix the bug.
---
Nitpick comments:
In `@Frontend/src/admin/store/adminStore.js`:
- Around line 33-35: The createPersistConfig call for 'admin-settings' is
redundantly passing name: 'admin-storage-settings' even though
PERSISTENCE_KEYS['admin-settings'] already resolves to 'admin-storage-settings';
remove the explicit name override from the createPersistConfig('admin-settings',
{ ... }) invocation and let the mapping in PERSISTENCE_KEYS supply the persisted
name (locate the call to createPersistConfig in adminStore.js and adjust the
options object to drop the name property).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f56b59ce-d844-4efc-a83c-5d0d4ae1fbdd
📒 Files selected for processing (7)
Frontend/src/admin/store/adminStore.jsFrontend/src/components/ErrorBoundary.jsxFrontend/src/main.jsxFrontend/src/store/adminStore.jsFrontend/src/store/authStore.jsFrontend/src/store/persistence.jsFrontend/src/store/ticketStore.js
| if (this.props.fallback) { | ||
| return this.props.fallback( | ||
| this.state.error, | ||
| this.handleReset, | ||
| this.buildDiagnosticPayload | ||
| ); | ||
| } |
There was a problem hiding this comment.
Unbound buildDiagnosticPayload reference breaks the fallback API.
buildDiagnosticPayload is a plain instance method, so passing this.buildDiagnosticPayload to props.fallback loses its this binding. When a consumer calls the third argument standalone, this.state is undefined and it throws. Internal callers work only because they invoke this.buildDiagnosticPayload() directly.
🐛 Proposed fix (convert to arrow field)
- buildDiagnosticPayload() {
+ buildDiagnosticPayload = () => {
const { error, errorInfo } = this.state;
return {Or pass a bound reference at the call site:
return this.props.fallback(
this.state.error,
this.handleReset,
- this.buildDiagnosticPayload
+ this.buildDiagnosticPayload.bind(this)
);📝 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.
| if (this.props.fallback) { | |
| return this.props.fallback( | |
| this.state.error, | |
| this.handleReset, | |
| this.buildDiagnosticPayload | |
| ); | |
| } | |
| if (this.props.fallback) { | |
| return this.props.fallback( | |
| this.state.error, | |
| this.handleReset, | |
| this.buildDiagnosticPayload.bind(this) | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Frontend/src/components/ErrorBoundary.jsx` around lines 80 - 86, The fallback
prop is receiving an unbound reference to buildDiagnosticPayload which loses
`this` and throws; fix by converting the instance method buildDiagnosticPayload
into an arrow class field (so it retains `this`) or, alternatively, pass a bound
function at the call site (e.g., provide () => this.buildDiagnosticPayload())
when calling this.props.fallback in the ErrorBoundary render path alongside
handleReset to ensure the third argument is always invoked with the correct
instance context; update the ErrorBoundary class method definition
(buildDiagnosticPayload) or the this.props.fallback invocation accordingly.
| </p> | ||
|
|
||
| {/* Error detail for dev — collapsible */} | ||
| {process.env.NODE_ENV !== 'production' && ( |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Vite statically replace process.env.NODE_ENV in client code by default, and what is the recommended import.meta.env equivalent?
💡 Result:
Yes—Vite statically replaces environment-related conditionals for the browser bundle at build time, via the special import.meta.env object (not via process.env in client code). Vite’s docs explain that constants exposed on import.meta.env are “statically replaced at build time” for better tree-shaking [1]. For the recommended equivalent to process.env.NODE_ENV in client code, use the built-in import.meta.env flags: - import.meta.env.PROD (boolean) - import.meta.env.DEV (boolean, the opposite of PROD) - import.meta.env.MODE (string) These are documented as built-in variables available in all cases [2]. Vite also explicitly notes that NODE_ENV (process.env.NODE_ENV) and modes are different concepts, and it maps mode values to import.meta.env.PROD/DEV/MODE depending on how you run/build [1][2].
Citations:
Prefer Vite import.meta.env for the dev-only gate
Vite’s idiomatic and build-time constants for client code are import.meta.env.DEV / import.meta.env.PROD (statically replaced via import.meta.env). Using process.env.NODE_ENV is not the recommended Vite mechanism and may be undefined in the browser unless you configure a define/polyfill.
♻️ Proposed change
- {process.env.NODE_ENV !== 'production' && (
+ {import.meta.env.DEV && (📝 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.
| {process.env.NODE_ENV !== 'production' && ( | |
| {import.meta.env.DEV && ( |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Frontend/src/components/ErrorBoundary.jsx` at line 103, The component
ErrorBoundary.jsx currently uses process.env.NODE_ENV in the JSX conditional;
replace that with Vite's build-time env flag by changing the dev-only gate to
use import.meta.env.DEV (e.g., change the condition around the JSX fragment that
reads process.env.NODE_ENV !== 'production' to import.meta.env.DEV) so the check
is statically replaced by Vite; update any related imports/usages in
ErrorBoundary.jsx to reference import.meta.env.DEV and remove reliance on
process.env.NODE_ENV.
| { | ||
| name: 'auth-storage', | ||
| partialize: (state) => ({ | ||
| // We keep profile persisted for quick UI transitions, | ||
| // but session is handled by Supabase cookie/localStorage | ||
| profile: state.profile | ||
| }), | ||
| } | ||
| ) | ||
| ); | ||
| createPersistConfig('auth', { | ||
| partialize: (state) => ({ | ||
| user: state.user, | ||
| profile: state.profile, | ||
| }), | ||
| }) | ||
| ) | ||
| ); |
There was a problem hiding this comment.
Syntax error: malformed persist() options argument.
createPersistConfig already returns the full persist options object, so it must be passed directly as the second argument to persist(). Wrapping it in an object literal { createPersistConfig(...) } is invalid syntax, and the trailing )/); are unbalanced. This file will not parse (confirmed by the static analyzer), breaking the entire build.
🐛 Proposed fix
- }),
- {
- createPersistConfig('auth', {
- partialize: (state) => ({
- user: state.user,
- profile: state.profile,
- }),
- })
- )
- );
+ }),
+ createPersistConfig('auth', {
+ partialize: (state) => ({
+ user: state.user,
+ profile: state.profile,
+ }),
+ })
+ )
+);🧰 Tools
🪛 Biome (2.4.16)
[error] 294-294: Expected a parameter but instead found ''auth''.
(parse)
[error] 295-295: Expected an identifier, an array pattern, or an object pattern but instead found '('.
(parse)
[error] 295-295: expected , but instead found )
(parse)
[error] 295-295: Expected a function body but instead found '=>'.
(parse)
[error] 296-296: expected , but instead found user
(parse)
[error] 299-299: Expected an expression but instead found '}'.
(parse)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Frontend/src/store/authStore.js` around lines 293 - 301, The persist() call
is given a malformed options argument: instead of passing the object returned by
createPersistConfig('auth', { partialize: (state) => ({ user: state.user,
profile: state.profile }) }), the code wraps that call incorrectly causing
unbalanced parentheses and a syntax error; fix by passing
createPersistConfig(...) directly as the second argument to persist (i.e., call
persist(store, createPersistConfig('auth', { partialize: (state) => ({ user:
state.user, profile: state.profile }) }))) so the partialize and auth key are
preserved and parentheses are balanced.
| const critical = Object.values(PERSISTENCE_KEYS); | ||
| for (let i = 0; i < localStorage.length; i++) { | ||
| const key = localStorage.key(i); | ||
| if (!critical.includes(key)) { | ||
| localStorage.removeItem(key); | ||
| } | ||
| } |
There was a problem hiding this comment.
Quota cleanup loop skips entries due to forward iteration during mutation.
localStorage.removeItem shrinks the collection and reindexes remaining keys, so advancing i forward skips every entry immediately after a removed one. This leaves much of the non-critical data in place, defeating the quota-recovery path that is the centerpiece of this change. Note that clearNonCriticalStorage (lines 107-112) already iterates backward correctly.
🐛 Iterate backward (or delegate to the existing helper)
const critical = Object.values(PERSISTENCE_KEYS);
- for (let i = 0; i < localStorage.length; i++) {
- const key = localStorage.key(i);
- if (!critical.includes(key)) {
- localStorage.removeItem(key);
- }
- }
+ for (let i = localStorage.length - 1; i >= 0; i--) {
+ const key = localStorage.key(i);
+ if (!critical.includes(key)) {
+ localStorage.removeItem(key);
+ }
+ }📝 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.
| const critical = Object.values(PERSISTENCE_KEYS); | |
| for (let i = 0; i < localStorage.length; i++) { | |
| const key = localStorage.key(i); | |
| if (!critical.includes(key)) { | |
| localStorage.removeItem(key); | |
| } | |
| } | |
| const critical = Object.values(PERSISTENCE_KEYS); | |
| for (let i = localStorage.length - 1; i >= 0; i--) { | |
| const key = localStorage.key(i); | |
| if (!critical.includes(key)) { | |
| localStorage.removeItem(key); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Frontend/src/store/persistence.js` around lines 72 - 78, The loop that purges
non-critical localStorage entries uses forward iteration and calls
localStorage.removeItem inside the loop, which causes skipped keys; change the
loop in the quota cleanup section to iterate backward (decrementing i) or call
the existing helper clearNonCriticalStorage instead so removals don't shift
remaining keys and all non-critical entries (based on PERSISTENCE_KEYS) are
reliably removed; update the loop surrounding PERSISTENCE_KEYS and
localStorage.key/localStorage.removeItem to use a reverse for loop or delegate
to clearNonCriticalStorage to fix the bug.
ritesh-1918
added
gssoc
|
Hi @zhengxing888! 🙌 Thank you so much for your excellent contribution: "feat: centralize Zustand persistence middleware + add ErrorBoundary components"! We really appreciate the high-quality code and effort you have put into the platform. Just a quick, friendly heads-up as we prepare our manual merging and verification queues—please make sure to complete all the mandatory community steps listed below. ⭐ Leaderboard tip: You are already following our project admin @ritesh-1918 and fully registered in our developer network! Your S-Tier point credentials are fully cleared for manual approval! 🙌 Once those manual steps are verified, we'll get your PR officially merged into the Let's build something amazing together! 🚀🔥 🌟 Project Support & Developer Network (Show Some Love!)As we prepare our manual verification and merging queues, please take a quick moment to ensure you have completed all four community steps:
Note: Having all four steps completed manually is required before your PR points are officially cleared. |
|
Superb implementation, @zhengxing888! I've successfully resolved all conflicts in your PR and queued it for merging into
Keep up the outstanding work! Let's build together! 🔥 |
2 participants
Changes
1. Centralized Persistence Middleware
store/persistence.js— single source of truth for localStorage keyspersist()calls with error handling (quota exceeded, storage unavailable)2. Error Boundary Component
components/ErrorBoundary.jsx— global error catcher with dark-themed UI3. Store Migration
Updated all stores to use the centralized config
Closes #1171
Summary by CodeRabbit