Repository navigation
Keep the main window on the app's own page, and draw a mail in a frame of its own - #571
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: WordPress/contributor-toolkit/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (13)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe main window now stays on its current page and routes other navigation attempts through external URL handling. Email HTML now renders in an empty-sandbox iframe instead of in the app document. The changes add unit and end-to-end coverage for these behaviors and update renderer lint rules and testing guidance. Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The change keeps outside navigation out of the app window and isolates HTML mail. Inactive mail links are an intentional limitation. No concrete issue requiring correction before merge remains; normal checks still apply. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The changes reduce exposure by keeping the app window on its own page and separating email content from the app interface. No introduced security weakness was established. Remaining uncertainty concerns runtime enforcement of the email frame boundary and failure-path validation. Retained concerns Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
## Why `AGENTS.md`, in its note about the Electron pin, says the Playground server crashes "about a second after "Start dev server"" and that the packaged smoke "never clicks "Start dev server"". Since the redesign (#595) the control in the page's header and in the details reads "Start development server". Someone following the note to check an Electron bump would look for a button that is not there. #617 corrected the same words in `TESTING.md` and left this file for a decision, since it is the project's instructions. ## What changes Two words, twice, in one line of `AGENTS.md`: "Start dev server" becomes "Start development server". Nothing else. The setup checklist's own button still reads "Start dev server and finish the wizard"; that is a different control, and the note is about starting a server on a site that is set up. ## How to test this Platforms: any. Nothing runs differently: it is prose. To see that the name is the control's: ``` git grep -n "Start development server" src/renderer/site-processes.cjs tests/e2e/helpers/ui.cjs ``` ## Risks and limitations - Review: not run by a separate reviewer. See below. - #571 changes one line of `AGENTS.md` too; if it is the same line, one of the two will need the other's words when trunk is merged across. ## Related Part of #542. Follows #617, before #591. --- <details> <summary>Review outcome (required — see AGENTS.md)</summary> - **Review:** not run — the change is two words repeated in one line of prose, at the maintainer's request. I checked that the line is still one line, that the name is the one `src/renderer/site-processes.cjs` gives the control, and that no other tracked doc still uses the old name (`git grep "Start dev server" AGENTS.md TESTING.md CONTRIBUTING.md .github`). - **Since review:** none. </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
## Why #542 replaces the app's interface with the one in the [prototype](https://github.com/jameskoster/wordpress-contributor-toolkit-prototype), drawn with the WordPress Design System. It was built in slices on `redesign/wpds`, so that trunk, which releases are cut from, never held a half-redesigned window. The window is whole now: the shell, the page header and details, every card and dialog, and the tray. This pull request is how the branch reaches trunk. It lands before the last three sub-issues, on purpose. The settings dialog (#559) and dark mode (#560) add to a finished window and do not need a branch to hide in, and the guide (#561) is easier to rewrite against what is on trunk. They follow as pull requests against trunk. ## What changes Everything `redesign/wpds` holds; this pull request adds nothing of its own. Each slice was reviewed in its own pull request against that branch, and says there how to test it and what it leaves out. - **The shell** (#555): a sites list, the page area and a footer, in a larger window. #590 - **The page header and the details** (#556): the site's menu in the header, and what the checkout is in a sidebar that can be put away. #594 - **The site's page** (#557): the server and the build watch in the header and the details (#595); the ticket card, the apply card with a preview dialog, and the list of a site's tickets (#596, #597, #599); the create-site, rename and delete dialogs (#600, #601); confirmations as notices in the window's corner (#602); the page's notices, update card and setup checklist (#605); the terminal, logs, mail and the review dialogs painted with the design system's tokens (#606, #608); and lint rules that keep it so (#609, #610). - **The tray** (#558): along the bottom of the window, holding the terminal, the logs or the mail, with a count of unseen debug.log lines on the footer (#612, #613, #614, #615). - Along the way: #611 (the site menu's update item says why it is held), and three that tidy up before landing: #616 (a journey's race with the page's clock), #617 (comments the finished redesign made untrue) and #618 (the server's button by its present name in `AGENTS.md`). **What a contributor will notice first:** the terminal, the logs and the mail are no longer on the page but behind three buttons at the foot of the window; the terminal is dark text on light; the server and the build watch are started from the header; a patch is previewed in a dialog before it is applied; and what worked is said in the window's corner. **Not in this pull request:** - The settings dialog (#559). Until it, nothing about how the window is arranged is remembered across restarts: the details, the tray's height, the apply card's fold. - Dark mode (#560). The terminal reads its colours once, when it is made, and will need to follow the theme. - The user guide and its screenshots (#561), which describe the old interface. They are published from release tags, so they are not wrong until a release ships this; #561 is next, ahead of #559 and #560, for that reason. - How a mail is drawn, and how the window handles a link. Both are unchanged and tracked separately. ## How to test this Platforms: macOS and Windows, both. Buildkite builds signed artifacts for this pull request; use the build for its current head, and check that it is: a push to the branch makes earlier builds stale. The suite, from the repository root, after `npm ci`: ``` npm run lint npm test npm run test:e2e ``` 113 journeys walk every slice on macOS and Windows on each push. What they do not do is run a real site: a clone of `wordpress-develop`, an install, a build and a server are stood in for. The one test that does, `real-setup.yml`, is run by hand and was run on this branch at `548c020` (https://github.com/WordPress/contributor-toolkit/actions/runs/37298667056): a Core site on macOS and on Windows and a Gutenberg site on macOS were created, installed, built and served, and passed. **The Gutenberg site on Windows did not finish.** Its first attempt was cancelled after 74 minutes inside the journey, a minute short of the limit that would have failed it with evidence, so it says nothing about where it was; a second attempt was still running when this was written and its result is not in this description. The commits since `548c020` change a journey, comments and prose. **A pass by hand, on a real machine** (any platform; Apple Silicon for the server, which no test on a pull request starts for real). It follows the order a contributor meets things in: 1. Open the app with no sites. **Create new site**, a Core site, in a folder with a space in its path. The setup checklist runs its steps; the terminal's button at the foot of the window shows what each is doing. 2. Let the checklist's last step start the server, and finish WordPress's own setup in the browser. Back in the app the header says "Server running" and "Build watching". Stop the server and start it again: from **Server running** in the header, or with **Stop** and **Start** beside "Server" in the details. 3. Press the three buttons at the left of the footer in turn. Terminal: type `help`. Logs: the server's lines, the build watch's, and Debug.log. Email: where SMTP is listening. Drag the tray's top edge. 4. Link a ticket by its number. The card shows the ticket and its pull requests and patches. 5. Apply one of them. The preview dialog names the files; **Apply and rebuild** runs the steps on the card. 6. Edit a file under `src/`, then **Review & submit changes**. The diff is the edit and nothing else. 7. Create a second site, or open one from before, and move between the two from the sites list. Each has its own terminal, logs and mail. 8. **Update to latest trunk** from the site's menu. **What must not have happened:** work lost in a link, an apply, a switch or an update that did not say so; `node_modules` rebuilt where nothing changed; a failure said only in a tray that stayed closed; one site's output in another's tray; anything on the page under the tray. **What cannot be tested by hand, and what was not:** none of the slices' manual steps were run by a person. I looked at screenshots and drove the app through the journeys; everything local was on macOS, with Windows left to CI. This pass is the first one by hand. ## Risks and limitations - **It is one commit on trunk**, since the repository allows only squash merges. The slices' pull requests are its record. `redesign/wpds` should not be branched from after this: it will have diverged from trunk. - **Reviewed in slices, not as a whole.** Twenty-two pull requests, all but a two-word change to `AGENTS.md` reviewed by a separate reviewer context before they were opened, most in two passes; see below. The branch was then swept once for what falls between slices, which found comments and one unread field (#617) and the three points that follow. - **Three questions and two failures still use the system's own dialogs**: the discards ask through its confirm box, and a path that could not be copied and a server started before there is a build are said in its alert. #557 meant to replace them and did so for deletions only. - **Some dialogs are repainted, not rebuilt.** The review dialog and its destinations, the mail dialog and the question an update asks about loose edits have the design system's colours and type on the old component library's parts. - **A real Gutenberg setup on Windows is not shown to pass on this branch.** See "How to test this". It passed on trunk before the redesign, in 35 minutes; here it was past 46 on its second attempt. Whether that is the runner or the branch is not known. - **The guide describes the old interface** until #561. Do not cut a release before it. - **The bundle roughly doubled** (#590): the sites list brings `DataViews` and what it needs. Launch time was not measured. - **Less is always in view.** A build's output, a new line in debug.log and a caught mail are in a tray that may be closed. A failure opens the terminal, or the logs if nothing else is open; unseen debug.log lines are counted on the footer; a mail that arrives shows nowhere until its tray is opened, which was decided and is not an oversight. - **Translation is about half done.** The shell, the cards, the tray and most dialogs go through the translator. Roughly 200 strings in components and hooks do not yet (the review dialog and its destinations, the setup checklist, the logs, most of what the hooks say), nor about 150 in the modules they take sentences from. Strings that are new are in English in the shipped translations until they are translated. - **Focus is lost in a few places**: when the app swaps what the tray holds from under it, when a notice with a focused button goes, when a row's own action removes the row. - **Some failures are still worded by the main process** ("Error invoking remote method…"): a failed rename, a failed setup. - **Things carried over, not made by this:** a start that throws leaves the server's button dead until a restart; erasing typed input in the terminal breaks once the line wraps; a line written to debug.log while it is being attached to can be shown twice. - **#571 and #603 will conflict with trunk after this**, in the mail's journey and helpers and in the dev server's hook. - Each slice's own list has more, and says which of its limits were chosen. ## Related Closes #557. Closes #558. Part of #542, which stays open for #559, #560 and #561. #555 and #556 are closed already. --- <details> <summary>Design decisions and alternatives considered</summary> - **An integration branch, and one landing.** A window drawn half by the old view and half by the new one could not be released, and the slices were too many to hold back one by one behind a flag. Fixes and changes with no visible effect went to trunk first and were merged across, so this diff is the redesign and little else. - **Landing before #559 to #561.** The plan was to land when all seven sub-issues were in the branch. The remaining three do not change what is there: two add to it and one documents it. Holding a finished window on a branch for them only lengthens the time trunk and the branch can drift apart. - **What the prototype lacks is kept and redrawn**, and what it has somewhere else moves to where it puts it. Its labels are adopted. Where a slice departs from it, the slice says so. </details> <details> <summary>Review outcome (required — see AGENTS.md)</summary> **No review of the whole diff was run. The record is the slices': twenty-two pull requests into `redesign/wpds`, each but one reviewed before it was opened by a separate, read-only reviewer context following `.github/instructions/code-review.instructions.md`, most in two passes (the change, then its fix-up), with the counts and the findings in each pull request's own review outcome. The one is #618, two words in a line of prose.** - **Review:** completed, per slice — #590, #594 to #602, #605, #606, #608 to #617 (no review was run for #618); everything marked to fix in a slice was fixed in that slice, and what was left is in its Risks. - **Review:** completed, for leftovers only — a separate agent context, read-only; `redesign/wpds` at `548c020` against trunk; not a review of correctness, which the slices had: a search for dead CSS and code, comments that waited for a slice that has landed, what is left of the old component library and of the system's dialogs, strings not yet translatable, and repository docs that name the branch. Nothing found in the stylesheet, the dependencies or the docs; thirteen comments and one unread field, fixed in #617; the rest is in Risks above. - **CodeRabbit:** not run. Its check reads "Review skipped: automatic reviews are disabled" on this pull request and on the slices, so the slices' pre-PR reviews are the reviews of record, each for the head it names. - **Since review:** nothing was added to the branch outside a reviewed slice, apart from three merges of trunk (`da19ac6`, `3344172`, `23d46e7`). </details> <details> <summary>Implementation notes</summary> - `git diff origin/trunk...origin/redesign/wpds`: 148 files, about 14,800 lines added and 3,000 removed; about 6,300 of the added lines are tests. - The site's view, which was one component of five thousand lines, is spread over the four regions the layout has: the header, the cards, the details and the tray. Decisions are in `src/renderer/*.cjs` with unit tests; components draw them. - New for the suite: journeys per region under `tests/e2e/journeys/`, lint rules of the repository's own (`eslint-rules/`), and Stylelint over the window's stylesheet. - The main process changed in one place for the redesign's sake: the debug.log tail says which lines the file already held (#614). </details> <details> <summary>Screenshots or recording</summary> Not attached: I have no way to upload images from the command line. The prototype shows the design; the signed build shows the app. </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
The main window is the one window with the preload bridge, and nothing held it to the page the app loads into it: it could be navigated or redirected to another page, and it could open a second window. `pinToOwnPage` in src/window-navigation.js cancels every navigation and redirect away from the page and denies every new window, the way trac-view.js pins the Trac window. An address the window refuses is handed to external-url.js, so an http or https link opens in the system browser and anything else is refused and logged. A page reloading itself is still allowed. That is also what #284 asks for: a middle click on a link to the site opens the browser, not a window of the app. Tests at three layers: the module with a stand-in webContents, the wire from `createWindow` in ipc-wiring, and a journey for the middle click. TESTING.md and AGENTS.md name the second journey that presses "Start dev server" with the server stubbed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A mail's HTML part was put into the app's own document, so markup the app did not write became part of the page: its styles applied to the whole window, and its links and forms acted on the window itself. The HTML part is now drawn in an `<iframe srcdoc>` with an empty `sandbox`: no scripts, no forms, no new windows, no navigating the window around it. The frame inherits the window's content security policy, which needed no change for it to render and keeps scripts and remote loads blocked inside it. A link in a mail does nothing, and the mail stays where it is. `react/no-danger` is on for the renderer, so a new `dangerouslySetInnerHTML` fails lint, and the review standard says what the rule is and why. mail.spec.js reads the rendered mail through its frame, and a second journey presses everything in a mail that asks to go somewhere. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
- `will-redirect` is also sent for frames inside the page, so the pin acts on it for the main frame only: a frame redirecting is not the window leaving, and its address is not one to hand to the browser. - The wiring test that asserted the pin ran before `loadFile` pinned an order nothing can observe. The test with the real modules already fails when the main window is not pinned, so the first one goes. - The wp-admin link is reached by two end-to-end specs now, so it is named once in tests/e2e/helpers/ui.cjs. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…frame's title The mail's frame said nothing about its colour scheme, so in the dark theme it was white only because Chromium paints a white page behind a frame whose scheme differs from the window's, and it sat in the box's dark padding. It now asks for the light scheme, the way a mail client draws a mail, carries the padding inside the frame and fills the box to its edge. Its size moves to shell.css, which the redesign's lint asks for, and its title goes through __(). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
783bd46 to
d286d96
Compare
The second mail in mail.spec.js was checked for the first one's text on the dialog only, which does not enter the frame the first one's HTML part is drawn in; it now checks that it has no frame. The i18n mail journey checks the frame's title, an attribute unwrapped() does not read. The comment over MAIL_DEFAULTS said the mail's markup wins, which holds for its styles and not for its <base>. AGENTS.md and TESTING.md name every journey that stubs the server's start, and the review standard says what moving content into a frame does to a locator. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Why
Two things about the app's main window were left open, and this closes both as a security hardening.
The main window was not held to the page the app loads into it: it could be navigated or redirected to another page, and it could open a second window. The Trac window has been pinned to its origin since it was written (
pinToTracinsrc/trac-view.js); the main window had nothing of the kind.And a mail's HTML part was put into the app's own document, so markup the app did not write became part of the page: its styles applied to the whole window, and its links and forms acted on the window itself.
What changes
The main window stays on its own page.
pinToOwnPagein the newsrc/window-navigation.jscancels every navigation away from the page, cancels a main-frame redirect, and denies every new window. An address the window refuses is handed tosrc/external-url.js, the gateurl:openalready uses: http and https open in the system browser, anything else is refused and logged. A page reloading itself is still allowed.createWindow()calls it, and the hand-off to the browser is one function insrc/main.jsshared withurl:open.A mail's HTML part is drawn in a frame of its own, an
<iframe srcdoc>with an emptysandbox: no scripts, no forms, no new windows, no navigating the window around it. The frame inherits the window's content security policy, which needed no change for the frame to render and keeps scripts and remote loads blocked inside it. A link in a mail does nothing, and the mail stays on screen. The frame asks for the light colour scheme, so a mail is drawn on a white page with dark text in either theme, the way a mail client draws one, and fills its box to the edge.The rule is written down where it is checked.
react/no-dangeris on for the renderer, so a newdangerouslySetInnerHTMLfails lint, and the review standard has a paragraph on both halves.Deliberately not in this PR: the content security policy is unchanged, links in a mail are not sent to the browser, and no other window is touched.
How to test this
Platforms: any. Use the Buildkite build for the current head of this branch.
Three journeys carry this, run from the repository root:
npx playwright test --project=journeys -g "a middle click on a link to the site"npx playwright test --project=journeys -g "a mail is read inside the app"npx playwright test --project=journeys -g "a mail the site sent"All three are red on
trunk's code. The first is red without the pin (a second app window opens and the address never reaches the browser). The second is red when the mail is drawn in the dialog's own document, when the frame is given more than the emptysandbox, and when a link replaces the mail in its frame. The third now reads the rendered mail through its frame.What they cannot reach is a real dev server and a mail a real site sent, since they stub the first and seed the second. The middle click needs a mouse with a clickable wheel; a trackpad has no middle button.
Starting state: a site that has finished setup.
adminuser.What must not have happened:
Risks and limitations
createWindow()andurl:open, so they need to be reconciled rather than both merged as they are.trunkat 9e48b4d (the redesign, translations and themes landed meanwhile). The rebased head had a fresh-context review; see the review outcome.Related
Related to #284 and #358. Follows #569, which moved the mail dialog into its own component.
Design decisions and alternatives considered
The page the window may stay on is the one it is on, read from the window when the event arrives, not an address fixed in
main.js. Both strings come from Chromium, so there is no path to normalise across platforms and no second copy of where the app's page lives.The pin takes one function,
openInBrowser, and decides nothing about schemes. Which addresses may reach the OS stays inexternal-url.js, in one place, and a refused one is logged there the same way as forurl:open.will-redirectacts on the main frame only. Electron sends it for frames inside the page too. A frame redirecting is not the window leaving, and its address is not one to hand to the browser.An empty
sandbox, and no sanitiser. A sanitiser is a list of what is dangerous that has to stay complete; the frame is a boundary that does not depend on what the mail contains. It also needs no new dependency.A
<base target="_blank">in front of the mail. Without it a link with no target loads in the frame itself, the frame may not load it, and the mail is replaced by an empty page. Aimed at a new window, which the frame may not open either, the link does nothing and the mail stays. Asrcdocdocument is always in standards mode, so putting it in front of a doctype changes nothing about how the mail is laid out.The content security policy was checked, not changed. A
srcdocframe renders underdefault-src 'self'as it stands and inherits the policy.Review outcome (required — see AGENTS.md)
3 [fix here] · 3 [follow-up]. All 3 fixed.
.github/instructions/code-review.instructions.md. Reviewed head 90098aa, base 3270f30 (trunk, no pull request existed yet).npm run lintclean;npm testandnpm run test:electronpass; all journeys pass on macOS.90098aa → bf568e8is the three fixes below and nothing else. Lint, both unit passes and all journeys re-run on bf568e8.trunkat 9e48b4d (now fb7674c, 74eeff6, 6f2d7d8). Conflicts were inAGENTS.md,TESTING.md,tests/e2e/helpers/ui.cjsandmail.spec.js, resolved to trunk's wording plus this PR's additions; the frame journey now opens the Email tray first. d286d96 follows: the frame asks for the light scheme and carries its padding, its size moves toshell.cssfor the redesign's lint, and its title is translated. On d286d96: lint clean,npm testandnpm run test:electronpass, the mail and site-link journeys pass on macOS, and the site-link journey is red with the pin disabled..github/instructions/code-review.instructions.md. Reviewed head d286d96, base 9e48b4d (trunk).npm run lintclean,npm testpasses. Outcome: 2 [fix here] · 1 [follow-up], all 🔵; both fixed in 4f8edb0:mail.spec.js, the check that the second mail shows nothing of the first searched the dialog only, which does not enter the frame the first one's HTML part is drawn in. It now also checks the second mail has no frame. Red with a stale frame drawn, where the old line stayed green.unwrapped()collects notitle.i18n.spec.jsnow checks it, and is red with the title left untranslated.trunkit opened a second window with the preload bridge, and that with the pin it does nothing after its confirmation.MAIL_DEFAULTS(a mail's styles win, its<base>does not),AGENTS.mdandTESTING.mdnaming every journey that stubs the server's start, and a sentence in the review standard's §5 on content moved into a frame.Fixed:
tests/e2e/helpers/ui.cjsnow.loadFile, an order nothing can observe. Removed; the test with the real modules already fails when the main window is not pinned.will-redirectalso fires for frames, and the pin would have treated one as the window leaving. It acts on the main frame only now, with a test.Follow-ups, each a decision rather than an omission:
url:openseparately.CodeRabbit: not requested yet.
Implementation notes
src/window-navigation.js:pinToOwnPage(webContents, { openInBrowser }).src/main.js:openInBrowser(url)wrapsopenExternalUrl;createWindow()pins the window and logs a browser that could not be opened;url:opencalls the same function.src/renderer/components/email-modal.jsx: the frame and what goes in front of every mail.src/renderer/shell.css: the frame's size, and a framed mail's box without padding.eslint.config.mjs:react/no-dangerforsrc/renderer/.tests/unit/window-navigation.test.cjs(layer 1),tests/unit/ipc-wiring.test.cjs(layer 3: the stub window now records its navigation listeners and its new-window handler),tests/e2e/journeys/mail.spec.jsandtests/e2e/journeys/site-links.spec.js(layer 4).AGENTS.mdandTESTING.mdaddsite-links.spec.js(and the two already missing,auto-start.spec.jsandi18n.spec.js) to the journeys that press "Start development server" with the server stubbed; the Electron-pin guidance is unchanged.page.reload()do not raisewill-navigate, so neither is affected;location.reload()raises it with the page's own address, which is the one navigation the pin lets through.Screenshots or recording
Nothing new appears on screen. A plain-text mail is drawn exactly as before, and an HTML mail is drawn in the same place in the dialog, on a white page in either theme.
🤖 Generated with Claude Code