Skip to content

Commit df56b25

Browse files
committed
fix(web): never state the placeholder version on the what's-new dialog
Review catch (nettee, #6163): `useAppVersion()` reads /api/version at runtime, so it necessarily boots on `APP_VERSION_PLACEHOLDER` and resolves a round-trip later. This dialog rendered as soon as /api/whats-new resolved and printed the hook unconditionally — so a highlights fetch that won that race painted "Open Design 0.0.0 is here" on first frame. An invented version string, on the one surface this PR exists to make truthful. The new suite could not see it because it mocked the hook to an already-resolved value. Fixed by falling back rather than gating the whole render: gating would delay the dialog behind a second round-trip on slow networks, while the highlights document already carries the running version in its `version` field — the daemon stamps it for display (see contracts/api/whats-new.ts), which is what fed the old bottom-right card's eyebrow. So `statedAppVersion()` prefers the resolved hook, falls back to the document, and returns null when neither can name one; on null the dialog waits, because highlights are worth nothing under a headline that lies. The same derived value feeds the title AND every `app_version` prop, so analytics cannot ship the placeholder either. `APP_VERSION_PLACEHOLDER` moves out of ./provider into a new leaf analytics/app-version.ts alongside an `isResolvedAppVersion()` predicate, so a surface that must not PRINT a placeholder can test for it without importing the analytics client. The provider keeps its own use of the constant. Regression tests, red before this commit (`.red-evidence-version-race.txt` reproduces nettee's exact string, `expected 'Open Design 0.0.0 is here…' not to contain '0.0.0'`): the placeholder never paints while /api/version is in flight, it never reaches the surface-view analytics, the title switches to the running version once the hook resolves, and a document with no usable version waits instead of inventing one. The suite's provider mock is now a mutable holder so both sides of the race are reachable. Also caught by this PR's own source guard: quoting the literal in a docblock tripped it, so the comment names the constant instead.
1 parent 82e9462 commit df56b25

4 files changed

Lines changed: 147 additions & 8 deletions

File tree

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
// The app version's placeholder identity, split out of ./provider so surfaces
2+
// that must not RENDER a placeholder can test for it without importing the
3+
// analytics client.
4+
//
5+
// `useAppVersion()` reads the daemon-pinned version from /api/version at
6+
// runtime, so it necessarily starts on a placeholder and resolves one round-trip
7+
// later. Analytics tolerates that by awaiting the shared fetch before capture
8+
// (see `resolveAppVersionForCapture`); UI cannot await, so any surface that
9+
// prints the version must ask whether it has resolved yet.
10+
11+
export const APP_VERSION_PLACEHOLDER = '0.0.0';
12+
13+
/**
14+
* Whether an app version is safe to show a user as fact.
15+
*
16+
* False for the pre-resolution placeholder and for anything blank, so a caller
17+
* can pick another real source (or wait) instead of stating a version nobody
18+
* reported.
19+
*/
20+
export function isResolvedAppVersion(version: string | null | undefined): boolean {
21+
if (version == null) return false;
22+
const trimmed = version.trim();
23+
return trimmed.length > 0 && trimmed !== APP_VERSION_PLACEHOLDER;
24+
}

apps/web/src/analytics/provider.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ import {
2727
setAnalyticsUserId,
2828
setConfigureGlobals,
2929
} from './client';
30+
import { APP_VERSION_PLACEHOLDER } from './app-version';
3031
import { patchExceptionTrackingAppVersion } from './error-tracking';
3132
import type { AnalyticsConfigureGlobals } from '@open-design/contracts/analytics';
3233
import {
@@ -92,7 +93,6 @@ function isSameOriginApiCall(url: unknown): boolean {
9293
}
9394
}
9495

95-
const APP_VERSION_PLACEHOLDER = '0.0.0';
9696
let runtimeAppVersion: string | null = null;
9797
let runtimeAppVersionPromise: Promise<string | null> | null = null;
9898

apps/web/src/components/WhatsNewPopup.tsx

Lines changed: 34 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import {
1212
whatsNewNotesFromBody,
1313
} from '../lib/whats-new';
1414
import { useAnalytics, useAppVersion } from '../analytics/provider';
15+
import { isResolvedAppVersion } from '../analytics/app-version';
1516
import { trackWhatsNewPopupClick, trackWhatsNewPopupSurfaceView } from '../analytics/events';
1617
import styles from './WhatsNewPopup.module.css';
1718

@@ -31,9 +32,33 @@ import styles from './WhatsNewPopup.module.css';
3132
// Fallback for the CTA when the highlight document omits an explicit link.
3233
const RELEASES_INDEX_URL = 'https://github.com/nexu-io/open-design/releases';
3334

35+
/**
36+
* The version this dialog is allowed to state, or null when nothing real can
37+
* name one yet.
38+
*
39+
* `useAppVersion()` resolves one /api/version round-trip after mount, and
40+
* /api/whats-new can win that race — so reading the hook unconditionally would
41+
* paint `APP_VERSION_PLACEHOLDER` on first frame, i.e. exactly the invented
42+
* version this surface exists to stop telling. Until the hook resolves we state
43+
* the highlights document's own `version`, which the daemon stamps with the
44+
* running version for display (see contracts/api/whats-new.ts): a second real
45+
* source, not a second guess. If neither has landed the dialog waits — the
46+
* highlights are worth nothing if the headline above them is a lie.
47+
*/
48+
function statedAppVersion(hookVersion: string, documentVersion: string): string | null {
49+
if (isResolvedAppVersion(hookVersion)) return hookVersion.trim();
50+
if (isResolvedAppVersion(documentVersion)) return documentVersion.trim();
51+
return null;
52+
}
53+
3454
type CardModel = {
3555
/** Highlight identity — recorded as "seen" so the dialog shows once per id. */
3656
id: string;
57+
/**
58+
* The running version as stamped on the highlights document. Stands in for
59+
* `useAppVersion()` until that resolves (see `statedAppVersion`).
60+
*/
61+
documentVersion: string;
3762
/** The release headline from the highlights document; labels the bullets. */
3863
headline: string;
3964
/** Release highlights, one row per line of the document's body. */
@@ -48,7 +73,7 @@ type CardModel = {
4873
export function WhatsNewPopup({ active }: { active: boolean }) {
4974
const { t, locale } = useI18n();
5075
const analytics = useAnalytics();
51-
const appVersion = useAppVersion();
76+
const hookAppVersion = useAppVersion();
5277
const [card, setCard] = useState<CardModel | null>(null);
5378
const surfaceTrackedRef = useRef(false);
5479
// Flips only once a decision is actually REACHED, never merely when a fetch
@@ -57,6 +82,7 @@ export function WhatsNewPopup({ active }: { active: boolean }) {
5782
// guard down so the next Home activation retries, instead of the teardown
5883
// re-arming a start-time guard and permanently swallowing the dialog.
5984
const decisionMadeRef = useRef(false);
85+
const appVersion = card == null ? null : statedAppVersion(hookAppVersion, card.documentVersion);
6086

6187
useEffect(() => {
6288
if (!active || decisionMadeRef.current) return;
@@ -72,6 +98,7 @@ export function WhatsNewPopup({ active }: { active: boolean }) {
7298
const localized = localizedWhatsNewContent(info.content, locale);
7399
setCard({
74100
id: info.id,
101+
documentVersion: info.version,
75102
headline: localized.title,
76103
notes: whatsNewNotesFromBody(localized.body),
77104
imageUrl: info.content.imageUrl ?? null,
@@ -87,7 +114,7 @@ export function WhatsNewPopup({ active }: { active: boolean }) {
87114
}, [active]);
88115

89116
useEffect(() => {
90-
if (!active || card == null || surfaceTrackedRef.current) return;
117+
if (!active || card == null || appVersion == null || surfaceTrackedRef.current) return;
91118
surfaceTrackedRef.current = true;
92119
trackWhatsNewPopupSurfaceView(analytics.track, {
93120
page_name: 'home',
@@ -102,7 +129,7 @@ export function WhatsNewPopup({ active }: { active: boolean }) {
102129
// focus-trapping modal cannot receive a stray Escape aimed at other UI, so
103130
// dismissal is always deliberate here.
104131
const dismiss = useCallback(() => {
105-
if (card == null) return;
132+
if (card == null || appVersion == null) return;
106133
markWhatsNewSeen(card.id);
107134
trackWhatsNewPopupClick(analytics.track, {
108135
page_name: 'home',
@@ -115,7 +142,7 @@ export function WhatsNewPopup({ active }: { active: boolean }) {
115142
}, [analytics.track, appVersion, card]);
116143

117144
const openLink = useCallback(() => {
118-
if (card == null) return;
145+
if (card == null || appVersion == null) return;
119146
markWhatsNewSeen(card.id);
120147
trackWhatsNewPopupClick(analytics.track, {
121148
page_name: 'home',
@@ -128,7 +155,9 @@ export function WhatsNewPopup({ active }: { active: boolean }) {
128155
setCard(null);
129156
}, [analytics.track, appVersion, card]);
130157

131-
if (!active || card == null) return null;
158+
// `appVersion == null` means neither the hook nor the document can name a
159+
// version yet; the dialog waits rather than titling itself with a guess.
160+
if (!active || card == null || appVersion == null) return null;
132161

133162
const dialog = (
134163
<Dialog

apps/web/tests/components/WhatsNewPopup.test.tsx

Lines changed: 88 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,12 +34,18 @@ vi.mock('../../src/providers/registry', () => ({
3434
openExternalUrl: vi.fn(),
3535
}));
3636

37+
// `useAppVersion()` is asynchronous in production: it boots on a placeholder and
38+
// only becomes the running version once /api/version resolves. Tests drive that
39+
// through this mutable holder so both sides of the race are reachable.
40+
const appVersion = vi.hoisted(() => ({ current: '0.16.1' }));
41+
const track = vi.hoisted(() => vi.fn());
42+
3743
vi.mock('../../src/analytics/provider', () => ({
38-
useAnalytics: () => ({ track: vi.fn() }),
44+
useAnalytics: () => ({ track }),
3945
// The running version the daemon reports. Deliberately different from the
4046
// highlight payload's `version` field so the suite can tell which one the
4147
// dialog renders.
42-
useAppVersion: () => '0.16.1',
48+
useAppVersion: () => appVersion.current,
4349
}));
4450

4551
const RUNNING_APP_VERSION = '0.16.1';
@@ -79,6 +85,7 @@ afterEach(() => {
7985
beforeEach(() => {
8086
// A highlight id the user has not seen yet → decision resolves to "show".
8187
window.localStorage.setItem(WHATS_NEW_LAST_SEEN_STORAGE_KEY, 'highlight-0-15-0');
88+
appVersion.current = RUNNING_APP_VERSION;
8289
});
8390

8491
describe('WhatsNewPopup fetch/show lifecycle', () => {
@@ -168,6 +175,85 @@ describe('WhatsNewPopup fetch/show lifecycle', () => {
168175
});
169176
});
170177

178+
// /api/version and /api/whats-new are independent round-trips, and
179+
// `useAppVersion()` deliberately boots on the '0.0.0' placeholder until the
180+
// former lands (see analytics/app-version.ts). A highlights fetch that wins
181+
// that race must never make this dialog state the placeholder — that is the
182+
// exact invented version the surface exists to stop telling.
183+
describe('WhatsNewPopup version resolution', () => {
184+
const PLACEHOLDER_VERSION = '0.0.0';
185+
186+
beforeEach(() => {
187+
appVersion.current = PLACEHOLDER_VERSION;
188+
mockedFetchWhatsNew.mockResolvedValue(SHOW_PAYLOAD);
189+
});
190+
191+
it('never paints the placeholder while /api/version is still in flight', async () => {
192+
renderCard(true);
193+
194+
await waitFor(() => {
195+
expect(screen.getByTestId('whats-new-popup')).toBeTruthy();
196+
});
197+
expect(screen.getByTestId('whats-new-popup').textContent).not.toContain(PLACEHOLDER_VERSION);
198+
// The daemon stamps the highlights document with the running version for
199+
// exactly this display purpose, so it is a real source to name meanwhile.
200+
expect(screen.getByText(`Open Design ${SHOW_PAYLOAD.version} is here`)).toBeTruthy();
201+
});
202+
203+
it('keeps the placeholder out of the surface-view analytics too', async () => {
204+
renderCard(true);
205+
206+
await waitFor(() => {
207+
expect(track).toHaveBeenCalled();
208+
});
209+
const versions = track.mock.calls
210+
.map(([, props]) => (props as { app_version?: string } | undefined)?.app_version)
211+
.filter((value): value is string => typeof value === 'string');
212+
expect(versions.length).toBeGreaterThan(0);
213+
expect(versions).not.toContain(PLACEHOLDER_VERSION);
214+
});
215+
216+
it('switches to the running version once /api/version resolves', async () => {
217+
const view = renderCard(true);
218+
219+
await waitFor(() => {
220+
expect(screen.getByText(`Open Design ${SHOW_PAYLOAD.version} is here`)).toBeTruthy();
221+
});
222+
223+
appVersion.current = RUNNING_APP_VERSION;
224+
view.rerender(
225+
<I18nProvider>
226+
<WhatsNewPopup active />
227+
</I18nProvider>,
228+
);
229+
230+
await waitFor(() => {
231+
expect(screen.getByText(`Open Design ${RUNNING_APP_VERSION} is here`)).toBeTruthy();
232+
});
233+
expect(screen.queryByText(`Open Design ${SHOW_PAYLOAD.version} is here`)).toBeNull();
234+
});
235+
236+
it('waits instead of inventing one when neither source can name a version', async () => {
237+
mockedFetchWhatsNew.mockResolvedValue({ ...SHOW_PAYLOAD, version: ' ' });
238+
239+
const view = renderCard(true);
240+
await act(async () => {});
241+
expect(screen.queryByTestId('whats-new-popup')).toBeNull();
242+
243+
// …and appears as soon as the running version lands, rather than being
244+
// permanently swallowed.
245+
appVersion.current = RUNNING_APP_VERSION;
246+
view.rerender(
247+
<I18nProvider>
248+
<WhatsNewPopup active />
249+
</I18nProvider>,
250+
);
251+
await waitFor(() => {
252+
expect(screen.getByText(`Open Design ${RUNNING_APP_VERSION} is here`)).toBeTruthy();
253+
});
254+
});
255+
});
256+
171257
describe('WhatsNewPopup content', () => {
172258
beforeEach(() => {
173259
mockedFetchWhatsNew.mockResolvedValue(SHOW_PAYLOAD);

0 commit comments

Comments
 (0)