fix: proper localstorage usage#5382
Conversation
|
@SukkaW is attempting to deploy a commit to the NextChat Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe changes involve refactoring the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Your build has completed! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (4)
app/utils/indexedDB-storage.ts (2)
6-8: Approve the environment check but suggest a minor improvement.The addition of the environment check is a good practice to avoid errors in non-browser environments. However, consider logging or handling the scenario where
windowis undefined to aid in debugging or monitoring.
18-20: Approve the environment check and suggest a minor improvement.The environment check is consistent and prevents errors in non-browser environments. Consider adding a log statement or a way to monitor these skipped operations for better observability.
app/locales/index.ts (2)
85-87: Approve the environment check and suggest handling other errors.The addition of the environment check is a good practice to avoid errors in non-browser environments. Consider adding error handling for other potential localStorage issues to make the function more robust.
96-98: Approve the environment check and suggest handling other errors.The addition of the environment check is a good practice to avoid errors in non-browser environments. Consider adding error handling for other potential localStorage issues to make the function more robust.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
Files selected for processing (4)
- app/components/mask.tsx (3 hunks)
- app/locales/index.ts (2 hunks)
- app/utils/indexedDB-storage.ts (4 hunks)
- package.json (1 hunks)
Files skipped from review due to trivial changes (1)
- package.json
Additional comments not posted (3)
app/utils/indexedDB-storage.ts (2)
34-36: Approve the environment check.The addition of the environment check is consistent with other methods and helps prevent errors in non-browser environments.
45-47: Approve the environment check.The addition of the environment check is consistent with other methods and helps prevent errors in non-browser environments.
app/components/mask.tsx (1)
429-432: Approve the use ofuseLocalStorageand suggest verifying the impact of state change.The refactoring to use
useLocalStoragefor managingfilterLangstate simplifies the code and improves maintainability. However, changing the state fromundefinedtonullmay affect other parts of the application. Verify that all components and functions that depend onfilterLanghandle thenullvalue correctly.Run the following script to verify the impact of the state change:
Also applies to: 540-540
Verification successful
The change from
undefinedtonullforfilterLangis handled correctly.The instances of
filterLangin the codebase show thatnullis appropriately managed. The filter function checks for falsy values, and the UI component setsfilterLangtonullunder specific conditions, ensuring that the change does not introduce issues.
filterLangis used in a filter function with a condition that handlesnull.- The UI component sets
filterLangtonullwhen necessary.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all components and functions that depend on `filterLang` handle the `null` value correctly. # Test: Search for the usage of `filterLang`. Expect: Proper handling of `null` value. rg --type typescript -A 5 $'filterLang'Length of output: 73
Script:
#!/bin/bash # Description: Verify all components and functions that depend on `filterLang` handle the `null` value correctly. # Test: Search for the usage of `filterLang`. Expect: Proper handling of `null` value. rg --type ts -A 5 $'filterLang'Length of output: 1099
|
我编译的时候看到这个报错信息了 但是这个并不影响整体逻辑，只是判断好组件在服务端运行导致的报错问题。 并不想因为这个引入额外的库 |
I saw this error message when compiling However, this does not affect the overall logic, but only determines the error caused by the component running on the server. I don’t want to introduce additional libraries because of this |
The extra library is for |
|
鉴于前面提到的不想增加额外的包，这个pr里面应该，只有三个文件更改，不更改package.json以及yarn.lock |
Done in |
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (1)
app/utils.ts (1)
412-621: Enhance Storage Handling with Custom HooksThe
createStoragefunction and theuseStoragehook provide a robust mechanism for interacting withlocalStorageandsessionStorage. These utilities handle SSR scenarios gracefully by providing a fallback mechanism whenwindowis undefined, which aligns with the PR's objectives to fix issues related tolocalStorageaccess in non-browser environments.
- Pros: The code is modular and cleanly separates concerns between setting, getting, and removing storage items. It also dispatches custom events to handle storage changes, which is a good practice for reactive UI updates.
- Cons: The complexity of the code has increased, which might impact maintainability. The use of custom events could be over-engineering if not utilized elsewhere in the application.
Consider simplifying the implementation if the custom events are not a necessary part of the application's architecture. Additionally, ensure that the documentation is updated to reflect these changes and guide developers on how to use these new hooks effectively.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- app/components/mask.tsx (3 hunks)
- app/utils.ts (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/components/mask.tsx
Additional context used
Biome
app/utils.ts
[error] 392-392: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
| const useIsomorphicLayoutEffect = | ||
| typeof window !== "undefined" ? useLayoutEffect : useEffect; | ||
|
|
||
| const noop = () => {}; | ||
|
|
||
| const stlProp = Object.getOwnPropertyDescriptor(Error, "stackTraceLimit"); | ||
| const hasSTL = stlProp?.writable && typeof stlProp.value === "number"; | ||
| const noSSRError = ( | ||
| errorMessage?: string | undefined, | ||
| nextjsDigest = "BAILOUT_TO_CLIENT_SIDE_RENDERING", | ||
| ) => { | ||
| const originalStackTraceLimit = Error.stackTraceLimit; | ||
|
|
||
| /** | ||
| * This is *only* safe to do when we know that nothing at any point in the | ||
| * stack relies on the `Error.stack` property of the noSSRError. By removing | ||
| * the strack trace of the error, we can improve the performance of object | ||
| * creation by a lot. | ||
| */ | ||
| if (hasSTL) { | ||
| Error.stackTraceLimit = 0; | ||
| } | ||
|
|
||
| const error = new Error(errorMessage); | ||
|
|
||
| /** | ||
| * Restore the stack trace limit to its original value after the error has | ||
| * been created. | ||
| */ | ||
| if (hasSTL) { | ||
| Error.stackTraceLimit = originalStackTraceLimit; | ||
| } | ||
|
|
||
| // Next.js marks errors with `NEXT_DYNAMIC_NO_SSR_CODE` digest as recoverable: | ||
| // https://github.com/vercel/next.js/blob/bef716ad031591bdf94058aaf4b8d842e75900b5/packages/next/src/shared/lib/lazy-dynamic/bailout-to-csr.ts#L2 | ||
| (error as any).digest = nextjsDigest; | ||
|
|
||
| (error as any).recoverableError = "NO_SSR"; | ||
|
|
||
| return error; | ||
| }; |
There was a problem hiding this comment.
Refactor Error Handling for SSR Compatibility
The noSSRError function is designed to handle errors specifically for server-side rendering scenarios by manipulating the Error.stackTraceLimit. This approach is clever as it reduces the performance overhead of error handling by limiting the stack trace. However, this might obscure useful debugging information in some cases.
- Pros: Reduces overhead by limiting the stack trace.
- Cons: Potentially loses valuable debugging information.
Consider providing a configuration option to toggle this behavior based on the environment or the severity of the error, allowing developers more control over error reporting.
|
|
||
| // This type utility is only used for workaround https://github.com/microsoft/TypeScript/issues/37663 | ||
| const isFunction = (x: unknown): x is Function => typeof x === "function"; | ||
|
|
There was a problem hiding this comment.
Type Definition Issue: Avoid Using 'Function' as a Type
The use of Function as a type for the isFunction utility (line 392) is flagged by static analysis tools. Using Function can lead to maintenance issues and bugs because it is too generic.
- Pros: Quick and easy way to check for function types.
- Cons: Lacks specificity, which can lead to bugs if non-function values that are callable (like classes) are passed.
Consider replacing Function with a more specific function type definition that includes the expected arguments and return type, enhancing type safety and clarity.
- const isFunction = (x: unknown): x is Function => typeof x === 'function';
+ const isFunction = (x: unknown): x is (...args: any[]) => any => typeof x === 'function';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 isFunction = (x: unknown): x is Function => typeof x === "function"; | |
| const isFunction = (x: unknown): x is (...args: any[]) => any => typeof x === "function"; |
Tools
Biome
[error] 392-392: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
|
4 participants
💻 变更类型 | Change Type
🔀 变更说明 | Description of Change
I noticed a
ReferenceError: localStorage is not definederror in my CI. The PR adds extra guards againstlocalStorageaccess in the server/CI.Summary by CodeRabbit
New Features
foxact, enhancing application functionality.Improvements
Bug Fixes