refactor: Use standard use-local-storage hook#930
Conversation
This commit replaces the custom `useLocalStorage` implementation
with the `@rm-hull/use-local-storage` package.
This migration allows for better management of serialization logic
directly within the hook implementation.
Key changes:
* The `useGeneralSettings` hook now returns an object
`{ settings, updateSettings }` instead of an array.
* The `useOtpParameters` hook utilizes the library's `Serializer`
interface to handle password-based encryption and decryption
of stored OTP data.
* Components consuming these hooks have been updated to use
object destructuring instead of array destructuring.
Summary of ChangesHello @rm-hull, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the application's local storage management by migrating from a custom Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Ensures that if `CryptoJS.AES.decrypt` fails (e.g., due to a bad password), we throw an explicit error message rather than relying on subsequent JSON parsing failure.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a great refactoring that replaces a custom useLocalStorage hook with the standard @rm-hull/use-local-storage library. This simplifies the codebase and improves maintainability by leveraging a dedicated library for local storage management. The changes are well-executed, including the introduction of a custom Encrypter serializer and updating consumer components to the new hook API. I've found one high-severity issue related to the handling of a potentially undefined password which could make the application fragile. My suggestion aims to make the implementation more robust.
| value: otpParams, | ||
| setValue: setOtpParams, | ||
| error, | ||
| } = useLocalStorage<OTP[]>("zaup2.otp-parameters", { serializer: new Encrypter(password!), initialValue: [] }); |
There was a problem hiding this comment.
Using the non-null assertion password! is risky. Although the application's structure with PasswordProtection currently seems to ensure password is defined when this hook is used, this creates a brittle dependency on the component hierarchy. A small change in the future could lead to a runtime crash if this hook is used where password is undefined.
A more robust approach is to make this hook self-contained and safe by using the disabled option from @rm-hull/use-local-storage when the password is not available. This will prevent any interaction with localStorage and avoid potential crashes.
| } = useLocalStorage<OTP[]>("zaup2.otp-parameters", { serializer: new Encrypter(password!), initialValue: [] }); | |
| } = useLocalStorage<OTP[]>("zaup2.otp-parameters", { serializer: password ? new Encrypter(password) : undefined, initialValue: [], disabled: !password }); |
Exposes the `isLoading` status from `useLocalStorage` via `useOtpParameters` to allow consumer components to handle initial data hydration states properly. Also integrates the global `Toaster` component for displaying notifications across the application.
Extract `Encrypter` logic into `CryptoJsSerializer` to clean up the `useOtpParameters` hook. This commit also introduces the `WebCryptoSerializer` utility which provides functions to decrypt data originally encrypted by CryptoJS using the browser's native `window.crypto` methods (AES-CBC + EVP_BytesToKey derivation). This prepares the codebase for potentially migrating away from `crypto-js`.
…storage * 'main' of github.com:rm-hull/zaup2: refactor: Improve alert structure and data flow (#931)
Introduces robust error handling for data retrieval and decryption:
* Displays an error toast notification if the OTP data fails to load
in the UI (`Group.tsx`).
* Catches decryption/parsing failures in `CryptoJsSerializer` and
throws an error that includes the original exception as the cause,
improving debugging capabilities.
Pull Request Test Coverage Report for Build 19000948021Details
💛 - Coveralls |
Refactors the `WebCryptoSerializer` to track password verification status. - Serialization is blocked if the password previously failed decryption checks. - Decryption failures now set the internal bad password state and throw a richer error. Also, removed local error handling (toasting) in `<Group />` component to ensure data loading errors are propagated via `throw error;` for centralized error boundary management.
The custom WebCrypto/CryptoJS based serializer was deleted. This file contained complex logic to ensure decryption compatibility with data originally encrypted using CryptoJS, including MD5-based `EVP_BytesToKey` derivation. This implementation is no longer required.
541e6a8 to
748cb27
Compare
use-local-storage hookuse-local-storage hook
This commit replaces the custom
useLocalStorageimplementation with the@rm-hull/use-local-storagepackage.This migration allows for better management of serialization logic directly within the hook implementation.
Key changes:
useGeneralSettingshook now returns an object{ settings, updateSettings }instead of an array.useOtpParametershook utilizes the library'sSerializerinterface to handle password-based encryption and decryption
of stored OTP data.
object destructuring instead of array destructuring.