fix: keep the settings window reachable when displays change (#655)

Co-authored-by: Tarik Alani <tarik.nzl@gmail.com>

Author: CtrlAltDale

src/app/overlayManager.ts

@@ -10,7 +10,11 @@ import { Notification } from 'electron';
 import { readData, writeData } from './storage/storage';
 import { getDashboard } from './storage/dashboards';
 import { getChromiumFlags, parseCustomSwitches } from './storage/chromiumFlags';
-import { trackSettingsWindowMovement } from './trackWindowMovement';
+import {
+  markCorrectedBounds,
+  trackSettingsWindowMovement,
+} from './trackWindowMovement';
+import { sanitizeWindowBounds } from './windowBounds';
 import logger from './logger';
 import { createRendererPerfArguments } from './perfRendererArguments';
 
@@ -911,7 +915,13 @@ export class OverlayManager {
     // Reveal the window once its content is ready, unless it should start
     // minimized to the system tray (the "Start minimized" general setting).
     browserWindow.once('ready-to-show', () => {
-      if (browserWindow.isDestroyed() || startHidden) return;
+      if (browserWindow.isDestroyed()) return;
+
+      // Runs before the startHidden check: a window that starts in the tray is
+      // shown later, and would otherwise be revealed somewhere unreachable.
+      ensureWindowOnScreen(browserWindow);
+
+      if (startHidden) return;
       browserWindow.show();
       browserWindow.focus();
     });
@@ -957,6 +967,61 @@ export class OverlayManager {
   }
 }
 
+/**
+ * Correct a window that has ended up where no display covers it.
+ *
+ * Validating the saved bounds on the way in only guards the restore path. This
+ * checks where the window actually landed, so it also catches a window placed
+ * off-screen by something other than a stale saved position — Electron's own
+ * default placement, or a display set that was not fully enumerated when the
+ * window was created. #539 was reported on a freshly installed Windows with no
+ * saved bounds at all, which the restore-path check cannot explain, so the
+ * guard is deliberately cause-agnostic: it asks only whether the window can be
+ * reached, never why it could not be.
+ */
+function ensureWindowOnScreen(browserWindow: BrowserWindow): void {
+  const actual = browserWindow.getBounds();
+  const corrected = sanitizeWindowBounds(
+    actual,
+    screen.getAllDisplays().map((display) => display.workArea),
+    screen.getPrimaryDisplay().workArea
+  );
+
+  if (!corrected) return;
+  if (corrected.x === actual.x && corrected.y === actual.y) return;
+
+  logger.warn(
+    `[OverlayManager] Settings window opened off-screen at x=${actual.x}, ` +
+      `y=${actual.y}; moved to x=${corrected.x}, y=${corrected.y}`
+  );
+
+  // Flagged before the move so the saved position survives it. Rescuing a
+  // window must not overwrite where the user put it, or reconnecting the
+  // monitor would no longer bring it back.
+  markCorrectedBounds(browserWindow, corrected);
+  browserWindow.setBounds(corrected);
+}
+
 function loadWindowBounds(): Electron.Rectangle | undefined {
-  return readData<Electron.Rectangle>('settingsWindowBounds');
+  const saved = readData<Electron.Rectangle>('settingsWindowBounds');
+  if (!saved) return undefined;
+
+  // Saved bounds outlive the display arrangement that produced them, so they
+  // are validated against the displays connected right now — otherwise a
+  // window saved on a monitor that has since been unplugged or rearranged is
+  // restored somewhere unreachable.
+  const bounds = sanitizeWindowBounds(
+    saved,
+    screen.getAllDisplays().map((display) => display.workArea),
+    screen.getPrimaryDisplay().workArea
+  );
+
+  if (bounds && (bounds.x !== saved.x || bounds.y !== saved.y)) {
+    logger.info(
+      `[OverlayManager] Saved settings window bounds were off-screen ` +
+        `(x=${saved.x}, y=${saved.y}); moved to x=${bounds.x}, y=${bounds.y}`
+    );
+  }
+
+  return bounds;
 }

src/app/trackWindowMovement.spec.ts

@@ -0,0 +1,146 @@
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
+import type { BrowserWindow } from 'electron';
+import {
+  markCorrectedBounds,
+  trackSettingsWindowMovement,
+} from './trackWindowMovement';
+import { writeData } from './storage/storage';
+
+vi.mock('./storage/storage', () => ({ writeData: vi.fn() }));
+
+const DEBOUNCE_MS = 200;
+
+/**
+ * Minimal stand-in for the parts of BrowserWindow this module uses: it records
+ * the handlers registered for 'moved' and 'resized' so a test can raise them,
+ * and returns whatever bounds the test last set.
+ */
+function fakeWindow(initial: Electron.Rectangle) {
+  const handlers: Record<string, (() => void)[]> = {};
+  let bounds = initial;
+
+  const win = {
+    on: (event: string, handler: () => void) => {
+      (handlers[event] ??= []).push(handler);
+      return win;
+    },
+    getBounds: () => bounds,
+  };
+
+  return {
+    win: win as unknown as BrowserWindow,
+    setBounds: (next: Electron.Rectangle) => {
+      bounds = next;
+    },
+    emit: (event: string) => (handlers[event] ?? []).forEach((h) => h()),
+  };
+}
+
+const ON_SCREEN = { x: 100, y: 100, width: 800, height: 700 };
+const OFF_SCREEN = { x: -20000, y: 250, width: 800, height: 700 };
+const RESCUED = { x: 622, y: 200, width: 800, height: 700 };
+
+describe('trackSettingsWindowMovement', () => {
+  beforeEach(() => {
+    vi.useFakeTimers();
+    vi.mocked(writeData).mockClear();
+  });
+
+  afterEach(() => {
+    vi.useRealTimers();
+  });
+
+  it('saves bounds after the user moves the window', () => {
+    const { win, setBounds, emit } = fakeWindow(ON_SCREEN);
+    trackSettingsWindowMovement(win);
+
+    setBounds({ ...ON_SCREEN, x: 300 });
+    emit('moved');
+    vi.advanceTimersByTime(DEBOUNCE_MS);
+
+    expect(writeData).toHaveBeenCalledWith('settingsWindowBounds', {
+      ...ON_SCREEN,
+      x: 300,
+    });
+  });
+
+  it('saves bounds after the user resizes the window', () => {
+    const { win, setBounds, emit } = fakeWindow(ON_SCREEN);
+    trackSettingsWindowMovement(win);
+
+    setBounds({ ...ON_SCREEN, width: 900 });
+    emit('resized');
+    vi.advanceTimersByTime(DEBOUNCE_MS);
+
+    expect(writeData).toHaveBeenCalledOnce();
+  });
+
+  it('debounces a burst of events into one save', () => {
+    const { win, setBounds, emit } = fakeWindow(ON_SCREEN);
+    trackSettingsWindowMovement(win);
+
+    setBounds({ ...ON_SCREEN, x: 200 });
+    emit('moved');
+    emit('moved');
+    emit('moved');
+    vi.advanceTimersByTime(DEBOUNCE_MS);
+
+    expect(writeData).toHaveBeenCalledOnce();
+  });
+
+  it('does not persist a position the app corrected to', () => {
+    // The window was saved off-screen, so startup rescues it. That rescue must
+    // not overwrite the saved position, or reconnecting the monitor would no
+    // longer bring the window back to where the user had put it.
+    const { win, setBounds, emit } = fakeWindow(OFF_SCREEN);
+    trackSettingsWindowMovement(win);
+
+    markCorrectedBounds(win, RESCUED);
+    setBounds(RESCUED);
+    // Raised explicitly. Electron does not currently emit these for a
+    // programmatic setBounds, so the saved position survives today by accident;
+    // this asserts it survives even if that changes.
+    emit('moved');
+    emit('resized');
+    vi.advanceTimersByTime(DEBOUNCE_MS);
+
+    expect(writeData).not.toHaveBeenCalled();
+  });
+
+  it('resumes saving once the user moves the window themselves', () => {
+    const { win, setBounds, emit } = fakeWindow(OFF_SCREEN);
+    trackSettingsWindowMovement(win);
+
+    markCorrectedBounds(win, RESCUED);
+    setBounds(RESCUED);
+    emit('moved');
+    vi.advanceTimersByTime(DEBOUNCE_MS);
+    expect(writeData).not.toHaveBeenCalled();
+
+    // The user drags it somewhere of their own choosing.
+    const chosen = { ...RESCUED, x: 900, y: 400 };
+    setBounds(chosen);
+    emit('moved');
+    vi.advanceTimersByTime(DEBOUNCE_MS);
+
+    expect(writeData).toHaveBeenCalledWith('settingsWindowBounds', chosen);
+  });
+
+  it('only suppresses the window that was corrected', () => {
+    const a = fakeWindow(ON_SCREEN);
+    const b = fakeWindow(ON_SCREEN);
+    trackSettingsWindowMovement(a.win);
+    trackSettingsWindowMovement(b.win);
+
+    markCorrectedBounds(a.win, RESCUED);
+    a.setBounds(RESCUED);
+    b.setBounds(RESCUED);
+
+    a.emit('moved');
+    b.emit('moved');
+    vi.advanceTimersByTime(DEBOUNCE_MS);
+
+    expect(writeData).toHaveBeenCalledOnce();
+    expect(writeData).toHaveBeenCalledWith('settingsWindowBounds', RESCUED);
+  });
+});

src/app/trackWindowMovement.ts

@@ -3,6 +3,38 @@ import { writeData } from './storage/storage';
 
 const DEBOUNCE_MS = 200;
 
+/**
+ * Bounds the app itself moved a window to, as opposed to the user dragging it.
+ *
+ * A window rescued from off-screen is repositioned with `setBounds`. If that
+ * were persisted it would overwrite the position the user actually chose, and
+ * reconnecting the monitor would no longer bring the window back — turning a
+ * temporary rescue into a permanent move.
+ *
+ * A programmatic `setBounds` does not currently raise `moved` or `resized`, so
+ * nothing is persisted anyway. That is Electron's present behaviour rather than
+ * a guarantee, and nothing else pins it. Recording the corrected rectangle and
+ * declining to save it makes the outcome deliberate instead of incidental.
+ *
+ * A WeakMap so a destroyed window takes its entry with it.
+ */
+const correctedBounds = new WeakMap<BrowserWindow, Electron.Rectangle>();
+
+const sameRect = (a: Electron.Rectangle, b: Electron.Rectangle): boolean =>
+  a.x === b.x && a.y === b.y && a.width === b.width && a.height === b.height;
+
+/**
+ * Record that this window was moved by the app rather than by the user, so the
+ * resulting position is not written back to disk. Call it alongside the
+ * `setBounds` that performs the correction.
+ */
+export const markCorrectedBounds = (
+  browserWindow: BrowserWindow,
+  bounds: Electron.Rectangle
+): void => {
+  correctedBounds.set(browserWindow, { ...bounds });
+};
+
 /**
  * Track settings window position and size changes
  */
@@ -22,5 +54,14 @@ export const trackSettingsWindowMovement = (browserWindow: BrowserWindow) => {
 
 function saveSettingsWindowBounds(browserWindow: BrowserWindow): void {
   const bounds = browserWindow.getBounds();
+
+  // Compared by value rather than suppressed with a flag and a timer: there is
+  // no assumption about when, or whether, the event arrives after setBounds.
+  // A user who drags the window to exactly the corrected position is not saved
+  // either, which costs nothing — that position is on-screen and the next
+  // launch would place the window there regardless.
+  const corrected = correctedBounds.get(browserWindow);
+  if (corrected && sameRect(bounds, corrected)) return;
+
   writeData('settingsWindowBounds', bounds);
 }