detached 24133f3

fix(cef): avoid placeholder webviews for popup panes

commit
24133f3546dd73096c08305fbbe129a5b5846a14
author
brice · · 2026-04-21 15:14 UTC
parent
31b1bbf
signature
GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email
2 files · +83 -2
Unified
UnifiedSplit
M internal/ui/coordinator/content/popup.go +9 -2
@@ -526,6 +526,11 @@ func (c *Coordinator) finishPopupCreate(
526526 Str("uri", logging.TruncateURL(req.TargetURI, logURLMaxLen)).
527527 Msg("popup OAuth check")
528528
529+ // Register the popup WebView before workspace insertion so split/stack UI
530+ // updates can reuse the real popup instead of acquiring a placeholder pane
531+ // WebView that races the actual popup flow.
532+ c.setWebViewLocked(paneID, popupWV)
533+
529534 // Insert into workspace IMMEDIATELY (WebView stays hidden)
530535 // This is required for WebKit to establish window.opener relationship
531536 if c.onInsertPopup != nil {
@@ -541,13 +546,15 @@ func (c *Coordinator) finishPopupCreate(
541546
542547 if err := c.onInsertPopup(ctx, popupInput); err != nil {
543548 log.Error().Err(err).Msg("failed to insert popup into workspace")
549+ if current := c.getWebViewLocked(paneID); current == popupWV {
550+ c.deleteWebViewLocked(paneID)
551+ }
544552 popupWV.Destroy()
545553 return nil
546554 }
547555 }
548556
549- // Register WebView in our map (after successful insertion)
550- c.setWebViewLocked(paneID, popupWV)
557+ // WebView already registered before insertion.
551558 if !req.NoJavaScriptAccess {
552559 c.storeReusableNamedPopup(parentPaneID, req.FrameName, popupWV)
553560 }
@@ -526,6 +526,11 @@ func (c *Coordinator) finishPopupCreate(
526 Str("uri", logging.TruncateURL(req.TargetURI, logURLMaxLen)).526 Str("uri", logging.TruncateURL(req.TargetURI, logURLMaxLen)).
527 Msg("popup OAuth check")527 Msg("popup OAuth check")
528 528
529+ // Register the popup WebView before workspace insertion so split/stack UI
530+ // updates can reuse the real popup instead of acquiring a placeholder pane
531+ // WebView that races the actual popup flow.
532+ c.setWebViewLocked(paneID, popupWV)
533+
529 // Insert into workspace IMMEDIATELY (WebView stays hidden)534 // Insert into workspace IMMEDIATELY (WebView stays hidden)
530 // This is required for WebKit to establish window.opener relationship535 // This is required for WebKit to establish window.opener relationship
531 if c.onInsertPopup != nil {536 if c.onInsertPopup != nil {
@@ -541,13 +546,15 @@ func (c *Coordinator) finishPopupCreate(
541 546
542 if err := c.onInsertPopup(ctx, popupInput); err != nil {547 if err := c.onInsertPopup(ctx, popupInput); err != nil {
543 log.Error().Err(err).Msg("failed to insert popup into workspace")548 log.Error().Err(err).Msg("failed to insert popup into workspace")
549+ if current := c.getWebViewLocked(paneID); current == popupWV {
550+ c.deleteWebViewLocked(paneID)
551+ }
544 popupWV.Destroy()552 popupWV.Destroy()
545 return nil553 return nil
546 }554 }
547 }555 }
548 556
549- // Register WebView in our map (after successful insertion)557+ // WebView already registered before insertion.
550- c.setWebViewLocked(paneID, popupWV)
551 if !req.NoJavaScriptAccess {558 if !req.NoJavaScriptAccess {
552 c.storeReusableNamedPopup(parentPaneID, req.FrameName, popupWV)559 c.storeReusableNamedPopup(parentPaneID, req.FrameName, popupWV)
553 }560 }
M internal/ui/coordinator/content/popup_test.go +74 -0
@@ -143,6 +143,80 @@ func TestNormalizePopupTargetURIForFallback_PreservesUnknownURLs(t *testing.T) {
143143 assert.Equal(t, raw, normalizePopupTargetURIForFallback(raw))
144144 }
145145
146+func TestHandlePopupCreate_RegistersPopupWebViewBeforeWorkspaceInsertion(t *testing.T) {
147+ ctx := context.Background()
148+ parentPaneID := entity.PaneID("parent-pane")
149+ parentWV := mocks.NewMockWebView(t)
150+ parentWV.EXPECT().ID().Return(port.WebViewID(101)).Once()
151+
152+ popupWV := mocks.NewMockWebView(t)
153+ popupWV.EXPECT().ID().Return(port.WebViewID(150)).Once()
154+ popupWV.EXPECT().Generation().Return(uint64(1)).Once()
155+ popupWV.EXPECT().SetCallbacks(mock.Anything).Once()
156+ popupWV.EXPECT().IsLoading().Return(false).Once()
157+ popupWV.EXPECT().URI().Return("").Once()
158+ popupWV.EXPECT().LoadURI(mock.Anything, "https://example.com/popup").Return(nil).Once()
159+
160+ factory := mocks.NewMockWebViewFactory(t)
161+ factory.EXPECT().Create(mock.Anything).Return(popupWV, nil).Once()
162+
163+ c := &Coordinator{
164+ webViews: make(map[entity.PaneID]port.WebView),
165+ pendingPopups: make(map[port.WebViewID]*PendingPopup),
166+ popupOAuth: make(map[port.WebViewID]*popupOAuthState),
167+ }
168+ c.SetPopupConfig(factory, nil, func() string { return "popup-pane" })
169+ c.SetOnInsertPopup(func(_ context.Context, input InsertPopupInput) error {
170+ require.Equal(t, entity.PaneID("popup-pane"), input.PopupPane.ID)
171+ require.Same(t, popupWV, c.getWebViewLocked(input.PopupPane.ID))
172+ return nil
173+ })
174+
175+ created := c.handlePopupCreate(ctx, parentPaneID, parentWV, port.PopupRequest{
176+ TargetURI: "https://example.com/popup",
177+ FrameName: "auth-popup",
178+ IsUserGesture: true,
179+ NoJavaScriptAccess: true,
180+ })
181+
182+ require.Same(t, popupWV, created)
183+}
184+
185+func TestHandlePopupCreate_CleansUpPreRegisteredPopupWebViewWhenInsertionFails(t *testing.T) {
186+ ctx := context.Background()
187+ parentPaneID := entity.PaneID("parent-pane")
188+ parentWV := mocks.NewMockWebView(t)
189+ parentWV.EXPECT().ID().Return(port.WebViewID(101)).Once()
190+
191+ popupWV := mocks.NewMockWebView(t)
192+ popupWV.EXPECT().ID().Return(port.WebViewID(151)).Once()
193+ popupWV.EXPECT().Destroy().Once()
194+
195+ factory := mocks.NewMockWebViewFactory(t)
196+ factory.EXPECT().Create(mock.Anything).Return(popupWV, nil).Once()
197+
198+ c := &Coordinator{
199+ webViews: make(map[entity.PaneID]port.WebView),
200+ pendingPopups: make(map[port.WebViewID]*PendingPopup),
201+ popupOAuth: make(map[port.WebViewID]*popupOAuthState),
202+ }
203+ c.SetPopupConfig(factory, nil, func() string { return "popup-pane" })
204+ c.SetOnInsertPopup(func(_ context.Context, input InsertPopupInput) error {
205+ require.Same(t, popupWV, c.getWebViewLocked(input.PopupPane.ID))
206+ return assert.AnError
207+ })
208+
209+ created := c.handlePopupCreate(ctx, parentPaneID, parentWV, port.PopupRequest{
210+ TargetURI: "https://example.com/popup",
211+ FrameName: "auth-popup",
212+ IsUserGesture: true,
213+ NoJavaScriptAccess: true,
214+ })
215+
216+ require.Nil(t, created)
217+ require.Nil(t, c.getWebViewLocked(entity.PaneID("popup-pane")))
218+}
219+
146220 func TestHandlePopupCreate_UsesRegularWebViewWhenPopupDisablesJavaScriptAccess(t *testing.T) {
147221 ctx := context.Background()
148222 parentPaneID := entity.PaneID("parent-pane")
@@ -143,6 +143,80 @@ func TestNormalizePopupTargetURIForFallback_PreservesUnknownURLs(t *testing.T) {
143 assert.Equal(t, raw, normalizePopupTargetURIForFallback(raw))143 assert.Equal(t, raw, normalizePopupTargetURIForFallback(raw))
144 }144 }
145 145
146+func TestHandlePopupCreate_RegistersPopupWebViewBeforeWorkspaceInsertion(t *testing.T) {
147+ ctx := context.Background()
148+ parentPaneID := entity.PaneID("parent-pane")
149+ parentWV := mocks.NewMockWebView(t)
150+ parentWV.EXPECT().ID().Return(port.WebViewID(101)).Once()
151+
152+ popupWV := mocks.NewMockWebView(t)
153+ popupWV.EXPECT().ID().Return(port.WebViewID(150)).Once()
154+ popupWV.EXPECT().Generation().Return(uint64(1)).Once()
155+ popupWV.EXPECT().SetCallbacks(mock.Anything).Once()
156+ popupWV.EXPECT().IsLoading().Return(false).Once()
157+ popupWV.EXPECT().URI().Return("").Once()
158+ popupWV.EXPECT().LoadURI(mock.Anything, "https://example.com/popup").Return(nil).Once()
159+
160+ factory := mocks.NewMockWebViewFactory(t)
161+ factory.EXPECT().Create(mock.Anything).Return(popupWV, nil).Once()
162+
163+ c := &Coordinator{
164+ webViews: make(map[entity.PaneID]port.WebView),
165+ pendingPopups: make(map[port.WebViewID]*PendingPopup),
166+ popupOAuth: make(map[port.WebViewID]*popupOAuthState),
167+ }
168+ c.SetPopupConfig(factory, nil, func() string { return "popup-pane" })
169+ c.SetOnInsertPopup(func(_ context.Context, input InsertPopupInput) error {
170+ require.Equal(t, entity.PaneID("popup-pane"), input.PopupPane.ID)
171+ require.Same(t, popupWV, c.getWebViewLocked(input.PopupPane.ID))
172+ return nil
173+ })
174+
175+ created := c.handlePopupCreate(ctx, parentPaneID, parentWV, port.PopupRequest{
176+ TargetURI: "https://example.com/popup",
177+ FrameName: "auth-popup",
178+ IsUserGesture: true,
179+ NoJavaScriptAccess: true,
180+ })
181+
182+ require.Same(t, popupWV, created)
183+}
184+
185+func TestHandlePopupCreate_CleansUpPreRegisteredPopupWebViewWhenInsertionFails(t *testing.T) {
186+ ctx := context.Background()
187+ parentPaneID := entity.PaneID("parent-pane")
188+ parentWV := mocks.NewMockWebView(t)
189+ parentWV.EXPECT().ID().Return(port.WebViewID(101)).Once()
190+
191+ popupWV := mocks.NewMockWebView(t)
192+ popupWV.EXPECT().ID().Return(port.WebViewID(151)).Once()
193+ popupWV.EXPECT().Destroy().Once()
194+
195+ factory := mocks.NewMockWebViewFactory(t)
196+ factory.EXPECT().Create(mock.Anything).Return(popupWV, nil).Once()
197+
198+ c := &Coordinator{
199+ webViews: make(map[entity.PaneID]port.WebView),
200+ pendingPopups: make(map[port.WebViewID]*PendingPopup),
201+ popupOAuth: make(map[port.WebViewID]*popupOAuthState),
202+ }
203+ c.SetPopupConfig(factory, nil, func() string { return "popup-pane" })
204+ c.SetOnInsertPopup(func(_ context.Context, input InsertPopupInput) error {
205+ require.Same(t, popupWV, c.getWebViewLocked(input.PopupPane.ID))
206+ return assert.AnError
207+ })
208+
209+ created := c.handlePopupCreate(ctx, parentPaneID, parentWV, port.PopupRequest{
210+ TargetURI: "https://example.com/popup",
211+ FrameName: "auth-popup",
212+ IsUserGesture: true,
213+ NoJavaScriptAccess: true,
214+ })
215+
216+ require.Nil(t, created)
217+ require.Nil(t, c.getWebViewLocked(entity.PaneID("popup-pane")))
218+}
219+
146 func TestHandlePopupCreate_UsesRegularWebViewWhenPopupDisablesJavaScriptAccess(t *testing.T) {220 func TestHandlePopupCreate_UsesRegularWebViewWhenPopupDisablesJavaScriptAccess(t *testing.T) {
147 ctx := context.Background()221 ctx := context.Background()
148 parentPaneID := entity.PaneID("parent-pane")222 parentPaneID := entity.PaneID("parent-pane")

Keyboard shortcuts

Single-key shortcuts are on. ctrl k and esc work whatever this says.

ctrl k :
go to a section, branch, tag, file or repository
?
this list
j k
move the row cursor down / up
enter
open the row under the cursor; on Files, a directory opens or closes in place
↑ ↓ ← → home end
in the file tree (focus on a row): previous / next row, close / open a directory or go to its parent / first entry, first / last row. They never open a file
l h
on Files: open a directory or a file / close a directory or go up to its parent, then to the parent page. Elsewhere: into the side panel (on a merge request, its files) / back to the content; there j k move between its controls
g g G
first / last row
/
filter the list; on Files, find a file anywhere in the repository (esc clears, enter opens the best match)
b
switch branch or tag
g s g f g c g b g m g i g r g a
tabs: summary, files, commits, branches & tags, merge requests, issues, releases, actions (when available)
g h
home
w
wide mode: use the whole width of the screen, or not (remembered)
y
copy a permalink pinned to the commit
d u x
diff: split / unified / collapse or expand all
e E
diff: the whole file in view / every file, with the lines the hunks hide
n
merge request: next note in the diff
esc
close a dialog, leave a field

Branches 8

Tags 42

↑↓ moveenter openesc close