fix(notices): hand the release notice only to a bundle built for the running version

After an update the service worker keeps serving the previous bundle until
the new one is installed. That shell announces the release layout too, so
the layout alone let it draw the notice with its own older texts and use it
up for the version now running with one click on X.

The bundle now sends the version it was built as (client/package.json, baked
in by define), and the server compares it with its own before it hands a
notice with a release block over. A shell of another version is told nothing
and gets the notice after the reload, from the bundle that can draw it. This
holds for every update, not only the one that introduced the parameter.

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<string> {
   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')

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<string> = new Set()): SystemNoticeDto[] {
+  getActiveFor(userId: number, supports: ReadonlySet<string> = 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 {

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 {

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);
   });
 });
 

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);
     });
   });
 

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']);
     });
   });