-
Notifications
You must be signed in to change notification settings - Fork 112
fix: deep linking to app not working #214
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts the OpenClient effect logic to trigger the native app deep link based on an internal flag and tab visibility rather than directly on the openClient prop, ensuring the deep link fires correctly and only once per trigger. Sequence diagram for triggering native deep link from OpenClient effectsequenceDiagram
actor User
participant WebApp
participant OpenClientEffect
participant Browser
participant NativeApp
User->>WebApp: Click open_in_app_button
WebApp->>WebApp: Set openClient_prop true
WebApp->>OpenClientEffect: Render with openClient true
OpenClientEffect->>OpenClientEffect: prevOpenClientRef.current set to true
rect rgb(230,230,230)
OpenClientEffect->>Browser: Listen for tab visibility changes
end
Browser-->>OpenClientEffect: Tab becomes visible (isTabVisible true)
WebApp-->>OpenClientEffect: currentUser available
OpenClientEffect->>OpenClientEffect: Check isTabVisible && currentUser && prevOpenClientRef.current
OpenClientEffect->>Browser: window.open(appflowy_flutter_deep_link, _self)
Browser->>NativeApp: Open appflowy_flutter://open-page
NativeApp-->>User: Display requested workspace and view
OpenClientEffect->>OpenClientEffect: prevOpenClientRef.current = false (prevent repeat)
Flow diagram for updated OpenClient deep link effect logicflowchart TD
A[Effect triggered
dependencies:
currentWorkspaceId,
viewId,
currentUser,
openClient,
rowId,
isTabVisible] --> B{isTabVisible?}
B -->|No| H[Do nothing]
B -->|Yes| C{currentUser exists?}
C -->|No| H
C -->|Yes| D{prevOpenClientRef.current is true?}
D -->|No| H
D -->|Yes| E[Construct appflowy_flutter deep link
with workspace_id,
view_id,
email,
optional row_id]
E --> F["window.open(deep_link, _self)"]
F --> G[Set prevOpenClientRef.current = false
to prevent repeat trigger]
G --> H
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've found 1 issue, and left some high level feedback:
- The effect dependency array still includes
openClient, but the variable is no longer used inside the effect; consider removing it from the dependency list to avoid unnecessary re-renders or confusion. - Previously
prevOpenClientRefwas explicitly synchronized withopenClient, but now it’s only set tofalse; if this ref is still intended to reflectopenClientstate, you may want to reintroduce that synchronization or rename the ref to better reflect its new purpose.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The effect dependency array still includes `openClient`, but the variable is no longer used inside the effect; consider removing it from the dependency list to avoid unnecessary re-renders or confusion.
- Previously `prevOpenClientRef` was explicitly synchronized with `openClient`, but now it’s only set to `false`; if this ref is still intended to reflect `openClient` state, you may want to reintroduce that synchronization or rename the ref to better reflect its new purpose.
## Individual Comments
### Comment 1
<location> `src/components/app/components/AppContextConsumer.tsx:90` </location>
<code_context>
}
-
- prevOpenClientRef.current = openClient;
}, [currentWorkspaceId, viewId, currentUser, openClient, rowId, isTabVisible]);
return <></>;
</code_context>
<issue_to_address>
**suggestion:** `openClient` is still in the dependency array but no longer used in the effect body.
Since `openClient` is no longer used in the effect, keeping it in the dependency array causes avoidable re-renders and makes the dependencies misleading. Please either remove it from the array or reintroduce it into the effect if it’s still intended to influence this logic.
```suggestion
}, [currentWorkspaceId, viewId, currentUser, rowId, isTabVisible]);
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
c814c72 to
84b69ac
Compare
Description
Checklist
General
Testing
Feature-Specific
Summary by Sourcery
Bug Fixes: