Skip to content

Commit c888570

Browse files
committed
Fix desktop release review feedback
1 parent 79fa0ff commit c888570

4 files changed

Lines changed: 102 additions & 6 deletions

File tree

.reviews/desktop-github-releases.md

Lines changed: 39 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,15 +4,52 @@
44

55
| Field | Value |
66
| ------------- | -------------------- |
7-
| Last reviewed | 2026-05-22 12:54 BST |
8-
| Total turns | 1 |
7+
| Last reviewed | 2026-05-22 13:24 BST |
8+
| Total turns | 2 |
99
| Open findings | 0 |
1010

1111
## Hotspots
1212

1313
- macOS update feed requires both GitHub release assets and packaged `app-update.yml`.
1414
- Public macOS auto-install still requires Developer ID signing and notarization.
1515
- Desktop/web compatibility depends on hosted API contract compatibility across at least one previous desktop release.
16+
- Mac release artifacts must stay pinned to the stable arm64 asset contract until universal/x64 distribution is intentionally designed.
17+
18+
## Turn 2 - 2026-05-22 13:24 BST
19+
20+
**Outcome:** No open Critical/High findings after importing the Codex PR review and fixing the two live review items.
21+
22+
**Risk:** High. The external findings touched the macOS release contract and the desktop/API compatibility headers that support server-driven minimum-version policy.
23+
24+
**Archetypes:** external PR finding import, release artifact contract, transient IPC failure recovery, desktop/web compatibility, operational preflight.
25+
26+
**Finding import:**
27+
28+
| Source | Finding | Current status | Bug class | Missed invariant / variant | Action |
29+
| ------ | ------- | -------------- | --------- | -------------------------- | ------ |
30+
| Codex PR review | Release packaging used `--mac` without an explicit architecture while public asset names and URLs are arm64-specific | Resolved | Contract Encoding / Compatibility | The published artifact architecture must match the stable updater/download contract on every build host | Added `--arm64` to the `electron-builder` invocation and rebuilt the release path once to confirm `arch=arm64` artifacts. |
31+
| Codex PR review | A transient `getDesktopAppInfo()` bridge rejection cached a rejected promise for the rest of the session | Resolved | Lifecycle And Transient Containers / Compatibility | Desktop version/platform header discovery must retry after transient IPC failure | Reset the cached app-info promise on rejection and added a regression test proving the second request recovers and sends version/platform headers. |
32+
33+
**Architecture review:** The arm64 guarantee belongs in the packaging script because the script owns the release artifact contract. The retry fix stays in `lib/browser/desktop-auth-token.ts`, the narrow browser bridge boundary that assembles desktop API headers. No server secrets or update policy rules moved into the Electron bundle.
34+
35+
**Branch-totality and sibling closure:** Rechecked the release script, publisher/preflight expectations, desktop update policy headers, preload app-info IPC shape, and the prior desktop GitHub release hotspots. The same app-info helper is used by route mutations and fetch-backed event streams, so both recover from the retry fix.
36+
37+
**Static/analyzer evidence:** `diff-review` and `architecture-standards` preflights were rerun. `fallow audit --changed-since origin/main` still reports advisory complexity/duplication in the broader desktop release scripts and Electron main process; this is not a zero-static-debt branch, and CI currently treats Fallow as advisory (`continue-on-error`). The latest PR review fixes did not add new branchy logic beyond resetting the cached promise on failure and pinning the builder architecture.
38+
39+
**Verification:**
40+
41+
- `pnpm vitest run tests/lib/browser/desktop-auth-token.test.ts tests/electron/desktop-updates.test.ts tests/desktop/renderer-smoke.test.tsx tests/lib/desktop-update-policy.test.ts`
42+
- `pnpm lint`
43+
- `pnpm typecheck`
44+
- `pnpm desktop:release:mac` - ran once before the merge-only rebuild policy was clarified; confirmed electron-builder used `arch=arm64` and emitted the stable `Recipe-Room-mac-arm64.*` artifacts.
45+
- `DESKTOP_RELEASE_ARTIFACTS=1 pnpm desktop:release:preflight`
46+
- `pnpm build`
47+
- `git diff --check`
48+
- `~/.codex/skills/diff-review/scripts/review-preflight.sh`
49+
- `~/.codex/skills/architecture-standards/scripts/architecture-preflight.sh`
50+
- `pnpm exec fallow audit --changed-since origin/main --format compact --quiet`
51+
52+
**Residual risk:** No GitHub Release was published and no final Electron distribution artifact should be treated as release-ready until after PR merge and a fresh main-branch rebuild. Preflight still reports the expected public-release pending items for Developer ID signing, Gatekeeper acceptance, notarization, and hosted/dashboard checks.
1653

1754
## Turn 1 - 2026-05-22 12:54 BST
1855

lib/browser/desktop-auth-token.ts

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,13 @@ async function getElectronDesktopAuthToken() {
1414
}
1515
}
1616

17-
let desktopAppInfoPromise: ReturnType<
18-
NonNullable<NonNullable<Window["electronApp"]>["getDesktopAppInfo"]>
19-
> | null = null
17+
type DesktopAppInfo = Awaited<
18+
ReturnType<
19+
NonNullable<NonNullable<Window["electronApp"]>["getDesktopAppInfo"]>
20+
>
21+
>
22+
23+
let desktopAppInfoPromise: Promise<DesktopAppInfo> | null = null
2024

2125
async function getElectronDesktopAppInfo() {
2226
if (
@@ -27,7 +31,12 @@ async function getElectronDesktopAppInfo() {
2731
return null
2832
}
2933

30-
desktopAppInfoPromise ??= window.electronApp.getDesktopAppInfo()
34+
desktopAppInfoPromise ??= window.electronApp
35+
.getDesktopAppInfo()
36+
.catch((error) => {
37+
desktopAppInfoPromise = null
38+
throw error
39+
})
3140

3241
return desktopAppInfoPromise.catch(() => null)
3342
}

scripts/package-electron-mac.mjs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -451,6 +451,7 @@ async function main() {
451451
"--projectDir",
452452
stageDir,
453453
"--mac",
454+
"--arm64",
454455
"--publish",
455456
"never",
456457
],
Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"
2+
3+
describe("desktop auth headers", () => {
4+
const originalElectronApp = window.electronApp
5+
6+
beforeEach(() => {
7+
vi.resetModules()
8+
})
9+
10+
afterEach(() => {
11+
Object.defineProperty(window, "electronApp", {
12+
configurable: true,
13+
value: originalElectronApp,
14+
})
15+
vi.restoreAllMocks()
16+
})
17+
18+
it("retries desktop app info after a transient bridge failure", async () => {
19+
const getDesktopAppInfo = vi
20+
.fn()
21+
.mockRejectedValueOnce(new Error("ipc failed"))
22+
.mockResolvedValueOnce({
23+
platform: "darwin",
24+
version: "1.2.3",
25+
})
26+
27+
Object.defineProperty(window, "electronApp", {
28+
configurable: true,
29+
value: {
30+
getDesktopAppInfo,
31+
getDesktopAuthToken: vi.fn().mockResolvedValue("desktop_token"),
32+
isElectron: true,
33+
platform: "darwin",
34+
},
35+
})
36+
37+
const { buildDesktopAuthHeaders } =
38+
await import("@/lib/browser/desktop-auth-token")
39+
40+
const firstHeaders = await buildDesktopAuthHeaders()
41+
const secondHeaders = await buildDesktopAuthHeaders()
42+
43+
expect(getDesktopAppInfo).toHaveBeenCalledTimes(2)
44+
expect(firstHeaders.get("X-Recipe-Room-Desktop-Version")).toBeNull()
45+
expect(firstHeaders.get("Authorization")).toBe("Bearer desktop_token")
46+
expect(secondHeaders.get("X-Recipe-Room-Desktop-Version")).toBe("1.2.3")
47+
expect(secondHeaders.get("X-Recipe-Room-Desktop-Platform")).toBe("darwin")
48+
})
49+
})

0 commit comments

Comments
 (0)