feat(macos): add native project workspace shell#374
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThis PR introduces the native SwiftPM macOS app for MatrixOS — a full workspace shell with Kanban board, collapsible sidebar, command palette, retained terminal sessions, file tree, code editor, and Git/artifacts placeholder surfaces. All previously-flagged issues from the review thread have been addressed: the auth-callback poll wake-up, stale token on reconnect,
Confidence Score: 5/5Safe to merge; the terminal session lifecycle, auth flow, and token refresh are all correctly wired after the previous round of fixes. All blocking issues from the prior review have been addressed. The remaining findings are contained quality items: a wrong shortcut hint label in the sidebar, a URLSession allocation pattern that bypasses connection reuse, and a brief gap in the session-name map during concurrent refreshes. None affects data integrity or security. macos/Sources/App/AppModel.swift (gatewayClient caching, sessionAttachNames gap, openAppTab stub) and macos/Sources/App/Workspace.swift (shortcut label mismatch). Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as SwiftUI Views
participant AM as AppModel (@MainActor)
participant PP as PrincipalProvider (actor)
participant WS as ShellWSClient (actor)
participant TS as TerminalSession (@MainActor)
UI->>AM: beginSignIn()
AM->>AM: runSignIn() → pollDeviceAuth()
AM->>PP: setToken(accessToken)
AM->>AM: selectProfile() → BoardStore rebuilt
AM->>AM: refresh() → loadSessions() + board.load()
UI->>AM: openCard(card)
AM->>PP: token() [auth check]
AM->>AM: upsertTab(for: card)
AM->>WS: makeTerminal(wsURL, principal, session, label)
Note over WS: tokenProvider closure captures principal ref
AM->>TS: session.start()
TS->>WS: connect()
loop reconnect loop
WS->>PP: tokenProvider() [fresh token per attempt]
WS->>WS: makeRequest(fromSeq)
WS->>WS: transport.open(request)
WS-->>TS: events (attached / output / reconnecting)
end
TS-->>UI: "@Published connectionState / lastSeq"
UI->>AM: closeTab(id)
AM->>AM: shutdownTerminalSession(for: id) [unconditional]
AM->>WS: session.shutdown()
Prompt To Fix All With AIFix the following 4 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 4
macos/Sources/App/Workspace.swift:335
**Sidebar shortcut hint shows "⌘T" for New Session, but no ⌘T shortcut is registered**
`OperatorCommands` binds "New Session" to `⌘⇧N`. The sidebar label `"⌘T"` is therefore incorrect — users who act on the hint will find it non-functional. The correct hint string should be `"⌘⇧N"` to match the registered binding.
### Issue 2 of 4
macos/Sources/App/AppModel.swift:613-615
**`openAppTab(slug:title:)` is a no-op stub that ignores its `title` parameter and never creates a workspace tab**
The function calls `switchPanel(.app(slug: slug))` and returns, so any future caller expecting a tab to appear in the tab bar will see nothing. The `title` parameter is silently ignored. Either the implementation should call `upsertTab` with a `.app`-kind `WorkspaceTab` and then `focusTab`, or the method should be removed from the public API until it is implemented.
### Issue 3 of 4
macos/Sources/App/AppModel.swift:569-572
**`gatewayClient()` creates a fresh `URLSession`-backed client on every call, preventing connection reuse**
Each invocation of `gatewayClient()` calls `makeClient(baseURL, principal)`, which allocates a new `URLSession` with an `.ephemeral` configuration. Since this helper is called on every board action (`loadSessions`, `loadPanelData`, `createTask`, `updateTaskStatus`, etc.), many sessions accumulate per screen and connection-pool reuse is lost. Caching the built client alongside the profile (and rebuilding only in `selectProfile`) would fix this.
### Issue 4 of 4
macos/Sources/App/AppModel.swift:689-698
**`sessionAttachNames.removeAll()` before sequential awaits opens a window where `openCard` resolves no attach name**
The map is cleared synchronously, then three `await client.get(…)` calls refill it. At each suspension point, a main-actor task that was enqueued concurrently (e.g., the auto-open task fired by a previous `loadSessions` call, or a user tap) can call `openCard`, which invokes `sessionAttachName(for:)`. With the map empty, `sessionAttachName` falls back to returning the raw session string. For sessions whose actual `attachName` differs from `name`, the WebSocket connects to the wrong identifier. Clearing and rebuilding could instead be done into a local `var byAttachName: [String: String] = [:]` and assigned atomically to `sessionAttachNames` only after all three responses merge.
Reviews (3): Last reviewed commit: "fix(macos): clean up closed terminal ses..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| case "error": | ||
| signIn = .failed("Sign-in failed. Check your connection and try again.") | ||
| default: | ||
| break | ||
| } |
There was a problem hiding this comment.
matrixos://auth?status=approved callback is a no-op
When the browser redirects back to the app after the user approves the device-auth request, this handler re-assigns signIn to the exact same .awaitingApproval value it already holds — the observable state doesn't change, no signal is sent to the polling task, and runSignIn() keeps sleeping for the full interval seconds (default 5 s) before it polls. Users who return from the browser will see the sign-in UI frozen until the next scheduled poll completes. The intent was presumably to trigger an immediate poll (cancel the Task.sleep, or use a CheckedContinuation/AsyncChannel to wake the loop), but the current code does nothing.
Prompt To Fix With AI
This is a comment left during a code review.
Path: macos/Sources/App/AppModel.swift
Line: 431-435
Comment:
**`matrixos://auth?status=approved` callback is a no-op**
When the browser redirects back to the app after the user approves the device-auth request, this handler re-assigns `signIn` to the exact same `.awaitingApproval` value it already holds — the observable state doesn't change, no signal is sent to the polling task, and `runSignIn()` keeps sleeping for the full `interval` seconds (default 5 s) before it polls. Users who return from the browser will see the sign-in UI frozen until the next scheduled poll completes. The intent was presumably to trigger an immediate poll (cancel the `Task.sleep`, or use a `CheckedContinuation`/`AsyncChannel` to wake the loop), but the current code does nothing.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
[codex] Accept. I will make the approved callback wake the polling loop immediately instead of reassigning the same sign-in state.
| openTabs.insert(home, at: 0) | ||
| } | ||
| if select || activeTabID == nil { | ||
| focusTab(id: home.id) | ||
| } | ||
| } | ||
|
|
||
| public func openAppTab(slug: String, title: String) { | ||
| switchPanel(.app(slug: slug)) | ||
| } |
There was a problem hiding this comment.
ShellWSClient at construction — reconnects fail permanently after expiry
openCard captures await principal.token() as a String and passes it to the makeTerminal factory, where it is stored as a let inside ShellWSClient. Every reconnect attempt in runUntilStopped calls makeRequest which reads self.token — the frozen snapshot. Once the token expires, every WebSocket upgrade will be rejected. The client enters an infinite backoff loop at 30 s intervals, the terminal pane shows a static .reconnecting or .error state, and the only recovery is reopening the tab after manually re-signing in. Passing principal (or any TokenProviding) by reference into ShellWSClient — and fetching a fresh token per-reconnect in makeRequest — would fix this without changing the calling convention.
Prompt To Fix With AI
This is a comment left during a code review.
Path: macos/Sources/App/AppModel.swift
Line: 571-580
Comment:
**Stale token baked into `ShellWSClient` at construction — reconnects fail permanently after expiry**
`openCard` captures `await principal.token()` as a `String` and passes it to the `makeTerminal` factory, where it is stored as a `let` inside `ShellWSClient`. Every reconnect attempt in `runUntilStopped` calls `makeRequest` which reads `self.token` — the frozen snapshot. Once the token expires, every WebSocket upgrade will be rejected. The client enters an infinite backoff loop at 30 s intervals, the terminal pane shows a static `.reconnecting` or `.error` state, and the only recovery is reopening the tab after manually re-signing in. Passing `principal` (or any `TokenProviding`) by reference into `ShellWSClient` — and fetching a fresh token per-reconnect in `makeRequest` — would fix this without changing the calling convention.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
[codex] Accept. I will pass a token provider into ShellWSClient and fetch the bearer token when each reconnect request is built.
| guard GatewayError.from(statusCode: http.statusCode) == nil else { | ||
| throw GatewayError.from(statusCode: http.statusCode) ?? .server | ||
| } |
There was a problem hiding this comment.
GatewayError.from(statusCode:) is called twice for the same status code — once to test and once to throw. The second call is redundant. Store the result once.
| guard GatewayError.from(statusCode: http.statusCode) == nil else { | |
| throw GatewayError.from(statusCode: http.statusCode) ?? .server | |
| } | |
| if let error = GatewayError.from(statusCode: http.statusCode) { | |
| throw error | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: macos/Sources/Net/DeviceAuthClient.swift
Line: 75-77
Comment:
`GatewayError.from(statusCode:)` is called twice for the same status code — once to test and once to throw. The second call is redundant. Store the result once.
```suggestion
if let error = GatewayError.from(statusCode: http.statusCode) {
throw error
}
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
[codex] Accept. I will store the GatewayError mapping once and throw that value.
| public func currentToken() -> String? { cached } | ||
|
|
||
| public func token() -> String? { cached } |
There was a problem hiding this comment.
currentToken() and token() return identical values; only token() is required by TokenProviding. The duplicate creates a confusing public surface and any future divergence would be a hard-to-spot bug. Remove currentToken().
| public func currentToken() -> String? { cached } | |
| public func token() -> String? { cached } | |
| public func token() -> String? { cached } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: macos/Sources/Net/PrincipalProvider.swift
Line: 25-27
Comment:
`currentToken()` and `token()` return identical values; only `token()` is required by `TokenProviding`. The duplicate creates a confusing public surface and any future divergence would be a hard-to-spot bug. Remove `currentToken()`.
```suggestion
public func token() -> String? { cached }
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
[codex] Accept. I will remove currentToken() and update the tests to use token().
| swift build --package-path "$PACKAGE_DIR" --product "$APP_NAME" | ||
| BUILD_BINARY="$(swift build --package-path "$PACKAGE_DIR" --show-bin-path)/$APP_NAME" |
There was a problem hiding this comment.
swift build is invoked twice — once to compile and once to resolve the binary path. This doubles build-time on a warm cache and can diverge if the environment changes between calls.
Prompt To Fix With AI
This is a comment left during a code review.
Path: script/build_and_run.sh
Line: 22-23
Comment:
`swift build` is invoked twice — once to compile and once to resolve the binary path. This doubles build-time on a warm cache and can diverge if the environment changes between calls.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
[codex] Accept. I will resolve the SwiftPM binary path once and invoke swift build once.
HamedMP
force-pushed
the
088-macos-native-shell
branch
from
d8ee458 to
784c7bc
Compare
|
[codex] Changes since last review:
Commit: 7ed921b |
| BUILD_DIR="$(swift build --package-path "$PACKAGE_DIR" --product "$APP_NAME" --show-bin-path)" | ||
| BUILD_BINARY="$BUILD_DIR/$APP_NAME" | ||
|
|
There was a problem hiding this comment.
swift build --show-bin-path is a query-only flag: it prints the output directory path and exits without compiling anything. Because no separate swift build invocation exists in this script, $BUILD_BINARY never exists on disk, and the subsequent cp "$BUILD_BINARY" "$APP_BINARY" will always fail with cp: ... No such file or directory. Every mode (run, debug, logs, verify) is broken.
The accepted fix from the previous review was to resolve the path once and call swift build once. The correct approach is to first capture the bin path (which is fast), then actually compile, then copy.
Prompt To Fix With AI
This is a comment left during a code review.
Path: script/build_and_run.sh
Line: 22-24
Comment:
**Build step missing — script always fails with "No such file"**
`swift build --show-bin-path` is a query-only flag: it prints the output directory path and exits without compiling anything. Because no separate `swift build` invocation exists in this script, `$BUILD_BINARY` never exists on disk, and the subsequent `cp "$BUILD_BINARY" "$APP_BINARY"` will always fail with `cp: ... No such file or directory`. Every mode (`run`, `debug`, `logs`, `verify`) is broken.
The accepted fix from the previous review was to resolve the path once and call `swift build` once. The correct approach is to first capture the bin path (which is fast), then actually compile, then copy.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
[codex] Accept. I will split path lookup from the actual compile so the script both resolves the binary path and performs one real swift build before copying.
| let wasActive = activeTabID == id | ||
| openTabs.remove(at: index) | ||
| if !wasActive { return } | ||
| if let session = terminalSessions.removeValue(forKey: id) { | ||
| session.shutdown() | ||
| } | ||
| terminal = nil | ||
| selectedCard = nil | ||
| openError = nil | ||
| let nextIndex = min(index, openTabs.count - 1) |
There was a problem hiding this comment.
When a user closes a background tab (wasActive == false), the function returns early at line 971 without removing the entry from terminalSessions or calling session.shutdown(). The ShellWSClient for that tab keeps reconnecting in the background — holding a live WebSocket and a running Task — for the lifetime of the app. Each background-tab close silently leaks a connection. The cleanup block should run unconditionally for the closed tab's session, with focus-shift logic applied only when the closed tab was active.
Prompt To Fix With AI
This is a comment left during a code review.
Path: macos/Sources/App/AppModel.swift
Line: 963-972
Comment:
**Non-active tab close leaks terminal sessions and WebSocket connections**
When a user closes a background tab (`wasActive == false`), the function returns early at line 971 without removing the entry from `terminalSessions` or calling `session.shutdown()`. The `ShellWSClient` for that tab keeps reconnecting in the background — holding a live WebSocket and a running `Task` — for the lifetime of the app. Each background-tab close silently leaks a connection. The cleanup block should run unconditionally for the closed tab's session, with focus-shift logic applied only when the closed tab was active.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
[codex] Accept. I will make terminal session cleanup unconditional for the closed tab and keep focus changes conditional on whether it was active.
| openTabs.append(tab) | ||
| if openTabs.count > 16 { | ||
| openTabs.removeFirst(openTabs.count - 16) | ||
| } | ||
| } | ||
| activeTabID = tab.id | ||
| return tab.id |
There was a problem hiding this comment.
When openTabs.count > 16, the oldest entries are removed from the openTabs array but their terminalSessions[id] entries are never cleaned up — no session.shutdown() is called. Each evicted tab's ShellWSClient continues its reconnect loop indefinitely. Since the tab is also gone from the UI, the user has no way to close these sessions. The evicted tab IDs should be collected before removal so their sessions can be shut down.
Prompt To Fix With AI
This is a comment left during a code review.
Path: macos/Sources/App/AppModel.swift
Line: 1402-1408
Comment:
**16-tab cap silently evicts tabs without shutting down their sessions**
When `openTabs.count > 16`, the oldest entries are removed from the `openTabs` array but their `terminalSessions[id]` entries are never cleaned up — no `session.shutdown()` is called. Each evicted tab's `ShellWSClient` continues its reconnect loop indefinitely. Since the tab is also gone from the UI, the user has no way to close these sessions. The evicted tab IDs should be collected before removal so their sessions can be shut down.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
[codex] Accept. I will collect evicted tab IDs and shut down/remove any cached terminal sessions for those tabs.
|
[codex] Changes since latest Greptile review:
Commit: b83766d |
1 participant
Summary
specs/086-macos-native-shell/production-launch-plan.mdStack
This PR is stacked on PR #372 (
087-native-auth-callback). Merge/deploy #372 first so the native app can receivematrixos://authcallbacks and attach generated zellij session names with underscores.Invariants
Validation
swift test --package-path macosNotes
This is intentionally a broad initial native package PR. Follow-up stack items should split by platform/gateway onboarding, native persistence/settings, Git workflows, and terminal/editor deepening.