Harden popup and restore cleanup
- commit
- 18f35c059e8747d2f09171476beff731fb8c7443
- author
- brice · · 2026-05-11 18:04 UTC
- parent
- a5ef447
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
4 files · +92 -36
M internal/ui/app.go +27 -17
| @@ -3804,38 +3804,48 @@ func (a *App) getActiveWebViewTarget() port.TextInputTarget { | |||
| 3804 | 3804 | // This is called when a popup uses tabbed behavior. | |
| 3805 | 3805 | func (a *App) attachPopupToTab(ctx context.Context, tabID entity.TabID, pane *entity.Pane, wv port.WebView) { | |
| 3806 | 3806 | log := logging.FromContext(ctx) | |
| 3807 | + | cleanupUnattachedPopup := func() { | |
| 3808 | + | if wv != nil { | |
| 3809 | + | wv.Destroy() | |
| 3810 | + | } | |
| 3811 | + | } | |
| 3807 | 3812 | ||
| 3808 | 3813 | wsView := a.workspaceViews[tabID] | |
| 3809 | 3814 | if wsView == nil { | |
| 3815 | + | cleanupUnattachedPopup() | |
| 3810 | 3816 | log.Warn().Str("tab_id", string(tabID)).Msg("workspace view not found for popup tab") | |
| 3811 | 3817 | return | |
| 3812 | 3818 | } | |
| 3813 | 3819 | if pane == nil { | |
| 3820 | + | cleanupUnattachedPopup() | |
| 3814 | 3821 | log.Warn().Str("tab_id", string(tabID)).Msg("popup pane is nil") | |
| 3815 | 3822 | return | |
| 3816 | 3823 | } | |
| 3817 | 3824 | if wsView.GetPaneView(pane.ID) == nil { | |
| 3825 | + | cleanupUnattachedPopup() | |
| 3818 | 3826 | log.Warn().Str("pane_id", string(pane.ID)).Msg("pane view not found for popup") | |
| 3819 | 3827 | return | |
| 3820 | 3828 | } | |
| 3829 | + | if a.contentCoord == nil { | |
| 3830 | + | cleanupUnattachedPopup() | |
| 3831 | + | log.Warn().Str("pane_id", string(pane.ID)).Msg("content coordinator not configured for popup") | |
| 3832 | + | return | |
| 3833 | + | } | |
| 3821 | 3834 | ||
| 3822 | - | // Register WebView with content coordinator | |
| 3823 | - | if a.contentCoord != nil { | |
| 3824 | - | // Track the WebView | |
| 3825 | - | a.contentCoord.RegisterPopupWebView(pane.ID, wv) | |
| 3826 | - | ||
| 3827 | - | // Wrap and attach widget | |
| 3828 | - | widget := a.contentCoord.WrapWidget(ctx, wv) | |
| 3829 | - | if widget == nil { | |
| 3830 | - | a.contentCoord.ReleaseWebView(ctx, pane.ID) | |
| 3831 | - | log.Warn().Str("pane_id", string(pane.ID)).Msg("failed to wrap popup webview") | |
| 3832 | - | return | |
| 3833 | - | } | |
| 3834 | - | if err := wsView.SetWebViewWidget(pane.ID, widget); err != nil { | |
| 3835 | - | a.contentCoord.ReleaseWebView(ctx, pane.ID) | |
| 3836 | - | log.Warn().Err(err).Str("pane_id", string(pane.ID)).Msg("pane view not found for popup") | |
| 3837 | - | return | |
| 3838 | - | } | |
| 3835 | + | // Register WebView with content coordinator. | |
| 3836 | + | a.contentCoord.RegisterPopupWebView(pane.ID, wv) | |
| 3837 | + | ||
| 3838 | + | // Wrap and attach widget. | |
| 3839 | + | widget := a.contentCoord.WrapWidget(ctx, wv) | |
| 3840 | + | if widget == nil { | |
| 3841 | + | a.contentCoord.ReleaseWebView(ctx, pane.ID) | |
| 3842 | + | log.Warn().Str("pane_id", string(pane.ID)).Msg("failed to wrap popup webview") | |
| 3843 | + | return | |
| 3844 | + | } | |
| 3845 | + | if err := wsView.SetWebViewWidget(pane.ID, widget); err != nil { | |
| 3846 | + | a.contentCoord.ReleaseWebView(ctx, pane.ID) | |
| 3847 | + | log.Warn().Err(err).Str("pane_id", string(pane.ID)).Msg("pane view not found for popup") | |
| 3848 | + | return | |
| 3839 | 3849 | } | |
| 3840 | 3850 | log.Debug(). | |
| 3841 | 3851 | Str("tab_id", string(tabID)). | |
| @@ -3804,38 +3804,48 @@ func (a *App) getActiveWebViewTarget() port.TextInputTarget { | |||||
| 3804 | // This is called when a popup uses tabbed behavior. | 3804 | // This is called when a popup uses tabbed behavior. | ||
| 3805 | func (a *App) attachPopupToTab(ctx context.Context, tabID entity.TabID, pane *entity.Pane, wv port.WebView) { | 3805 | func (a *App) attachPopupToTab(ctx context.Context, tabID entity.TabID, pane *entity.Pane, wv port.WebView) { | ||
| 3806 | log := logging.FromContext(ctx) | 3806 | log := logging.FromContext(ctx) | ||
| 3807 | + | cleanupUnattachedPopup := func() { | |||
| 3808 | + | if wv != nil { | |||
| 3809 | + | wv.Destroy() | |||
| 3810 | + | } | |||
| 3811 | + | } | |||
| 3807 | 3812 | ||||
| 3808 | wsView := a.workspaceViews[tabID] | 3813 | wsView := a.workspaceViews[tabID] | ||
| 3809 | if wsView == nil { | 3814 | if wsView == nil { | ||
| 3815 | + | cleanupUnattachedPopup() | |||
| 3810 | log.Warn().Str("tab_id", string(tabID)).Msg("workspace view not found for popup tab") | 3816 | log.Warn().Str("tab_id", string(tabID)).Msg("workspace view not found for popup tab") | ||
| 3811 | return | 3817 | return | ||
| 3812 | } | 3818 | } | ||
| 3813 | if pane == nil { | 3819 | if pane == nil { | ||
| 3820 | + | cleanupUnattachedPopup() | |||
| 3814 | log.Warn().Str("tab_id", string(tabID)).Msg("popup pane is nil") | 3821 | log.Warn().Str("tab_id", string(tabID)).Msg("popup pane is nil") | ||
| 3815 | return | 3822 | return | ||
| 3816 | } | 3823 | } | ||
| 3817 | if wsView.GetPaneView(pane.ID) == nil { | 3824 | if wsView.GetPaneView(pane.ID) == nil { | ||
| 3825 | + | cleanupUnattachedPopup() | |||
| 3818 | log.Warn().Str("pane_id", string(pane.ID)).Msg("pane view not found for popup") | 3826 | log.Warn().Str("pane_id", string(pane.ID)).Msg("pane view not found for popup") | ||
| 3819 | return | 3827 | return | ||
| 3820 | } | 3828 | } | ||
| 3829 | + | if a.contentCoord == nil { | |||
| 3830 | + | cleanupUnattachedPopup() | |||
| 3831 | + | log.Warn().Str("pane_id", string(pane.ID)).Msg("content coordinator not configured for popup") | |||
| 3832 | + | return | |||
| 3833 | + | } | |||
| 3821 | 3834 | ||||
| 3822 | - | // Register WebView with content coordinator | 3835 | + | // Register WebView with content coordinator. |
| 3823 | - | if a.contentCoord != nil { | 3836 | + | a.contentCoord.RegisterPopupWebView(pane.ID, wv) |
| 3824 | - | // Track the WebView | 3837 | + | |
| 3825 | - | a.contentCoord.RegisterPopupWebView(pane.ID, wv) | 3838 | + | // Wrap and attach widget. |
| 3826 | - | 3839 | + | widget := a.contentCoord.WrapWidget(ctx, wv) | |
| 3827 | - | // Wrap and attach widget | 3840 | + | if widget == nil { |
| 3828 | - | widget := a.contentCoord.WrapWidget(ctx, wv) | 3841 | + | a.contentCoord.ReleaseWebView(ctx, pane.ID) |
| 3829 | - | if widget == nil { | 3842 | + | log.Warn().Str("pane_id", string(pane.ID)).Msg("failed to wrap popup webview") |
| 3830 | - | a.contentCoord.ReleaseWebView(ctx, pane.ID) | 3843 | + | return |
| 3831 | - | log.Warn().Str("pane_id", string(pane.ID)).Msg("failed to wrap popup webview") | 3844 | + | } |
| 3832 | - | return | 3845 | + | if err := wsView.SetWebViewWidget(pane.ID, widget); err != nil { |
| 3833 | - | } | 3846 | + | a.contentCoord.ReleaseWebView(ctx, pane.ID) |
| 3834 | - | if err := wsView.SetWebViewWidget(pane.ID, widget); err != nil { | 3847 | + | log.Warn().Err(err).Str("pane_id", string(pane.ID)).Msg("pane view not found for popup") |
| 3835 | - | a.contentCoord.ReleaseWebView(ctx, pane.ID) | 3848 | + | return |
| 3836 | - | log.Warn().Err(err).Str("pane_id", string(pane.ID)).Msg("pane view not found for popup") | |||
| 3837 | - | return | |||
| 3838 | - | } | |||
| 3839 | } | 3849 | } | ||
| 3840 | log.Debug(). | 3850 | log.Debug(). | ||
| 3841 | Str("tab_id", string(tabID)). | 3851 | Str("tab_id", string(tabID)). | ||
M internal/ui/app_browser_launch_test.go +33 -3
| @@ -1763,6 +1763,7 @@ type fakeRecordingWebView struct { | |||
| 1763 | 1763 | setZoomLevelLastLevel float64 | |
| 1764 | 1764 | openDevToolsCalls int | |
| 1765 | 1765 | printPageCalls int | |
| 1766 | + | destroyCalls int | |
| 1766 | 1767 | } | |
| 1767 | 1768 | ||
| 1768 | 1769 | func (f *fakeRecordingWebView) ID() port.WebViewID { return f.id } | |
| @@ -1832,8 +1833,33 @@ func (f *fakeRecordingWebView) Favicon() port.Texture { retu | |||
| 1832 | 1833 | func (f *fakeRecordingWebView) Generation() uint64 { return 0 } | |
| 1833 | 1834 | func (f *fakeRecordingWebView) IsFullscreen() bool { return false } | |
| 1834 | 1835 | func (f *fakeRecordingWebView) IsPlayingAudio() bool { return false } | |
| 1835 | - | func (f *fakeRecordingWebView) IsDestroyed() bool { return false } | |
| 1836 | - | func (f *fakeRecordingWebView) Destroy() {} | |
| 1836 | + | func (f *fakeRecordingWebView) IsDestroyed() bool { return f.destroyCalls > 0 } | |
| 1837 | + | func (f *fakeRecordingWebView) Destroy() { f.destroyCalls++ } | |
| 1838 | + | ||
| 1839 | + | func TestApp_AttachPopupToTabDestroysPopupWhenWorkspaceViewMissing(t *testing.T) { | |
| 1840 | + | ctx := context.Background() | |
| 1841 | + | popupWV := &fakeRecordingWebView{id: 1} | |
| 1842 | + | app := &App{workspaceViews: map[entity.TabID]*component.WorkspaceView{}} | |
| 1843 | + | ||
| 1844 | + | app.attachPopupToTab(ctx, entity.TabID("missing-tab"), entity.NewPane(entity.PaneID("popup-pane")), popupWV) | |
| 1845 | + | ||
| 1846 | + | if popupWV.destroyCalls != 1 { | |
| 1847 | + | t.Fatalf("popup webview destroy calls = %d, want 1", popupWV.destroyCalls) | |
| 1848 | + | } | |
| 1849 | + | } | |
| 1850 | + | ||
| 1851 | + | func TestApp_AttachPopupToTabDestroysPopupWhenPaneNil(t *testing.T) { | |
| 1852 | + | ctx := context.Background() | |
| 1853 | + | popupWV := &fakeRecordingWebView{id: 1} | |
| 1854 | + | tabID := entity.TabID("tab-1") | |
| 1855 | + | app := &App{workspaceViews: map[entity.TabID]*component.WorkspaceView{tabID: &component.WorkspaceView{}}} | |
| 1856 | + | ||
| 1857 | + | app.attachPopupToTab(ctx, tabID, nil, popupWV) | |
| 1858 | + | ||
| 1859 | + | if popupWV.destroyCalls != 1 { | |
| 1860 | + | t.Fatalf("popup webview destroy calls = %d, want 1", popupWV.destroyCalls) | |
| 1861 | + | } | |
| 1862 | + | } | |
| 1837 | 1863 | ||
| 1838 | 1864 | func TestApp_AttachPopupToTabSkipsRegistrationWhenPaneViewMissing(t *testing.T) { | |
| 1839 | 1865 | ctx := context.Background() | |
| @@ -1847,11 +1873,15 @@ func TestApp_AttachPopupToTabSkipsRegistrationWhenPaneViewMissing(t *testing.T) | |||
| 1847 | 1873 | }, | |
| 1848 | 1874 | } | |
| 1849 | 1875 | ||
| 1850 | - | app.attachPopupToTab(ctx, tabID, pane, &fakeRecordingWebView{id: 1}) | |
| 1876 | + | popupWV := &fakeRecordingWebView{id: 1} | |
| 1877 | + | app.attachPopupToTab(ctx, tabID, pane, popupWV) | |
| 1851 | 1878 | ||
| 1852 | 1879 | if got := contentCoord.GetWebView(pane.ID); got != nil { | |
| 1853 | 1880 | t.Fatalf("popup webview was registered for missing pane view: %v", got) | |
| 1854 | 1881 | } | |
| 1882 | + | if popupWV.destroyCalls != 1 { | |
| 1883 | + | t.Fatalf("popup webview destroy calls = %d, want 1", popupWV.destroyCalls) | |
| 1884 | + | } | |
| 1855 | 1885 | } | |
| 1856 | 1886 | ||
| 1857 | 1887 | func TestApp_AttachPopupToTabReleasesRegistrationWhenWrapFails(t *testing.T) { | |
| @@ -1763,6 +1763,7 @@ type fakeRecordingWebView struct { | |||||
| 1763 | setZoomLevelLastLevel float64 | 1763 | setZoomLevelLastLevel float64 | ||
| 1764 | openDevToolsCalls int | 1764 | openDevToolsCalls int | ||
| 1765 | printPageCalls int | 1765 | printPageCalls int | ||
| 1766 | + | destroyCalls int | |||
| 1766 | } | 1767 | } | ||
| 1767 | 1768 | ||||
| 1768 | func (f *fakeRecordingWebView) ID() port.WebViewID { return f.id } | 1769 | func (f *fakeRecordingWebView) ID() port.WebViewID { return f.id } | ||
| @@ -1832,8 +1833,33 @@ func (f *fakeRecordingWebView) Favicon() port.Texture { retu | |||||
| 1832 | func (f *fakeRecordingWebView) Generation() uint64 { return 0 } | 1833 | func (f *fakeRecordingWebView) Generation() uint64 { return 0 } | ||
| 1833 | func (f *fakeRecordingWebView) IsFullscreen() bool { return false } | 1834 | func (f *fakeRecordingWebView) IsFullscreen() bool { return false } | ||
| 1834 | func (f *fakeRecordingWebView) IsPlayingAudio() bool { return false } | 1835 | func (f *fakeRecordingWebView) IsPlayingAudio() bool { return false } | ||
| 1835 | - | func (f *fakeRecordingWebView) IsDestroyed() bool { return false } | 1836 | + | func (f *fakeRecordingWebView) IsDestroyed() bool { return f.destroyCalls > 0 } |
| 1836 | - | func (f *fakeRecordingWebView) Destroy() {} | 1837 | + | func (f *fakeRecordingWebView) Destroy() { f.destroyCalls++ } |
| 1838 | + | ||||
| 1839 | + | func TestApp_AttachPopupToTabDestroysPopupWhenWorkspaceViewMissing(t *testing.T) { | |||
| 1840 | + | ctx := context.Background() | |||
| 1841 | + | popupWV := &fakeRecordingWebView{id: 1} | |||
| 1842 | + | app := &App{workspaceViews: map[entity.TabID]*component.WorkspaceView{}} | |||
| 1843 | + | ||||
| 1844 | + | app.attachPopupToTab(ctx, entity.TabID("missing-tab"), entity.NewPane(entity.PaneID("popup-pane")), popupWV) | |||
| 1845 | + | ||||
| 1846 | + | if popupWV.destroyCalls != 1 { | |||
| 1847 | + | t.Fatalf("popup webview destroy calls = %d, want 1", popupWV.destroyCalls) | |||
| 1848 | + | } | |||
| 1849 | + | } | |||
| 1850 | + | ||||
| 1851 | + | func TestApp_AttachPopupToTabDestroysPopupWhenPaneNil(t *testing.T) { | |||
| 1852 | + | ctx := context.Background() | |||
| 1853 | + | popupWV := &fakeRecordingWebView{id: 1} | |||
| 1854 | + | tabID := entity.TabID("tab-1") | |||
| 1855 | + | app := &App{workspaceViews: map[entity.TabID]*component.WorkspaceView{tabID: &component.WorkspaceView{}}} | |||
| 1856 | + | ||||
| 1857 | + | app.attachPopupToTab(ctx, tabID, nil, popupWV) | |||
| 1858 | + | ||||
| 1859 | + | if popupWV.destroyCalls != 1 { | |||
| 1860 | + | t.Fatalf("popup webview destroy calls = %d, want 1", popupWV.destroyCalls) | |||
| 1861 | + | } | |||
| 1862 | + | } | |||
| 1837 | 1863 | ||||
| 1838 | func TestApp_AttachPopupToTabSkipsRegistrationWhenPaneViewMissing(t *testing.T) { | 1864 | func TestApp_AttachPopupToTabSkipsRegistrationWhenPaneViewMissing(t *testing.T) { | ||
| 1839 | ctx := context.Background() | 1865 | ctx := context.Background() | ||
| @@ -1847,11 +1873,15 @@ func TestApp_AttachPopupToTabSkipsRegistrationWhenPaneViewMissing(t *testing.T) | |||||
| 1847 | }, | 1873 | }, | ||
| 1848 | } | 1874 | } | ||
| 1849 | 1875 | ||||
| 1850 | - | app.attachPopupToTab(ctx, tabID, pane, &fakeRecordingWebView{id: 1}) | 1876 | + | popupWV := &fakeRecordingWebView{id: 1} |
| 1877 | + | app.attachPopupToTab(ctx, tabID, pane, popupWV) | |||
| 1851 | 1878 | ||||
| 1852 | if got := contentCoord.GetWebView(pane.ID); got != nil { | 1879 | if got := contentCoord.GetWebView(pane.ID); got != nil { | ||
| 1853 | t.Fatalf("popup webview was registered for missing pane view: %v", got) | 1880 | t.Fatalf("popup webview was registered for missing pane view: %v", got) | ||
| 1854 | } | 1881 | } | ||
| 1882 | + | if popupWV.destroyCalls != 1 { | |||
| 1883 | + | t.Fatalf("popup webview destroy calls = %d, want 1", popupWV.destroyCalls) | |||
| 1884 | + | } | |||
| 1855 | } | 1885 | } | ||
| 1856 | 1886 | ||||
| 1857 | func TestApp_AttachPopupToTabReleasesRegistrationWhenWrapFails(t *testing.T) { | 1887 | func TestApp_AttachPopupToTabReleasesRegistrationWhenWrapFails(t *testing.T) { | ||
M internal/ui/component/workspace_view.go +28 -13
| @@ -4,7 +4,6 @@ package component | |||
| 4 | 4 | import ( | |
| 5 | 5 | "context" | |
| 6 | 6 | "errors" | |
| 7 | - | "fmt" | |
| 8 | 7 | "sync" | |
| 9 | 8 | "sync/atomic" | |
| 10 | 9 | "time" | |
| @@ -218,12 +217,9 @@ func (wv *WorkspaceView) SetWorkspace(ctx context.Context, ws *entity.Workspace) | |||
| 218 | 217 | } | |
| 219 | 218 | ||
| 220 | 219 | // Set initial active pane. A persisted workspace can contain a stale active | |
| 221 | - | // pane ID after restore/rebuild; that should not abort initialization. | |
| 222 | - | if ws.ActivePaneID != "" { | |
| 223 | - | if err := wv.setActivePaneIDInternal(ws.ActivePaneID, true); err != nil { | |
| 224 | - | return fmt.Errorf("set initial active pane: %w", err) | |
| 225 | - | } | |
| 226 | - | } | |
| 220 | + | // pane ID after restore/rebuild; choose a valid fallback instead of leaving | |
| 221 | + | // later active-pane logic pointing at a missing pane. | |
| 222 | + | wv.restoreActivePaneInternal() | |
| 227 | 223 | ||
| 228 | 224 | // Update single-pane mode based on pane count | |
| 229 | 225 | wv.updateSinglePaneModeInternal() | |
| @@ -235,7 +231,7 @@ func (wv *WorkspaceView) SetWorkspace(ctx context.Context, ws *entity.Workspace) | |||
| 235 | 231 | func (wv *WorkspaceView) SetActivePaneID(paneID entity.PaneID) error { | |
| 236 | 232 | wv.mu.Lock() | |
| 237 | 233 | currentActiveID := wv.getActivePaneIDInternal() | |
| 238 | - | err := wv.setActivePaneIDInternal(paneID, false) | |
| 234 | + | err := wv.setActivePaneIDInternal(paneID) | |
| 239 | 235 | callback := wv.onActivePaneChanged | |
| 240 | 236 | wv.mu.Unlock() | |
| 241 | 237 | if err != nil { | |
| @@ -247,9 +243,32 @@ func (wv *WorkspaceView) SetActivePaneID(paneID entity.PaneID) error { | |||
| 247 | 243 | return nil | |
| 248 | 244 | } | |
| 249 | 245 | ||
| 246 | + | // restoreActivePaneInternal restores the active pane after building paneViews. | |
| 247 | + | // Must be called with the lock held. | |
| 248 | + | func (wv *WorkspaceView) restoreActivePaneInternal() { | |
| 249 | + | if wv.workspace == nil { | |
| 250 | + | return | |
| 251 | + | } | |
| 252 | + | ||
| 253 | + | paneID := wv.workspace.ActivePaneID | |
| 254 | + | if paneID != "" { | |
| 255 | + | if _, ok := wv.paneViews[paneID]; ok { | |
| 256 | + | _ = wv.setActivePaneIDInternal(paneID) | |
| 257 | + | return | |
| 258 | + | } | |
| 259 | + | wv.logger.Debug().Str("pane_id", string(paneID)).Msg("stale active pane ignored") | |
| 260 | + | wv.workspace.ActivePaneID = "" | |
| 261 | + | } | |
| 262 | + | ||
| 263 | + | for fallbackID := range wv.paneViews { | |
| 264 | + | _ = wv.setActivePaneIDInternal(fallbackID) | |
| 265 | + | return | |
| 266 | + | } | |
| 267 | + | } | |
| 268 | + | ||
| 250 | 269 | // setActivePaneIDInternal updates active pane without locking. | |
| 251 | 270 | // Updates the domain model as the single source of truth. | |
| 252 | - | func (wv *WorkspaceView) setActivePaneIDInternal(paneID entity.PaneID, allowMissing bool) error { | |
| 271 | + | func (wv *WorkspaceView) setActivePaneIDInternal(paneID entity.PaneID) error { | |
| 253 | 272 | currentActiveID := wv.getActivePaneIDInternal() | |
| 254 | 273 | ||
| 255 | 274 | wv.logger.Debug(). | |
| @@ -261,10 +280,6 @@ func (wv *WorkspaceView) setActivePaneIDInternal(paneID entity.PaneID, allowMiss | |||
| 261 | 280 | // should leave the current active pane styling and overlays untouched. | |
| 262 | 281 | newPV, ok := wv.paneViews[paneID] | |
| 263 | 282 | if !ok { | |
| 264 | - | if allowMissing { | |
| 265 | - | wv.logger.Debug().Str("pane_id", string(paneID)).Msg("stale active pane ignored") | |
| 266 | - | return nil | |
| 267 | - | } | |
| 268 | 283 | return ErrPaneNotFound | |
| 269 | 284 | } | |
| 270 | 285 | ||
| @@ -4,7 +4,6 @@ package component | |||||
| 4 | import ( | 4 | import ( | ||
| 5 | "context" | 5 | "context" | ||
| 6 | "errors" | 6 | "errors" | ||
| 7 | - | "fmt" | |||
| 8 | "sync" | 7 | "sync" | ||
| 9 | "sync/atomic" | 8 | "sync/atomic" | ||
| 10 | "time" | 9 | "time" | ||
| @@ -218,12 +217,9 @@ func (wv *WorkspaceView) SetWorkspace(ctx context.Context, ws *entity.Workspace) | |||||
| 218 | } | 217 | } | ||
| 219 | 218 | ||||
| 220 | // Set initial active pane. A persisted workspace can contain a stale active | 219 | // Set initial active pane. A persisted workspace can contain a stale active | ||
| 221 | - | // pane ID after restore/rebuild; that should not abort initialization. | 220 | + | // pane ID after restore/rebuild; choose a valid fallback instead of leaving |
| 222 | - | if ws.ActivePaneID != "" { | 221 | + | // later active-pane logic pointing at a missing pane. |
| 223 | - | if err := wv.setActivePaneIDInternal(ws.ActivePaneID, true); err != nil { | 222 | + | wv.restoreActivePaneInternal() |
| 224 | - | return fmt.Errorf("set initial active pane: %w", err) | |||
| 225 | - | } | |||
| 226 | - | } | |||
| 227 | 223 | ||||
| 228 | // Update single-pane mode based on pane count | 224 | // Update single-pane mode based on pane count | ||
| 229 | wv.updateSinglePaneModeInternal() | 225 | wv.updateSinglePaneModeInternal() | ||
| @@ -235,7 +231,7 @@ func (wv *WorkspaceView) SetWorkspace(ctx context.Context, ws *entity.Workspace) | |||||
| 235 | func (wv *WorkspaceView) SetActivePaneID(paneID entity.PaneID) error { | 231 | func (wv *WorkspaceView) SetActivePaneID(paneID entity.PaneID) error { | ||
| 236 | wv.mu.Lock() | 232 | wv.mu.Lock() | ||
| 237 | currentActiveID := wv.getActivePaneIDInternal() | 233 | currentActiveID := wv.getActivePaneIDInternal() | ||
| 238 | - | err := wv.setActivePaneIDInternal(paneID, false) | 234 | + | err := wv.setActivePaneIDInternal(paneID) |
| 239 | callback := wv.onActivePaneChanged | 235 | callback := wv.onActivePaneChanged | ||
| 240 | wv.mu.Unlock() | 236 | wv.mu.Unlock() | ||
| 241 | if err != nil { | 237 | if err != nil { | ||
| @@ -247,9 +243,32 @@ func (wv *WorkspaceView) SetActivePaneID(paneID entity.PaneID) error { | |||||
| 247 | return nil | 243 | return nil | ||
| 248 | } | 244 | } | ||
| 249 | 245 | ||||
| 246 | + | // restoreActivePaneInternal restores the active pane after building paneViews. | |||
| 247 | + | // Must be called with the lock held. | |||
| 248 | + | func (wv *WorkspaceView) restoreActivePaneInternal() { | |||
| 249 | + | if wv.workspace == nil { | |||
| 250 | + | return | |||
| 251 | + | } | |||
| 252 | + | ||||
| 253 | + | paneID := wv.workspace.ActivePaneID | |||
| 254 | + | if paneID != "" { | |||
| 255 | + | if _, ok := wv.paneViews[paneID]; ok { | |||
| 256 | + | _ = wv.setActivePaneIDInternal(paneID) | |||
| 257 | + | return | |||
| 258 | + | } | |||
| 259 | + | wv.logger.Debug().Str("pane_id", string(paneID)).Msg("stale active pane ignored") | |||
| 260 | + | wv.workspace.ActivePaneID = "" | |||
| 261 | + | } | |||
| 262 | + | ||||
| 263 | + | for fallbackID := range wv.paneViews { | |||
| 264 | + | _ = wv.setActivePaneIDInternal(fallbackID) | |||
| 265 | + | return | |||
| 266 | + | } | |||
| 267 | + | } | |||
| 268 | + | ||||
| 250 | // setActivePaneIDInternal updates active pane without locking. | 269 | // setActivePaneIDInternal updates active pane without locking. | ||
| 251 | // Updates the domain model as the single source of truth. | 270 | // Updates the domain model as the single source of truth. | ||
| 252 | - | func (wv *WorkspaceView) setActivePaneIDInternal(paneID entity.PaneID, allowMissing bool) error { | 271 | + | func (wv *WorkspaceView) setActivePaneIDInternal(paneID entity.PaneID) error { |
| 253 | currentActiveID := wv.getActivePaneIDInternal() | 272 | currentActiveID := wv.getActivePaneIDInternal() | ||
| 254 | 273 | ||||
| 255 | wv.logger.Debug(). | 274 | wv.logger.Debug(). | ||
| @@ -261,10 +280,6 @@ func (wv *WorkspaceView) setActivePaneIDInternal(paneID entity.PaneID, allowMiss | |||||
| 261 | // should leave the current active pane styling and overlays untouched. | 280 | // should leave the current active pane styling and overlays untouched. | ||
| 262 | newPV, ok := wv.paneViews[paneID] | 281 | newPV, ok := wv.paneViews[paneID] | ||
| 263 | if !ok { | 282 | if !ok { | ||
| 264 | - | if allowMissing { | |||
| 265 | - | wv.logger.Debug().Str("pane_id", string(paneID)).Msg("stale active pane ignored") | |||
| 266 | - | return nil | |||
| 267 | - | } | |||
| 268 | return ErrPaneNotFound | 283 | return ErrPaneNotFound | ||
| 269 | } | 284 | } | ||
| 270 | 285 | ||||
M internal/ui/component/workspace_view_test.go +4 -3
| @@ -266,12 +266,13 @@ func TestSetWorkspace_StaleActivePaneID_DoesNotFailInitialization(t *testing.T) | |||
| 266 | 266 | ||
| 267 | 267 | wv := component.NewWorkspaceView(ctx, mockFactory) | |
| 268 | 268 | ||
| 269 | - | mockOverlay, _ := setupWorkspacePaneViewMocks(t, mockFactory) | |
| 269 | + | mockOverlay, mockBorderBox := setupWorkspacePaneViewMocks(t, mockFactory) | |
| 270 | 270 | mockStackBox := setupStackedLeafMocks(t, mockFactory, mockOverlay) | |
| 271 | 271 | ||
| 272 | 272 | mockStackBox.EXPECT().SetVisible(true).Once() | |
| 273 | 273 | mockBox.EXPECT().Append(mockStackBox).Once() | |
| 274 | 274 | mockBox.EXPECT().AddCssClass("single-pane").Once() | |
| 275 | + | mockBorderBox.EXPECT().AddCssClass("pane-active").Once() | |
| 275 | 276 | ||
| 276 | 277 | pane := entity.NewPane(entity.PaneID("pane-1")) | |
| 277 | 278 | node := &entity.PaneNode{ID: "node-1", Pane: pane} | |
| @@ -287,8 +288,8 @@ func TestSetWorkspace_StaleActivePaneID_DoesNotFailInitialization(t *testing.T) | |||
| 287 | 288 | // Assert | |
| 288 | 289 | require.NoError(t, err) | |
| 289 | 290 | assert.Equal(t, 1, wv.PaneCount()) | |
| 290 | - | assert.Equal(t, entity.PaneID("stale-pane"), wv.GetActivePaneID()) | |
| 291 | - | assert.False(t, wv.GetPaneView(pane.ID).IsActive()) | |
| 291 | + | assert.Equal(t, pane.ID, wv.GetActivePaneID()) | |
| 292 | + | assert.True(t, wv.GetPaneView(pane.ID).IsActive()) | |
| 292 | 293 | } | |
| 293 | 294 | ||
| 294 | 295 | func TestWorkspaceView_HoverFocusLockState(t *testing.T) { | |
| @@ -266,12 +266,13 @@ func TestSetWorkspace_StaleActivePaneID_DoesNotFailInitialization(t *testing.T) | |||||
| 266 | 266 | ||||
| 267 | wv := component.NewWorkspaceView(ctx, mockFactory) | 267 | wv := component.NewWorkspaceView(ctx, mockFactory) | ||
| 268 | 268 | ||||
| 269 | - | mockOverlay, _ := setupWorkspacePaneViewMocks(t, mockFactory) | 269 | + | mockOverlay, mockBorderBox := setupWorkspacePaneViewMocks(t, mockFactory) |
| 270 | mockStackBox := setupStackedLeafMocks(t, mockFactory, mockOverlay) | 270 | mockStackBox := setupStackedLeafMocks(t, mockFactory, mockOverlay) | ||
| 271 | 271 | ||||
| 272 | mockStackBox.EXPECT().SetVisible(true).Once() | 272 | mockStackBox.EXPECT().SetVisible(true).Once() | ||
| 273 | mockBox.EXPECT().Append(mockStackBox).Once() | 273 | mockBox.EXPECT().Append(mockStackBox).Once() | ||
| 274 | mockBox.EXPECT().AddCssClass("single-pane").Once() | 274 | mockBox.EXPECT().AddCssClass("single-pane").Once() | ||
| 275 | + | mockBorderBox.EXPECT().AddCssClass("pane-active").Once() | |||
| 275 | 276 | ||||
| 276 | pane := entity.NewPane(entity.PaneID("pane-1")) | 277 | pane := entity.NewPane(entity.PaneID("pane-1")) | ||
| 277 | node := &entity.PaneNode{ID: "node-1", Pane: pane} | 278 | node := &entity.PaneNode{ID: "node-1", Pane: pane} | ||
| @@ -287,8 +288,8 @@ func TestSetWorkspace_StaleActivePaneID_DoesNotFailInitialization(t *testing.T) | |||||
| 287 | // Assert | 288 | // Assert | ||
| 288 | require.NoError(t, err) | 289 | require.NoError(t, err) | ||
| 289 | assert.Equal(t, 1, wv.PaneCount()) | 290 | assert.Equal(t, 1, wv.PaneCount()) | ||
| 290 | - | assert.Equal(t, entity.PaneID("stale-pane"), wv.GetActivePaneID()) | 291 | + | assert.Equal(t, pane.ID, wv.GetActivePaneID()) |
| 291 | - | assert.False(t, wv.GetPaneView(pane.ID).IsActive()) | 292 | + | assert.True(t, wv.GetPaneView(pane.ID).IsActive()) |
| 292 | } | 293 | } | ||
| 293 | 294 | ||||
| 294 | func TestWorkspaceView_HoverFocusLockState(t *testing.T) { | 295 | func TestWorkspaceView_HoverFocusLockState(t *testing.T) { | ||