diff --git a/client/src/store/systemNoticeStore.ts b/client/src/store/systemNoticeStore.ts index 614c21c29..f7540b5e0 100644 --- a/client/src/store/systemNoticeStore.ts +++ b/client/src/store/systemNoticeStore.ts @@ -7,6 +7,10 @@ import { systemNoticeDtoSchema, type SystemNoticeDto } from '@trek/shared'; // SystemNoticeBanner/Modal consumers don't need to change their imports. export type SystemNoticeDTO = SystemNoticeDto; +// Baked in by `define` in vite.config.js; a build without it (a bare tsc run) sends +// nothing and therefore gets no release notice, which is the safe side. +const UI_VERSION: string = typeof __TREK_UI_VERSION__ === 'string' ? __TREK_UI_VERSION__ : ''; + interface SystemNoticeState { notices: SystemNoticeDTO[]; loaded: boolean; @@ -25,12 +29,14 @@ export const useSystemNoticeStore = create()((set, get) => ({ if (get().fetching || get().loaded) return; set({ fetching: true }); try { - // Names the layouts this bundle can draw. The server holds the release notice - // back from a bundle that does not say so: after an update the service worker + // Names the layouts this bundle can draw and the version it was built as. The + // server hands the release notice only to a bundle that can draw it AND was + // built for the version the server runs: after an update the service worker // serves the previous bundle until the new one is installed, and that bundle - // would draw the release notice as bare keys and let the reader dismiss it for - // good. It gets the notice after the reload, from this very line. - const res = await axios.get('/system-notices/active', { params: { supports: 'release' } }); + // would draw the release notice as bare keys or with its own older texts, and + // let the reader dismiss it for good. It gets the notice after the reload, + // from this very line. + const res = await axios.get('/system-notices/active', { params: { supports: 'release', ui: UI_VERSION } }); const notices = parseInDev(systemNoticeDtoSchema.array(), res.data, 'systemNotices.fetch'); set({ notices, loaded: true, fetching: false }); } catch (err) { diff --git a/client/src/vite-env.d.ts b/client/src/vite-env.d.ts index 97ce4524c..f77306978 100644 --- a/client/src/vite-env.d.ts +++ b/client/src/vite-env.d.ts @@ -1,2 +1,5 @@ /// /// + +/** The version this bundle was built as (client/package.json), set by `define` in vite.config.js. */ +declare const __TREK_UI_VERSION__: string; diff --git a/client/tests/unit/stores/systemNoticeStore.test.ts b/client/tests/unit/stores/systemNoticeStore.test.ts index 0e96965a0..551e022e4 100644 --- a/client/tests/unit/stores/systemNoticeStore.test.ts +++ b/client/tests/unit/stores/systemNoticeStore.test.ts @@ -15,7 +15,7 @@ describe('systemNoticeStore', () => { // can draw the release layout, so an older bundle never shows it as bare keys and // never dismisses it for good. This bundle has to announce it on every fetch. describe('FE-SYSNOTICE-001: fetch() announces the layouts this bundle can draw', () => { - it('sends supports=release with the active-notices request', async () => { + it('sends supports=release and the version this bundle was built as', async () => { const urls: URL[] = []; server.use( http.get('/api/system-notices/active', ({ request }) => { @@ -28,6 +28,10 @@ describe('systemNoticeStore', () => { expect(urls).toHaveLength(1); expect(urls[0].searchParams.get('supports')).toBe('release'); + // The value `define` bakes in from client/package.json: the server compares it + // with its own version before it hands the release notice over. + expect(urls[0].searchParams.get('ui')).toBe(__TREK_UI_VERSION__); + expect(urls[0].searchParams.get('ui')).toMatch(/^\d+\.\d+\.\d+/); expect(useSystemNoticeStore.getState().loaded).toBe(true); }); diff --git a/client/vite.config.js b/client/vite.config.js index ded3c086d..931ecd203 100644 --- a/client/vite.config.js +++ b/client/vite.config.js @@ -3,6 +3,13 @@ import { defineConfig } from 'vite'; import { VitePWA } from 'vite-plugin-pwa'; import { visualizer } from 'rollup-plugin-visualizer'; import { rtlTextAlias } from './rtlTextAlias.js'; +import { readFileSync } from 'node:fs'; + +// The version this bundle is built as, baked in at build time. The release image +// bumps every package.json before it builds, so this matches the server's +// APP_VERSION there; a source checkout matches the server's own package.json. +// The server hands the release notice only to a bundle built for its version. +const UI_VERSION = JSON.parse(readFileSync(new URL('./package.json', import.meta.url), 'utf8')).version; // `npm run build:analyze` writes dist/stats.html — a treemap of what actually ended // up in each chunk. The plain build only reports chunk sizes, which tells you a chunk @@ -13,6 +20,7 @@ const DEV_PORT = Number(process.env.TREK_DEV_PORT) || 5173; const API_TARGET = process.env.TREK_DEV_API || 'http://localhost:3001'; export default defineConfig(({ mode }) => ({ + define: { __TREK_UI_VERSION__: JSON.stringify(UI_VERSION) }, plugins: [ react(), mode === 'analyze' && diff --git a/client/vitest.config.ts b/client/vitest.config.ts index 8c135d1f3..27b403c27 100644 --- a/client/vitest.config.ts +++ b/client/vitest.config.ts @@ -1,8 +1,12 @@ import { defineConfig } from 'vitest/config'; import react from '@vitejs/plugin-react'; import { rtlTextAlias } from './rtlTextAlias.js'; +import { readFileSync } from 'node:fs'; + +const UI_VERSION = JSON.parse(readFileSync(new URL('./package.json', import.meta.url), 'utf8')).version; export default defineConfig({ + define: { __TREK_UI_VERSION__: JSON.stringify(UI_VERSION) }, plugins: [react()], test: { alias: [rtlTextAlias], diff --git a/server/src/nest/system-notices/system-notices.controller.ts b/server/src/nest/system-notices/system-notices.controller.ts index d23a4e2d3..e31f990b3 100644 --- a/server/src/nest/system-notices/system-notices.controller.ts +++ b/server/src/nest/system-notices/system-notices.controller.ts @@ -6,9 +6,9 @@ import { JwtAuthGuard } from '../auth/jwt-auth.guard'; import { CurrentUser } from '../auth/current-user.decorator'; /** - * `?supports=release` names the layouts the calling bundle can draw, comma separated. - * A missing or malformed value means none, which is what every bundle before this - * parameter sent. + * `?supports=release` names the layouts the calling bundle can draw, comma separated, + * and `?ui=4.3.0` the version the bundle was built as. A missing or malformed value + * means none, which is what every bundle before these parameters sent. */ function parseSupports(raw: string | string[] | undefined): Set { const values = Array.isArray(raw) ? raw : [raw]; @@ -30,8 +30,12 @@ export class SystemNoticesController { constructor(private readonly notices: SystemNoticesService) {} @Get('active') - active(@CurrentUser() user: User, @Query('supports') supports?: string | string[]): SystemNoticeDto[] { - return this.notices.getActiveFor(user.id, parseSupports(supports)); + active( + @CurrentUser() user: User, + @Query('supports') supports?: string | string[], + @Query('ui') ui?: string | string[], + ): SystemNoticeDto[] { + return this.notices.getActiveFor(user.id, parseSupports(supports), typeof ui === 'string' ? ui.trim() : undefined); } @Post(':id/dismiss') diff --git a/server/src/nest/system-notices/system-notices.service.ts b/server/src/nest/system-notices/system-notices.service.ts index 4b143de66..02f5cae4a 100644 --- a/server/src/nest/system-notices/system-notices.service.ts +++ b/server/src/nest/system-notices/system-notices.service.ts @@ -1,6 +1,7 @@ import { Injectable } from '@nestjs/common'; +import semver from 'semver'; import type { SystemNoticeDto } from '@trek/shared'; -import { getActiveNoticesFor, dismissNotice } from '../../systemNotices/service'; +import { getActiveNoticesFor, dismissNotice, getCurrentAppVersion } from '../../systemNotices/service'; import { AddonsService } from '../addons/addons.service'; import { RuntimeEnvService } from '../app-config/runtime-env.service'; @@ -19,22 +20,35 @@ export class SystemNoticesService { ) {} /** - * `supports` names the layouts the calling bundle can draw. A notice with a release - * block is held back from a bundle that does not name `release`: after an update the - * service worker keeps serving the previous bundle until the new one is installed, - * and that bundle would draw the release notice as bare keys and let the reader - * dismiss it for good, because the dismissal is recorded against the server's - * version. A bundle that never sends the parameter loses only the notice it could - * not read anyway and gets it after the reload. Nothing is spent by holding it back: - * a notice is used up by a dismissal and by nothing else. + * `supports` names the layouts the calling bundle can draw, `uiVersion` the version + * that bundle was built as. A notice with a release block goes only to a bundle that + * names `release` AND was built for the version the server is running: after an + * update the service worker keeps serving the previous bundle until the new one is + * installed, and that bundle would draw the release notice with its own, older + * texts (or as bare keys) and let the reader dismiss it for good, because the + * dismissal is recorded against the server's version. The version match is what + * makes this hold for every update, not only the one that introduced the + * parameter: a 4.3.0 shell asking a 4.3.1 server is told nothing, and gets the + * notice after the reload from the bundle that can draw it. A bundle that sends + * neither loses only the notice it could not read anyway. Nothing is spent by + * holding it back: a notice is used up by a dismissal and by nothing else. */ - getActiveFor(userId: number, supports: ReadonlySet = new Set()): SystemNoticeDto[] { + getActiveFor(userId: number, supports: ReadonlySet = new Set(), uiVersion?: string): SystemNoticeDto[] { const notices = getActiveNoticesFor( userId, (addonId) => this.addons.isAddonEnabled(addonId), this.env.isManaged(), ) as SystemNoticeDto[]; - return supports.has('release') ? notices : notices.filter(n => !n.release); + return supports.has('release') && this.bundleMatchesServer(uiVersion) + ? notices + : notices.filter(n => !n.release); + } + + /** Whether the bundle asking was built for the version this server runs. */ + private bundleMatchesServer(uiVersion: string | undefined): boolean { + const ui = uiVersion ? semver.coerce(uiVersion)?.version : undefined; + const app = semver.coerce(getCurrentAppVersion())?.version; + return !!ui && !!app && ui === app; } dismiss(userId: number, noticeId: string): boolean { diff --git a/server/src/systemNotices/service.ts b/server/src/systemNotices/service.ts index f53bcf1a7..71398f081 100644 --- a/server/src/systemNotices/service.ts +++ b/server/src/systemNotices/service.ts @@ -5,7 +5,7 @@ import { SYSTEM_NOTICES } from './registry.js'; import { evaluate } from './conditions.js'; import type { SystemNotice, SystemNoticeDTO } from './types.js'; -function getCurrentAppVersion(): string { +export function getCurrentAppVersion(): string { const fromEnv = semver.valid(readEnv().app.appVersion ?? ''); if (fromEnv) return fromEnv; try { diff --git a/server/tests/integration/systemNotices.test.ts b/server/tests/integration/systemNotices.test.ts index afbc2b4e2..26b4b4b89 100644 --- a/server/tests/integration/systemNotices.test.ts +++ b/server/tests/integration/systemNotices.test.ts @@ -48,6 +48,7 @@ import { resetTestDb } from '../helpers/test-db'; import { createUser, createAdmin } from '../helpers/factories'; import { authCookie } from '../helpers/auth'; import { SYSTEM_NOTICES } from '../../src/systemNotices/registry'; +import { getCurrentAppVersion } from '../../src/systemNotices/service'; import type { SystemNotice } from '../../src/systemNotices/types'; let nestApp: INestApplication; @@ -235,12 +236,13 @@ describe('GET /api/system-notices/active', () => { const { user } = createUser(testDb); testDb.prepare('UPDATE users SET login_count = 5, first_seen_version = ? WHERE id = ?').run('3.0.0', user.id); - // The client announces the release layout with `?supports=release`; a bundle that - // predates it never gets the notice (pinned separately below). + // The client announces the release layout and the version it was built as; a + // bundle that predates that, or one built for another version, never gets the + // notice (pinned separately below). const shows = async () => { const res = await request(app) .get('/api/system-notices/active') - .query({ supports: 'release' }) + .query({ supports: 'release', ui: getCurrentAppVersion() }) .set('Cookie', authCookie(user.id)); expect(res.status).toBe(200); return res.body.some((n: { id: string }) => n.id === 'release-notes'); @@ -273,9 +275,10 @@ describe('GET /api/system-notices/active', () => { // Right after an update the browser still runs the bundle the service worker cached, // which has neither the copy for the new release notice nor a way to tell. It would - // draw the keys and, on close, spend the notice for this whole version. So the notice - // waits for a client that announces the release layout; the reload brings one. - it('keeps the release notes from a client that does not announce the release layout', async () => { + // draw the keys, or its own older texts, and on close spend the notice for this whole + // version. So the notice waits for a client that announces the release layout AND + // was built for the version now running; the reload brings one. + it('keeps the release notes from a client that does not announce the release layout for this version', async () => { const { user } = createUser(testDb); testDb.prepare('UPDATE users SET login_count = 5, first_seen_version = ? WHERE id = ?').run('3.0.0', user.id); @@ -289,9 +292,15 @@ describe('GET /api/system-notices/active', () => { expect(without.body.some((n: { id: string }) => n.id === 'release-notes')).toBe(false); expect(without.body.some((n: { release?: unknown }) => n.release !== undefined)).toBe(false); - const withLayout = await fetchActive({ supports: 'release' }); - expect(withLayout.status).toBe(200); - expect(withLayout.body.some((n: { id: string }) => n.id === 'release-notes')).toBe(true); + // The layout alone is the shell of the previous version after an update. + const layoutOnly = await fetchActive({ supports: 'release' }); + expect(layoutOnly.body.some((n: { id: string }) => n.id === 'release-notes')).toBe(false); + const olderBundle = await fetchActive({ supports: 'release', ui: '4.0.0' }); + expect(olderBundle.body.some((n: { id: string }) => n.id === 'release-notes')).toBe(false); + + const matching = await fetchActive({ supports: 'release', ui: getCurrentAppVersion() }); + expect(matching.status).toBe(200); + expect(matching.body.some((n: { id: string }) => n.id === 'release-notes')).toBe(true); }); }); diff --git a/server/tests/unit/nest/system-notices.controller.test.ts b/server/tests/unit/nest/system-notices.controller.test.ts index e7e5c64b4..c81995437 100644 --- a/server/tests/unit/nest/system-notices.controller.test.ts +++ b/server/tests/unit/nest/system-notices.controller.test.ts @@ -33,7 +33,7 @@ describe('SystemNoticesController (parity with the legacy /api/system-notices ro it('returns the evaluated notices for the current user', () => { const getActiveFor = vi.fn().mockReturnValue([notice]); expect(makeController({ getActiveFor }).active(user)).toEqual([notice]); - expect(getActiveFor).toHaveBeenCalledWith(7, new Set()); + expect(getActiveFor).toHaveBeenCalledWith(7, new Set(), undefined); }); // The layouts a bundle announces with `?supports=` reach the service as a set, so @@ -41,25 +41,34 @@ describe('SystemNoticesController (parity with the legacy /api/system-notices ro it('passes the announced layouts through as a set', () => { const getActiveFor = vi.fn().mockReturnValue([]); makeController({ getActiveFor }).active(user, 'release'); - expect(getActiveFor).toHaveBeenCalledWith(7, new Set(['release'])); + expect(getActiveFor).toHaveBeenCalledWith(7, new Set(['release']), undefined); + }); + + it('passes the bundle version through, trimmed, and only when it is one string', () => { + const getActiveFor = vi.fn().mockReturnValue([]); + const ctrl = makeController({ getActiveFor }); + ctrl.active(user, 'release', ' 4.3.0 '); + expect(getActiveFor).toHaveBeenLastCalledWith(7, new Set(['release']), '4.3.0'); + ctrl.active(user, 'release', ['4.3.0', '4.3.1']); + expect(getActiveFor).toHaveBeenLastCalledWith(7, new Set(['release']), undefined); }); it('splits a comma separated list and a repeated parameter alike', () => { const getActiveFor = vi.fn().mockReturnValue([]); const ctrl = makeController({ getActiveFor }); ctrl.active(user, ' release, banner ,,'); - expect(getActiveFor).toHaveBeenLastCalledWith(7, new Set(['release', 'banner'])); + expect(getActiveFor).toHaveBeenLastCalledWith(7, new Set(['release', 'banner']), undefined); ctrl.active(user, ['release', 'banner']); - expect(getActiveFor).toHaveBeenLastCalledWith(7, new Set(['release', 'banner'])); + expect(getActiveFor).toHaveBeenLastCalledWith(7, new Set(['release', 'banner']), undefined); }); it('announces nothing for an empty or missing parameter', () => { const getActiveFor = vi.fn().mockReturnValue([]); const ctrl = makeController({ getActiveFor }); ctrl.active(user, ''); - expect(getActiveFor).toHaveBeenLastCalledWith(7, new Set()); + expect(getActiveFor).toHaveBeenLastCalledWith(7, new Set(), undefined); ctrl.active(user, undefined); - expect(getActiveFor).toHaveBeenLastCalledWith(7, new Set()); + expect(getActiveFor).toHaveBeenLastCalledWith(7, new Set(), undefined); }); }); diff --git a/server/tests/unit/nest/system-notices.service.test.ts b/server/tests/unit/nest/system-notices.service.test.ts index de9bfb894..1fbf25779 100644 --- a/server/tests/unit/nest/system-notices.service.test.ts +++ b/server/tests/unit/nest/system-notices.service.test.ts @@ -8,10 +8,11 @@ */ import { describe, it, expect, vi, beforeEach } from 'vitest'; -const { mockGetActive, mockDismiss } = vi.hoisted(() => ({ mockGetActive: vi.fn(), mockDismiss: vi.fn() })); +const { mockGetActive, mockDismiss, mockAppVersion } = vi.hoisted(() => ({ mockGetActive: vi.fn(), mockDismiss: vi.fn(), mockAppVersion: vi.fn(() => '4.3.0') })); vi.mock('../../../src/systemNotices/service', () => ({ getActiveNoticesFor: mockGetActive, dismissNotice: mockDismiss, + getCurrentAppVersion: mockAppVersion, })); import { SystemNoticesService } from '../../../src/nest/system-notices/system-notices.service'; @@ -70,15 +71,37 @@ describe('SystemNoticesService', () => { expect(svc.getActiveFor(7, new Set(['banner'])).map(n => n.id)).toEqual(['outage']); }); - it('delivers it, in place, once the client announces the release layout', () => { + it('delivers it, in place, once the client announces the release layout for the running version', () => { mockGetActive.mockReturnValue([release, generic]); - expect(svc.getActiveFor(7, new Set(['release'])).map(n => n.id)).toEqual(['release-notes', 'outage']); + expect(svc.getActiveFor(7, new Set(['release']), '4.3.0').map(n => n.id)).toEqual(['release-notes', 'outage']); + }); + + it('drops it for a bundle that announces the layout but was built for another version', () => { + // The shell the service worker serves right after an update: it can draw the + // layout, but with the texts of the version it was built for, and its X would + // use the notice up for the version now running. + mockGetActive.mockReturnValue([release, generic]); + expect(svc.getActiveFor(7, new Set(['release']), '4.2.1').map(n => n.id)).toEqual(['outage']); + mockAppVersion.mockReturnValueOnce('4.3.1'); + expect(svc.getActiveFor(7, new Set(['release']), '4.3.0').map(n => n.id)).toEqual(['outage']); + }); + + it('drops it for a bundle that names no version at all', () => { + mockGetActive.mockReturnValue([release, generic]); + expect(svc.getActiveFor(7, new Set(['release'])).map(n => n.id)).toEqual(['outage']); + expect(svc.getActiveFor(7, new Set(['release']), '').map(n => n.id)).toEqual(['outage']); + expect(svc.getActiveFor(7, new Set(['release']), 'dev').map(n => n.id)).toEqual(['outage']); + }); + + it('reads the version loosely, as the server reads its own', () => { + mockGetActive.mockReturnValue([release, generic]); + expect(svc.getActiveFor(7, new Set(['release']), 'v4.3.0').map(n => n.id)).toEqual(['release-notes', 'outage']); }); it('always delivers a notice without a release block', () => { mockGetActive.mockReturnValue([generic]); expect(svc.getActiveFor(7).map(n => n.id)).toEqual(['outage']); - expect(svc.getActiveFor(7, new Set(['release'])).map(n => n.id)).toEqual(['outage']); + expect(svc.getActiveFor(7, new Set(['release']), '4.3.0').map(n => n.id)).toEqual(['outage']); }); });