-
Notifications
You must be signed in to change notification settings - Fork 64
feat(core-beta): let the PostHog payload word the activation notice #1552
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
d0b3ee7
b57efe2
f0d0297
142d39b
eb4a944
b167d70
d077eaf
0efd88b
689d24e
1121186
5e6ced9
ff832a6
0af3a08
ad08a8f
ba10ca6
a261653
35f146c
79739f0
bdbdca7
bbd33dd
5d13478
ce0b6ea
df3b090
8a2508a
302037d
351e808
a391b3a
6997d44
d8b4b02
7a98bf1
f6ca0f5
2a23c42
68231a5
c88c798
bcac4cd
4d351d9
87572b1
7041e29
4d349f1
8c635db
b4c0741
1eaa22b
d0b79c4
94b2b22
f1cad37
5e100fe
7520949
2b6ac0a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,137 @@ | ||
| /** | ||
| * E2E: payload-controlled wording for the Core beta activation notice. | ||
| * | ||
| * The companion spec (`beta-activation-notice.test.ts`) covers the generic card and the whole | ||
| * trigger path. This one covers only what the PostHog payload adds: a `description` reaching | ||
| * the card as a feature name, through the real parser and the real launch. | ||
| * | ||
| * Tagged `@linux` only, for the two reasons documented in `fakeComfyInstall.ts`: the | ||
| * interpreter stub cannot be a PE executable on Windows, and nothing isolates `userData` on | ||
| * macOS, so the ops-flag seed would hit the real profile. | ||
| * | ||
| * Run: `pnpm exec playwright test --project=linux e2e/beta-activation-notice-named.test.ts` | ||
| */ | ||
| import os from 'node:os' | ||
| import path from 'node:path' | ||
| import { mkdir, mkdtemp, rm } from 'node:fs/promises' | ||
| import { expect, test, type ElectronApplication } from '@playwright/test' | ||
| import { launchApp, type AppContext } from './launchApp' | ||
| import { clickInstallTile, expectChooserVisible } from './support/chooserHelpers' | ||
| import { WebContentsPage } from './support/cdpPages' | ||
| import { opsFlagsGrantSeed, reserveFreePort, writeFakeComfyInstall } from './support/fakeComfyInstall' | ||
| import { captureHostWindow } from './support/windowCapture' | ||
|
|
||
| // A real launch does not fit the default 45s budget. | ||
| test.describe.configure({ mode: 'serial', timeout: 180_000 }) | ||
|
|
||
| const INSTALL_ID = 'inst-beta-notice-named' | ||
| const INSTALL_NAME = 'Named Beta Fixture' | ||
| const GRANT_ARG = '--enable-assets' | ||
| const GRANT_MIN_CORE = '0.3.80' | ||
| /** What the payload calls the feature. Deliberately not derivable from the arg token, so a | ||
| * card showing it proves the payload reached the copy rather than a table in Desktop. */ | ||
| const FEATURE_NAME = 'Asset library' | ||
|
|
||
| let ctx: AppContext | ||
| let installPath: string | ||
| let port: number | ||
| let previousPosthogHost: string | undefined | ||
|
|
||
| /** See the companion spec: a closed port makes the flag fetch `unreachable`, which is the | ||
| * documented path where the persisted `ops-flags.json` is authoritative. */ | ||
| const UNREACHABLE_POSTHOG_HOST = 'http://127.0.0.1:1' | ||
|
|
||
| function coachmarkPopup(app: ElectronApplication): WebContentsPage { | ||
| return new WebContentsPage(app, 'comfyTitleTooltip') | ||
| } | ||
|
|
||
| test.beforeAll(async () => { | ||
| previousPosthogHost = process.env['POSTHOG_HOST'] | ||
| process.env['POSTHOG_HOST'] = UNREACHABLE_POSTHOG_HOST | ||
|
|
||
| installPath = await mkdtemp(path.join(os.tmpdir(), 'comfyui-beta-notice-named-')) | ||
| port = await reserveFreePort() | ||
| await writeFakeComfyInstall({ installPath, port }) | ||
|
|
||
| ctx = await launchApp({ | ||
| settings: { | ||
| firstUseCompleted: true, | ||
| telemetryEnabled: true, | ||
| betaFeaturesEnabled: true, | ||
| hasSeenCentralPillHint: true | ||
| }, | ||
| installations: [ | ||
| { | ||
| id: INSTALL_ID, | ||
| name: INSTALL_NAME, | ||
| sourceId: 'comfybuilder', | ||
| sourceLabel: 'ComfyBuilder', | ||
| installPath, | ||
| status: 'installed', | ||
| launchArgs: `--port ${port}`, | ||
| launchMode: 'window', | ||
| browserPartition: 'unique', | ||
| seen: true, | ||
| comfyVersion: { | ||
| commit: 'b1c2d3e4f5a6b1c2d3e4f5a6b1c2d3e4f5a6b1c2', | ||
| baseTag: 'v0.3.99', | ||
| commitsAhead: 0, | ||
| baseTagVerified: true | ||
| } | ||
| } | ||
| ], | ||
| // Env-delivered so main writes it to the real `configDir()`; the harness cannot place | ||
| // that file on macOS, where `userData` ignores the HOME override. | ||
| opsFlags: opsFlagsGrantSeed({ | ||
| arg: GRANT_ARG, | ||
| minCoreVersion: GRANT_MIN_CORE, | ||
| description: FEATURE_NAME | ||
| }) | ||
| }) | ||
| await expectChooserVisible(ctx.panel) | ||
| }) | ||
|
|
||
| test.afterAll(async () => { | ||
| await ctx?.cleanup() | ||
| if (installPath) await rm(installPath, { recursive: true, force: true }) | ||
| if (previousPosthogHost === undefined) delete process.env['POSTHOG_HOST'] | ||
| else process.env['POSTHOG_HOST'] = previousPosthogHost | ||
| }) | ||
|
|
||
| test('a payload-supplied feature name reaches the card @linux', async () => { | ||
| await clickInstallTile(ctx.panel, INSTALL_NAME) | ||
|
|
||
| await ctx.panel.waitFor( | ||
| async () => | ||
| (await ctx.app.evaluate( | ||
| ({ webContents }, port) => | ||
| webContents.getAllWebContents().some((wc) => wc.getURL().includes(String(port))), | ||
| port | ||
| )) === true, | ||
| { timeout: 90_000, message: 'ComfyUI stub never came up / the host never attached' } | ||
| ) | ||
|
|
||
| const popup = coachmarkPopup(ctx.app) | ||
| await popup.waitFor( | ||
| async () => { | ||
| try { | ||
| return await popup.exists('.coachmark') | ||
| } catch { | ||
| return false | ||
| } | ||
| }, | ||
| { timeout: 30_000, message: 'beta activation notice never appeared' } | ||
| ) | ||
|
|
||
| expect(await popup.textOf('.coachmark-title')).toBe(`The ${FEATURE_NAME} beta is on`) | ||
| expect(await popup.textOf('.coachmark-text')).toContain(FEATURE_NAME) | ||
| // Naming the feature must not cost the card its way out. | ||
| expect(await popup.textOf('.coachmark-action')).toBe('Settings') | ||
| // The raw token still never reaches the user. | ||
| expect(await popup.textOf('.coachmark-text')).not.toContain(GRANT_ARG) | ||
|
|
||
| const shotPath = path.join(test.info().outputDir, '03-notice-named-by-payload.png') | ||
| await mkdir(path.dirname(shotPath), { recursive: true }) | ||
| const shot = await captureHostWindow(ctx.app, ctx.panel, shotPath) | ||
| test.info().attach('named notice', { path: shot, contentType: 'image/png' }) | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,3 +7,40 @@ They are kept here as a starting point for future translation work. To activate | |
| a translation, bring its file up to date with `en.json` (ensure all keys are | ||
| present and correctly translated), then move it back into the parent `locales/` | ||
| directory. The launcher auto-discovers any `.json` file in `locales/`. | ||
|
|
||
| ## Placeholder contract | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Fresh evidence in this reviewed commit shows the breakdown has drifted again: AGENTS.md reference: AGENTS.md:L3-L7 Useful? React with 👍 / 👎. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed, and your numbers reproduce exactly: 20 files, +935/−171 = 1106; product 9 files +280/−77 = 357 (32.3%), tests 8 files +607/−94 = 701 (63.4%), documentation 1 file +36/−0 = 36 (3.3%), localization 2 files +12/−0 = 12 (1.1%). You caught two distinct things and the second is the more useful one. The drift itself was mine: I regenerate the table from the diff, and I pushed the documentation commit after regenerating, which makes the table describe the previous head. That is the third time this has happened on this PR, always the same way round. The missing Documentation category was a real defect in how I generate it, not just a stale run. My categoriser tested the |
||
|
|
||
| Some strings interpolate a value, written `{likeThis}`. A placeholder is **not | ||
| always a word of the sentence's own language**, and translating around one | ||
| requires knowing which kind it is. | ||
|
|
||
| ### `{feature}` — the beta-notice keys | ||
|
|
||
| `titleBar.betaNoticeTitleNamed`, `titleBar.betaNoticeBodyNamed`, | ||
| `titleBar.betaNoticeOffTitleNamed` and `titleBar.betaNoticeOffBodyNamed` | ||
| interpolate `{feature}`: | ||
| the name of a beta feature, e.g. `Asset library`. | ||
|
|
||
| It is an **opaque proper name**. It is supplied at runtime by a remote | ||
| configuration payload, it is the same string for every user regardless of | ||
| locale, and it is **English today**. Desktop cannot translate it and cannot | ||
| know its grammatical gender or number. | ||
|
|
||
| So, when translating these four strings: | ||
|
|
||
| - **Keep `{feature}` a modifier, never the grammatical head.** In English it | ||
| modifies a constant head noun — "The *{feature}* **beta** is on" — and it is | ||
| that head noun ("beta") the sentence agrees with. Keep an equivalent constant | ||
| head in your language and let it carry the agreement: *la bêta {feature}*, | ||
| *die {feature}-Beta*, *бета-функция {feature}*. | ||
| - **Do not make the sentence agree with, decline, or inflect `{feature}`.** A | ||
| form like "{feature} est activé" or "{feature} включён" needs the name's | ||
| gender, which is unknowable — it would be guessing, and the guess changes | ||
| whenever ops names a new feature. | ||
| - **Word order is yours.** Putting the name after the head noun instead of | ||
| before it is expected, not a problem. | ||
| - An article immediately before `{feature}` should agree with the head noun, | ||
| not with the name. | ||
|
|
||
| If your language cannot express this without inflecting the name, say so rather | ||
| than picking a gender — the template needs changing, not the translation. | ||
Uh oh!
There was an error while loading. Please reload this page.