diff --git a/projects/pathway-browser/src/app/analysis-summary/summary-stream.spec.ts b/projects/pathway-browser/src/app/analysis-summary/summary-stream.spec.ts index af59ae53..9d38ec9e 100644 --- a/projects/pathway-browser/src/app/analysis-summary/summary-stream.spec.ts +++ b/projects/pathway-browser/src/app/analysis-summary/summary-stream.spec.ts @@ -43,7 +43,16 @@ describe('the start event', () => { expect(event).toMatchObject({ start: { analysisType: null } }); }); - it('takes the release as the string the wire sends', () => { + it('takes the release as an int, which is what the field is becoming', () => { + // The service is changing this from a string to an int to match + // `/api/answer`, which always sent one. Both spellings are accepted so that + // neither side of that change breaks the panel. + expect(parseFrame(frame('start', { release: 97 }))).toMatchObject({ + start: { release: 97 }, + }); + }); + + it('still takes the string form the field had before that', () => { // `"97"`, not `97`. Requiring a number here silently nulled every release, // and the field is what tells one release's cached summary from another's. expect(parseFrame(frame('start', { release: '97' }))).toMatchObject({ @@ -57,6 +66,16 @@ describe('the start event', () => { }); }); + it('never turns an absent release into release 0', () => { + // `Number(null)`, `Number('')` and `Number(false)` are all 0, and 0 is a + // number that caches and compares perfectly happily while naming no release + // that has ever existed. + for (const release of [null, '', ' ', false, []]) { + expect(parseFrame(frame('start', { release }))).toMatchObject({ start: { release: null } }); + } + expect(parseFrame(frame('start', {}))).toMatchObject({ start: { release: null } }); + }); + it('treats a missing cached flag as not cached', () => { expect(parseFrame(frame('start', {}))).toMatchObject({ start: { cached: false } }); }); diff --git a/projects/pathway-browser/src/app/analysis-summary/summary-stream.ts b/projects/pathway-browser/src/app/analysis-summary/summary-stream.ts index ce463a7a..354f0b92 100644 --- a/projects/pathway-browser/src/app/analysis-summary/summary-stream.ts +++ b/projects/pathway-browser/src/app/analysis-summary/summary-stream.ts @@ -125,6 +125,19 @@ function asReason(value: unknown): RefusalReason | null { } /** Turns one complete SSE frame into an event, or null if it is not one we act on. */ +/** + * The release number, from either spelling, or null. + * + * Only a number or a non-empty string can be one, and it must be positive: the + * field's whole job is telling one release's cached summary from another's, and + * a 0 conjured out of `null` would do that job wrongly rather than not at all. + */ +function releaseOf(value: unknown): number | null { + if (typeof value !== 'number' && (typeof value !== 'string' || value.trim() === '')) return null; + const release = Number(value); + return Number.isFinite(release) && release > 0 ? release : null; +} + export function parseFrame(frame: string): SummaryEvent | null { let name = ''; const dataLines: string[] = []; @@ -151,10 +164,16 @@ export function parseFrame(frame: string): SummaryEvent | null { return { kind: 'start', start: { - // A string on the wire ("97"), not a number. Coerced rather than - // required, because what this is for is telling one release's cache - // from another's, not arithmetic. - release: Number.isFinite(Number(data['release'])) ? Number(data['release']) : null, + // Accepts both spellings on purpose. It arrived as the string "97" + // while `/api/answer` sent an int for the same field; the service is + // making it an int here too, and a parser that accepted only one of + // them would have broken on one side of that change or the other. + // + // Not a bare `Number()`: `Number(null)`, `Number('')` and + // `Number(false)` are all 0, so an absent or empty release would + // become release 0 -- a number that compares, sorts and caches + // perfectly well while naming no release that exists. + release: releaseOf(data['release']), analysisType: typeof data['analysis_type'] === 'string' && TYPES.includes(data['analysis_type']) ? (data['analysis_type'] as AnalysisType) diff --git a/projects/pathway-browser/src/app/analysis-summary/summary.service.ts b/projects/pathway-browser/src/app/analysis-summary/summary.service.ts index 367497ff..8f3db883 100644 --- a/projects/pathway-browser/src/app/analysis-summary/summary.service.ts +++ b/projects/pathway-browser/src/app/analysis-summary/summary.service.ts @@ -106,7 +106,6 @@ export class SummaryService { */ readonly expired = computed(() => this._state() === 'gone'); - private release: number | null = null; private readonly cache = new Map(); private inFlight: AbortController | null = null; /** What to ask again for once a challenge is solved. */ @@ -302,7 +301,12 @@ export class SummaryService { switch (event.kind) { case 'start': this._started.set(true); - this.release = event.start.release; + // `start.release` is deliberately not kept. It was held in a field + // nothing ever read, which the dead-code gate cannot see because a + // private field is not an export. The cache key does not use it -- + // this cache lives in one page's memory, and a release during a + // session would replace the process holding it -- so storing it + // only invited a later reader to trust a value nobody maintains. this._analysisType.set(event.start.analysisType); this._applied.set(event.start.disclosure); break;