fix(instance): refresh the QR code while the dialog is open - #34
Open
michumichifu wants to merge 1 commit into
Open
fix(instance): refresh the QR code while the dialog is open#34michumichifu wants to merge 1 commit into
michumichifu wants to merge 1 commit into
Conversation
The server rotates the pairing QR code every few seconds, but the dashboard called GET /instance/connect only once, when the dialog was opened. From then on the image on screen never changed, so anyone who did not scan within the first rotation was scanning a code the server had already discarded. The pairing then fails in a confusing way: the phone shows "logging in" and ends with "Could not log in. Check your phone's internet connection and scan the QR code again", which points the user at their network instead of at the stale code. Poll the endpoint every 10 seconds while the dialog is open. This does not open extra connections: once the instance is in 'connecting', connectToWhatsapp() returns the QR code already held in memory. The interval is cleared when the dialog closes, and the dialog closes itself as soon as the instance reports 'open'.
Reviewer's GuideAdds controlled dialog state and polling logic to keep the instance QR code refreshed while the pairing dialog is open, and automatically closes the dialog once the instance connects. Sequence diagram for refreshed instance QR code polling while dialog is opensequenceDiagram
actor User
participant DashboardInstance
participant Dialog
participant InstanceAPI
User->>Dialog: click DialogTrigger
Dialog-->>DashboardInstance: onOpenChange setQrDialogOpen(true)
DashboardInstance->>InstanceAPI: GET_instance_connect via handleConnect(instanceName, false)
InstanceAPI-->>DashboardInstance: qrCode
DashboardInstance->>DashboardInstance: setQRCode(qrCode)
DashboardInstance-->>Dialog: render with qrDialogOpen(true)
loop every QRCODE_REFRESH_INTERVAL_MS while qrDialogOpen and instance
DashboardInstance->>InstanceAPI: GET_instance_connect via handleConnect(instanceName, false)
InstanceAPI-->>DashboardInstance: qrCode
DashboardInstance->>DashboardInstance: setQRCode(qrCode)
end
InstanceAPI-->>DashboardInstance: instance.connectionStatus becomes open
DashboardInstance->>DashboardInstance: setQrDialogOpen(false)
DashboardInstance-->>Dialog: render with qrDialogOpen(false)
Dialog-->>User: close QR modal and stop polling
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The QR polling effect depends on
handleConnectbut doesn't include it in the dependency array; consider wrappinghandleConnectinuseCallbackand adding it as a dependency to avoid stale closures if its implementation changes. closeQRCodePopupresetsqrCodeandpairingCodebut does not updateqrDialogOpen, which can desynchronize the controlled dialog state when the popup is closed programmatically; consider callingsetQrDialogOpen(false)there as well.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The QR polling effect depends on `handleConnect` but doesn't include it in the dependency array; consider wrapping `handleConnect` in `useCallback` and adding it as a dependency to avoid stale closures if its implementation changes.
- `closeQRCodePopup` resets `qrCode` and `pairingCode` but does not update `qrDialogOpen`, which can desynchronize the controlled dialog state when the popup is closed programmatically; consider calling `setQrDialogOpen(false)` there as well.
## Individual Comments
### Comment 1
<location path="src/pages/instance/DashboardInstance/index.tsx" line_range="102-110" />
<code_context>
};
+ // Keep the displayed QR code in sync with the one the server is serving.
+ useEffect(() => {
+ if (!qrDialogOpen || !instance) return;
+
+ const intervalId = setInterval(() => {
+ handleConnect(instance.name, false);
+ }, QRCODE_REFRESH_INTERVAL_MS);
+
+ return () => clearInterval(intervalId);
+ }, [qrDialogOpen, instance?.name]);
+
+ // Stop refreshing as soon as the instance is connected.
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider including `handleConnect` in the effect dependencies or stabilizing it with `useCallback`.
Because this effect calls `handleConnect`, it should either be part of the dependency array or be memoized with `useCallback`. Otherwise, its identity (and logic) can change without retriggering the effect, leading to stale closures and failing the hooks lint rule. Please either memoize and include it in the deps, or explicitly justify its exclusion and suppress the lint here.
Suggested implementation:
```typescript
// Keep the displayed QR code in sync with the one the server is serving.
useEffect(() => {
if (!qrDialogOpen || !instance) return;
const intervalId = setInterval(() => {
handleConnect(instance.name, false);
}, QRCODE_REFRESH_INTERVAL_MS);
return () => clearInterval(intervalId);
}, [qrDialogOpen, instance?.name, handleConnect]);
```
To fully satisfy the hooks lint rule and avoid stale closures, you should also:
1. Ensure `handleConnect` is stabilized with `useCallback`, e.g. `const handleConnect = useCallback((name, shouldOpen = true) => { ... }, [/* its dependencies */]);`.
2. If `useCallback` is not yet imported in this file, add `useCallback` to the React import (or `import { useCallback } from "react";` depending on existing conventions).
3. If you intentionally do not want `handleConnect` in the dependency array, instead wrap the effect in an `// eslint-disable-next-line react-hooks/exhaustive-deps` comment and document why `handleConnect` is safe to exclude.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+102
to
+110
| useEffect(() => { | ||
| if (!qrDialogOpen || !instance) return; | ||
|
|
||
| const intervalId = setInterval(() => { | ||
| handleConnect(instance.name, false); | ||
| }, QRCODE_REFRESH_INTERVAL_MS); | ||
|
|
||
| return () => clearInterval(intervalId); | ||
| }, [qrDialogOpen, instance?.name]); |
There was a problem hiding this comment.
suggestion (bug_risk): Consider including handleConnect in the effect dependencies or stabilizing it with useCallback.
Because this effect calls handleConnect, it should either be part of the dependency array or be memoized with useCallback. Otherwise, its identity (and logic) can change without retriggering the effect, leading to stale closures and failing the hooks lint rule. Please either memoize and include it in the deps, or explicitly justify its exclusion and suppress the lint here.
Suggested implementation:
// Keep the displayed QR code in sync with the one the server is serving.
useEffect(() => {
if (!qrDialogOpen || !instance) return;
const intervalId = setInterval(() => {
handleConnect(instance.name, false);
}, QRCODE_REFRESH_INTERVAL_MS);
return () => clearInterval(intervalId);
}, [qrDialogOpen, instance?.name, handleConnect]);To fully satisfy the hooks lint rule and avoid stale closures, you should also:
- Ensure
handleConnectis stabilized withuseCallback, e.g.const handleConnect = useCallback((name, shouldOpen = true) => { ... }, [/* its dependencies */]);. - If
useCallbackis not yet imported in this file, adduseCallbackto the React import (orimport { useCallback } from "react";depending on existing conventions). - If you intentionally do not want
handleConnectin the dependency array, instead wrap the effect in an// eslint-disable-next-line react-hooks/exhaustive-depscomment and document whyhandleConnectis safe to exclude.
Author
|
Update: this is now running in production on our own deployment, built on top of Two things worth reporting from the real run. It works. The QR image now changes on its own while the dialog stays open, and pairing succeeded on the first scan of an instance that had been failing all afternoon. The stale-code problem was worse than the description suggests. While debugging an instance that would not pair, the server was already on its third code while the dialog still showed the first one: Every scan of that dialog was a scan of a code the server had discarded two rotations earlier, and the phone reported it as "Could not log in. Check your phone's internet connection" — which is why this went unnoticed for months on our side: the error message points at the phone, not at the code. The polling does not open extra connections, measured rather than assumed. On a throwaway instance sitting in Same socket throughout, which matches the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The dashboard requests a QR code once, when the dialog opens, and never again:
The server, however, keeps rotating the pairing code every few seconds. From the
second rotation onwards, the image on screen is a code the server has already
discarded, and there is nothing on the page telling the user that.
What the user sees when they scan a stale code is not a QR error. The phone shows
"logging in…" for a while and then:
So the failure is reported as a network problem on the phone, which sends people
looking in the wrong place. On our own deployment this made pairing feel
unreliable for months, across several instances.
Evidence
Server log for one pairing attempt, with the code counter already at 3 while the
dialog still displayed the first one:
Fix
Poll
GET /instance/connectevery 10 seconds while the dialog is open.This does not create extra connections. Once the instance is in
connecting,the endpoint returns the code currently held in memory and opens nothing:
Only the
closebranch starts a connection, and by the time the interval firesthe first time the instance has already moved to
connecting.The interval is cleared when the dialog closes, and a second effect closes the
dialog as soon as the instance reports
open, so polling stops on successinstead of running until the user dismisses the modal.
Notes
never more than one poll behind.
open/onOpenChange) because the effect needsto know whether it is visible.
npm run type-checkandeslintpass.prettier --checkreports this file asunformatted both before and after the change, so it was left untouched rather
than mixing a whole-file reformat into the diff.
Summary by Sourcery
Refresh the instance QR code periodically while the connection dialog is open to prevent users from scanning stale codes.
Bug Fixes:
Enhancements: