Skip to content

[4a/5] feat(desktop): window management primitives#396

Open
johnwschoi wants to merge 1 commit intomainfrom
pr/desktop-6a-window-primitives
Open

[4a/5] feat(desktop): window management primitives#396
johnwschoi wants to merge 1 commit intomainfrom
pr/desktop-6a-window-primitives

Conversation

@johnwschoi
Copy link
Copy Markdown
Contributor

Window creation, state persistence, and multi-window tracking layer.

Split from original PR #372 - Part 1 of 3

Window state persistence (window-state.ts)

  • Per-window bounds, last viewed path, display affinity
  • Saved to userData on close, restored on next launch

Multi-window tracking (window-registry.ts)

  • Centralized registry for all BrowserWindow instances
  • Focus tracking, lifecycle events
  • Batch operations (loadUrlInAllWindows, saveAllStates)

Window factory (window-factory.ts)

  • Creates BrowserWindows from persisted state or defaults
  • Handles disconnected screen transitions
  • Security boundary via will-navigate origin checks

Files: 7 new files, 1770 lines
Stack: PR #371#372a → #372b → #372c → #373

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Apr 24, 2026

Greptile Summary

This PR introduces the window management layer for the Kanban desktop app: per-window state persistence (window-state.ts), a centralized window registry (window-registry.ts), and a factory that wires them together (window-factory.ts). All three P0/P1 issues from previous review rounds — off-screen window placement, Promise.all short-circuiting, and unvalidated shell.openExternal — are fully addressed in this revision.

Confidence Score: 5/5

Safe to merge — all previously raised P0/P1 issues are resolved and no new critical issues were found.

All three blocking findings from earlier rounds (off-screen placement, Promise.all short-circuit, unvalidated openExternal) are properly addressed. Security fundamentals are solid: contextIsolation, sandbox, origin-pinned will-navigate guard, http/https-only setWindowOpenHandler, and input validation on persisted state. Atomic writes and the MAX_RESTORED_WINDOWS cap address data-integrity and DoS concerns. Test coverage is comprehensive with real-FS and mock-Electron suites.

No files require special attention.

Important Files Changed

Filename Overview
packages/desktop/src/window-state.ts Solid persistence layer: atomic writes via tmp+rename, validation of all loaded fields, MAX_RESTORED_WINDOWS cap, and clampBoundsToDisplays for off-screen recovery.
packages/desktop/src/window-registry.ts Centralized registry with correct macOS hide-on-close semantics, Promise.allSettled in loadUrlInAllWindows, origin-pinned will-navigate guard, and http/https-only setWindowOpenHandler.
packages/desktop/src/window-factory.ts Window factory with renderer recovery (did-fail-load + render-process-gone), isSafeInitialPath guard on initialPath, and disconnected-screen broadcast.
packages/desktop/test/window-registry.test.ts Comprehensive mock-based tests covering will-navigate guard edge cases, macOS close semantics, visibility helpers, and multi-window creation.
packages/desktop/test/window-state.test.ts Thorough round-trip, atomic-write, clampBoundsToDisplays, and MAX_RESTORED_WINDOWS tests with real filesystem I/O and proper temp-dir cleanup.

Sequence Diagram

sequenceDiagram
    participant App as App (main)
    participant WF as WindowFactory
    participant WR as WindowRegistry
    participant WS as window-state.ts
    participant BW as BrowserWindow
    participant Renderer

    App->>WS: loadAllWindowStates(userDataPath)
    WS-->>App: PersistedWindowState[]

    loop for each saved state
        App->>WF: create({ savedState })
        WF->>WR: createWindow({ savedState, preloadPath, ... })
        WR->>WS: clampBoundsToDisplays(savedState, screen.getAllDisplays())
        WS-->>WR: clamped bounds
        WR->>BW: new BrowserWindow({ x, y, width, height })
        WR->>WR: attach will-navigate origin guard
        WR->>WR: attach setWindowOpenHandler (http/https only)
        WR->>WR: attach macOS hide-on-close
        WR-->>WF: BrowserWindow
        WF->>WF: attachRendererRecovery(window)
        WF->>BW: loadURL(runtimeUrl)
        WF-->>App: BrowserWindow
    end

    Note over App,Renderer: Runtime connects / reconnects
    App->>WR: loadUrlInAllWindows(baseUrl)
    WR->>WR: buildEntryUrl (uses lastViewedPath or projectId)
    WR->>BW: loadURL(url) per window [Promise.allSettled]

    Note over Renderer,BW: Renderer-initiated navigation
    Renderer->>BW: will-navigate(url)
    BW->>WR: handler: check currentUrl origin
    alt same origin and http/https
        WR-->>BW: allow
    else cross-origin or non-http
        WR->>BW: event.preventDefault()
    end

    Note over App,WS: App quit
    App->>WR: saveAllStates(userDataPath)
    WR->>WS: saveAllWindowStates(states) [atomic: tmp -> rename]
Loading

Reviews (9): Last reviewed commit: "[4a/5] feat(desktop): window management ..." | Re-trigger Greptile

Base automatically changed from pr/desktop-5-preflight to main April 24, 2026 17:30
@johnwschoi johnwschoi requested review from arafatkatze, maxpaulus43 and saoudrizwan and removed request for arafatkatze, maxpaulus43 and saoudrizwan April 24, 2026 17:32
@johnwschoi johnwschoi force-pushed the pr/desktop-6a-window-primitives branch from ffc697e to 3a2e830 Compare April 24, 2026 17:36
maxpaulus43
maxpaulus43 previously approved these changes Apr 24, 2026
johnwschoi added a commit that referenced this pull request Apr 25, 2026
The `branches: [main]` filter on `pull_request` excludes PRs whose
base is a sibling branch — meaning stacked PRs (#397 -> #396, #398 ->
#397, etc.) report zero checks until they're collapsed onto main.
Add `pr/**` to the allow-list so each link in the stack gets the
same typecheck/test signal as a top-level PR.
johnwschoi added a commit that referenced this pull request Apr 25, 2026
The `branches: [main]` filter on `pull_request` excludes PRs whose
base is a sibling branch — meaning stacked PRs (#397 -> #396, #398 ->
#397, etc.) report zero checks until they're collapsed onto main.
Add `pr/**` to the allow-list so each link in the stack gets the
same typecheck/test signal as a top-level PR.
johnwschoi added a commit that referenced this pull request Apr 25, 2026
The `branches: [main]` filter on `pull_request` excludes PRs whose
base is a sibling branch — meaning stacked PRs (#397 -> #396, #398 ->
#397, etc.) report zero checks until they're collapsed onto main.
Add `pr/**` to the allow-list so each link in the stack gets the
same typecheck/test signal as a top-level PR.
johnwschoi added a commit that referenced this pull request Apr 25, 2026
The `branches: [main]` filter on `pull_request` excludes PRs whose
base is a sibling branch — meaning stacked PRs (#397 -> #396, #398 ->
#397, etc.) report zero checks until they're collapsed onto main.
Add `pr/**` to the allow-list so each link in the stack gets the
same typecheck/test signal as a top-level PR.
@johnwschoi johnwschoi requested a review from maxpaulus43 April 27, 2026 15:02
Comment thread .github/workflows/ci.yml
@johnwschoi johnwschoi force-pushed the pr/desktop-6a-window-primitives branch 2 times, most recently from 31ab548 to d3e2686 Compare April 27, 2026 17:23
@johnwschoi johnwschoi force-pushed the pr/desktop-6a-window-primitives branch from d3e2686 to 26f7757 Compare April 27, 2026 20:40
@robinnewhouse
Copy link
Copy Markdown
Collaborator

@greptileai

Comment thread packages/desktop/src/window-registry.ts
Window creation, state persistence, and multi-window tracking layer.

**Window state persistence** (window-state.ts)
- Per-window bounds, last viewed path, display affinity
- Saved to userData on close, restored on next launch

**Multi-window tracking** (window-registry.ts)
- Centralized registry for all BrowserWindow instances
- Focus tracking, lifecycle events
- Batch operations (loadUrlInAllWindows, saveAllStates)

**Window factory** (window-factory.ts)
- Creates BrowserWindows from persisted state or defaults
- Handles disconnected screen transitions
- Security boundary via will-navigate origin checks

**Supporting files**
- disconnected.html: Native fallback UI for runtime crashes
- preload.ts: Minimal security bridge (IPC channel setup)

Part 1 of 3-way split from original PR #372.
Next: runtime-orchestrator (372b), then main wiring (372c).
@johnwschoi johnwschoi force-pushed the pr/desktop-6a-window-primitives branch from 26f7757 to 3e89365 Compare April 28, 2026 00:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants