Repository navigation
fix: keep the pubky sign-in sheet open when a link cold-starts bitkit - #966
Conversation
On a cold start the same link reaches the app through onOpenURL and the scene delegate replay, and the startup gate can route the first delivery before the second arrives. Ignore a repeat of the last routed link for a few seconds, and hide the pubky approval sheet only for its own presentation so a replaced sheet cannot hide its successor.
The sheet content's onDisappear can fire when the auth check covers it. Hide on the sheet's onDismiss instead, for the presentation that last appeared.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
 [Medium impact] The changes reach deep-link routing and Pubky sign-in sheet presentation. The PR appears safe to merge, with a non-blocking concern about how the view chooses the dismissal ID. Findings
SummaryKeeps the Pubky sign-in sheet open when a cold-start link arrives twice.
Author Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[URL delivery] --> B[retainDeepLink]
B --> C{Same routed URL within 3 seconds?}
C -->|Yes| D[Ignore repeat]
C -->|No| E[Wait for startup gates]
E --> F[Route URL and record time]
F --> G[Show Pubky sign-in sheet]
G --> H[Dismiss using presentationID]
Reviews (1) · Last reviewed commit: "fix: keep the sign-in sheet open while i..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
A replacement sheet can appear before the replaced sheet's dismiss runs, so a shared last-shown ID could let that dismiss hide the replacement. Track shown presentations in order and let each dismiss consume the oldest.
|
Pushed 1dae42f: each sign-in sheet dismiss now consumes the oldest shown presentation instead of a shared last-shown ID, so the old sheet's dismiss can't hide its replacement even when the replacement appears first. Answers the review thread on |
talosmachina
left a comment
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Keeps the Pubky sign-in sheet open when a link cold-starts Bitkit by ignoring a duplicate delivery of an already routed link and by tying the approval sheet's dismiss to its own presentation. Reviewed 1dae42f, full tier (sign-in), reasoned from the code and CI; iOS does not build on this reviewer's host.
What I checked, and 3 candidates I ruled out
Read in full: SheetViewModel.swift, AppViewModel.swift (retainDeepLink, routePendingDeepLinkIfReady), the MainNavView.swift and PubkyAuthApprovalSheet.swift hunks, both test files
Call sites traced: retainDeepLink from AppScene (onOpenURL, the launch and handoff paths), hideDismissedSheet, markSheetShown
Ruled out
- A second, different link swallowed: the duplicate guard compares the full URL and a time window, so a new sign-in link inside the window still routes.
- A replaced sheet hiding its successor: dismissals pop presentations in the order they appeared and only hide when that presentation is still active, which the two new
SheetViewModelTestscases cover in both callback orders. - Dismiss before appear:
hideDismissedSheetreturns early when nothing was marked shown, so a dismiss with no recorded presentation is a no-op.
CI: unit and integration tests and build-local still running at review time.
Merge confidence: 4/5, the tests cover the change but CI had not finished at review time.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Code scan at 1dae42f: no findings. I reviewed it as an auth and deep-link change.
Checked:
retainDeepLink(AppViewModel.swift:233-241) ignores only an identical URL within 3 s of routing it (lastRoutedDeepLink, set at:257). A sign-in link with a differentsecretstill routes.MainNavViewexists only after PIN verification (AppScene.swift:864-872), so a link that arrives before unlock stays pending as before.- Dismissal is pinned to the sheet that was shown.
SheetConfiguration.presentationIDis a UUID per show.markSheetShowndedupes, so the PINfullScreenCoverre-appearing does not mark it twice.hideDismissedSheethides only the active presentation, and the interactive dismiss still nilsactiveSheetConfiguration. A stale queue entry can at most make the fallback a no-op; it never hides another sheet.showSheetnils before presenting, so there is no in-place swap with the same ID. - Approval uses the displayed sheet's
config.request(PubkyAuthApprovalSheet.swift:683-690). No TOCTOU is introduced. PubkyAuthURLSchemeTestscovers the duplicate ignored at +0.35 s and the same link routing after the window.SheetViewModelTestscovers both callback orders and the re-appear after PIN.- Not performance-relevant (a date comparison and a small array). No UI text changed.
Device gate (partial): 1dae42f — not run yet. Pending: journeys/pubky-auth/open-signin-link.xml on the simulator (cold start, sheet still open after 5 s, Cancel), plus the same link re-opened within 3 s (ignored) and after 3 s (sheet shows). The device result will follow.
|
Outside this PR's scope, no change needed here. The cold-start sign-in journey points the wrong way across repos. The description of |
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Approving at 1dae42f. The code scan from my earlier review stands, with no findings.
Device gate: 1dae42f — journeys/pubky-auth/open-signin-link.xml: 12/12 actions pass on the iPhone 17 Pro simulator (iOS 26.5), against a fresh profile created through Bitkit on staging.
- Cold start: the Authorize sheet appeared 4 s after
openurland was still up at +17 s. The second delivery 0.12 s later was dropped:Two more cold starts also kept the sheet.14:58:48.275 Received deeplink 14:58:48.391 Ignoring duplicate delivery of a routed deep link - Relay origin
https://httprelay.pubky.app, no Paykit access row. Cancel closes the sheet, and a duplicatesecretshows the invalid-request toast. - The same link re-opened within 3 s, while the sheet is open or after Cancel, is ignored, with one sheet at most. Re-opened after 5 s it shows the sheet each time.
perf: not relevant; l10n: no UI text changed.
3 participants
Closes #957
Refs:
Description
onOpenURLand through the scene delegate's replay. The startup gate can route the first delivery before the second arrives, so the link was handled twice and the second handling replaced the open sheet.AppViewModel.retainDeepLink: a repeat of the last routed link within 3 seconds is ignored. Both delivery paths stay, since the scene replay was added on purpose with the Pubky auth links (feat: open pubky auth links #722). Opening the same link again later still works.SheetViewModelkeeps shown presentations in order and each dismiss consumes the oldest (markSheetShown,hideDismissedSheet). The old check compared only the sheet ID, so a replaced sign-in sheet's late dismiss hid the new sign-in sheet.Out of Scope
Bitkit/MainNavView.swift: other sheets still dismiss by sheet ID. Only the sign-in sheet can be replaced by a sheet with the same ID through a repeated link.Design
N/A — no UI changes.
Preview
Five cold starts from the Shop staging sign-in link, no PIN (2x): each opens one Authorize sheet that is still open 18 s later; the app log shows the second delivery arriving 0.17–0.21 s after routing and being ignored each time.
cold-2x.mp4
With PIN on: Authorize opens the PIN check over the sheet; backing out of it leaves the sheet open (2x).
pin-2x.mp4
QA Notes
Journeys
open-signin-link.xml— the sheet is still open 5 s after a cold startManual Tests
Automated Checks
PubkyAuthURLSchemeTests.swift— a second delivery of a routed link routes once; the same link later routes againSheetViewModelTests.swift— a replaced sheet's dismiss keeps the newer sheet with the same ID, whether the newer sheet appears before or after that dismissPubkyContactLinkTests.swift,SceneDelegateTests.swift