デバイスカード内の値を保持する - #47
Hidden character warning
Conversation
|
@codex review |
Deploying switchbotcontroller with
|
| Latest commit: |
6982181
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://31c993ec.switchbotcontroller.pages.dev |
| Branch Preview URL: | https://34.switchbotcontroller.pages.dev |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6982181add
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const nextValue = resolveInitialValue(storedValue, defaultValueRef.current); | ||
| latestStateRef.current = nextValue; | ||
| lastPersistedRef.current = nextValue; | ||
| hydratedRef.current = true; | ||
| setState(nextValue); |
There was a problem hiding this comment.
Hydration unconditionally applies the stored/default draft after electronStore.get returns, so if a user changes a control before that async read completes, their new value is overwritten by nextValue. On slower IPC/disk paths this causes visible snap-back to stale values right after opening a device card; guard this update so hydration does not clobber state that has already been edited locally.
Useful? React with 👍 / 👎.
| const valueToPersist = latestStateRef.current; | ||
| lastPersistedRef.current = valueToPersist; | ||
| void persistDeviceControlState(storageKey, valueToPersist); |
There was a problem hiding this comment.
The debounce path updates lastPersistedRef before attempting the async store write, so a failed electronStore.set is still treated as persisted and later effects skip retries because state appears in sync. In environments with transient write failures (e.g., permission/disk issues), this silently drops the latest draft until the user makes another change.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to persist device control states as drafts using a new usePersistedDeviceControlState hook. The hook is integrated into various control components and uses an Electron store with a 250ms debounce for updates. Feedback points out that using JSON.stringify for equality checks is inefficient and potentially fragile, and that resetting state to defaults during hydration causes UI flickering. Additionally, the reliability of persisting data in the useEffect cleanup function was questioned.
| const isEqualValue = (left: unknown, right: unknown) => | ||
| JSON.stringify(left) === JSON.stringify(right); |
There was a problem hiding this comment.
Using JSON.stringify for deep equality checks is generally considered an anti-pattern in JavaScript/TypeScript. It is computationally expensive and fragile because property order in objects is not guaranteed to be stable (though it often is in modern engines). If the order of keys in the persisted object differs from the default object, this check will fail, leading to unnecessary writes to the store. Consider using a simple shallow equality check for flat objects or a dedicated deep equality utility.
| useEffect(() => { | ||
| let cancelled = false; | ||
| hydratedRef.current = false; | ||
| setState(defaultValueRef.current); |
There was a problem hiding this comment.
Calling setState(defaultValueRef.current) immediately when the storageKey changes causes the UI to reset to default values before the persisted state is loaded from the store. This results in a visible "flash" of default values (e.g., a slider jumping from 50% to 80%). Consider maintaining the previous state until the new state is hydrated, or exposing a hydrated flag from the hook so the component can show a loading state or skeleton.
| if (isEqualValue(latestStateRef.current, lastPersistedRef.current)) return; | ||
| const valueToPersist = latestStateRef.current; | ||
| lastPersistedRef.current = valueToPersist; | ||
| void persistDeviceControlState(keyAtMount, valueToPersist); |
There was a problem hiding this comment.
Executing an asynchronous operation (persistDeviceControlState) inside a useEffect cleanup function is risky. While it works for component navigation, if the application is shutting down, there is no guarantee that the IPC call will complete before the process terminates. Since this is for "draft" values, the impact is low, but it's worth noting that critical data should be persisted more reliably.
1 participant
No description provided.