Fix Naver/Daum login not syncing back to the in-app WebView - #95
Conversation
Login popups were opened in the system default browser via NSWorkspace, which stores cookies in a completely separate data store from the app's WKWebView. Logging in there never reflected in the app. Popups are now presented in an in-app window using the same WKWebViewConfiguration WebKit hands to createWebViewWith(_:for:), so the login session lands in the same cookie store as the main dictionary view. The main view reloads once the popup closes.
![typeey[bot]](https://avatars.githubusercontent.com/in/3879190?s=60&v=4){kind=small}
There was a problem hiding this comment.
The popup lifecycle and routing policy address the previous delegate-state and target=_blank regressions cleanly. Verification passed at 0ac7cc0, including lint, dependency setup, generation, build, and the test suite.
The real-account and password-manager scenarios remain appropriately called out as manual follow-up checks.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Allkdic/Views/DictionaryWebView.swift">
<violation number="1" location="Allkdic/Views/DictionaryWebView.swift:184">
P3: `needsSessionReload` is set unconditionally in the popup's `onClose`, regardless of whether the user actually logged in. Because the `.transient` popover auto-closes the moment the login window takes focus, this then triggers a full `webView.reload()` on the next popover open for every login-popup interaction — including a user who opens the login window and immediately closes it without authenticating. That reload discards the dictionary page's scroll/search state needlessly. Consider only scheduling the reload when a login actually completed.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| let controller = LoginPopupController(configuration: configuration, title: self.dictionary.title) | ||
| controller.onClose = { [weak self] in | ||
| self?.loginPopupController = nil | ||
| self?.needsSessionReload = true |
There was a problem hiding this comment.
P3: needsSessionReload is set unconditionally in the popup's onClose, regardless of whether the user actually logged in. Because the .transient popover auto-closes the moment the login window takes focus, this then triggers a full webView.reload() on the next popover open for every login-popup interaction — including a user who opens the login window and immediately closes it without authenticating. That reload discards the dictionary page's scroll/search state needlessly. Consider only scheduling the reload when a login actually completed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Allkdic/Views/DictionaryWebView.swift, line 184:
<comment>`needsSessionReload` is set unconditionally in the popup's `onClose`, regardless of whether the user actually logged in. Because the `.transient` popover auto-closes the moment the login window takes focus, this then triggers a full `webView.reload()` on the next popover open for every login-popup interaction — including a user who opens the login window and immediately closes it without authenticating. That reload discards the dictionary page's scroll/search state needlessly. Consider only scheduling the reload when a login actually completed.</comment>
<file context>
@@ -133,14 +148,45 @@ private struct WebView: NSViewRepresentable {
+ let controller = LoginPopupController(configuration: configuration, title: self.dictionary.title)
+ controller.onClose = { [weak self] in
+ self?.loginPopupController = nil
+ self?.needsSessionReload = true
+ }
+ controller.show()
</file context>
There was a problem hiding this comment.
Not changing this one, deliberately — the alternatives are less reliable than the cost they save.
To make the reload conditional we would need to detect that a login actually succeeded, and every available signal is guesswork:
- Cookie diffing — opening the login page mutates CSRF/tracking/session-temp cookies even when the user cancels, so this produces false positives.
- Known session cookie names (
NID_AUT/NID_SES,_kawlt) — undocumented, provider-private, and free to change without notice. - Closed via
window.close()vs. closed by the user — the closest thing to a real signal, but flows that redirect the opener instead of self-closing, or that self-close on failure, break it in both directions.
The failure modes are not symmetric. A needless reload costs the user their scroll position and current search. A missed reload silently leaves the app logged out — which is precisely the bug this PR exists to fix, and it would fail in exactly the hard-to-reproduce way that made the original report tricky.
Worth noting the reload is already deferred to the next popover open rather than firing immediately, so it does not interrupt anything mid-interaction, and the popup only ever appears because the user deliberately clicked a login button.
Happy to revisit if the cancel-then-reopen path turns out to be common in practice, but I would rather not trade a correctness guarantee for a state-preservation nicety on a heuristic.
The tuist tap's cask declares `conflicts_with formula:`, a key Homebrew 6 removed, so evaluating the tap aborts and every `tuist` lookup through it fails. Any CI run now dies at the install step. mise is what Tuist's CI docs recommend, and pinning the version in mise.toml keeps CI and local development on the same toolchain. See tuist/homebrew-tuist#41.
Dictionary pages use window.open for two unrelated things: login, which must stay in the app to share its cookie store, and target=_blank content links, which belong in the default browser per #66. Telling them apart needs a rule, and the rule needs to be testable. Matching is on exact hosts. A suffix match would let ad and content hosts under naver.com or daum.net into a window that shares logged-in cookies. Blank popups are the one exception, since window.open("", name, features) is navigated only after the window exists; those are accepted only when the page opening them is a dictionary page. Modelled as plain values rather than WKNavigationAction because WebKit's action objects cannot be constructed in a test.
The popup web view reused the dictionary Coordinator as its navigation and UI delegate, so popup navigations drove the main view: they flipped its loading indicator, injected the dictionary's custom CSS into the login page and ran its input-focusing script against it. LoginPopupController now owns the popup window, its web view and its own delegates, and the Coordinator routes each popup by the policy instead of sending them all to one place. The main view reloads on the next popover open rather than immediately, so the focus script no longer runs against a hidden popover. That also fixes reloadIfNeeded() never restoring lastLoadedURL, which made the view reload on every popover open after a web content process crash.
The local key monitor is installed once and never removed, so it swallowed Escape for every window in the app. The login window made that visible: Escape could not close it, and the app has no menu bar, so there is no Cmd+W either. Also reset the status item icon when the popover closes by itself, which is what happens when the login window takes focus away from a transient popover.
0ac7cc0 to
a3f8d99
Compare
2 participants
Summary
This continues #94 by @DusanBaek, which could not be merged as-is: CI was red and two reviewers (typeey, cubic) requested changes. The original fix is cherry-picked here with authorship preserved (
337dced), and four commits are added on top to fix CI and address the review feedback. #94 will be closed in favor of this PR.The fix (
337dced, unchanged from #94): login popups (Naver, Daum) were opened viaNSWorkspace.shared.openin the system browser, which has a cookie store separate from the app'sWKWebView— so logging in there never reached the app. Popups now open in an in-appNSWindowbuilt from theWKWebViewConfigurationWebKit passes tocreateWebViewWith, sharing the cookie store with the main dictionary view.What's new on top of #94
CI was broken independently of this change
brew tap tuist/tuist && brew install --formula tuistnow fails: the tap's cask declaresconflicts_with formula:, a key Homebrew 6 removed, so anytuistlookup through the tap aborts (tuist/homebrew-tuist#41). This affectsmaintoo, not just this branch. Replaced with a pinnedmise.toml(tuist = "4.200.5") andjdx/mise-action@v2in bothci.ymlandrelease.yml— this is what Tuist's CI docs now recommend;tuist/tuist-actionis archived.Review: popup delegates were reusing shared state
Both reviewers flagged that #94 set
popupWebView.navigationDelegate = self; popupWebView.uiDelegate = self, reusing the main dictionaryCoordinator. That coordinator drives the single main web view and owns shared state, so each popup navigation during login would flip the main view'sisLoading, inject the dictionary's custom CSS into the login page, run its input-focusing script, and pollutelastLoadedURL.Fixed with
LoginPopupController, which owns the popup window, its web view, and its own delegates. The coordinator no longer touches popup navigation.A regression #94 would have shipped
#94 deleted the
createWebViewWithbody wholesale, which removed the behavior added by4d208b4("Fix #66: Open target=_blank links in default browser"). #66 is about Naver content links (e.g. "활용형 더보기") that open viatarget=_blank— full-site pages unusable at popover size, deliberately sent to the default browser. Opening every popup in-app would have regressed that.Popups are now routed by destination via
PopupRequest/PopupDispositioninstead:nid.naver.com,logins.daum.net,accounts.kakao.com) → in-app window (shares the cookie store, the whole point of the fix).Matching is on exact hosts, not suffixes — a suffix match would let ad/content hosts under
naver.comordaum.netinto a window that shares logged-in cookies. The one exception is blank popups (window.open("", name, features)is navigated only after the window exists), accepted only when the opening page is a dictionary page.Other fixes made while addressing the reviews
reloadIfNeeded()never restoredlastLoadedURL, which made the view reload on every popover open after a web content process crash.LSUIElement, so there is no menu bar and no Cmd+W. The monitor is now scoped to the popover's window..transientpopover closed by itself, which now happens whenever the login window takes focus. AddedNSPopoverDelegate.popoverDidClose.NSApp.activate()instead ofactivate(ignoringOtherApps:), since0746a14removed the latter because it made Stage Manager slide other apps' windows aside.Test plan
swiftformat --lintcleanxcodebuild buildsucceedsxcodebuild test— 42 tests, 0 failures (17 new, covering the routing policy: login hosts,target=_blankcontent links via both.linkActivatedand script, lookalike hosts likenid.naver.com.example.com, blank-popup bootstrap from trusted vs untrusted sources, and custom/file/javascript schemes)The last five items were flagged on #94 and are still outstanding — they need a human with real Naver/Daum accounts to verify.
Summary by cubic
Fixes Naver/Daum login not syncing back to the app by opening login popups in an in‑app window that shares the main
WKWebViewcookie store. Keeps content links opened with target=_blank in the default browser.Bug Fixes
nid.naver.com,logins.daum.net,accounts.kakao.com) open in-app; blank popups from dictionary pages allowed; other http(s) open in the system browser; everything else denied.LoginPopupControllerwith its own delegates; forwardnaversearchapp://andkakaotalk://links from the popup to the system; reload the dictionary view on the next popover open after the popup closes.reloadIfNeeded()to persistlastLoadedURL.Dependencies
tuistviamisepinned inmise.tomlusingjdx/mise-action@v2; remove the failing Homebrew tap.Written for commit a3f8d99. Summary will update on new commits.