From 3bdc536d288d87e4349ddf22a7c388319c4663b0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= Date: Sat, 26 Sep 2026 13:06:17 +0000 Subject: [PATCH 1/2] fix(mobile): hide placeholder session titles and disambiguate duplicate artifact names --- .../src/lib/artifacts/artifact-crawl.test.ts | 36 +++++++++++ .../src/lib/artifacts/artifact-crawl.ts | 18 +++++- .../artifact-mirror-manifest.test.ts | 63 +++++++++++++++++++ .../lib/artifacts/artifact-mirror-manifest.ts | 58 +++++++++++++++++ .../artifacts-file-provider-contract.test.ts | 9 ++- 5 files changed, 178 insertions(+), 6 deletions(-) diff --git a/apps/mobile/src/lib/artifacts/artifact-crawl.test.ts b/apps/mobile/src/lib/artifacts/artifact-crawl.test.ts index 524a539111..a8d6a84e1b 100644 --- a/apps/mobile/src/lib/artifacts/artifact-crawl.test.ts +++ b/apps/mobile/src/lib/artifacts/artifact-crawl.test.ts @@ -487,4 +487,40 @@ describe('buildSessionArtifacts', () => { { id: 's2', title: 'Named', updatedAt: UPDATED_AT, files: [] }, ]); }); + + it('hides the backend placeholder title behind the fallback label', () => { + const sessions = buildSessionArtifacts( + [ + rowOf('s1', 'New session - 2026-09-22T01:09:45.623Z'), + rowOf('s2', 'Child session - 2026-09-22T01:09:45.623Z'), + ], + new Map() + ); + + expect(sessions.map(session => session.title)).toEqual(['Session s1', 'Session s2']); + }); + + it('disambiguates artifacts that share a filename within a session', () => { + const artifacts = new Map([ + [ + 's1', + [ + { id: 'file-1', mime: 'application/pdf', filename: 'report.pdf', size: 1 }, + { id: 'file-2', mime: 'application/pdf', filename: 'report.pdf', size: 2 }, + ], + ], + ]); + + expect(buildSessionArtifacts([rowOf('s1', 'Named')], artifacts)).toEqual([ + { + id: 's1', + title: 'Named', + updatedAt: UPDATED_AT, + files: [ + { id: 'file-1', name: 'report.pdf', mime: 'application/pdf', size: 1 }, + { id: 'file-2', name: 'report (2).pdf', mime: 'application/pdf', size: 2 }, + ], + }, + ]); + }); }); diff --git a/apps/mobile/src/lib/artifacts/artifact-crawl.ts b/apps/mobile/src/lib/artifacts/artifact-crawl.ts index 8f946f1aaf..6886d7ee4c 100644 --- a/apps/mobile/src/lib/artifacts/artifact-crawl.ts +++ b/apps/mobile/src/lib/artifacts/artifact-crawl.ts @@ -9,7 +9,9 @@ import { type ArtifactMirrorSession, safeArtifactDisplayName, safeArtifactSessionName, + uniqueArtifactDisplayNames, } from '@/lib/artifacts/artifact-mirror-manifest'; +import { sessionDisplayTitle } from '@/lib/session-display-title'; import { trpcClient } from '@/lib/trpc'; /** @@ -334,7 +336,12 @@ export async function fetchSessionMessagesPage( * Assemble the mirror's session entries. A session keeps its folder even with * no files. Its label is sanitized ({@link safeArtifactSessionName}) rather than * taken verbatim: the title is free text, and both file browsers need one - * non-empty path component, with `Session ` as the fallback. + * non-empty path component, with `Session ` as the fallback. The title is + * first gated through {@link sessionDisplayTitle}, so a row still carrying the + * backend's `New session - ` placeholder gets the fallback label instead + * of the machine string the app itself would never paint. Duplicate filenames + * within a session are disambiguated by {@link uniqueArtifactDisplayNames}, so + * the browser shows one row per artifact. */ export function buildSessionArtifacts( sessions: MirrorSessionRow[], @@ -342,9 +349,14 @@ export function buildSessionArtifacts( ): ArtifactMirrorSession[] { return sessions.map(session => ({ id: session.id, - title: safeArtifactSessionName({ id: session.id, title: session.title }), + title: safeArtifactSessionName({ + id: session.id, + title: sessionDisplayTitle(session.title) ?? null, + }), updatedAt: session.updatedAt, - files: (artifactsBySession.get(session.id) ?? []).map(artifact => toMirrorFile(artifact)), + files: uniqueArtifactDisplayNames( + (artifactsBySession.get(session.id) ?? []).map(artifact => toMirrorFile(artifact)) + ), })); } diff --git a/apps/mobile/src/lib/artifacts/artifact-mirror-manifest.test.ts b/apps/mobile/src/lib/artifacts/artifact-mirror-manifest.test.ts index 43b1ed82e8..4f0396e5dc 100644 --- a/apps/mobile/src/lib/artifacts/artifact-mirror-manifest.test.ts +++ b/apps/mobile/src/lib/artifacts/artifact-mirror-manifest.test.ts @@ -8,6 +8,7 @@ import { safeArtifactDisplayName, selectSessionsWithinBudget, serializeArtifactMirrorManifest, + uniqueArtifactDisplayNames, } from '@/lib/artifacts/artifact-mirror-manifest'; import { utf8ByteLength } from '@/lib/utf8-utils'; @@ -211,6 +212,68 @@ describe('safeArtifactDisplayName', () => { }); }); +describe('uniqueArtifactDisplayNames', () => { + const fileOf = (id: string, name: string, mime = 'application/pdf') => ({ + id, + name, + mime, + size: 1, + }); + + it('leaves distinct names untouched', () => { + const files = [fileOf('f1', 'a.pdf'), fileOf('f2', 'b.pdf')]; + + expect(uniqueArtifactDisplayNames(files).map(file => file.name)).toEqual(['a.pdf', 'b.pdf']); + }); + + it('suffixes duplicates before the extension, keeping the first name', () => { + const files = [ + fileOf('f1', 'report.pdf'), + fileOf('f2', 'report.pdf'), + fileOf('f3', 'report.pdf'), + ]; + + expect(uniqueArtifactDisplayNames(files).map(file => file.name)).toEqual([ + 'report.pdf', + 'report (2).pdf', + 'report (3).pdf', + ]); + }); + + it('suffixes a name with no extension', () => { + const files = [fileOf('f1', 'notes'), fileOf('f2', 'notes')]; + + expect(uniqueArtifactDisplayNames(files).map(file => file.name)).toEqual([ + 'notes', + 'notes (2)', + ]); + }); + + it('keeps every suffixed name inside the byte bound and its extension', () => { + const longBase = safeArtifactDisplayName({ + id: 'f1', + name: `${'ä'.repeat(300)}.pdf`, + mime: 'application/pdf', + }); + const files = [fileOf('f1', longBase), fileOf('f2', longBase)]; + + const names = uniqueArtifactDisplayNames(files).map(file => file.name); + expect(names[0]).not.toBe(names[1]); + for (const value of names) { + expect(utf8ByteLength(value)).toBeLessThanOrEqual(200); + expect(value.endsWith('.pdf')).toBe(true); + } + }); + + it('does not mutate the input files', () => { + const files = [fileOf('f1', 'report.pdf'), fileOf('f2', 'report.pdf')]; + + uniqueArtifactDisplayNames(files); + + expect(files.map(file => file.name)).toEqual(['report.pdf', 'report.pdf']); + }); +}); + describe('selectSessionsWithinBudget', () => { it('keeps every session when the files fit', () => { const sessions = [ diff --git a/apps/mobile/src/lib/artifacts/artifact-mirror-manifest.ts b/apps/mobile/src/lib/artifacts/artifact-mirror-manifest.ts index 7377c9487a..2e7732a411 100644 --- a/apps/mobile/src/lib/artifacts/artifact-mirror-manifest.ts +++ b/apps/mobile/src/lib/artifacts/artifact-mirror-manifest.ts @@ -156,6 +156,64 @@ export function safeArtifactSessionName({ return boundArtifactDisplayName(safeId.length > 0 ? `Session ${safeId}` : SESSION_NAME_FALLBACK); } +/** + * Make every display name unique within one session. + * + * Two artifacts can share an agent-supplied filename, and the file browser + * paints `name` verbatim, so duplicates appear as indistinguishable rows in + * the session folder. The first occurrence keeps its name; a later duplicate + * gets a short ` (n)` suffix before the extension, with the stem truncated so + * the suffix and the extension both stay inside the byte bound. The input is + * never mutated. + */ +export function uniqueArtifactDisplayNames(files: ArtifactMirrorFile[]): ArtifactMirrorFile[] { + const used = new Set(); + return files.map(file => { + const name = uniqueArtifactDisplayName(file.name, file.id, file.mime, used); + used.add(name); + return name === file.name ? file : { ...file, name }; + }); +} + +/** + * A display name that is not in `used` yet. A collision takes a counter suffix + * before its extension; the suffix is guaranteed to survive truncation because + * the stem is bounded against the same byte budget, and the extension is capped + * so the counter never runs out of room. One candidate per taken name is tried, + * so a free one always exists. + */ +function uniqueArtifactDisplayName( + baseName: string, + id: string, + mime: string, + used: ReadonlySet +): string { + if (!used.has(baseName)) { + return baseName; + } + const { stem, extension } = splitArtifactExtension(baseName); + const boundedExtension = truncateUtf8(extension, MAX_ARTIFACT_DISPLAY_NAME_BYTES / 2); + for (let index = 2; index <= used.size + 2; index += 1) { + const suffix = ` (${index})`; + const stemBudget = + MAX_ARTIFACT_DISPLAY_NAME_BYTES - utf8ByteLength(boundedExtension) - utf8ByteLength(suffix); + const candidate = `${truncateUtf8(stem, stemBudget)}${suffix}${boundedExtension}`; + if (!used.has(candidate)) { + return candidate; + } + } + return safeArtifactDisplayName({ id, name: '', mime }); +} + +/** Split a sanitized display name into its stem and extension (with the dot). */ +function splitArtifactExtension(name: string): { extension: string; stem: string } { + const extensionStart = name.lastIndexOf('.'); + if (extensionStart > 0 && extensionStart < name.length - 1) { + return { extension: name.slice(extensionStart), stem: name.slice(0, extensionStart) }; + } + return { extension: '', stem: name }; +} + /** * Drop whole sessions, oldest `updatedAt` first, until the summed file sizes * fit `maxBytes`. The session entries stay — only their files leave — so an diff --git a/apps/mobile/src/lib/artifacts/artifacts-file-provider-contract.test.ts b/apps/mobile/src/lib/artifacts/artifacts-file-provider-contract.test.ts index de0bd53e6a..9c820b9ea3 100644 --- a/apps/mobile/src/lib/artifacts/artifacts-file-provider-contract.test.ts +++ b/apps/mobile/src/lib/artifacts/artifacts-file-provider-contract.test.ts @@ -123,9 +123,12 @@ describe('artifacts File Provider extension contract', () => { // title (free text: nullable, unbounded, may carry a path separator) before // it lands in the manifest, which is also the label Android shows. expect(manifestWriterSource).toContain('export function safeArtifactSessionName'); - expect(crawlSource).toContain( - 'safeArtifactSessionName({ id: session.id, title: session.title })' - ); + // The crawl gates the raw title through `sessionDisplayTitle` first, so a + // row still carrying the backend placeholder reaches the sanitizer as a + // missing title and falls back to `Session ` rather than the machine + // string the app never paints. + expect(crawlSource).toContain('title: safeArtifactSessionName({'); + expect(crawlSource).toContain('title: sessionDisplayTitle(session.title) ?? null,'); }); it('returns the mirrored file through the app group container', () => { From 7da128024d17a2b1e04c5cb3f644f8a7d9557dca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= Date: Sat, 26 Sep 2026 15:09:01 +0000 Subject: [PATCH 2/2] fix(mobile): satisfy artifact mirror lint rules - extract the unique-name tests to their own file to stay under max-lines - pass the artifact to uniqueArtifactDisplayName to fit max-params - name the splitArtifactExtension return type so the anti-slop rule accepts it --- .../artifact-mirror-manifest-names.test.ts | 77 +++++++++++++++++++ .../artifact-mirror-manifest.test.ts | 63 --------------- .../lib/artifacts/artifact-mirror-manifest.ts | 25 +++--- 3 files changed, 90 insertions(+), 75 deletions(-) create mode 100644 apps/mobile/src/lib/artifacts/artifact-mirror-manifest-names.test.ts diff --git a/apps/mobile/src/lib/artifacts/artifact-mirror-manifest-names.test.ts b/apps/mobile/src/lib/artifacts/artifact-mirror-manifest-names.test.ts new file mode 100644 index 0000000000..fba9adf7f1 --- /dev/null +++ b/apps/mobile/src/lib/artifacts/artifact-mirror-manifest-names.test.ts @@ -0,0 +1,77 @@ +import { describe, expect, it, vi } from 'vitest'; + +import { + safeArtifactDisplayName, + uniqueArtifactDisplayNames, +} from '@/lib/artifacts/artifact-mirror-manifest'; +import { utf8ByteLength } from '@/lib/utf8-utils'; + +vi.mock('expo-file-system', () => ({ + Directory: vi.fn(), + File: vi.fn(), + Paths: {}, +})); + +vi.mock('expo-sharing', () => ({ + isAvailableAsync: vi.fn(), + shareAsync: vi.fn(), +})); + +function fileOf(id: string, name: string, mime = 'application/pdf') { + return { id, name, mime, size: 1 }; +} + +describe('uniqueArtifactDisplayNames', () => { + it('leaves distinct names untouched', () => { + const files = [fileOf('f1', 'a.pdf'), fileOf('f2', 'b.pdf')]; + + expect(uniqueArtifactDisplayNames(files).map(file => file.name)).toEqual(['a.pdf', 'b.pdf']); + }); + + it('suffixes duplicates before the extension, keeping the first name', () => { + const files = [ + fileOf('f1', 'report.pdf'), + fileOf('f2', 'report.pdf'), + fileOf('f3', 'report.pdf'), + ]; + + expect(uniqueArtifactDisplayNames(files).map(file => file.name)).toEqual([ + 'report.pdf', + 'report (2).pdf', + 'report (3).pdf', + ]); + }); + + it('suffixes a name with no extension', () => { + const files = [fileOf('f1', 'notes'), fileOf('f2', 'notes')]; + + expect(uniqueArtifactDisplayNames(files).map(file => file.name)).toEqual([ + 'notes', + 'notes (2)', + ]); + }); + + it('keeps every suffixed name inside the byte bound and its extension', () => { + const longBase = safeArtifactDisplayName({ + id: 'f1', + name: `${'ä'.repeat(300)}.pdf`, + mime: 'application/pdf', + }); + const files = [fileOf('f1', longBase), fileOf('f2', longBase)]; + + const names = uniqueArtifactDisplayNames(files).map(file => file.name); + expect(names[0]).not.toBe(names[1]); + for (const value of names) { + expect(utf8ByteLength(value)).toBeLessThanOrEqual(200); + expect(value.endsWith('.pdf')).toBe(true); + } + }); + + it('does not mutate the input files', () => { + const files = [fileOf('f1', 'report.pdf'), fileOf('f2', 'report.pdf')]; + + uniqueArtifactDisplayNames(files); + + expect(files.map(file => file.name)).toEqual(['report.pdf', 'report.pdf']); + }); +}); diff --git a/apps/mobile/src/lib/artifacts/artifact-mirror-manifest.test.ts b/apps/mobile/src/lib/artifacts/artifact-mirror-manifest.test.ts index 4f0396e5dc..43b1ed82e8 100644 --- a/apps/mobile/src/lib/artifacts/artifact-mirror-manifest.test.ts +++ b/apps/mobile/src/lib/artifacts/artifact-mirror-manifest.test.ts @@ -8,7 +8,6 @@ import { safeArtifactDisplayName, selectSessionsWithinBudget, serializeArtifactMirrorManifest, - uniqueArtifactDisplayNames, } from '@/lib/artifacts/artifact-mirror-manifest'; import { utf8ByteLength } from '@/lib/utf8-utils'; @@ -212,68 +211,6 @@ describe('safeArtifactDisplayName', () => { }); }); -describe('uniqueArtifactDisplayNames', () => { - const fileOf = (id: string, name: string, mime = 'application/pdf') => ({ - id, - name, - mime, - size: 1, - }); - - it('leaves distinct names untouched', () => { - const files = [fileOf('f1', 'a.pdf'), fileOf('f2', 'b.pdf')]; - - expect(uniqueArtifactDisplayNames(files).map(file => file.name)).toEqual(['a.pdf', 'b.pdf']); - }); - - it('suffixes duplicates before the extension, keeping the first name', () => { - const files = [ - fileOf('f1', 'report.pdf'), - fileOf('f2', 'report.pdf'), - fileOf('f3', 'report.pdf'), - ]; - - expect(uniqueArtifactDisplayNames(files).map(file => file.name)).toEqual([ - 'report.pdf', - 'report (2).pdf', - 'report (3).pdf', - ]); - }); - - it('suffixes a name with no extension', () => { - const files = [fileOf('f1', 'notes'), fileOf('f2', 'notes')]; - - expect(uniqueArtifactDisplayNames(files).map(file => file.name)).toEqual([ - 'notes', - 'notes (2)', - ]); - }); - - it('keeps every suffixed name inside the byte bound and its extension', () => { - const longBase = safeArtifactDisplayName({ - id: 'f1', - name: `${'ä'.repeat(300)}.pdf`, - mime: 'application/pdf', - }); - const files = [fileOf('f1', longBase), fileOf('f2', longBase)]; - - const names = uniqueArtifactDisplayNames(files).map(file => file.name); - expect(names[0]).not.toBe(names[1]); - for (const value of names) { - expect(utf8ByteLength(value)).toBeLessThanOrEqual(200); - expect(value.endsWith('.pdf')).toBe(true); - } - }); - - it('does not mutate the input files', () => { - const files = [fileOf('f1', 'report.pdf'), fileOf('f2', 'report.pdf')]; - - uniqueArtifactDisplayNames(files); - - expect(files.map(file => file.name)).toEqual(['report.pdf', 'report.pdf']); - }); -}); - describe('selectSessionsWithinBudget', () => { it('keeps every session when the files fit', () => { const sessions = [ diff --git a/apps/mobile/src/lib/artifacts/artifact-mirror-manifest.ts b/apps/mobile/src/lib/artifacts/artifact-mirror-manifest.ts index 2e7732a411..8e703e1bda 100644 --- a/apps/mobile/src/lib/artifacts/artifact-mirror-manifest.ts +++ b/apps/mobile/src/lib/artifacts/artifact-mirror-manifest.ts @@ -169,7 +169,7 @@ export function safeArtifactSessionName({ export function uniqueArtifactDisplayNames(files: ArtifactMirrorFile[]): ArtifactMirrorFile[] { const used = new Set(); return files.map(file => { - const name = uniqueArtifactDisplayName(file.name, file.id, file.mime, used); + const name = uniqueArtifactDisplayName(file, used); used.add(name); return name === file.name ? file : { ...file, name }; }); @@ -182,16 +182,11 @@ export function uniqueArtifactDisplayNames(files: ArtifactMirrorFile[]): Artifac * so the counter never runs out of room. One candidate per taken name is tried, * so a free one always exists. */ -function uniqueArtifactDisplayName( - baseName: string, - id: string, - mime: string, - used: ReadonlySet -): string { - if (!used.has(baseName)) { - return baseName; +function uniqueArtifactDisplayName(file: ArtifactMirrorFile, used: ReadonlySet): string { + if (!used.has(file.name)) { + return file.name; } - const { stem, extension } = splitArtifactExtension(baseName); + const { stem, extension } = splitArtifactExtension(file.name); const boundedExtension = truncateUtf8(extension, MAX_ARTIFACT_DISPLAY_NAME_BYTES / 2); for (let index = 2; index <= used.size + 2; index += 1) { const suffix = ` (${index})`; @@ -202,11 +197,17 @@ function uniqueArtifactDisplayName( return candidate; } } - return safeArtifactDisplayName({ id, name: '', mime }); + return safeArtifactDisplayName({ id: file.id, name: '', mime: file.mime }); } +/** The stem and extension (with the dot) of a sanitized display name. */ +type ArtifactDisplayNameParts = { + extension: string; + stem: string; +}; + /** Split a sanitized display name into its stem and extension (with the dot). */ -function splitArtifactExtension(name: string): { extension: string; stem: string } { +function splitArtifactExtension(name: string): ArtifactDisplayNameParts { const extensionStart = name.lastIndexOf('.'); if (extensionStart > 0 && extensionStart < name.length - 1) { return { extension: name.slice(extensionStart), stem: name.slice(0, extensionStart) };