Support deep-link protocol registration for AppImage builds#2743
Draft
gantoine wants to merge 3 commits into
Draft
Support deep-link protocol registration for AppImage builds#2743gantoine wants to merge 3 commits into
gantoine wants to merge 3 commits into
Conversation
…ork stack OAuth token exchange and refresh run in the Electron main process, where the global `fetch` is Node's undici. undici ignores the system proxy and certificate configuration that Chromium honours, so on some Linux setups (e.g. behind a proxy or with a custom trust store) token exchange failed with a bare "fetch failed" even though the renderer's API calls — which already go through Chromium — succeeded. Add an `IHttpClient` platform port backed by Electron's `net.fetch` (Chromium networking) and inject it into `OAuthService`, so main-process requests behave like the renderer's. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014aMsqBjTdturnFTpnTsD1f
…m's network stack" This reverts commit 7b91a4d.
The AppImage build never registered the posthog-code:// protocol handler, so after OAuth at us.posthog.com the browser couldn't hand the posthog-code://callback?code=... redirect back to the app — users had to create the .desktop file and run xdg-mime by hand. app.setAsDefaultProtocolClient is a no-op for AppImages: there is no installed .desktop file for xdg to point at, and the AppImage mount path (/tmp/.mount_*) changes every launch. On AppImage builds we now write a desktop entry to the user applications dir with Exec pointing at the stable $APPIMAGE path and register it as the default handler for each scheme (mirroring the manual xdg-mime default step). Also declare the scheme in the maker's bundled .desktop entry so AppImage integrators (e.g. AppImageLauncher) pick it up. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014aMsqBjTdturnFTpnTsD1f
|
React Doctor found no issues in the changed files. 🎉 Reviewed by React Doctor for commit |
Contributor
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
apps/code/src/main/utils/linux-appimage-protocol.test.ts:62-80
**Prefer parameterised tests for `isAppImage`**
The three cases here — `(linux, APPIMAGE set) → true`, `(linux, APPIMAGE missing) → false`, `(darwin, APPIMAGE set) → false` — all exercise the same predicate with different `(platform, envVar, expected)` inputs, which is the canonical use case for `it.each`. As a single table-driven test it would be easier to extend with new platforms and avoids the repeated `setPlatform` / `process.env` setup boilerplate.
### Issue 2 of 2
apps/code/src/main/services/deep-link/service.ts:55-59
**Dead `.catch()` — `registerAppImageSchemes` never rejects**
`registerAppImageSchemes` swallows every failure path internally: `runXdg` always resolves (warnings are logged, errors are not re-thrown), and the `writeFileSync` / `mkdirSync` catch block returns rather than throwing. The `.catch()` here can therefore never fire, making it a superfluous part. The `void` plus `.catch(log.error)` pattern is idiomatic only when the callee can reject — if you want to surface any future errors here, the callee's error handling would need to change too.
```suggestion
if (isAppImage()) {
void registerAppImageSchemes(schemes);
}
```
Reviews (1): Last reviewed commit: "fix(deep-links): register posthog-code:/..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+62
to
+80
| describe("isAppImage", () => { | ||
| it("is true on linux with APPIMAGE set", () => { | ||
| setPlatform("linux"); | ||
| process.env.APPIMAGE = "/home/u/Apps/posthog_code.appimage"; | ||
| expect(isAppImage()).toBe(true); | ||
| }); | ||
|
|
||
| it("is false when APPIMAGE is not set", () => { | ||
| setPlatform("linux"); | ||
| delete process.env.APPIMAGE; | ||
| expect(isAppImage()).toBe(false); | ||
| }); | ||
|
|
||
| it("is false on non-linux platforms even with APPIMAGE set", () => { | ||
| setPlatform("darwin"); | ||
| process.env.APPIMAGE = "/whatever"; | ||
| expect(isAppImage()).toBe(false); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
isAppImage
The three cases here — (linux, APPIMAGE set) → true, (linux, APPIMAGE missing) → false, (darwin, APPIMAGE set) → false — all exercise the same predicate with different (platform, envVar, expected) inputs, which is the canonical use case for it.each. As a single table-driven test it would be easier to extend with new platforms and avoids the repeated setPlatform / process.env setup boilerplate.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/main/utils/linux-appimage-protocol.test.ts
Line: 62-80
Comment:
**Prefer parameterised tests for `isAppImage`**
The three cases here — `(linux, APPIMAGE set) → true`, `(linux, APPIMAGE missing) → false`, `(darwin, APPIMAGE set) → false` — all exercise the same predicate with different `(platform, envVar, expected)` inputs, which is the canonical use case for `it.each`. As a single table-driven test it would be easier to extend with new platforms and avoids the repeated `setPlatform` / `process.env` setup boilerplate.
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!
Comment on lines
+55
to
59
| if (isAppImage()) { | ||
| void registerAppImageSchemes(schemes).catch((error) => { | ||
| log.error("Failed to register AppImage URL schemes", error); | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
.catch() — registerAppImageSchemes never rejects
registerAppImageSchemes swallows every failure path internally: runXdg always resolves (warnings are logged, errors are not re-thrown), and the writeFileSync / mkdirSync catch block returns rather than throwing. The .catch() here can therefore never fire, making it a superfluous part. The void plus .catch(log.error) pattern is idiomatic only when the callee can reject — if you want to surface any future errors here, the callee's error handling would need to change too.
Suggested change
| if (isAppImage()) { | |
| void registerAppImageSchemes(schemes).catch((error) => { | |
| log.error("Failed to register AppImage URL schemes", error); | |
| }); | |
| } | |
| if (isAppImage()) { | |
| void registerAppImageSchemes(schemes); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/main/services/deep-link/service.ts
Line: 55-59
Comment:
**Dead `.catch()` — `registerAppImageSchemes` never rejects**
`registerAppImageSchemes` swallows every failure path internally: `runXdg` always resolves (warnings are logged, errors are not re-thrown), and the `writeFileSync` / `mkdirSync` catch block returns rather than throwing. The `.catch()` here can therefore never fire, making it a superfluous part. The `void` plus `.catch(log.error)` pattern is idiomatic only when the callee can reject — if you want to surface any future errors here, the callee's error handling would need to change too.
```suggestion
if (isAppImage()) {
void registerAppImageSchemes(schemes);
}
```
How can I resolve this? If you propose a fix, please make it concise.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
AppImage builds lack an installed
.desktopfile, soapp.setAsDefaultProtocolClient()(which points xdg at one) is a no-op. This breaks OAuth callbacks: the browser cannot handposthog-code://callback?...back to the app after authentication.Changes
Added AppImage-specific protocol registration that mirrors manual
xdg-mime defaultsetup:New module
linux-appimage-protocol.ts: Detects AppImage runtime (APPIMAGEenv var), builds freedesktop.desktopentries pointing at the stable$APPIMAGEpath, stages the app icon to a persistent location, and registers URL schemes viaxdg-mime.Updated
DeepLinkService: Collects all schemes (dev/prod, legacy) and callsregisterAppImageSchemes()on AppImage builds. Failures are best-effort and do not block startup.Updated
forge.config.ts: Declaredx-scheme-handler/posthog-codein the bundled.desktopentry so AppImage integrators (e.g., AppImageLauncher) register the handler at install time.The solution is defensive: it only runs on Linux with
APPIMAGEset, handles missing xdg utilities gracefully, and does not interfere with non-AppImage builds.How did you test this?
linux-appimage-protocol.test.ts) covering:isAppImage()detection on Linux with/withoutAPPIMAGE.desktopentry generation with correct Exec path and MIME typesxdg-mime defaultregistrationexecFileto verify correct paths and command sequences without side effectshttps://claude.ai/code/session_014aMsqBjTdturnFTpnTsD1f