Repository navigation
Translate the Review & submit dialog - #635
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
zaerl
left a comment
There was a problem hiding this comment.
Seems ok to me. Just need to fix that small conflict.
| __('The patch will say it was written at <strong><event /></strong>.'), | ||
| // The name goes in as an element, not into the string: it is | ||
| // free text, and a `<` in it would be read as markup. | ||
| { strong: <strong />, event: <>{wporg.event}</> } |
There was a problem hiding this comment.
I don't understand exactly why Claude made this. strong: <strong /> is ok, but event is a simple string and can be %s. But maybe I am missing something here. It's fine to leave it as is.
| <Stack direction="column" align="center" justify="center" gap="lg" className="patch-diff-loading"> | ||
| <Spinner /> | ||
| <Text variant="body-lg" className="muted-label">Generating patch...</Text> | ||
| <Text variant="body-lg" className="muted-label">{__('Generating patch…')}</Text> |
There was a problem hiding this comment.
Claude changes all the ... in …. This is probably good, but I'm not 100% sure.
zaerl
left a comment
There was a problem hiding this comment.
Thanks for this, great work. Let's just fix those conflicts, and we'll be OK. 🚢
The diff pane no longer tells an empty or failed patch apart by its English text: main returns an empty patch, a failure sets patchLoadFailed, and the pane says "No changes." or "Error: %s" in the contributor's language. The dialog, its diff pane, the Trac and mentor destinations, and the sentences they take from changes-note, pr-checkout, wporg-handle and patch-provenance are wrapped, with createInterpolateElement for the sentences that carry markup or a name. @wordpress/element becomes a direct dependency for it. Fixes #624.
…he pseudo-locale The journeys now open the dialog with nothing to send, with a username and an event remembered (one with markup in its name), and over someone else's patch with and without a name. The applied patch's headline gets an element of its own, so it reads as one sentence beside the next one.
529b7ae to
845676d
Compare
## Why The Open a pull request card, its GitHub sign-in, and the errors main sends it were plain English in a translated window. This is the #625 batch of #622, which needed the catalog in main from #634. ## What changes - **The card** (`pull-request-destination.jsx`) wraps every string. Sentences with a link or code in them use `createInterpolateElement`, with logins, branches and repository names passed in as elements so they are never read as markup. The failure messages are a function instead of a module-level constant. - **Per-project text** in `project-type.cjs` (`cards.*`) is getters, read when the card renders. Main's refusal with nothing linked used to splice the work item's label into a sentence; it is now `cards.prNeedsWorkItem`, one sentence per project type. - **Main process:** the sign-in errors in `github-auth.cjs` and the pull request errors in `github-pr.cjs` are wrapped, including the step labels passed to `failure()`, which the issue did not list. The "Trunk has moved" message uses `_n`, and a GitHub failure is composed with `%1$s: %2$s%3$s` so a translator can change the punctuation. GitHub's own reason and the bracketed diagnostic for GitHub support stay as they are. Not wrapped, per #622: the pull request body and the default pull request title, because they leave the app. ## How to test this The new journeys cover the card in the pseudo-locale on both projects. Run them from the repository root on either platform: ``` npx playwright test --project=journeys tests/e2e/journeys/i18n.spec.js -g "pull request card" ``` Both fail without this change. The Core journey walks every state #625 lists: signed out, "Not now" and "Show this again", the device code, signed in with the form and its fold, a `rate-limited` failure, a dry run, and a pull request opened from a checkout behind trunk. The Gutenberg one walks signed out, signed in with the fold open, and the opened pull request, checking the words that are Gutenberg's own. What the journeys cannot reach is a real GitHub. They stub every `github:*` handler, so the main-process errors never reach the screen in them. Unit tests in `github-auth`, `github-pr` and `pr-stage` cover those strings in the pseudo-locale instead. To see one on screen by hand (either platform, current head): 1. From the repository root, run `npx electron . --lang=en-XA`. 2. Open a Core site with a linked ticket and an edit, and click **Review & submit changes**. 3. In **Open a pull request**, click **Sign in with GitHub** and finish in the browser. - Expected: every line of the card is accented and in brackets, apart from your login, your fork's name, the code and the branch. **What must not have happened:** no English an English speaker sees has changed. The English journeys in `open-pull-request.spec.js`, `review-changes.spec.js` and `pr-checkout.spec.js` still pass with their exact strings. ## Risks and limitations - The other open batches (#636, #644, #645, #646) also append tests to the end of `i18n.spec.js`, so whichever merges later resolves that by keeping both. - One finding is deferred: `prFailureMessage` decides between five user-facing strings inside a `.jsx` file the suite cannot load. It predates this PR (it was the old constant map), so moving it to a `.cjs` module with a test is a follow-up. ## Related Fixes #625. Part of #622. Follows #634. --- <details> <summary>Design decisions and alternatives considered</summary> - **Not stacked on #635.** #622 asks for batches that review on their own. Now that #635 has merged, its `unwrapped()` scanner change and the `@wordpress/element` dependency, which this PR also needed, come from trunk. - **The textarea placeholder** is three strings joined with a newline. A single `__()` with `\n` in it is refused by `@wordpress/i18n-no-collapsible-whitespace`. - **"Signed in as …"** is wrapped in its own `<span>`, so the sentence and the **Sign out** button beside it are separate elements, as in #635. </details> <details> <summary>Review outcome (required — see AGENTS.md)</summary> 2 [fix here] · 1 [follow-up]. Both [fix here] findings are fixed; the follow-up is deferred (see Risks). - 🟡 The Gutenberg journey linked no issue, so it never showed the form, the fold or the loop-back. Fixed: it links one in the record and walks those states. - 🔵 No test showed the main-process strings or the stage labels come out translated. Fixed: pseudo-locale unit tests for the scope refusal, a composed `failure()`, the plural stale message and `prStageLabel`. - Style: three dead entries in the journey's `THEIRS` filter were removed. - **Review:** completed. Fresh agent context, head `11ef362` / base `c34aa45`. `npm run lint` clean, `npm test` 2002 pass / 0 fail, i18n and PR journeys 25 pass. - **Since review:** `11ef362 → ef457fd`. The only change is the test fixes above, checked by the authoring session, not re-reviewed by a fresh context. `src` and `tests` lint clean, unit suite 2005 pass / 0 fail, i18n journeys 12 pass. The review did not run the journeys itself; it was read-only. - **After #635 merged:** `ef457fd → 9559a1f` (trunk merged from GitHub) → `012fca8` (trunk at `e94f507` merged; `i18n.spec.js` resolved as trunk's file plus this PR's two tests, nothing else changed). Checked by the authoring session, not re-reviewed: `src` and `tests` lint clean, unit suite 2036 pass / 0 fail, i18n, open-pull-request, review-changes and pr-checkout journeys 31 pass. </details> <details> <summary>Screenshots or recording</summary> None. Nothing changes on screen in English, and the pseudo-locale journeys are the check for the translated state. </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code)
## Why The setup checklist and the trunk update's cards and banners were still plain English in a translated app. This is the #626 batch of #622. ## What changes - **Wrapped:** the setup checklist, the trunk update card, the question an update asks over edits, the banners for an old trunk and an incomplete update, the "Up to date with trunk" notice, and the sentences they take from `setup-steps.cjs`, `update-plan.cjs`, `next-action.cjs` and `project-type.cjs`'s `setup` copy. Also the dirty-tree dialog's errors in `use-trunk-update.jsx` and the alert in `use-dev-server.jsx`. - **Translated when shown, not at load:** `update-plan.cjs`'s messages and step labels were module-level constants and are now functions (`SKIP_INSTALL_MESSAGE` became `skipInstallMessage()`, and so on); `project-type.cjs`'s `setup.*` are getters, like `description` already was. - **Whole sentences instead of parts:** the step counters (`sprintf('step %1$d of %2$d')`), the file count and the day count (`_n`), "Next step: %s", and the update summary, which was three fragments and is now one of four sentences (`updateSummarySentence`). The link-ticket hint took the work item's label as a noun ("Link a %s…") and now takes its kind and has one sentence each, as #630 asks for. - Commands and paths (`npm install`, `npm run build`, `package.json`, `src/`) are passed in as `%s`. - Deliberately not here: the lines the app writes to the Terminal belong to #627, apart from the skip-install message, which #626 names because the update card shows it too. ## How to test this Covered by journeys in `tests/e2e/journeys/i18n.spec.js`, all red without this change. From the repository root: `npx playwright test --project=journeys tests/e2e/journeys/i18n.spec.js -g "checklist|trunk banners|did not finish"`. In the en-XA pseudo-locale they reach the checklist with steps done and ready, the checklist after a failed install, the old-trunk banner, the question an update asks over an edit (each answer chosen), the update card on its first step, and the banner after an update that did not finish, and fail on any visible English. To look at it by hand, on either platform, on the current head: run `npx electron . --lang=en-XA` from the repository root and open a site whose trunk is more than 14 days old. 1. **Expected:** the banner about the old trunk and its **Update to latest trunk** button are accented and in brackets. 2. Edit a file in the site and click that button. **Expected:** the dialog that asks what to do with the edit is bracketed throughout, apart from the file's path. **What must not have happened:** in English, the checklist, the cards and the banners read exactly as before. Not reached by a journey, and why: - The setup chain's "Setting this site up for you — step N of M" and "Setup stopped." notices: they only appear after a new site's clone, which a journey would have to fake end to end. - The "Up to date with trunk" notice: it needs an update to finish. Its sentences come from `updateSummarySentence`, which a unit test covers. - The alert about starting the server before the build: no control in the app can reach it (the checklist's button is disabled until the build is done, and the header and details only offer the server once the wizard is skipped). It is wrapped anyway. ## Risks and limitations - Review: 2 [fix here] · 0 [follow-up], both low, both fixed. - `tray.spec.js`'s "the tray's edge is moved with the arrow keys…" failed once in the full run. It fails about one run in five on `trunk` too, so it is not this branch. - The other open batches (#643, #644, #645, #646) also append tests to the end of `tests/e2e/journeys/i18n.spec.js`, so whichever merges later resolves that by keeping both. ## Related Fixes #626. Part of #622. --- <details> <summary>Design decisions and alternatives considered</summary> The dirty-tree answers' details carry their own leading dash ("— a .diff on your machine…"), with the space outside the string: punctuation is the translator's, and a string that starts with a space is easy to break. The link-ticket hint takes `workItemNoun` instead of `workItemLabel`, which removes one of the places #630 lists as splicing the label into a sentence. </details> <details> <summary>Review outcome (required, see AGENTS.md)</summary> - **Review:** completed. Reviewer: a separate agent context (Claude Code, read-only) against `.github/instructions/code-review.instructions.md`; head `1790e42` / base `8dbed36`. `npm run lint` clean, `npm test` 1954 pass / 0 fail, journeys 117 pass. Outcome: 2 [fix here] · 0 [follow-up], both 🔵. - Translatable-string rules [fix here]: the extractor gave the dirty-tree details' notes to their labels as well, and gave the dirty-tree errors' notes to the "Unknown error" fallback on the same line. **Fixed** in `4b925eb`: each note sits directly above its own string, and the fallback is on its own line. - Tests [fix here]: nothing would fail if the new `update-plan.cjs` message functions went back to English constants. **Fixed** in `4b925eb`: a unit test loads a catalog after the module is required. Checked by turning one back into a constant, which fails it. - Notes on wording, also fixed: a stale comment in `work-item.cjs`, and the translator example "2m 5s", which the app writes as "2m 05s". - It also confirmed the dev-server alert is unreachable from the UI. - **Since review:** `1790e42 → 4b925eb` checked; the change is the fixes above. Lint clean, `npm test` 1955 pass, journeys 116 pass and 1 known flaky (above) on the new head. - **After #635 merged:** `4b925eb → 7e0e1ad`, rebased onto trunk `e94f507`. Only `tests/e2e/journeys/i18n.spec.js` conflicted: resolved as trunk's file plus this PR's four tests, dropping this PR's copies of the imports and `MY_EDIT`, which trunk now has. #635 brought the identical `trunkDate` change to `git-site.cjs`, so it is no longer in this diff. Checked by the authoring session, not re-reviewed: build and lint clean, `npm test` 2035 pass / 0 fail / 2 skipped, the full journey suite 134 pass. </details> <details> <summary>Implementation notes</summary> - The renamed `update-plan.cjs` exports have no consumer left under their old names in `src`, `tests` or `docs`. - `main.js` reads `project-type.cjs` but never `setup`, so the getters change nothing there. </details> <details> <summary>Screenshots or recording</summary> Nothing changes in English. In the pseudo-locale every screen in this batch is bracketed, which the journeys assert. </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Francesco Bigiarini <francesco.bigiarini@gmail.com>
## Why The Terminal and the Logs were plain English in a translated window: the terminal's help and refusals, every line the setup, update and apply chains write there, the build watch and dev server lines, the Logs' tabs and empty-pane notes, and main's own lines from the trunk update, the pull request switch, the npm runner and the server start. This is the #627 batch of #622. ## What changes - **Every line written to the Terminal or a Logs pane** goes through `__`/`_n`/`sprintf`, with commands, paths, keys and file names passed in as `%s`. Each line is one or more whole sentences, so the terminal can show a translated line on its own. - **Sentences that were spliced together are written whole.** The apply chain built "The ${noun} is ${verb}" and "${verb} — …" from five verb and noun pairs; it is now a table of whole sentences per pair (`applyLines` in `watch-activity.cjs`), the way `applyDoneMessage` in `confirmations.cjs` works. `applyFinishMessage` used to regex the English "— open the site to try it out." out of a line; the caller now passes the line without it. The switch progress (`switch-progress.cjs`), the setup's `Status: ${phase}`, main's `Update failed during ${stage}` and the npm "letting go" line each get one sentence per case. - **Inline markup** (the hints under the terminal, the empty Logs panes) uses `createInterpolateElement`, with `npm run build`, `help`, `Ctrl+C`, `package.json`, `src/`, `build/` and `error_log()` passed in as elements. - **The dev server's "see Help → Open App Log"** takes the menu item as `%s`, from the same `__('Open App Log')` the menu uses (left over from #634). - **Module-level constants became functions:** `COPY_BUTTON_LABELS`, `SETUP_END_MESSAGES` and the setup status line move from `index.jsx` to `confirmations.cjs` as `copyButtonLabel`, `setupEndMessage` and `setupStatusLine`, where the suite can reach them; `STALE_ASSETS` and main's `ENGINE_RETRY_NOTICE` become functions too. 110 new strings. Not wrapped, per #622 or because the text is not ours: Git's own progress phases (`Updating files 3/10`), npm's and the server's output, the stub `appendNpm` buffer nothing shows, the `42%` progress figure, and the lines owned by sibling batches (`SKIP_INSTALL_MESSAGE` and the dirty-tree errors are #626's in #636; the patch apply lines in main are #628's; the blocked-site errors are #629's). ## How to test this The new journeys cover both trays in the pseudo-locale. Run them from the repository root, on either platform: ``` npx playwright test --project=journeys tests/e2e/journeys/i18n.spec.js -g "Terminal is fully|Logs are fully" ``` Both fail without this change (checked by stashing `src/` and rebuilding). The Terminal journey opens the tray on a built site, reads the help printed at start, types `help` and an unknown command, joins the terminal's rows into whole lines and checks every printed line starts with `[`, then scans the hints under it. The Logs journey scans the empty Build watch and Debug.log panes, starts the dev server with stubbed handlers so the watch starts and the server refuses, and scans the server's failure line, the watch's start and exit lines and the tab titles as they change. What the journeys cannot reach: they stub IPC, so main's lines (trunk update, PR switch, npm runner, server start) never reach the screen in them, and neither do the apply, update and setup chains, which need a real install and build. Unit tests cover those in the pseudo-locale instead: `watch-activity`, `update-handoff`, `dev-server-command`, `switch-progress`, `confirmations`, `trunk-update-fetch.integration` and three in `ipc-wiring` (the server's start failure, the trunk update's failure lines and the engine retry notice). To see the chains on screen by hand (either platform, current head): 1. From the repository root, run `npx electron . --lang=en-XA`. 2. Open a built Core site, open the **Terminal** tray and type `help`. - Expected: every line except the `$` prompt is accented and in brackets, and the help's descriptions still start in one column. 3. Click **Update to latest trunk** (in its pseudo-localised form) and let it run. - Expected: the lines the app writes ("Fetching latest trunk…", "Now on trunk as of …", "Running npm run build…", "Update complete — …") are bracketed; Git's and npm's own output is not. **What must not have happened:** no English an English speaker sees has changed, apart from the one line break and the one plural named in Risks. All 132 journeys pass with their exact English strings, including `terminal.spec.js`, `tray.spec.js`, `logs.spec.js`, `build-watch.spec.js` and `dev-server.spec.js`. ## Risks and limitations - Two changes to the English, both forced by the rules: the help's last paragraph was one sentence broken over two lines with a `\n`, which a translated string cannot hold, so it is now two sentences on two lines ("…npm run build once." / "Run them here whenever…"); `LONGEST_HELP_LINE` in `terminal.spec.js` follows it. And the switch progress said "1 files"; with `_n` it says "1 file". - Merge conflicts with the other open batches: #643, #636 and the parallel #628 and #629 all append tests to the end of `i18n.spec.js` (#635 did too; this branch is rebased on it). #636 also adds `import { __ }` to `use-dev-server.jsx` and `sprintf` to `index.jsx`'s import, where this branch adds `sprintf` too; those lines will need a one-line resolution, whichever merges second. - Main's PR-switch lines and `npmLetGoLine` in `main.js` are covered only by `npm run i18n:pot` extracting them and lint checking their comments; the trunk update, server start and engine retry lines have `ipc-wiring` tests. - Still English in the terminal during an apply: the lines owned by #628 (main's "Applying …" and the `patch-apply.js` lines) and #626's `SKIP_INSTALL_MESSAGE` (in #636). They land with those batches. - The diff is about 870 lines, over the ~800 guideline. Most of it is the per-case sentence table and tests; it is one tray's worth of strings and did not split cleanly. - Review: 3 [fix here], all fixed; 3 [follow-up], deferred (see the review outcome). ## Related Fixes #627. Part of #622. Follows #634 and #635. --- <details> <summary>Design decisions and alternatives considered</summary> - **The help's columns.** Each help line is one string, `%s Show this help text`, with the command padded into `%s`, so every printed line opens with the translated string's bracket and the descriptions still line up after the command column. The alternative, a translated description after an untranslated command, would leave each line starting in English. - **The apply sentences as a table**, not a frame like "%1$s but the build failed": a translation cannot rely on "The patch is applied" fitting in front of "but". The two `Restored` lines share their English but have a `_x` context per noun, so a language with gendered participles can say them differently. - **"Start the build watch, or run %s in the Terminal."** is its own sentence after the one saying the site still runs the old assets, instead of being copied into ten strings. - **Setup status per phase:** main sends only `cloning` and `done`; each gets its own line and anything else says "Status update", where it used to print the code. - **`Debug.log` and `Debug.log (%d)`** are wrapped so a translator can name the tab in their language; the file name in sentences is left as is. </details> <details> <summary>Review outcome (required — see AGENTS.md)</summary> 3 [fix here] · 3 [follow-up]. All 3 [fix here] findings are fixed. - 🟡 `copyButtonLabel`, `setupStatusLine` and `setupEndMessage` were branches choosing user-facing sentences inside `index.jsx`, which the suite cannot load. Fixed: moved to `confirmations.cjs` with a test in English and in the pseudo-locale. - 🔵 `engineRetryNotice` was made a function so it is translated when shown, and nothing checked it. Fixed: an `ipc-wiring` test drives the engine retry in the pseudo-locale. - 🔵 The Terminal journey dropped any line starting with `[`, so translated text followed by English would pass. Fixed: each line must be one bracketed string from its first character to its last. - Follow-up: the apply flow in the terminal is still partly English (main's "Applying …" line, `patch-apply.js`, `SKIP_INSTALL_MESSAGE`). Those belong to #628 and #626 (#636), not this batch. - Follow-up: the five `compiling` sentences in `applyLines` repeat `compilingMessage()`'s text, on purpose, so each can be translated whole; the test now pins all five against `compilingMessage()`. - Follow-up: `npmLetGoLine` reads `process.platform`, so only one platform's branch runs in a test. The split was already inline before this branch; injecting the platform is a separate change. - One review note was checked and not taken: it said the renderer no longer imports `switch-progress.cjs`, but `index.jsx` still does, so that module's header comment stays. - **Review:** completed. Fresh agent context (Explore subagent given the diff and the review instructions), head `c7a1ba8` / base `9fd167e`. Lint clean, `npm test` passing, read-only, so it did not run the journeys. - **Since review:** `c7a1ba8 → 274bb2d`. The review fixes above, then a rebase onto trunk `e94f507` after #635 merged (only `i18n.spec.js` conflicted: both appended tests). Checked by the authoring session, not re-reviewed by a fresh context: `npm run lint` clean, `npm test` 2044 pass / 0 fail, all 132 journeys pass, `npm run i18n:pot` clean. - **After CI:** `274bb2d → 6179bd9`. The Terminal journey failed on CI's macOS runner: it waited for the help's first line, which on that window had scrolled out of the rows xterm draws, because pseudo-locale lines are longer and wrap. It now waits for the help's last line. Reproduced locally at 1024×640 (failed before, passes after). Test-only change, checked by the authoring session, not re-reviewed: the i18n and terminal journeys pass (20). </details> <details> <summary>Screenshots or recording</summary> None. Nothing changes on screen in English beyond the help's line break, and the pseudo-locale journeys are the check for the translated state. </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Francesco Bigiarini <francesco.bigiarini@gmail.com>
## Why
When a patch or a pull request will not go on, the apply card explains
why: which files missed, how many of the changes, and what to do next.
In any language but English that whole explanation is still English, and
most of it was built by splicing words into sentences (`` `change${s}`
``, a `where` clause, `'includes' : 'include'`), so it could not be
translated as it was. #628 is the batch of #622 that covers it.
## What changes
- The failure breakdown (`apply-conflict.cjs`), the applied-patch notice
and the preview's "whose work is this" sentences (`applied-layer.cjs`),
and a pull request's preview, checkout notice and refusals
(`pr-checkout.cjs`) are wrapped, along with the hook's own errors in
`use-apply-patch.jsx`.
- Main's side is wrapped too: the per-file refusals and the terminal
lines in `patch-apply.js`, the parse errors in `patch-plan.cjs`, the
pasted-URL errors in `patch-sources.cjs`, the Trac download error in
`trac-view.js`, and the `git:apply-patch` and `git:preview-patch`
handlers in `main.js`.
- Spliced sentences are written whole: one string per case and per file
count, `_n` on the count each sentence is about, numbered placeholders,
and separate strings for an unnamed patch and for an issue against a
ticket. About 127 strings in all.
- `REASONS`/`REVERT_REASONS` and `DISPOSABLE_EXIT`/`SLOT_HELD` are now
functions, so they are translated when shown. The applied notice's title
is now one sentence with the patch's name inside it, not the name
followed by the rest of the sentence.
- English is unchanged. A throwaway script compared trunk's three
renderer modules with this branch's across about 32,000 inputs and found
the English output identical.
## How to test this
The journey covers this: `the apply card is fully translatable when a
patch or a pull request will not go on, and once a patch is applied` in
`tests/e2e/journeys/i18n.spec.js`. Run it from the repository root
(either platform) with `npx playwright test --project=journeys -g "will
not go on"` after `npm run build:once`. It fails on trunk's `src/`
(checked by reverting `src/` and rebuilding). It runs real patches
through the real engine in the pseudo-locale and scans the preview of a
patch that lands on the contributor's own edits, the failure notice when
that patch does not fit, the refusal of a non-GitHub pull request URL, a
pull request's preview (with `git:preview-pr` stubbed), and the notice
once a patch is applied.
What the journey cannot reach, and is covered in the unit suite in the
pseudo-locale instead:
- The pull-request framings of a failure (closed, stale, the
contributor's own work in the file, a file the applied patch brought):
they need a real pull request that fails against a real checkout.
`tests/unit/apply-conflict.test.cjs` reads each one against the exact
string it should choose.
- Main's own sentences, which the window shows only when they are not
replaced by a breakdown: `patch-apply.integration`, `patch-plan`,
`patch-sources`, `trac-view` and `ipc-wiring` each check theirs.
- A checked-out pull request's notice, whose headline belongs to another
batch (#627).
For a look by hand, either platform, current head: run `npx electron .
--lang=en-XA` from the repository root, open a Core site, edit
`src/wp-login.php`, then on **Apply a patch or PR** choose **Diff** and
pick a `.diff` that changes the line you edited.
1. The preview says you have your own edits to `src/wp-login.php`,
bracketed and accented.
2. Click **Apply and rebuild**. The card says how many of the patch's
changes no longer fit, and lists the file. Every sentence is bracketed,
apart from the path and the lines of code.
3. On **Pull request**, paste `https://gitlab.com/a/b/pull/1` and click
**Apply PR**. The refusal is bracketed (with a full stop after the
bracket, see Risks).
**What must not have happened:** in English, nothing on the card reads
differently from trunk. The English journeys for the card (`apply-card`,
`pr-checkout`, `gutenberg-site`, `watch-during-changes`, `ticket-card`,
`site-header`) still pass unchanged.
## Risks and limitations
- Review: 1 [fix here] · 2 [follow-up]. The fix is in, and so are both
follow-ups, since each was one line in a file this PR already touches
(details below).
- The card adds a full stop to an error that does not end in one
(`applyFailureWords` in `apply-card.cjs`), so a translated refusal shows
as `[…].` in the pseudo-locale, and a language whose sentences end in
`。` gets `。.`. #630 removes that once main's errors carry their own
punctuation. Adding it to main's sentences here would have changed the
English, so the journey allows that one stop explicitly.
- Left in English on purpose: the snapshot guards in `patch-apply.js`
("Path is outside the checkout" and two others), the missing-ticket
guard in `trac-view.js` and `'Site is not registered'` in the apply
handler. They are internal checks behind paths that Git, the scrape or
the site registry already accepted, and a contributor should not be able
to reach them. `prSubmissionRefusal` and `prSubmissionBlocked` in
`pr-checkout.cjs` are #624's, wrapped by #635, which has merged. The
verb and noun passed to `runApplyInstallAndBuild` (`'Restored'`,
`'Checked out'`) are keys for `applyDoneMessage`, not text, and the
terminal lines built from them are #627's.
- Some sentences keep wording that reads oddly, because English must not
change. When a patch has no name, one now says "changes from the patch
you applied, which you applied". That case cannot happen today, since
main always stores a label.
- Merge conflicts: the other open batches (#643, #635, #636, and #627
and #629 in parallel) also append tests to the end of
`tests/e2e/journeys/i18n.spec.js`. #635 adds the same `@wordpress/i18n`
import to `pr-checkout.cjs`. Each is a one-line conflict.
## Related
Fixes #628. Part of #622.
---
<details>
<summary>Design decisions and alternatives considered</summary>
- **Two strings where a sentence names a file count as well as a change
count.** `_n` takes one number. The plural follows the change count, as
#628 asks, and the file count picks between a one-file and a many-file
string. The alternative, a nested `_n` for "in %d files" spliced in, is
the splice #622 rules out.
- **Joined sentences.** The pull-request headlines that say "does not
fit" and then name whose files failed are built from whole sentences
joined with a space, not one string per combination. Each piece is a
complete sentence, and four cases times two file counts would have meant
sixteen near-duplicate strings for translators.
- **`reverting: ''` for an unnamed patch.** `describeApplyFailure` used
to get `'That patch'` as the label. Now it gets `''` and words the
unnamed case as its own sentence, so "is this a revert" is read from the
type of the option (`typeof reverting === 'string'`) rather than from
the label being truthy.
- **No scanner change.** Nothing in this batch puts an element inside a
sentence, so the `unwrapped()` hunk from #643 is not needed here. The
"Near %s" line is cut at the code by design, and the journey leaves its
two pieces out by name.
</details>
<details>
<summary>Review outcome (required — see AGENTS.md)</summary>
- **Review:** completed. Reviewer: a fresh-context Explore subagent,
given only the diff and
`.github/instructions/code-review.instructions.md`. Reviewed head
`aa3bd889ba64565bfc773fdb690dbb72c0d19c2c`, base
`9fd167e6c370837bd832dbe87a46abf78ec55053`. Lint and the unit suite were
clean before the pass. Outcome: **1 [fix here] · 2 [follow-up]**.
Security, performance and cross-platform: no findings.
- 🟡 [fix here], architecture: `patch-apply.js` put `__('writing %s')`
around the app's own translated fragments (`git apply exited %s`, `could
not verify %1$s after git apply: %2$s`). **Fixed**: each case is now one
whole string, and `writing %s` wraps only Git's own stderr line. The
English is unchanged.
- 🔵 [follow-up]: `'(unnamed binary file)'` in `planApply`'s skip list
(`patch-plan.cjs`) was not wrapped, so the preview showed it in English
inside a translated sentence. **Fixed here** (one line).
- 🔵 [follow-up]: the Trac attachment URL refusal in `trac-view.js` was
not wrapped. **Fixed here** (one line). The rest of that finding is
`prSubmissionRefusal`/`prSubmissionBlocked`, which belong to #635, and
the `'Site is not registered'` guard, which is left as it is (see
Risks).
- Style notes acted on: the `apply-card.cjs` header comment no longer
claims `watch-activity.cjs` and `update-plan.cjs` are translated, and
the pull request preview's install note reuses the apply card's existing
msgid instead of adding a second one. Not acted on: a context for the
generic `'%1$s and %2$s'` (nothing else uses it yet), and the awkward
"which you applied" wording, which keeps the English as it was.
- The reviewer confirmed that no English changed, that the one equality
match (failures to conflicts) is the same translated string on both
sides, that nothing is translated at module load, and that every new
test fails without the wrapping.
- **Since review:** `aa3bd88 → 729dd12`. The author checked the fixes
above against the same standard: lint, `npm test` and `npm run i18n:pot`
are clean, and the `i18n`, `apply-card`, `patch-apply` and `pr-checkout`
journeys pass. No new findings. Then `729dd12 → 46c2cd4`: a rebase onto
trunk `e94f507` after #635 merged, with two conflicts. In
`pr-checkout.cjs` there is now one `@wordpress/i18n` import, #635's
wrapping of `prSubmissionRefusal`/`prSubmissionBlocked` is kept, and
this PR's wrapping of the rest is kept. In `i18n.spec.js` both appended
tests are kept in full, and this PR's journey now uses trunk's top-level
`write`/`LOGIN` import. The author checked the rebase but it was not
re-reviewed: `npm ci`, build, lint, `npm run i18n:pot`, `npm test` (2041
pass, 2 skipped, 0 fail) are clean. The full journey suite passed 131 of
131 on the third full run. The first two full runs each had 2 timeouts,
in `toasts.spec.js:114` (the Dismiss tooltip) and one
`site-header.spec.js` menu hover (`:212`, then `:145`). Each passed when
rerun alone, and both specs passed three times on trunk's `src/` and
three times on this branch, but no trunk run was made under the same
machine load, so a slower settle caused by this branch is not ruled out.
CI's journeys are the check.
</details>
<details>
<summary>Implementation notes</summary>
- `prCheckoutRefusal` runs in main as well as in the renderer: main
words the refusal for the terminal and the done event, and the renderer
passes an unknown code's error through as main worded it.
- `apply-conflict.cjs` pairs a failure with its breakdown by string
equality. Both come from the same `conflictSentence` call in main, so
the pair still matches when translated, and `patch-apply.integration`
asserts it in the pseudo-locale.
- Checks run from the repository root: `npm run build:once`, `npm run
lint`, `npm test` (2040 passed, 2 skipped, 0 failures), `npm run
i18n:pot` (every new string with a placeholder has its `translators:`
comment in the .pot). The full journey suite: 125 passed at `aa3bd88`,
and the four journeys touched by the follow-up commit passed again at
`729dd12`.
</details>
<details>
<summary>Screenshots or recording</summary>
No layout changes: the same notices show the same English. The
pseudo-locale journey above is the visual check.
</details>
🤖 Generated with [Claude Code](https://claude.com/claude-code)
## Why The ticket card's questions and notices, the merge-in-progress and legacy-site notices, the deep-link banners, the folder-open failures, and the input errors for a ticket or issue number were plain English in a translated window. This is the #629 batch of #622, and needed the catalog in main from #634. ## What changes - **Whole sentences, per kind.** Every sentence that spliced in a noun (`ticket`/`issue`), an English article, or a count is now one string per kind of work item, with `_n` for counts. This covers the dirty-trunk question (`ticket-actions.cjs`), the trunk-moved notice and the move's refusals (`ticket-trunk-notice.cjs`), the changes note (`changes-note.cjs`), and the carried-work notice in `index.jsx`. - **The changes note** used to be `lead` / link / `middle` / link / `end`. It is now one sentence per case with `<review>` and `<discard>` marked in it, filled in by `createInterpolateElement`. `DiscardChangesLink` takes its words as children for that. - **Constants became functions:** `LEGACY_SITE_ERROR` → `legacySiteError()`, `DISCARD_CONFIRM_MESSAGE` → `discardConfirmMessage()`, and the merge kinds table → `kindWords(kind)`. Git commands go in as `%s`; the "resolve the files, then git add them and run %s" frame and the `<every file the patch touched…>` placeholder are wrapped. - **Main process:** the ticket and issue parsers' refusals (main returns them), the `ticket-branches.js` errors that reach `res.error`, the rebase handler's three refusals and the dirty-trunk error in `main.js`, and the editor's exit-code error in `editor-launch.js`. - `@wordpress/element` becomes a direct dependency, the same line #635 and #643 add. Not wrapped, per #622: the WIP commit message, which leaves the app. ## How to test this The new journeys in `tests/e2e/journeys/i18n.spec.js` walk every state #629 lists, in the pseudo-locale. Run them from the repository root on either platform: ``` npx playwright test --project=journeys tests/e2e/journeys/i18n.spec.js -g "ticket card's questions|ticket from a link|merge left open|folder will not open" ``` All five fail without this change (checked by putting trunk's `src/` back and rebuilding). They cover: a refused ticket number; loose edits on trunk and the question about them; both discard confirmations (read from `window.confirm`, since Electron's are not Playwright dialogs); the carried notice and the changes note with its two links; a parked ticket whose trunk moved; edits saved before a clean start; a deep link with no site, on a Core site and on a Gutenberg site; a merge left open by a terminal; a legacy site; and `editor:open` and `dir:show` stubbed to return each `reason`. What the journeys cannot reach: the main-process refusals that need a real Git failure (the move's conflict, "No such branch", a second branch for the same issue) and the editor's exit code. Unit tests add the pseudo-locale filter and check those: `ticket-branches.integration`, `editor-launch`, `trac-ticket`, `github-issue`, `merge-in-progress`, `legacy-site` and `ticket-trunk-notice`. To see the card by hand (either platform, current head): 1. From the repository root, run `npx electron . --lang=en-XA`. 2. Open a Core site on trunk, edit `src/wp-login.php`, type a ticket number in the ticket field and click **Link ticket**. - Expected: the question and its four buttons are accented and in brackets. 3. Click the button that takes the edits into the ticket. - Expected: the carried notice and the changes note under the ticket are accented and in brackets. The note's two links are accented words inside its sentence. **What must not have happened:** no English an English speaker sees has changed. The full journey suite (129, including `ticket-branches`, `ticket-rebase`, `merge-in-progress`, `legacy-site`, `gutenberg-site`, `site-header`, `review-changes` and `trunk-update`) passes with its exact English strings, and the unit tests that pin whole English sentences pass unchanged. ## Risks and limitations - Other open batches also append tests to the end of `i18n.spec.js`: #643, #635 and #636, and the parallel #627 and #628. Whichever merges later resolves that by keeping both. This branch carries the same `unwrapped()` scanner hunk as #643 and #635, byte for byte, so that part merges cleanly. - #635 adds an `@wordpress/i18n` require to `changes-note.cjs` on the same line this branch does, and #636 edits `use-trunk-update.jsx` and the `@wordpress/i18n` import in `index.jsx`. The `index.jsx` import line is identical to #636's. The others are small conflicts for whichever merges second. - Two English sentences changed in states the app cannot reach: `rebaseRefusal` with no ticket number used to say "link #the ticket again" and "which trunk #the ticket started from". They now say "the ticket". The card only offers the move when a ticket is linked. - Kept as they were: a few strings still say "ticket" on a Gutenberg site, as they did before (the merge and legacy notices' "linking tickets", main's dirty-trunk and rebase errors, "Could not save the ticket."). Each is wrapped whole, not reworded. Main's rebase errors are replaced by the card's own per-kind wording before display. - Not wrapped, because nothing shows them: `ticket-branches.js`'s refusals to commit on trunk, to switch with uncommitted trunk work (`dirty-trunk`, asked as a question instead), and its own no-base error (worded by the card from the code). - The dirty-trunk question is two whole sentences joined with a space, as the merge refusal already joins its title and body. - Review: 1 [fix here] (fixed) and 1 [follow-up] (deferred, below). - Deferred follow-up: the carried-work notice still picks its plural inside `index.jsx`, where the unit suite cannot reach it. It was a ternary there before, and #629 asks for this rewrite at this spot. Moving it into a `.cjs` helper beside `dirtyTrunkQuestion` would make both forms testable. ## Related Fixes #629. Part of #622. --- <details> <summary>Design decisions and alternatives considered</summary> - **The changes note returns one marked-up string** rather than the five parts. The parts fixed English word order around the two links; one sentence with `<review>` and `<discard>` lets a translator move them. - **The Trac and GitHub hosts and the repository go in as `%s`**, as #622 asks for product names and keys. - **The merge restore placeholder** keeps its angle brackets outside the translated words (`<%s>`), so the pseudo-locale accents the words and a translator does not have to keep the brackets. - **The journeys use their own `require` lines** at the end of the file rather than editing the shared one at the top, so they do not conflict with the other batches' edits to it. </details> <details> <summary>Review outcome (required — see AGENTS.md)</summary> 1 [fix here] · 1 [follow-up]. The [fix here] is fixed; the follow-up is deferred (see Risks). - 🔵 Tests: the issue variants of the dirty-trunk question had no test that pinned them whole. Fixed: `tests/unit/ticket-actions.test.cjs` now pins each one with an exact `assert.equal`. - 🔵 Architecture, [follow-up]: the carried-work notice's plural is decided in `index.jsx`. It was decided there before this PR too. Deferred. - The reviewer compared old and new output by hand for every module in the diff and found no reachable English change. It noted that the unreachable no-ticket wording of `rebaseRefusal` changed, which is listed in Risks. - **Review:** completed. Fresh read-only agent context, head `126b51e` / base `9fd167e`, all 27 files. Before it: `npm run lint` clean, `npm test` 2039 pass / 0 fail / 2 skipped, `npm run i18n:pot` clean with every translator comment in the .pot, full journey suite 129 pass. - **Since review:** `126b51e → 0bb2fa3` added the test for the [fix here] finding. Then `0bb2fa3 → 149ff18`, rebased onto trunk at `e94f507` (base `e94f507`) after #635 merged. The rebase resolved two conflicts: `changes-note.cjs`'s `@wordpress/i18n` import now takes `_n` alongside #635's `__` and `sprintf`, and the journeys appended to `i18n.spec.js` now follow #635's, using trunk's `gitOk` import. The scanner hunk and the `@wordpress/element` line were already on trunk, so the branch's copies dropped out. Checked by the authoring session, not re-reviewed by a fresh context: `npm run lint` clean, `npm test` 2041 pass / 0 fail / 2 skipped, `npm run i18n:pot` clean, full journey suite 135 pass. </details> <details> <summary>Screenshots or recording</summary> None. Nothing changes on screen in English, and the pseudo-locale journeys are the check for the translated state. </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Francesco Bigiarini <francesco.bigiarini@gmail.com>
Why
The Review & submit dialog was still plain English in a translated app. This is the #624 batch of #622.
What changes
'No changes.'and failures were prefixed'Error', and the dialog compared against both. Main now returns an empty patch, a failed load setspatchLoadFailedwith the raw error, and the pane says "No changes." or "Error: %s" in the contributor's language.changes-note.cjs(patchReviewContext,discardDisabledReason),pr-checkout.cjs(prSubmissionRefusal,prSubmissionBlocked),wporg-handle.cjsandpatch-provenance.cjs._n; "Save, then open #N" and "Save patch as %s" usesprintf; the heading is one sentence per kind (ticket, issue);<strong>Save</strong>, the event name and the applied patch's name go throughcreateInterpolateElement. User text (an event name, a patch's file name) goes in as an element, never into the string, so a<in it stays text.@wordpress/elementis now a direct dependency (a one-line lockfile change; it was already installed through@wordpress/components).How to test this
Covered by journeys in
tests/e2e/journeys/i18n.spec.js, all red without this change. From the repository root:npx playwright test --project=journeys tests/e2e/journeys/i18n.spec.js -g "Review & submit|nothing to send|mentor card". They open the dialog in the en-XA pseudo-locale with an old trunk, with no ticket and with one, with nothing to send, with a remembered username and event, over someone else's pull request, and over an applied patch with and without a name, and fail on any visible English.To look at it by hand, on either platform, on the current head: run
npx electron . --lang=en-XAfrom the repository root, open a site with an edited file and click the pseudo-localised Review & submit changes.Expected: every string in the dialog is accented and in brackets except the pull request card, the Copy button, file paths and the diff itself.
What must not have happened: in English, the dialog reads exactly as before, apart from "Generating patch..." becoming "Generating patch…". A tree with nothing to send still shows "No changes." and offers no destinations, and a diff that fails to load still says so instead of claiming there are no changes (
pr-checkout.spec.jscovers that one).Not covered: the invalid-username error. It is built in main, which has no catalog until #634 lands, so its journey step is left for after that.
Risks and limitations
Related
Fixes #624. Part of #622. Pairs with #634 for the two messages that come from main.
Design decisions and alternatives considered
The journey's scanner (
unwrapped) now treats a text node as translated when the element around it, past bold, italic or code, holds one bracketed string from first character to last. A sentence with a link or a bold word in it is several text nodes, and only the whole is bracketed. "[Foo] English [Bar]" still fails, so English spliced between two wrapped strings is still caught. Sentences that sit side by side in one notice each got their own<span>so the rule can see where one ends.'(<discard />)'is a translatable string of punctuation: some languages write brackets differently.Review outcome (required, see AGENTS.md)
.github/instructions/code-review.instructions.md; head90ae783/ base8dbed36.npm run lintclean,npm test1954 pass / 0 fail, journeys 115 pass. Outcome: 1 [fix here] · 3 [follow-up], all 🔵.529b7ae; reaching them also showed the applied patch's headline shared an element with the next sentence, now its own<span>.loadErrorinpatch-diff-pane.jsxand the applied-patch headline inreview-dialog.jsxpick a string inside.jsx, where the unit suite cannot reach them. Deferred: both are two-way choices now covered by journeys.90ae783 → 529b7aechecked; the change is the journeys and the one<span>. Lint clean,npm test1954 pass, journeys 119 pass on the new head.Implementation notes
tests/e2e/helpers/git-site.cjsgains atrunkDateoption onmakeSite: the app reads a site's age from its trunk commit, not from its record.NOT_A_HANDLEinwporg-handle.cjsbecamenotAHandle(), so it is translated when said.Screenshots or recording
Nothing changes in English. In the pseudo-locale the dialog is bracketed throughout, which the journeys assert.
🤖 Generated with Claude Code