Clips: "Open desktop app" button#2179
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Here's a visual recap of what changed: Open the full interactive recap |
![builder-io-integration[bot]](https://avatars.githubusercontent.com/in/353721?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Builder reviewed your changes and found 1 potential issue 🟡
Review Details
Incremental Code Review Summary
The latest commit adds a reactive downloaded-state subscription using useSyncExternalStore and handles cold-start deep links with get_current(). Those changes address the previously reported same-tab CTA staleness and native cold-start concerns. The previously reported dismissed-promo/download conflation remains present and was intentionally not reposted because its existing review thread is still open.
New Finding
- 🟡 MEDIUM — The deep-link path uses a partial
present_popoverhelper that only focuses and repositions the window. During an active recording, Clips intentionally parks the popover at 2×2 pixels; openingclips://then leaves that parked size intact, so “Open desktop app” can appear to do nothing. The deep-link path should reuse the canonical Clips popover presentation helper that restores the window and clears relevant parked/voice-wake state.
The subscription and cold-start implementations otherwise look appropriate, and the dev server is healthy. 🧪 Browser testing: Will run after this review (PR touches UI code).
| let urls: Vec<String> = | ||
| event.urls().iter().map(|u| u.to_string()).collect(); | ||
| dlog!("[clips-tray] deep link opened: {:?}", urls); | ||
| present_popover(&dl_handle); |
There was a problem hiding this comment.
🟡 Restore parked popover before presenting it from a deep link
This deep-link callback calls the new lightweight present_popover, which does not restore the popover from its deliberate 2×2 parked size during an active recording. A clips:// click can therefore focus an effectively invisible window; reuse the canonical Clips presentation path (for example clips::force_show_popover) that restores the parked window and clears the related state.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Builder reviewed your changes and found 1 potential issue 🟡
Review Details
Incremental Code Review Summary
The latest head is unchanged in the affected implementation from the prior incremental pass. The previously reported native deep-link presentation issue remains present and was intentionally not reposted because its review thread is still open; developer agreement is noted. The latest review confirms the browser-side state subscription is structurally sound, but found additional protocol-launch edge cases.
New Findings
- 🟡 MEDIUM — Any generic browser blur/visibility loss during the protocol-launch window is treated as proof that the app opened, which can suppress the fallback and persist an incorrect downloaded state when focus was lost for an unrelated reason.
- LOW — Promo dismissal does not notify the downloaded-state subscribers, leaving mounted CTAs stale in the same session.
- LOW — The 800 ms timeout can redirect while a browser’s external-protocol confirmation dialog is still awaiting user input.
A separate missed promo-bar label update was identified, but its call site is outside the current PR diff hunk and therefore cannot be safely annotated in this review. The useSyncExternalStore subscription and cold-start get_current() handling otherwise look correct. 🧪 Browser testing: Will run after this review (PR touches UI code).
| const onLeave = () => { | ||
| if (launched) return; | ||
| launched = true; | ||
| markDesktopAppDownloaded(); |
There was a problem hiding this comment.
🟡 Do not treat every browser blur as a successful app launch
onLeave marks the app as downloaded for any window.blur or visibility loss during the 800 ms window. Switching tabs/apps or an external-protocol confirmation dialog can cause the same signal without a successful handoff, suppressing the fallback and leaving users with an incorrect “Open desktop app” state.
2 participants
The "Download desktop app" button now says "Open desktop app" once a user has the app installed, and clicking it launches the app directly instead of taking them to the download page.
Button behavior: After downloading, the button skips the Chrome/desktop installer picker and goes straight to launching the app. If the app isn't found (e.g. on an older version that doesn't support the new URL scheme), it falls back to the download page so the user can update.
Existing users: Users who previously dismissed the download promo will also see "Open desktop app" right away, without needing to click through anything first.
Desktop app: The Tauri app now registers the
clips://URL scheme, so clicking "Open desktop app" in the browser can launch the running app directly.