diff --git a/e2e/beta-activation-notice-named.test.ts b/e2e/beta-activation-notice-named.test.ts new file mode 100644 index 000000000..a66ada10c --- /dev/null +++ b/e2e/beta-activation-notice-named.test.ts @@ -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' }) +}) diff --git a/e2e/beta-activation-notice.test.ts b/e2e/beta-activation-notice.test.ts index 762d0e9c4..044102d9a 100644 --- a/e2e/beta-activation-notice.test.ts +++ b/e2e/beta-activation-notice.test.ts @@ -232,6 +232,31 @@ test('the card is anchored on the bell it points at @linux', async () => { const b = beak.getBoundingClientRect() return (b.left + b.right) / 2 })()`) + + /** Everything the offset is computed from, for the failure message. + * + * A bare "expected <= 2, received 8" cannot say WHICH term is wrong, and this assertion + * has already failed on a runner where it passes locally at the same window size. The + * offset is `view.x + beak - bell`, so a failure is one of: the view not centred on the + * bell, the card not centred in the view, or the beak not centred in the card. Each has a + * different cause and a different fix, and one CI failure should be enough to tell them + * apart instead of costing another round trip. */ + const geometry = async (): Promise => { + const card = await coachmarkPopup(ctx.app).evaluate(`(() => { + const c = document.querySelector('.coachmark') + const b = document.querySelector('.coachmark-beak') + if (!c || !b) return 'card=' + const cr = c.getBoundingClientRect(), br = b.getBoundingClientRect() + return 'cardLeft=' + cr.left + ' cardWidth=' + cr.width + + ' beakInCard=' + ((br.left + br.right) / 2 - cr.left) + + ' beakInlineStyle=' + (b.style.left || '') + + ' pageWidth=' + window.innerWidth + })()`) + return ( + `bell=${bellCentre} viewX=${popup!.x} viewWidth=${popup!.width} ${card}` + + ` | viewCentreVsBell=${popup!.x + popup!.width / 2 - bellCentre}` + ) + } expect(await beakCentre(), 'no beak found on the card').toBeGreaterThanOrEqual(0) // When nothing clamped it, the beak must land ON the bell. Asserted only in the unclamped @@ -247,7 +272,9 @@ test('the card is anchored on the bell it points at @linux', async () => { await expect .poll(async () => Math.abs(popup!.x + (await beakCentre()) - bellCentre), { timeout: 10_000, - message: 'the beak must point at the bell, not merely sit in a view that is centred on it', + message: + 'the beak must point at the bell, not merely sit in a view that is centred on it. ' + + (await geometry()), }) .toBeLessThanOrEqual(2) } @@ -308,10 +335,10 @@ test('the settings link lands on the beta opt-in row and retires the card @linux test('the notice is spent: a second launch stays silent @linux', async () => { // The whole point of persisting on retire rather than on show. Read through the same IPC // the title bar uses, so this asserts the contract the renderer actually depends on. - const stillPending = await ctx.titleBar.evaluate( + const stillPending = await ctx.titleBar.evaluate( `window.api.getPendingBetaNotice(${JSON.stringify(INSTALL_ID)})`, ) - expect(stillPending).toEqual([]) + expect(stillPending).toBeNull() const announced = await ctx.titleBar.evaluate( `window.api.getSetting('betaNoticeAnnouncedArgs')`, diff --git a/e2e/support/fakeComfyInstall.ts b/e2e/support/fakeComfyInstall.ts index 81f3237fa..8049b26e2 100644 --- a/e2e/support/fakeComfyInstall.ts +++ b/e2e/support/fakeComfyInstall.ts @@ -201,11 +201,24 @@ export async function writeFakeComfyInstall(opts: { export function opsFlagsGrantSeed(opts: { arg: string minCoreVersion: string + /** Optional per-flag notice wording, written in the payload's own wire shape so the fixture + * exercises the real parser rather than the already-parsed type. */ + description?: string + notice?: 'silent' }): Record { return { desktop_core_beta_features: { value: 'treatment', - payload: { flags: [{ arg: opts.arg, min_core_version: opts.minCoreVersion }] }, + payload: { + flags: [ + { + arg: opts.arg, + min_core_version: opts.minCoreVersion, + ...(opts.description === undefined ? {} : { description: opts.description }), + ...(opts.notice === undefined ? {} : { notice: opts.notice }), + }, + ], + }, }, } } diff --git a/locales/drafts/README.md b/locales/drafts/README.md index c3ce680dd..5f5901371 100644 --- a/locales/drafts/README.md +++ b/locales/drafts/README.md @@ -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 + +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. diff --git a/locales/en.json b/locales/en.json index 43cd120d8..d743725f9 100644 --- a/locales/en.json +++ b/locales/en.json @@ -89,6 +89,12 @@ "pillHintDismiss": "Got it", "betaNoticeTitle": "A beta feature is on", "betaNoticeBody": "This instance started with a beta feature enabled. You can turn beta features off in Settings.", + "betaNoticeTitleNamed": "The {feature} beta is on", + "betaNoticeBodyNamed": "This instance started with the {feature} beta enabled. You can turn beta features off in Settings.", + "betaNoticeOffTitle": "A beta feature is off", + "betaNoticeOffBody": "We've turned a beta feature off for this instance. You can manage beta features in Settings.", + "betaNoticeOffTitleNamed": "The {feature} beta is off", + "betaNoticeOffBodyNamed": "We've turned the {feature} beta off for this instance. You can manage beta features in Settings.", "betaNoticeDismiss": "Got it", "betaNoticeSettings": "Settings" }, diff --git a/locales/zh.json b/locales/zh.json index ff2f52440..88921f216 100644 --- a/locales/zh.json +++ b/locales/zh.json @@ -89,6 +89,12 @@ "pillHintDismiss": "知道了", "betaNoticeTitle": "已启用 Beta 功能", "betaNoticeBody": "此实例启动时启用了一项 Beta 功能。你可以在设置中关闭 Beta 功能。", + "betaNoticeTitleNamed": "已启用 {feature} Beta 功能", + "betaNoticeBodyNamed": "此实例启动时启用了 {feature} Beta 功能。你可以在设置中关闭 Beta 功能。", + "betaNoticeOffTitle": "已关闭一项 Beta 功能", + "betaNoticeOffBody": "我们已为此实例关闭了一项 Beta 功能。你可以在设置中管理 Beta 功能。", + "betaNoticeOffTitleNamed": "已关闭 {feature} Beta 功能", + "betaNoticeOffBodyNamed": "我们已为此实例关闭了 {feature} Beta 功能。你可以在设置中管理 Beta 功能。", "betaNoticeDismiss": "知道了", "betaNoticeSettings": "设置" }, diff --git a/src/main/lib/betaActivationNotice.test.ts b/src/main/lib/betaActivationNotice.test.ts index 7b5f8edca..d404ed790 100644 --- a/src/main/lib/betaActivationNotice.test.ts +++ b/src/main/lib/betaActivationNotice.test.ts @@ -21,45 +21,118 @@ import { clearBetaActivationClaim, peekBetaActivationNotice, readAnnouncedBetaArgs, - selectNewlyActiveBetaArgs + resolveBetaActivationNotice, + selectNewlyActiveBetaGrants } from './betaActivationNotice' +import type { CoreBetaGrant } from './coreBetaGrants' const announced = (): unknown => store.get(BETA_NOTICE_ANNOUNCED_ARGS_KEY) +/** A grant as `buildLaunchArgs` hands it over: the version window is already spent by then, + * so only the arg and the payload's notice wording matter here. */ +function grant(arg: string, notice?: CoreBetaGrant['notice']): CoreBetaGrant { + return { arg, minCoreVersion: '0.3.80', ...(notice ? { notice } : {}) } +} + +/** The args a set of applied grants would announce. */ +function announcedArgsFor( + applied: readonly CoreBetaGrant[], + spokenFor: ReadonlySet = new Set() +): string[] { + return selectNewlyActiveBetaGrants(applied, spokenFor).map((g) => g.arg) +} + +const pendingArgs = (installationId: string): string[] => + peekBetaActivationNotice(installationId)?.args.slice() ?? [] + beforeEach(() => { store.clear() _resetForTest() }) -describe('selectNewlyActiveBetaArgs', () => { +describe('selectNewlyActiveBetaGrants', () => { it('announces an enable-grant nobody has spoken for yet', () => { - expect(selectNewlyActiveBetaArgs(['--enable-assets'], new Set())).toEqual(['--enable-assets']) + expect(announcedArgsFor([grant('--enable-assets')])).toEqual(['--enable-assets']) }) - it('never announces a disable-grant', () => { - // `--disable-assets` is the remote force-OFF. The card says "a beta feature is on" and - // points at the opt-out switch, so announcing one would state the opposite of what - // happened and offer an action that does not apply. - expect(selectNewlyActiveBetaArgs(['--disable-assets'], new Set())).toEqual([]) - expect(selectNewlyActiveBetaArgs(['--disable-assets', '--enable-agent'], new Set())).toEqual([ + it('does not announce an unnamed disable-grant', () => { + // `--disable-assets` is the remote force-OFF. The generic copy says a feature is on and + // points at the opt-out, so with no payload-supplied name there is nothing truthful to + // put on a card. + expect(announcedArgsFor([grant('--disable-assets')])).toEqual([]) + expect(announcedArgsFor([grant('--disable-assets'), grant('--enable-agent')])).toEqual([ '--enable-agent' ]) }) + it('stays silent for an arg with neither prefix, rather than calling it a force-off', () => { + // The allowlist is documented as growing ahead of Core. A future entry with neither prefix + // plus a description would otherwise render "The X beta is off" for something switched ON + // — a degradation from silence to a false statement. + expect( + selectNewlyActiveBetaGrants( + [{ arg: '--use-assets', minCoreVersion: '0.3.80', notice: { description: 'Assets' } }], + new Set() + ) + ).toEqual([]) + }) + + it('announces a NAMED disable-grant, because the payload supplied what was missing', () => { + const fresh = selectNewlyActiveBetaGrants( + [grant('--disable-assets', { description: 'Asset library' })], + new Set() + ) + expect(fresh).toEqual([ + { arg: '--disable-assets', direction: 'disabled', description: 'Asset library' } + ]) + }) + + it('honours a payload that asked for no card', () => { + // Not every granted flag is user-visible; ops can grant one without training people to + // dismiss cards. + expect(announcedArgsFor([grant('--enable-assets', { silent: true })])).toEqual([]) + }) + + it('honours a silent first occurrence over a duplicate that omits it', () => { + // De-duplication has to claim the arg before the silent skip, or the second entry + // announces the very thing the first asked to keep quiet. + expect( + announcedArgsFor([grant('--enable-assets', { silent: true }), grant('--enable-assets')]) + ).toEqual([]) + }) + + it('silences only the grant that asked for it', () => { + expect( + announcedArgsFor([grant('--enable-assets', { silent: true }), grant('--enable-agent')]) + ).toEqual(['--enable-agent']) + }) + + it('carries a payload-supplied feature name through to the card', () => { + expect( + selectNewlyActiveBetaGrants( + [grant('--enable-assets', { description: 'Asset library' })], + new Set() + ) + ).toEqual([{ arg: '--enable-assets', direction: 'enabled', description: 'Asset library' }]) + }) + it('withholds an arg already spoken for', () => { - expect(selectNewlyActiveBetaArgs(['--enable-assets'], new Set(['--enable-assets']))).toEqual([]) + expect(announcedArgsFor([grant('--enable-assets')], new Set(['--enable-assets']))).toEqual([]) }) it('announces a LATER grant even once an earlier one is spoken for', () => { // The whole reason the store is a list rather than a boolean: a second beta feature // months from now still owes the user a heads-up. expect( - selectNewlyActiveBetaArgs(['--enable-assets', '--enable-agent'], new Set(['--enable-assets'])) + announcedArgsFor( + [grant('--enable-assets'), grant('--enable-agent')], + new Set(['--enable-assets']) + ) ).toEqual(['--enable-agent']) }) it('collapses a repeated arg so one launch cannot double-announce it', () => { - expect(selectNewlyActiveBetaArgs(['--enable-assets', '--enable-assets'], new Set())).toEqual([ + expect(announcedArgsFor([grant('--enable-assets'), grant('--enable-assets')])).toEqual([ '--enable-assets' ]) }) @@ -79,6 +152,64 @@ describe('the allowlist invariant this module depends on', () => { }) }) +describe('resolveBetaActivationNotice', () => { + it('has nothing to show when nothing is pending', () => { + expect(resolveBetaActivationNotice([])).toBeNull() + }) + + it('names the feature when the card covers exactly one named grant', () => { + expect( + resolveBetaActivationNotice([ + { arg: '--enable-assets', direction: 'enabled', description: 'Asset library' } + ]) + ).toEqual({ args: ['--enable-assets'], direction: 'enabled', description: 'Asset library' }) + }) + + it('covers only the grants matching the direction it reports', () => { + // A launch can both enable and withdraw. One card cannot honestly describe both, so it + // takes the enables and leaves the withdrawal queued for its own card. + const notice = resolveBetaActivationNotice([ + { arg: '--enable-agent', direction: 'enabled', description: null }, + { arg: '--disable-assets', direction: 'disabled', description: 'Asset library' } + ]) + expect(notice).toEqual({ args: ['--enable-agent'], direction: 'enabled', description: null }) + }) + + it('drops the name when the card covers two grants', () => { + // Two features at once have no single honest name, so the card falls back to generic + // rather than naming one of them and implying it is the whole story. + expect( + resolveBetaActivationNotice([ + { arg: '--enable-assets', direction: 'enabled', description: 'Asset library' }, + { arg: '--enable-agent', direction: 'enabled', description: 'Agent' } + ]) + ).toEqual({ + args: ['--enable-assets', '--enable-agent'], + direction: 'enabled', + description: null + }) + }) + + it('reads as enabled when anything was turned on', () => { + // "A beta feature is on" is true of a launch that turned one on, whatever else it + // withdrew; the reverse claim would not be. + expect( + resolveBetaActivationNotice([ + { arg: '--disable-assets', direction: 'disabled', description: 'Asset library' }, + { arg: '--enable-agent', direction: 'enabled', description: null } + ])?.direction + ).toBe('enabled') + }) + + it('reads as disabled only when every covered grant was a force-off', () => { + expect( + resolveBetaActivationNotice([ + { arg: '--disable-assets', direction: 'disabled', description: 'Asset library' } + ])?.direction + ).toBe('disabled') + }) +}) + describe('readAnnouncedBetaArgs', () => { it('reads the persisted list', () => { store.set(BETA_NOTICE_ANNOUNCED_ARGS_KEY, ['--enable-assets']) @@ -104,82 +235,122 @@ describe('readAnnouncedBetaArgs', () => { describe('arm / peek / acknowledge', () => { it('queues a first activation for the install that launched it', () => { - armBetaActivationNotice('inst-1', ['--enable-assets']) - expect(peekBetaActivationNotice('inst-1')).toEqual(['--enable-assets']) - expect(peekBetaActivationNotice('inst-2')).toEqual([]) + armBetaActivationNotice('inst-1', [grant('--enable-assets')]) + expect(pendingArgs('inst-1')).toEqual(['--enable-assets']) + expect(peekBetaActivationNotice('inst-2')).toBeNull() }) it('queues nothing when the launch applied no grants', () => { armBetaActivationNotice('inst-1', []) - expect(peekBetaActivationNotice('inst-1')).toEqual([]) + expect(peekBetaActivationNotice('inst-1')).toBeNull() + }) + + it('carries the payload wording through to the pending card', () => { + armBetaActivationNotice('inst-1', [grant('--enable-assets', { description: 'Asset library' })]) + expect(peekBetaActivationNotice('inst-1')).toEqual({ + args: ['--enable-assets'], + direction: 'enabled', + description: 'Asset library' + }) }) it('leaves the notice pending across repeated reads', () => { // Persisting on show rather than on retire would spend a card the user may never have // seen — window closed, app quit, bell not rendered. - armBetaActivationNotice('inst-1', ['--enable-assets']) - expect(peekBetaActivationNotice('inst-1')).toEqual(['--enable-assets']) - expect(peekBetaActivationNotice('inst-1')).toEqual(['--enable-assets']) + armBetaActivationNotice('inst-1', [grant('--enable-assets')]) + expect(pendingArgs('inst-1')).toEqual(['--enable-assets']) + expect(pendingArgs('inst-1')).toEqual(['--enable-assets']) expect(announced()).toBeUndefined() }) it('persists the args and clears the queue on acknowledge', () => { - armBetaActivationNotice('inst-1', ['--enable-assets']) + armBetaActivationNotice('inst-1', [grant('--enable-assets')]) acknowledgeBetaActivationNotice('inst-1') expect(announced()).toEqual(['--enable-assets']) - expect(peekBetaActivationNotice('inst-1')).toEqual([]) + expect(peekBetaActivationNotice('inst-1')).toBeNull() }) it('stays silent on every later launch of the same feature', () => { - armBetaActivationNotice('inst-1', ['--enable-assets']) + armBetaActivationNotice('inst-1', [grant('--enable-assets')]) acknowledgeBetaActivationNotice('inst-1') - armBetaActivationNotice('inst-1', ['--enable-assets']) - expect(peekBetaActivationNotice('inst-1')).toEqual([]) + armBetaActivationNotice('inst-1', [grant('--enable-assets')]) + expect(peekBetaActivationNotice('inst-1')).toBeNull() }) it('stays silent when the same feature is revoked and later re-granted', () => { // The list is append-only, so a grant taken back and handed out again does not read as // news the second time. - armBetaActivationNotice('inst-1', ['--enable-assets']) + armBetaActivationNotice('inst-1', [grant('--enable-assets')]) acknowledgeBetaActivationNotice('inst-1') armBetaActivationNotice('inst-1', []) // revoked: nothing applied - armBetaActivationNotice('inst-1', ['--enable-assets']) // re-granted - expect(peekBetaActivationNotice('inst-1')).toEqual([]) + armBetaActivationNotice('inst-1', [grant('--enable-assets')]) // re-granted + expect(peekBetaActivationNotice('inst-1')).toBeNull() + }) + + it('announces a named force-off of a feature it already announced turning on', () => { + // Distinct arg tokens, so `--disable-assets` gets its own once-ever: being told a beta + // arrived does not cover being told it was withdrawn. + armBetaActivationNotice('inst-1', [grant('--enable-assets')]) + acknowledgeBetaActivationNotice('inst-1') + armBetaActivationNotice('inst-1', [grant('--disable-assets', { description: 'Asset library' })]) + expect(peekBetaActivationNotice('inst-1')).toEqual({ + args: ['--disable-assets'], + direction: 'disabled', + description: 'Asset library' + }) }) it('tells a SECOND install about a feature the first never announced', () => { - armBetaActivationNotice('inst-1', ['--enable-assets']) + armBetaActivationNotice('inst-1', [grant('--enable-assets')]) acknowledgeBetaActivationNotice('inst-1') - armBetaActivationNotice('inst-2', ['--enable-assets', '--enable-agent']) - expect(peekBetaActivationNotice('inst-2')).toEqual(['--enable-agent']) + armBetaActivationNotice('inst-2', [grant('--enable-assets'), grant('--enable-agent')]) + expect(pendingArgs('inst-2')).toEqual(['--enable-agent']) }) it('merges into what other installs already announced rather than replacing it', () => { store.set(BETA_NOTICE_ANNOUNCED_ARGS_KEY, ['--enable-assets']) - armBetaActivationNotice('inst-2', ['--enable-agent']) + armBetaActivationNotice('inst-2', [grant('--enable-agent')]) acknowledgeBetaActivationNotice('inst-2') expect(announced()).toEqual(['--enable-assets', '--enable-agent']) }) + it('acknowledges only the grants the card described, leaving the rest queued', () => { + // The bug this guards: the card said "a beta feature is on", then acknowledgement consumed + // the undescribed withdrawal too — and because the list is append-only, that withdrawal + // could never be announced again on any install. + armBetaActivationNotice('inst-1', [ + grant('--enable-agent'), + grant('--disable-assets', { description: 'Asset library' }) + ]) + expect(peekBetaActivationNotice('inst-1')?.args).toEqual(['--enable-agent']) + + acknowledgeBetaActivationNotice('inst-1', ['--enable-agent']) + expect(announced()).toEqual(['--enable-agent']) + // The withdrawal survives and gets its own, correctly worded card. + expect(peekBetaActivationNotice('inst-1')).toEqual({ + args: ['--disable-assets'], + direction: 'disabled', + description: 'Asset library' + }) + }) + it('retires the args the card displayed, not whatever is queued at retire time', () => { // A relaunch can re-arm while the sticky card floats. Acknowledging the queue would then - // persist a grant the user was never shown — and the list is append-only, so it could - // never be announced again on any install. - armBetaActivationNotice('inst-1', ['--enable-assets']) - armBetaActivationNotice('inst-1', ['--enable-assets', '--enable-agent']) + // persist a grant the user was never shown. + armBetaActivationNotice('inst-1', [grant('--enable-assets')]) + armBetaActivationNotice('inst-1', [grant('--enable-assets'), grant('--enable-agent')]) acknowledgeBetaActivationNotice('inst-1', ['--enable-assets']) expect(announced()).toEqual(['--enable-assets']) - // The grant that was never on the card is still owed one. - expect(peekBetaActivationNotice('inst-1')).toEqual(['--enable-agent']) + expect(peekBetaActivationNotice('inst-1')?.args).toEqual(['--enable-agent']) }) it('an empty shownArgs falls back to the queue, which is why the handler refuses one', () => { // Documents the contract the IPC handler depends on: `[]` is indistinguishable from // "the renderer named nothing", so the handler must reject a malformed array rather than // filter it down to one — otherwise junk input retires the whole queue permanently. - armBetaActivationNotice('inst-1', ['--enable-assets', '--enable-agent']) + armBetaActivationNotice('inst-1', [grant('--enable-assets'), grant('--enable-agent')]) acknowledgeBetaActivationNotice('inst-1', []) expect(announced()).toEqual(['--enable-assets', '--enable-agent']) }) @@ -191,81 +362,90 @@ describe('arm / peek / acknowledge', () => { it("re-arming replaces the install's pending set with the latest launch's grants", () => { // Each launch is the authority on what is on its own command line. - armBetaActivationNotice('inst-1', ['--enable-assets']) - armBetaActivationNotice('inst-1', ['--enable-assets', '--enable-agent']) - expect(peekBetaActivationNotice('inst-1')).toEqual(['--enable-assets', '--enable-agent']) + armBetaActivationNotice('inst-1', [grant('--enable-assets')]) + armBetaActivationNotice('inst-1', [grant('--enable-assets'), grant('--enable-agent')]) + expect(pendingArgs('inst-1')).toEqual(['--enable-assets', '--enable-agent']) + }) + + it('acknowledges a silenced grant is never queued, so it never reaches the store', () => { + armBetaActivationNotice('inst-1', [grant('--enable-assets', { silent: true })]) + acknowledgeBetaActivationNotice('inst-1') + expect(announced()).toBeUndefined() + // And a later payload that drops `silent` still owes the user the card. + armBetaActivationNotice('inst-1', [grant('--enable-assets')]) + expect(pendingArgs('inst-1')).toEqual(['--enable-assets']) }) it('gives every install its own card, and lets the announced list do the silencing', () => { // Queues are per-install. Two instances really do both have the feature on, and each has // its own title bar, so each gets told. Suppressing the second permanently silenced an // install whose user might never see the other window at all. - armBetaActivationNotice('inst-1', ['--enable-assets']) - armBetaActivationNotice('inst-2', ['--enable-assets']) - expect(peekBetaActivationNotice('inst-1')).toEqual(['--enable-assets']) - expect(peekBetaActivationNotice('inst-2')).toEqual(['--enable-assets']) + armBetaActivationNotice('inst-1', [grant('--enable-assets')]) + armBetaActivationNotice('inst-2', [grant('--enable-assets')]) + expect(pendingArgs('inst-1')).toEqual(['--enable-assets']) + expect(pendingArgs('inst-2')).toEqual(['--enable-assets']) }) // The ordering the arm-time filter misses: BOTH queues are populated first, and only then // does one install acknowledge. It clears its own queue and persists the arg, so the other // install's copy is already sitting in memory when its title bar asks. it('does not serve a queued card for an arg another install acknowledged first', () => { - armBetaActivationNotice('inst-1', ['--enable-assets']) - armBetaActivationNotice('inst-2', ['--enable-assets']) - expect(peekBetaActivationNotice('inst-2')).toEqual(['--enable-assets']) + armBetaActivationNotice('inst-1', [grant('--enable-assets')]) + armBetaActivationNotice('inst-2', [grant('--enable-assets')]) + expect(pendingArgs('inst-2')).toEqual(['--enable-assets']) // inst-2 is still booting; inst-1's user dismisses theirs. acknowledgeBetaActivationNotice('inst-1', ['--enable-assets']) - expect(peekBetaActivationNotice('inst-2')).toEqual([]) + expect(peekBetaActivationNotice('inst-2')).toBeNull() }) it('keeps an unseen grant queued when only the other one was acknowledged', () => { // Filtered, not dropped: inst-2 still has something worth saying. - armBetaActivationNotice('inst-1', ['--enable-assets']) - armBetaActivationNotice('inst-2', ['--enable-assets', '--enable-agent']) + armBetaActivationNotice('inst-1', [grant('--enable-assets')]) + armBetaActivationNotice('inst-2', [grant('--enable-assets'), grant('--enable-agent')]) acknowledgeBetaActivationNotice('inst-1', ['--enable-assets']) - expect(peekBetaActivationNotice('inst-2')).toEqual(['--enable-agent']) + expect(pendingArgs('inst-2')).toEqual(['--enable-agent']) }) it('stays silent everywhere once any install has acknowledged the arg', () => { // This is what "once" means, and it is the only mechanism that survives a restart. - armBetaActivationNotice('inst-1', ['--enable-assets']) + armBetaActivationNotice('inst-1', [grant('--enable-assets')]) acknowledgeBetaActivationNotice('inst-1', ['--enable-assets']) expect(readAnnouncedBetaArgs()).toEqual(['--enable-assets']) - armBetaActivationNotice('inst-2', ['--enable-assets']) - expect(peekBetaActivationNotice('inst-2')).toEqual([]) + armBetaActivationNotice('inst-2', [grant('--enable-assets')]) + expect(peekBetaActivationNotice('inst-2')).toBeNull() }) it('clears a stale claim when the next launch applies no grants', () => { // A beta launch that failed to boot leaves a claim behind. If the user then turns beta off // and relaunches, the card must not still say a beta feature is on. - armBetaActivationNotice('inst-1', ['--enable-assets']) + armBetaActivationNotice('inst-1', [grant('--enable-assets')]) armBetaActivationNotice('inst-1', []) - expect(peekBetaActivationNotice('inst-1')).toEqual([]) + expect(peekBetaActivationNotice('inst-1')).toBeNull() }) // Arming already repairs this on the install's NEXT launch. These cover the window in // between, which the relaunch cannot: the failed launch's progress takeover ends first. it('drops a claim left by a launch that never started', () => { - armBetaActivationNotice('inst-1', ['--enable-assets']) - expect(peekBetaActivationNotice('inst-1')).toEqual(['--enable-assets']) + armBetaActivationNotice('inst-1', [grant('--enable-assets')]) + expect(peekBetaActivationNotice('inst-1')?.args).toEqual(['--enable-assets']) clearBetaActivationClaim('inst-1') - expect(peekBetaActivationNotice('inst-1')).toEqual([]) + expect(peekBetaActivationNotice('inst-1')).toBeNull() }) it('discards only the unannounced claim, never an arg already announced', () => { - armBetaActivationNotice('inst-1', ['--enable-assets']) + armBetaActivationNotice('inst-1', [grant('--enable-assets')]) acknowledgeBetaActivationNotice('inst-1', ['--enable-assets']) expect(readAnnouncedBetaArgs()).toEqual(['--enable-assets']) clearBetaActivationClaim('inst-1') expect(readAnnouncedBetaArgs()).toEqual(['--enable-assets']) // Announced means spent: a later launch of the same arg stays silent. - armBetaActivationNotice('inst-1', ['--enable-assets']) - expect(peekBetaActivationNotice('inst-1')).toEqual([]) + armBetaActivationNotice('inst-1', [grant('--enable-assets')]) + expect(peekBetaActivationNotice('inst-1')).toBeNull() }) }) diff --git a/src/main/lib/betaActivationNotice.ts b/src/main/lib/betaActivationNotice.ts index fc7dee3b6..98a1ef6f5 100644 --- a/src/main/lib/betaActivationNotice.ts +++ b/src/main/lib/betaActivationNotice.ts @@ -17,18 +17,36 @@ * bar's own gate (`useBetaActivationNotice`) opens and asks for it. */ import * as settings from '../settings' +import type { CoreBetaGrant } from './coreBetaGrants' /** Args already announced, as a durable string list. A LIST rather than a boolean so a second * beta feature granted months later still gets its own heads-up; append-only, so a grant that * is revoked and later re-granted stays silent the second time. */ export const BETA_NOTICE_ANNOUNCED_ARGS_KEY = 'betaNoticeAnnouncedArgs' -/** Only grants that turn something ON announce. `--disable-*` exists in the allowlist as a - * remote force-OFF (see `CORE_BETA_GRANTABLE_ARGS`), and the notice's copy — "a beta feature - * is on", pointing at the opt-out switch — would be flatly wrong for one: it names the - * opposite of what happened and offers an action that does not apply. Silent is the honest - * reading until the payload can carry its own copy. */ const ENABLE_PREFIX = '--enable-' +const DISABLE_PREFIX = '--disable-' + +/** One grant the user has not been told about, plus the wording its payload asked for. */ +export interface PendingBetaGrant { + readonly arg: string + /** Whether this grant turned the feature on or off. */ + readonly direction: 'enabled' | 'disabled' + /** Payload-supplied feature name, or `null` for the generic wording. */ + readonly description: string | null +} + +/** What the title bar needs to render one card: which args it covers (so retiring it can + * acknowledge exactly those), and the copy to use. */ +export interface BetaActivationNotice { + readonly args: readonly string[] + /** `'disabled'` only when EVERY covered grant was a force-off; a launch that turned + * something on is "a beta feature is on" regardless of what else it turned off. */ + readonly direction: 'enabled' | 'disabled' + /** Non-null only when the card covers exactly one grant AND its payload named the feature. + * Two features at once have no single honest name, so that falls back to generic. */ + readonly description: string | null +} /** * Pending notices by installation id, drained by the title bar of that install's host window. @@ -37,7 +55,7 @@ const ENABLE_PREFIX = '--enable-' * closed, app quit, bell not reachable — must REPLAY on the next launch rather than being lost, * so nothing is written to disk until the user actually retires the card. */ -const pendingByInstallation = new Map() +const pendingByInstallation = new Map() /** The persisted list, defensive about content: `settings.json` is user-writable, so a * hand-edited non-array or a non-string entry has to read as "nothing announced yet" rather @@ -58,24 +76,66 @@ export function readAnnouncedBetaArgs(): string[] { * The grants from this launch the user has not been told about yet. * * Pure so the trigger rule is testable without settings or a launch: takes what was applied - * plus what is already spoken for, returns what is new. Order follows `appliedArgs` and - * duplicates collapse, so a payload naming an arg twice cannot double-announce it. + * plus what is already spoken for, returns what is new. Order follows `applied` and duplicates + * collapse, so a payload naming an arg twice cannot double-announce it. + * + * Three ways a grant stays silent: + * - the payload asked for it (`notice: 'silent'`), for a flag with nothing to tell the user; + * - it is a `--disable-*` force-off with no payload-supplied name, because the generic + * wording describes turning something ON and there would be nothing truthful to say; + * - it has already been announced, here or on another install. + * A named force-off DOES announce: the payload has supplied the one thing the generic copy + * could not, so the card can say which beta was withdrawn. */ -export function selectNewlyActiveBetaArgs( - appliedArgs: readonly string[], +export function selectNewlyActiveBetaGrants( + applied: readonly CoreBetaGrant[], spokenFor: ReadonlySet -): string[] { - const fresh: string[] = [] +): PendingBetaGrant[] { + const fresh: PendingBetaGrant[] = [] const seen = new Set(spokenFor) - for (const arg of appliedArgs) { - if (!arg.startsWith(ENABLE_PREFIX)) continue - if (seen.has(arg)) continue - seen.add(arg) - fresh.push(arg) + for (const grant of applied) { + if (seen.has(grant.arg)) continue + // Claimed before any skip below, so the de-duplication the doc comment promises holds even + // when the FIRST occurrence is the one that gets skipped — otherwise + // `[{arg: X, silent}, {arg: X}]` would announce X after asking for silence. + seen.add(grant.arg) + if (grant.notice?.silent === true) continue + const description = grant.notice?.description ?? null + // Derived from the prefix PAIR, not as a binary else. An allowlist entry with neither + // prefix is possible (`oppositeArg` already handles that case, and the list is documented + // as growing ahead of Core); defaulting it to `disabled` would turn a card that used to + // stay silent into one that actively says a feature was switched off when it was not. + if (!grant.arg.startsWith(ENABLE_PREFIX) && !grant.arg.startsWith(DISABLE_PREFIX)) continue + const direction = grant.arg.startsWith(ENABLE_PREFIX) ? 'enabled' : 'disabled' + if (direction === 'disabled' && description === null) continue + fresh.push({ arg: grant.arg, direction, description }) } return fresh } +/** + * Collapse this install's pending grants into the single card the title bar renders. + * + * Exported and pure because the collapse rules are the interesting part: what a card may + * honestly claim when it covers more than one grant. Returns `null` when nothing is pending, + * which is how the renderer decides whether to show anything at all. + */ +export function resolveBetaActivationNotice( + pending: readonly PendingBetaGrant[] +): BetaActivationNotice | null { + if (pending.length === 0) return null + // One direction per card, and the card covers ONLY the grants in it. A single launch can + // both turn something on and withdraw something else; collapsing those into one card would + // describe one of them and then acknowledge both, so the undescribed withdrawal could never + // be announced again on any install. Enables go first because "a beta feature is on" is the + // more urgent thing to say; the rest stay pending and get their own card next launch. + const direction = pending.some((grant) => grant.direction === 'enabled') ? 'enabled' : 'disabled' + const covered = pending.filter((grant) => grant.direction === direction) + // A name only belongs on the card when it names everything the card covers. + const description = covered.length === 1 ? covered[0]!.description : null + return { args: covered.map((grant) => grant.arg), direction, description } +} + /** * Queue a notice for any grant this launch turned on for the first time. * @@ -84,7 +144,7 @@ export function selectNewlyActiveBetaArgs( */ export function armBetaActivationNotice( installationId: string, - appliedArgs: readonly string[] + applied: readonly CoreBetaGrant[] ): void { try { // Replace, never append. Arming happens before the spawn is known to have succeeded, so a @@ -93,12 +153,12 @@ export function armBetaActivationNotice( // turned off must clear the old claim, not inherit it and then announce a feature that is // no longer on. pendingByInstallation.delete(installationId) - if (appliedArgs.length === 0) return + if (applied.length === 0) return // Queues are per-install and independent. The persisted announced list is the only thing // that silences an arg, and it is what "once" actually means: it survives restarts, which // no in-memory cross-install bookkeeping can. See the note on `readAnnouncedBetaArgs`. const spokenFor = new Set(readAnnouncedBetaArgs()) - const fresh = selectNewlyActiveBetaArgs(appliedArgs, spokenFor) + const fresh = selectNewlyActiveBetaGrants(applied, spokenFor) if (fresh.length === 0) return pendingByInstallation.set(installationId, fresh) } catch (err) { @@ -125,7 +185,7 @@ export function clearBetaActivationClaim(installationId: string): void { } } -/** What this install's title bar should announce, or `[]`. Read-only: the pending entry +/** The card this install's title bar should raise, or `null`. Read-only: the pending entry * survives until `acknowledgeBetaActivationNotice`, so a card that is shown but never retired * (window closed, app quit) comes back on the next launch. * @@ -136,12 +196,13 @@ export function clearBetaActivationClaim(installationId: string): void { * hold for cards already queued, not merely for launches that come afterwards. * * Filtered rather than dropped: an install queued for two grants keeps the one still unseen - * when only the other has been announced. */ -export function peekBetaActivationNotice(installationId: string): string[] { + * when only the other has been announced — and the wording layer resolves from what is left, + * so a card that survives the filter is worded for the grants it actually represents. */ +export function peekBetaActivationNotice(installationId: string): BetaActivationNotice | null { const queued = pendingByInstallation.get(installationId) ?? [] - if (queued.length === 0) return [] + if (queued.length === 0) return null const announced = new Set(readAnnouncedBetaArgs()) - return queued.filter((arg) => !announced.has(arg)) + return resolveBetaActivationNotice(queued.filter((grant) => !announced.has(grant.arg))) } /** @@ -157,13 +218,16 @@ export function acknowledgeBetaActivationNotice( ): void { const queued = pendingByInstallation.get(installationId) if (!queued || queued.length === 0) return - // Retire exactly what the card DISPLAYED. Re-deriving from the queue at retire time would - // acknowledge whatever is pending now, and a relaunch can re-arm between show and retire - // while the sticky card floats — persisting a grant set the user was never shown, which the - // append-only list then makes unannounceable forever. Falls back to the queue only when no - // args were supplied (an older renderer), which is the pre-existing behaviour. - const covered = - shownArgs && shownArgs.length > 0 ? queued.filter((a) => shownArgs.includes(a)) : queued + // Retire exactly what the card DISPLAYED. Two things make that different from "the queue": + // a card covers only one direction, so a mixed launch deliberately leaves the rest pending; + // and a relaunch can re-arm between show and retire while the sticky card floats. Either + // way, acknowledging more than was shown persists a grant the user never saw, which the + // append-only list then makes unannounceable forever. Falls back to resolving the queue only + // when the renderer named nothing. + const shown = shownArgs?.length ? shownArgs : resolveBetaActivationNotice(queued)?.args + if (!shown || shown.length === 0) return + const coveredSet = new Set(shown) + const covered = queued.filter((grant) => coveredSet.has(grant.arg)).map((grant) => grant.arg) if (covered.length === 0) return try { const merged = [...new Set([...readAnnouncedBetaArgs(), ...covered])] @@ -174,7 +238,8 @@ export function acknowledgeBetaActivationNotice( // for this session while leaving nothing on disk. const persisted = new Set(readAnnouncedBetaArgs()) if (!covered.every((arg) => persisted.has(arg))) return - const remaining = queued.filter((a) => !covered.includes(a)) + // The queue holds grant objects here, not bare args. + const remaining = queued.filter((grant) => !coveredSet.has(grant.arg)) if (remaining.length > 0) pendingByInstallation.set(installationId, remaining) else pendingByInstallation.delete(installationId) } catch (err) { diff --git a/src/main/lib/coreBetaGrants.test.ts b/src/main/lib/coreBetaGrants.test.ts index c9f8867d2..268e84274 100644 --- a/src/main/lib/coreBetaGrants.test.ts +++ b/src/main/lib/coreBetaGrants.test.ts @@ -189,6 +189,100 @@ describe('parseCoreBetaGrants', () => { }) }) +describe('parseCoreBetaGrants notice wording', () => { + /** The exact payload shape live in the prod acceptance-test flag. A flag object carrying + * nothing but `arg` + `min_core_version` MUST keep granting — the notice fields are copy, + * added after that payload was written, and cannot become required. */ + it('grants a payload entry that says nothing about the notice', () => { + expect( + parseCoreBetaGrants(true, { + flags: [{ arg: '--enable-assets', min_core_version: '0.36.0' }] + }) + ).toEqual([{ arg: '--enable-assets', minCoreVersion: '0.36.0' }]) + }) + + it('carries a silent request and a feature name onto the grant', () => { + expect( + parseCoreBetaGrants(true, { + flags: [ + { arg: '--enable-assets', min_core_version: '0.3.80', description: 'Asset library' }, + { arg: '--enable-agent', min_core_version: '0.3.80', notice: 'silent' } + ] + }) + ).toEqual([ + { + arg: '--enable-assets', + minCoreVersion: '0.3.80', + notice: { description: 'Asset library' } + }, + { arg: '--enable-agent', minCoreVersion: '0.3.80', notice: { silent: true } } + ]) + }) + + it('only the exact string "silent" suppresses the card', () => { + // `notice: true` reads as "yes, notify" at least as naturally as "yes, silent", and a + // rollout silenced by accident is invisible until someone asks why nobody was told. + for (const notice of [true, 1, 'SILENT', 'quiet', null]) { + expect( + parseCoreBetaGrants(true, { + flags: [{ arg: '--enable-assets', min_core_version: '0.3.80', notice }] + }) + ).toEqual([{ arg: '--enable-assets', minCoreVersion: '0.3.80' }]) + } + }) + + it('trims a description and drops a blank one', () => { + expect( + parseCoreBetaGrants(true, { + flags: [{ arg: '--enable-assets', min_core_version: '0.3.80', description: ' Assets ' }] + }) + ).toEqual([ + { arg: '--enable-assets', minCoreVersion: '0.3.80', notice: { description: 'Assets' } } + ]) + expect( + parseCoreBetaGrants(true, { + flags: [{ arg: '--enable-assets', min_core_version: '0.3.80', description: ' ' }] + }) + ).toEqual([{ arg: '--enable-assets', minCoreVersion: '0.3.80' }]) + }) + + it('drops an over-long or non-string description instead of refusing the grant', () => { + // Copy never gates a flag: a name too long for the card, or the wrong type entirely, costs + // the card its wording and nothing else. + for (const description of ['x'.repeat(49), 42, { text: 'Assets' }, ['Assets']]) { + expect( + parseCoreBetaGrants(true, { + flags: [{ arg: '--enable-assets', min_core_version: '0.3.80', description }] + }) + ).toEqual([{ arg: '--enable-assets', minCoreVersion: '0.3.80' }]) + } + }) + + it.each([ + ['a newline', 'Assets\nbrowser'], + ['a C0 control', 'Assets\u0007browser'], + ['a bidi override', 'Assets\u202Ebrowser'], + ['a zero-width joiner', 'Assets\u200Dbrowser'] + ])('drops a description containing %s', (_label, description) => { + // The name is rendered verbatim in desktop chrome next to a Settings action, so anything + // that can reshape or reverse the sentence falls back to the generic wording. + expect( + parseCoreBetaGrants(true, { + flags: [{ arg: '--enable-assets', min_core_version: '0.3.80', description }] + }) + ).toEqual([{ arg: '--enable-assets', minCoreVersion: '0.3.80' }]) + }) + + it('keeps a description exactly at the limit', () => { + const description = 'x'.repeat(48) + expect( + parseCoreBetaGrants(true, { + flags: [{ arg: '--enable-assets', min_core_version: '0.3.80', description }] + }) + ).toEqual([{ arg: '--enable-assets', minCoreVersion: '0.3.80', notice: { description } }]) + }) +}) + describe('selectCoreBetaGrantArgs', () => { const unboundedGrant = { arg: '--enable-assets', diff --git a/src/main/lib/coreBetaGrants.ts b/src/main/lib/coreBetaGrants.ts index edc8f033c..d0373e41e 100644 --- a/src/main/lib/coreBetaGrants.ts +++ b/src/main/lib/coreBetaGrants.ts @@ -33,15 +33,51 @@ export const CORE_BETA_GRANTABLE_ARGS = [ '--enable-agent' ] as const +/** How a grant's activation notice should be worded, when it is announced at all. Both fields + * are optional and independent of whether the grant APPLIES — copy never gates a flag. */ +export type CoreBetaNotice = { + /** `true` when the payload asked for no card at all. Ops-controlled because not every + * granted flag is user-visible: a diagnostic or an internal rollout has nothing to tell the + * user, and a card for it is noise that trains people to dismiss the real ones. */ + readonly silent?: true + /** Human name of the feature, e.g. `"Asset library"`. Supplied by the payload rather than + * mapped in Desktop because the allowlist is installed ahead of the features it names — a + * table here would have to ship before anyone knew what to call them. Absent means the + * card falls back to its generic wording. + * + * NOT localized, and not localizable from here: it arrives as one string for every user, + * in whatever language ops wrote it — English today. The card's SENTENCE is translated + * around it. That asymmetry is why the notice templates treat this as an opaque token and + * never as the word they agree with; see the placeholder contract in + * `locales/drafts/README.md`. */ + readonly description?: string +} + export type CoreBetaGrant = { readonly arg: string readonly minCoreVersion: string readonly maxCoreVersion?: string + /** Notice wording for this grant. Absent when the payload said nothing about it. */ + readonly notice?: CoreBetaNotice } const MAX_FLAGS = 32 const CORE_BETA_ARG_RE = /^--[a-z][a-z0-9-]+$/ +/** Cap on a payload-supplied feature name. Bounds the card's HEIGHT: the bubble is a fixed + * ~280px wide, so a long name wraps to more and more lines until the card covers what it is + * annotating. (Width is handled in CSS — `overflow-wrap` breaks an unbroken token that would + * otherwise overflow.) An over-long description is dropped rather than cut, so the card falls + * back to wording that is at least correct. */ +const MAX_DESCRIPTION_LENGTH = 48 + +/** A feature name is rendered verbatim in desktop chrome, beside an action that opens + * Settings — so it is held to printable characters only. Newlines would reshape the card, + * C0/C1 controls can do worse, and a bidi override (U+202E) can visually reverse the + * sentence around it. The payload is hand-authored by operators, so this guards a typo as + * much as anything else; a name that fails it falls back to the generic wording. */ +const PRINTABLE_DESCRIPTION = /^[^\p{Cc}\p{Cf}\p{Cs}\p{Co}\p{Cn}\p{Zl}\p{Zp}]+$/u + // Prevent a control payload copied between PostHog variants from enrolling users. const OFF_VARIANTS = new Set(['control', 'off', 'false', 'disabled']) @@ -55,6 +91,42 @@ function parseCoreVersion(value: unknown): string | null { return semver.valid(value.replace(/^v/, '')) } +/** + * Read the optional notice wording off one payload entry. + * + * Every malformed shape degrades to "the payload said nothing", never to a refusal: this + * governs COPY, and losing a grant because someone typed the feature name wrong would be a + * far worse failure than showing the generic card. Returns `undefined` when nothing usable + * was supplied, so the field is simply absent on the grant. + */ +function parseCoreBetaNotice(candidate: object): CoreBetaNotice | undefined { + const notice: { silent?: true; description?: string } = {} + + // Only the exact string `'silent'` suppresses. A boolean `true` is deliberately NOT accepted: + // `notice: true` reads as "yes, notify" at least as naturally as "yes, silent", and a + // payload that silences a rollout by accident is invisible until someone asks why nobody + // was told. + if ('notice' in candidate && (candidate as { notice?: unknown }).notice === 'silent') { + notice.silent = true + } + + if ('description' in candidate) { + const raw = (candidate as { description?: unknown }).description + if (typeof raw === 'string') { + const trimmed = raw.trim() + if ( + trimmed.length > 0 && + trimmed.length <= MAX_DESCRIPTION_LENGTH && + PRINTABLE_DESCRIPTION.test(trimmed) + ) { + notice.description = trimmed + } + } + } + + return notice.silent === undefined && notice.description === undefined ? undefined : notice +} + export function parseCoreBetaGrants( value: FeatureFlagValue | undefined, payload: unknown @@ -84,11 +156,13 @@ export function parseCoreBetaGrants( } if (flags.some((flag) => flag.arg === candidate.arg)) continue - flags.push( - maxCoreVersion === undefined - ? { arg: candidate.arg, minCoreVersion } - : { arg: candidate.arg, minCoreVersion, maxCoreVersion } - ) + const notice = parseCoreBetaNotice(candidate) + flags.push({ + arg: candidate.arg, + minCoreVersion, + ...(maxCoreVersion === undefined ? {} : { maxCoreVersion }), + ...(notice === undefined ? {} : { notice }) + }) } // Naming a flag and its opposite is an operator mistake, not a precedence order. Applying // either one would pick a silent winner from payload order, so the whole payload grants diff --git a/src/main/lib/ipc/registerSettingsHandlers.ts b/src/main/lib/ipc/registerSettingsHandlers.ts index babb00ece..7ae12da96 100644 --- a/src/main/lib/ipc/registerSettingsHandlers.ts +++ b/src/main/lib/ipc/registerSettingsHandlers.ts @@ -342,7 +342,7 @@ export function registerSettingsHandlers(): void { // during launch, when the host window may still be mid-attach or under the progress // takeover, and the title bar drains it once its own gate opens. ipcMain.handle('get-pending-beta-notice', (_event, installationId: unknown) => { - if (typeof installationId !== 'string' || installationId === '') return [] + if (typeof installationId !== 'string' || installationId === '') return null return peekBetaActivationNotice(installationId) }) diff --git a/src/main/lib/ipc/sessionActions/launch.test.ts b/src/main/lib/ipc/sessionActions/launch.test.ts index b274dd418..3d9b65544 100644 --- a/src/main/lib/ipc/sessionActions/launch.test.ts +++ b/src/main/lib/ipc/sessionActions/launch.test.ts @@ -49,7 +49,7 @@ const launchHarness = vi.hoisted(() => ({ /** Settings can throw on read: `resolveBetaFeaturesEnabled` writes the default back on first * read, so a read-only or full disk surfaces here. */ betaEnabledThrows: false, - grants: [] as { arg: string; minCoreVersion: string }[], + grants: [] as { arg: string; minCoreVersion: string; notice?: CoreBetaNotice }[], /** Runs while `acquireLaunchResources` is in flight — after the launching marker exists and * before either path's pre-spawn abort gate, which is exactly the window under test. */ duringResourceAcquire: null as null | (() => void), @@ -155,7 +155,7 @@ import type { createExecutionTap } from '../../executionTap' import type { createHardwareTap } from '../../hardwareTap' import type { LaunchProgressTracker } from '../../launchProgress' import type { ComfyArgsSchema } from '../../comfy-args' -import type { CoreBetaGrant } from '../../coreBetaGrants' +import type { CoreBetaGrant, CoreBetaNotice } from '../../coreBetaGrants' import * as telemetry from '../../telemetry' import { makeSendOutput, @@ -311,12 +311,12 @@ describe('_cleanupFailedLaunchSetup', () => { // dropped here: otherwise the title bar announces a beta feature for a Core that never ran. it('drops a beta claim armed by a launch that then failed to spawn', () => { _resetBetaNotice() - armBetaActivationNotice(INSTALL, ['--enable-assets']) - expect(peekBetaActivationNotice(INSTALL)).toEqual(['--enable-assets']) + armBetaActivationNotice(INSTALL, [{ arg: '--enable-assets', minCoreVersion: '0.3.80' }]) + expect(peekBetaActivationNotice(INSTALL)?.args).toEqual(['--enable-assets']) _cleanupFailedLaunchSetup(INSTALL, new AbortController()) - expect(peekBetaActivationNotice(INSTALL)).toEqual([]) + expect(peekBetaActivationNotice(INSTALL)).toBeNull() }) it('ends the log stream when one was opened', () => { @@ -996,12 +996,12 @@ describe('core beta report placement', () => { it('arms the activation notice from the same latch that reports the grant', async () => { const id = 'harness-arms-beta-notice' - expect(peekBetaActivationNotice(id)).toEqual([]) + expect(peekBetaActivationNotice(id)).toBeNull() const res = await handleLaunch(ctxFor(id)) expect(res.ok).toBe(true) - expect(peekBetaActivationNotice(id)).toEqual(['--enable-assets']) + expect(peekBetaActivationNotice(id)?.args).toEqual(['--enable-assets']) }) it('arms nothing on a launch whose grants the args schema refused', async () => { @@ -1015,7 +1015,7 @@ describe('core beta report placement', () => { expect(res.ok).toBe(true) expect(spawnArgs).not.toContain('--enable-assets') - expect(peekBetaActivationNotice(id)).toEqual([]) + expect(peekBetaActivationNotice(id)).toBeNull() }) it('arms nothing for an install that opted out of beta features', async () => { @@ -1025,7 +1025,33 @@ describe('core beta report placement', () => { const res = await handleLaunch(ctxFor(id)) expect(res.ok).toBe(true) - expect(peekBetaActivationNotice(id)).toEqual([]) + expect(peekBetaActivationNotice(id)).toBeNull() + }) + + it('arms nothing when the payload asked for a silent grant', async () => { + // Copy control, not flag control: the arg still reaches the command line, the user just + // is not told about it. + launchHarness.grants = [{ ...HARNESS_GRANT, notice: { silent: true } }] + const id = 'harness-silent-grant' + + const res = await handleLaunch(ctxFor(id)) + + expect(res.ok).toBe(true) + expect(spawnArgs).toContain('--enable-assets') + expect(peekBetaActivationNotice(id)).toBeNull() + }) + + it('carries the payload feature name onto the pending card', async () => { + launchHarness.grants = [{ ...HARNESS_GRANT, notice: { description: 'Asset library' } }] + const id = 'harness-named-grant' + + await handleLaunch(ctxFor(id)) + + expect(peekBetaActivationNotice(id)).toEqual({ + args: ['--enable-assets'], + direction: 'enabled', + description: 'Asset library' + }) }) it('stays silent on the NEXT launch once the notice has been acknowledged', async () => { @@ -1036,7 +1062,7 @@ describe('core beta report placement', () => { await handleLaunch(ctxFor(id)) - expect(peekBetaActivationNotice(id)).toEqual([]) + expect(peekBetaActivationNotice(id)).toBeNull() }) /** The commit `harnessInstall`'s record names, i.e. what the version gate believes is running. */ diff --git a/src/main/lib/ipc/sessionActions/launch.ts b/src/main/lib/ipc/sessionActions/launch.ts index e06d3cb15..cec8ef194 100644 --- a/src/main/lib/ipc/sessionActions/launch.ts +++ b/src/main/lib/ipc/sessionActions/launch.ts @@ -787,10 +787,7 @@ async function runLaunch( // Same latch, same reason: a grant is only worth announcing once it is provably on this // launch's command line. Queued rather than shown — the host window may still be mid-attach // or under the progress takeover, so the title bar drains this when its own gate opens. - armBetaActivationNotice( - installationId, - coreBeta.applied.map((grant) => grant.arg) - ) + armBetaActivationNotice(installationId, coreBeta.applied) try { emitCoreBetaTelemetry({ appliedArgs: coreBeta.applied.map((grant) => grant.arg), diff --git a/src/renderer/src/comfyTitleBar/TitleBarApp.test.ts b/src/renderer/src/comfyTitleBar/TitleBarApp.test.ts index dda8d7b01..15785b3cd 100644 --- a/src/renderer/src/comfyTitleBar/TitleBarApp.test.ts +++ b/src/renderer/src/comfyTitleBar/TitleBarApp.test.ts @@ -1242,7 +1242,7 @@ describe('TitleBarApp', () => { setSetting = vi.fn().mockResolvedValue(undefined) // Nothing pending, so the beta notice never competes for the single popup and these // assertions keep counting only pill-hint shows. - getPendingBetaNotice = vi.fn().mockResolvedValue([]) + getPendingBetaNotice = vi.fn().mockResolvedValue(null) ;(window as unknown as { api: unknown }).api = { getSetting, setSetting, @@ -1347,8 +1347,22 @@ describe('TitleBarApp', () => { let openGlobalSettings: ReturnType let setSetting: ReturnType - function installApiMock(opts: { pending?: string[]; pillHintSeen?: boolean } = {}): void { - getPendingBetaNotice = vi.fn().mockResolvedValue(opts.pending ?? ['--enable-assets']) + function installApiMock( + opts: { + /** `null` = main has nothing to announce. */ + pending?: { + args: string[] + direction: 'enabled' | 'disabled' + description: string | null + } | null + pillHintSeen?: boolean + } = {} + ): void { + const pending = + opts.pending === undefined + ? { args: ['--enable-assets'], direction: 'enabled' as const, description: null } + : opts.pending + getPendingBetaNotice = vi.fn().mockResolvedValue(pending) acknowledgeBetaNotice = vi.fn().mockResolvedValue(undefined) openGlobalSettings = vi.fn() setSetting = vi.fn().mockResolvedValue(undefined) @@ -1397,8 +1411,7 @@ describe('TitleBarApp', () => { expect(betaCards().length).toBe(1) const payload = betaCards()[0]! expect(payload.title).toBe('A beta feature is on') - // Copy is generic on purpose: the card never names the arg, so it cannot be wrong - // about which feature turned on. + // With no payload-supplied name the copy stays generic, and it never leaks the raw arg. expect(payload.body).not.toContain('--enable-assets') // The action is what makes it more than an FYI. expect(payload.actionLabel).toBe('Settings') @@ -1406,7 +1419,7 @@ describe('TitleBarApp', () => { }) it('stays silent when main has nothing pending', async () => { - installApiMock({ pending: [] }) + installApiMock({ pending: null }) const wrapper = await mountBar() expect(betaCards().length).toBe(0) wrapper.unmount() @@ -1480,7 +1493,11 @@ describe('TitleBarApp', () => { expect(acknowledgeBetaNotice).not.toHaveBeenCalled() // And the card can be raised again once the popup frees up. - getPendingBetaNotice.mockResolvedValue(['--enable-something-else']) + getPendingBetaNotice.mockResolvedValue({ + args: ['--enable-something-else'], + direction: 'enabled', + description: null + }) bridgeState.coachmarkDismissedCallbacks.forEach((cb) => cb({ kind: 'pill-hint' })) await flushPromises() expect(betaCards().length).toBe(2) @@ -1574,7 +1591,9 @@ describe('TitleBarApp', () => { // card comes down unspent — acknowledging the new install would permanently consume a // notice that was never shown for it. getPendingBetaNotice.mockImplementation(async (id: string) => - id === 'inst-1' ? ['--enable-assets'] : [] + id === 'inst-1' + ? { args: ['--enable-assets'], direction: 'enabled', description: null } + : null ) const wrapper = await mountBar() expect(betaCards().length).toBe(1) @@ -1595,7 +1614,9 @@ describe('TitleBarApp', () => { // The gate watcher keys on install-less/lockdown, neither of which moves on a retarget, // so without an explicit re-query the new install's notice is never asked for again. getPendingBetaNotice.mockImplementation(async (id: string) => - id === 'inst-2' ? ['--enable-agent'] : [] + id === 'inst-2' + ? { args: ['--enable-agent'], direction: 'enabled', description: null } + : null ) const wrapper = await mountBar() expect(betaCards().length).toBe(0) @@ -1615,7 +1636,11 @@ describe('TitleBarApp', () => { const wrapper = await mountBar() expect(betaCards().length).toBe(1) - getPendingBetaNotice.mockResolvedValue(['--enable-agent']) + getPendingBetaNotice.mockResolvedValue({ + args: ['--enable-agent'], + direction: 'enabled', + description: null + }) bridgeState.coachmarkDismissedCallbacks.forEach((cb) => cb({ kind: 'beta-notice' })) await flushPromises() expect(acknowledgeBetaNotice).toHaveBeenCalledWith('inst-1', ['--enable-assets']) @@ -1648,6 +1673,36 @@ describe('TitleBarApp', () => { wrapper.unmount() }) + it('names the feature when the PostHog payload supplied a name', async () => { + installApiMock({ + pending: { args: ['--enable-assets'], direction: 'enabled', description: 'Asset library' } + }) + const wrapper = await mountBar() + const payload = betaCards()[0]! + expect(payload.title).toBe('The Asset library beta is on') + expect(payload.body).toContain('Asset library') + wrapper.unmount() + }) + + it('reads the other way round for a named remote force-off', async () => { + // A withdrawn beta is not "a beta feature is on". Main only resolves this direction for + // a grant the payload named, so there is always something to put in the sentence. + installApiMock({ + pending: { + args: ['--disable-assets'], + direction: 'disabled', + description: 'Asset library' + } + }) + const wrapper = await mountBar() + const payload = betaCards()[0]! + expect(payload.title).toBe('The Asset library beta is off') + expect(payload.title).not.toContain('is on') + // Still points at Settings — the beta program switch is what the user can act on. + expect(payload.actionLabel).toBe('Settings') + wrapper.unmount() + }) + it('routes a pill-hint dismiss away from the beta notice', async () => { // One popup, two owners: a retirement addressed to the hint must not spend the // notice's once-ever acknowledgement. diff --git a/src/renderer/src/comfyTitleBar/TitleBarApp.vue b/src/renderer/src/comfyTitleBar/TitleBarApp.vue index a17a7ffd6..5d615d860 100644 --- a/src/renderer/src/comfyTitleBar/TitleBarApp.vue +++ b/src/renderer/src/comfyTitleBar/TitleBarApp.vue @@ -554,12 +554,39 @@ const betaNotice = useBetaActivationNotice({ isLoadingLockdown, anchorRef: announcementBtnRef, isSuppressed: () => coachmark.isShowing.value, - // Getters, not strings: `syncLocale()` runs on mount, after this setup block, so a - // snapshot taken here is English regardless of the user's persisted locale. - title: () => t('titleBar.betaNoticeTitle'), - body: () => t('titleBar.betaNoticeBody'), - dismissLabel: () => t('titleBar.betaNoticeDismiss'), - actionLabel: () => t('titleBar.betaNoticeSettings') + // Four wordings, picked by what main could establish: whether the grant turned the feature + // on or withdrew it, and whether the PostHog payload named it. The generic pair is the + // fallback, so an unnamed feature still gets a card that is true. + copyFor: ({ direction, description }) => { + // Static keys rather than composed ones: `createAppI18n` disables missing-key warnings, so + // a rename in en.json would otherwise degrade silently to a card titled with the literal + // key. Written out, the four are greppable and fail visibly. + // + // The `{feature}` slot in the *Named variants is an OPAQUE PROPER NAME, supplied by the + // flag payload and not localized. In every template it modifies the constant head noun + // ("beta"), so what a gendered or case-marking language agrees with is that head noun and + // never the slotted name. A translation that promotes `{feature}` to the grammatical head + // — "{feature} est activé", "{feature} включён" — needs a gender Desktop does not have and + // cannot get. en and zh are the only shipping locales and zh has neither gender nor case, + // so nothing exercises this today; the contract is written down so a third locale cannot + // introduce the fragile form silently. Contract for translators: + // `locales/drafts/README.md`. + const keys = + direction === 'disabled' + ? description + ? (['titleBar.betaNoticeOffTitleNamed', 'titleBar.betaNoticeOffBodyNamed'] as const) + : (['titleBar.betaNoticeOffTitle', 'titleBar.betaNoticeOffBody'] as const) + : description + ? (['titleBar.betaNoticeTitleNamed', 'titleBar.betaNoticeBodyNamed'] as const) + : (['titleBar.betaNoticeTitle', 'titleBar.betaNoticeBody'] as const) + const params = { feature: description ?? '' } + return { + title: t(keys[0], params), + body: t(keys[1], params), + dismissLabel: t('titleBar.betaNoticeDismiss'), + actionLabel: t('titleBar.betaNoticeSettings') + } + } }) /** Wrap the pill opener so opening the drawer retires the coachmark diff --git a/src/renderer/src/comfyTitleBar/useBetaActivationNotice.test.ts b/src/renderer/src/comfyTitleBar/useBetaActivationNotice.test.ts index 56aa829fb..0096122de 100644 --- a/src/renderer/src/comfyTitleBar/useBetaActivationNotice.test.ts +++ b/src/renderer/src/comfyTitleBar/useBetaActivationNotice.test.ts @@ -27,16 +27,22 @@ describe('useBetaActivationNotice copy resolution', () => { isLoadingLockdown: ref(false), anchorRef: shallowRef(anchor), isSuppressed: () => false, - title: copy, - body: copy, - dismissLabel: copy, - actionLabel: copy + copyFor: () => ({ + title: copy(), + body: copy(), + dismissLabel: copy(), + actionLabel: copy() + }) }) } beforeEach(() => { ;(window as unknown as { api: unknown }).api = { - getPendingBetaNotice: vi.fn().mockResolvedValue(['--enable-assets']), + getPendingBetaNotice: vi.fn().mockResolvedValue({ + args: ['--enable-assets'], + direction: 'enabled', + description: null + }), acknowledgeBetaNotice: vi.fn().mockResolvedValue(undefined), openGlobalSettings: vi.fn() } @@ -73,9 +79,11 @@ describe('useBetaActivationNotice copy resolution', () => { // The user switches language, then a second grant clears its version gate. translated = 'second-language' notice.forgetWithoutAcknowledging() - ;(window.api.getPendingBetaNotice as ReturnType).mockResolvedValue([ - '--enable-something-else' - ]) + ;(window.api.getPendingBetaNotice as ReturnType).mockResolvedValue({ + args: ['--enable-something-else'], + direction: 'enabled', + description: null + }) await notice.maybeShow() expect(showCoachmark).toHaveBeenCalledTimes(2) diff --git a/src/renderer/src/comfyTitleBar/useBetaActivationNotice.ts b/src/renderer/src/comfyTitleBar/useBetaActivationNotice.ts index ab6752ef2..ab400f2e8 100644 --- a/src/renderer/src/comfyTitleBar/useBetaActivationNotice.ts +++ b/src/renderer/src/comfyTitleBar/useBetaActivationNotice.ts @@ -1,7 +1,14 @@ import { ref, type Ref, type ShallowRef } from 'vue' +import type { BetaActivationNotice } from '../types/ipc' /** The Settings row the notice's link flashes — the beta opt-in switch itself, so the - * "turn it off" the copy promises is the thing under the user's cursor when Settings opens. */ + * "turn it off" the copy promises is the thing under the user's cursor when Settings opens. + * + * Deliberately the same target for a withdrawal card. Turning this switch off drops every + * grant including a `--disable-*` one, so it would restore the feature the card just said was + * withdrawn — but the card only offers to "manage beta features", which is exactly what this + * row does. Pointing a withdrawal somewhere else would mean inventing a second destination + * for a path that no shipped core can reach yet. */ export const BETA_FEATURES_FIELD_ID = 'betaFeaturesEnabled' interface BetaNoticeBridge { @@ -35,16 +42,20 @@ interface UseBetaActivationNoticeOpts { anchorRef: Readonly> /** True while another card owns the single popup (currently the pill hint). */ isSuppressed: () => boolean - /** Card copy, resolved by the caller at show time rather than passed as strings. + /** Copy for the card main actually resolved. A callback rather than fixed strings because + * the wording depends on the notice: the PostHog payload may name the feature, and a + * remote force-off reads the opposite way from an activation. i18n stays with the caller. * - * Lazy deliberately: the title bar's i18n instance starts in English and `syncLocale()` - * does not run until mount, so anything read during setup is an English snapshot that - * never updates — wrong copy for a non-English user, and permanently wrong for every - * later card in this renderer. Called when the card is built instead. */ - title: () => string - body: () => string - dismissLabel: () => string - actionLabel: () => string + * Being a callback also keeps the copy lazy, which is load-bearing on its own: the title + * bar's i18n instance starts in English and `syncLocale()` does not run until mount, so + * anything read during setup is an English snapshot that never updates — wrong for a + * non-English user, and permanently wrong for every later card in this renderer. */ + copyFor: (notice: BetaActivationNotice) => { + title: string + body: string + dismissLabel: string + actionLabel: string + } } interface BetaActivationNoticeApi { @@ -123,16 +134,27 @@ export function useBetaActivationNotice( ) } - /** The args main is holding for this install, or `[]`. Returned rather than reduced to a - * boolean because the args are the card's identity — see `retiredKeys`. */ - async function pendingArgs(installationId: string): Promise { + /** The card main is holding for this install, or `null`. + * + * Takes the id rather than re-reading it, so it cannot disagree with the one `maybeShow` + * captured and validated. Validates `direction` against its two literals: an unrecognised + * value would otherwise fall through to `copyFor`, whose `=== 'disabled'` test would then + * silently pick the "is on" wording for a withdrawal. */ + async function pendingNotice(installationId: string): Promise { try { const pending = await window.api.getPendingBetaNotice(installationId) - return Array.isArray(pending) ? pending.filter((a) => typeof a === 'string') : [] + if (!pending || !Array.isArray(pending.args) || pending.args.length === 0) return null + if (pending.direction !== 'enabled' && pending.direction !== 'disabled') return null + const description = typeof pending.description === 'string' ? pending.description : null + return { + args: pending.args.filter((a) => typeof a === 'string'), + direction: pending.direction, + description + } } catch { // Read failed; stay silent. Unlike the pill hint's "treat as unseen", guessing wrong // here would announce a beta feature that may not be on at all. - return [] + return null } } @@ -165,26 +187,27 @@ export function useBetaActivationNotice( * in-flight claim across the whole thing without a `try` nested in the guards. */ async function showIfPending(installationId: string): Promise { if (!opts.bridge) return - const args = await pendingArgs(installationId) - if (args.length === 0) return - const key = noticeKey(installationId, args) + const notice = await pendingNotice(installationId) + if (!notice) return + const key = noticeKey(installationId, notice.args) if (retiredKeys.has(key)) return // Re-check after the await; the host could have flipped state or the pill hint could have // claimed the popup while we were asking. const anchor = opts.anchorRef.value if (!gatePasses() || !anchor || opts.installationId() !== installationId) return + const copy = opts.copyFor(notice) const rect = anchor.getBoundingClientRect() shownForInstall = installationId shownKey = key - shownArgs = args + shownArgs = notice.args isShowing.value = true opts.bridge.showCoachmark({ kind: 'beta-notice', - title: opts.title(), - body: opts.body(), - dismissLabel: opts.dismissLabel(), - actionLabel: opts.actionLabel(), + title: copy.title, + body: copy.body, + dismissLabel: copy.dismissLabel, + actionLabel: copy.actionLabel, leftX: Math.round(rect.left), rightX: Math.round(rect.right), bottomY: Math.round(rect.bottom) diff --git a/src/renderer/src/comfyTitleTooltip/TitleTooltipApp.vue b/src/renderer/src/comfyTitleTooltip/TitleTooltipApp.vue index 5333c2881..c97a46263 100644 --- a/src/renderer/src/comfyTitleTooltip/TitleTooltipApp.vue +++ b/src/renderer/src/comfyTitleTooltip/TitleTooltipApp.vue @@ -281,6 +281,14 @@ onUnmounted(() => { border-top-left-radius: 3px; } +/* The card is `max-width: 280px` with `overflow: hidden` on the viewport, and a + payload-supplied feature name can be a single unbroken token — which would otherwise be + clipped rather than wrapped. */ +.coachmark-title, +.coachmark-text { + overflow-wrap: anywhere; +} + .coachmark-title { font-size: 13px; font-weight: 600; diff --git a/src/renderer/src/types/ipc.ts b/src/renderer/src/types/ipc.ts index 4330452f7..9e8e74a08 100644 --- a/src/renderer/src/types/ipc.ts +++ b/src/renderer/src/types/ipc.ts @@ -1,6 +1,7 @@ // Re-export IPC types from the canonical shared location for renderer convenience. export type { Unsubscribe, + BetaActivationNotice, Installation, RunningInstance, Source, diff --git a/src/types/ipc.ts b/src/types/ipc.ts index 19b2feacf..e1d6473c7 100644 --- a/src/types/ipc.ts +++ b/src/types/ipc.ts @@ -11,6 +11,13 @@ export type { FirstUseMode } import type { AuthStatus, Workspace } from '../main/cloud/types' export type { AuthStatus, Workspace } +// One Core beta activation card, as main resolves it for the title bar. Re-exported from its +// producer rather than restated here: this file's header forbids duplicating types, and an +// independent copy would drift silently — `ipcMain.handle` is ungeneric and `ipcRenderer.invoke` +// returns `Promise`, so nothing would fail the build. +import type { BetaActivationNotice } from '../main/lib/betaActivationNotice' +export type { BetaActivationNotice } + /** Every renderer-safe Build catalog state. */ export type DevPlatformBuildState = | 'installable' @@ -1436,11 +1443,12 @@ export interface ElectronApi { getSetting(key: string): Promise // Core beta activation notice - /** Core beta args this install turned on for the first time and has not - * announced yet, or `[]`. Read repeatedly without side effects — the - * pending set is only cleared by `acknowledgeBetaNotice`, so a card that is - * shown but never retired comes back on the next launch. */ - getPendingBetaNotice(installationId: string): Promise + /** The activation card this install owes the user, or `null`. Read repeatedly + * without side effects — the pending set is only cleared by + * `acknowledgeBetaNotice`, so a card that is shown but never retired comes + * back on the next launch. `description` carries the feature name the + * PostHog payload supplied, when it supplied one. */ + getPendingBetaNotice(installationId: string): Promise /** Retire this install's activation notice: the args are persisted as * announced and never raise a card again. Called when the user dismisses * the card or follows its settings link.