Validation only (do not merge): trunk merge for #358 - #391
Closed
mokagio wants to merge 4 commits into
Closed
Conversation
The site links each cancel their own navigation in an onClick handler. A middle click fires auxclick, which a click handler never sees, so Chromium's "open in a new window" default ran and the site loaded inside a bare app window with no address bar and no way back. Refuse navigation and window creation once for the app window instead, and hand the address to the system browser. Every link is covered, including ones added later, and the address goes out through the same external-url.js gate the renderer already uses. Fixes #284
Two findings from the review pass on this branch. The window guard only intercepted http and https, so anything else was left to navigate the main window. That window is the one with the preload bridge attached, and it renders content the app does not author — captured email bodies among it — where a link can point at a local path or at one relative to the app's own file: origin. It now refuses every navigation except a reload, which is the one case that has to keep working; a plain deny breaks reloading the window. The onFailed reporter also only covered the link path. openExternalUrl ignored it and let a rejecting openExternal propagate out of the url:open handler, where none of the renderer's 23 call sites catches anything — so an ordinary click on an address the OS cannot open was still a link that did nothing with nothing in the log. Reporting moved into openExternalUrl, which is where both paths meet. Tests for both fail on the previous code.
Both were out of scope for #284. The navigation hole they closed — the app window following a file: or relative link out of its own page — is on trunk already and is not something this fix introduced, and the missing failure log on the url:open path is a gap in external-url.js rather than in the link handling. Neither belongs in a PR that fixes one reported bug. Both are worth doing on their own; noted in the PR description so they are not lost.
Trunk moved every test into `tests/unit/` in #377, and this branch predates that, so the merge raises a `CONFLICT (file location)` for the new `test/window-links.test.cjs`. Git relocates the file on its own; it does not rewrite what is inside it. Accepting the move alone leaves `require('../src/window-links.js')` pointing one directory too high, and the test throws `MODULE_NOT_FOUND` the first time it runs. This resolution accepts the relocation and takes the require to `../../src/window-links.js`, the depth every other file in `tests/unit/` already uses. --- Generated with the help of Claude Code, https://code.claude.com Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Validates merging #358 onto current trunk after tests moved to tests/unit/. No findings identified.
Changes:
- Corrects the relocated test’s source import depth.
- Retains #358’s external browser link handling and coverage.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/main.js |
Wires external link handling into the main window. |
src/window-links.js |
Routes web links to the system browser. |
tests/unit/window-links.test.cjs |
Tests link handling using the corrected import path. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Collaborator
Author
|
Test completed. Closing. |
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
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.
Why
Every open PR needs
trunkmerged in before the Buildkite pipeline is unarchived (AINFRA-2941), otherwise a signed build off a stale branch re-embeds the App Store Connect key that #390 just stopped leaking.#358 looks like the easy one — GitHub reports it
MERGEABLE, 12 commits behind. It is not. Trunk moved every test intotests/unit/in #377, and #358 addstest/window-links.test.cjsunder the directory that no longer exists. Merging trunk raises aCONFLICT (file location); git relocates the file itself but does not rewrite it, sorequire('../src/window-links.js')ends up one directory too high and the test throwsMODULE_NOT_FOUND.Nothing would have caught that. The Buildkite pipeline is archived, so the failure would have surfaced later on someone else's PR.
What changes
Nothing of its own. This is #358's head with
trunkmerged in and the require taken to../../src/window-links.js— the depth every other file intests/unit/already uses.The point is to prove that shape on CI before pushing it to a contributor's fork, and to find out whether the same trap is waiting in the other branches that still carry a top-level
test/— #255's stack and #277.How to test this
Platforms: macOS and Windows — the change is a directory depth, so the Windows leg is the one worth watching.
Starting state: this branch's GitHub Actions run against the current head commit.
tests/unit/window-links.test.cjsis among the tests that ran.What must not have happened: the relocated test must not be silently skipped. A green run where nothing executed
window-linksproves nothing, which is the whole failure mode this PR is checking for.Risks and limitations
This cannot exercise macOS signing or notarization with the new App Manager key — that lives in the Buildkite pipeline, which is archived. It does cover the packaged smoke job's
.codesigningcanary from #390, which runs on GitHub Actions without secrets.Opened as a normal PR rather than a draft on purpose:
e2e.ymlskips drafts, so a draft would come back green having run neither the journeys nor the packaged jobs.Related
Part of AINFRA-2941. Validates the trunk merge for #358; follow-up to #390.