From 8b59aba419df9d01cc399d2dcd95f820f01009fa Mon Sep 17 00:00:00 2001 From: Dustin Kelley Date: Tue, 7 Jul 2026 11:52:47 -0500 Subject: [PATCH 01/13] fix(core): align HighlightsClient with the live highlights API contract The client sent requests the API rejects on every call, so highlights could never be fetched, created, or deleted: - Send the OAuth token as an Authorization: Bearer header (the API rejects the previous lat query parameter with 401) - Use the API's bible_id naming on the wire for GET/POST/DELETE while keeping version_id in the SDK's public types, mapped at the boundary - Send the required { request_id, highlight: { ... } } envelope on create for idempotent retries - Require version_id and passage_id on getHighlights and version_id on deleteHighlight, matching the API's required parameters - Validate responses with Zod and map from the wire shape - Un-skip the highlights tests and make the MSW handlers enforce the documented contract so future drift fails tests Co-authored-by: Cursor --- .changeset/fix-highlights-api-contract.md | 12 + packages/core/src/__tests__/handlers.ts | 85 ++- .../core/src/__tests__/highlights.test.ts | 508 ++++-------------- packages/core/src/client.ts | 13 +- packages/core/src/highlights.ts | 118 ++-- packages/core/src/schemas/highlight.ts | 42 +- packages/hooks/src/useHighlights.test.tsx | 109 +--- packages/hooks/src/useHighlights.ts | 8 +- 8 files changed, 331 insertions(+), 564 deletions(-) create mode 100644 .changeset/fix-highlights-api-contract.md diff --git a/.changeset/fix-highlights-api-contract.md b/.changeset/fix-highlights-api-contract.md new file mode 100644 index 00000000..1a203994 --- /dev/null +++ b/.changeset/fix-highlights-api-contract.md @@ -0,0 +1,12 @@ +--- +'@youversion/platform-core': minor +'@youversion/platform-react-hooks': minor +--- + +Fix `HighlightsClient` to match the live highlights API contract. The client previously sent requests the API rejects on every call (401/400), so highlights could never be fetched, created, or deleted. + +- Auth token is now sent as an `Authorization: Bearer ` header instead of a `lat` query parameter +- Query/body fields now use the API's `bible_id` naming on the wire (the SDK keeps `version_id` in its public types and maps at the boundary) +- `createHighlight` now sends the required `{ request_id, highlight: { ... } }` envelope for idempotent retries +- `getHighlights` now requires `version_id` and `passage_id` (verse or chapter USFM), and `deleteHighlight` requires `version_id`, matching the API's required parameters; `useHighlights` options are updated accordingly +- API responses are validated with Zod and mapped from the wire shape diff --git a/packages/core/src/__tests__/handlers.ts b/packages/core/src/__tests__/handlers.ts index 8a42d59c..6b64952b 100644 --- a/packages/core/src/__tests__/handlers.ts +++ b/packages/core/src/__tests__/handlers.ts @@ -1,5 +1,5 @@ import { http, HttpResponse } from 'msw'; -import type { Collection, Highlight, Language } from '../types'; +import type { Collection, Language } from '../types'; import { mockLanguages } from './MockLanguages'; import { mockVersions, mockVersionKJV } from './MockVersions'; import { mockOrganizations } from './MockOrganizations'; @@ -22,6 +22,17 @@ if (!apiHost) { throw new Error('YVP_API_HOST environment variable must be set to run handler tests.'); } +/** Mirrors the live API's 401 when no `Authorization: Bearer` header is sent. */ +function requireBearerAuth(request: Request): Response | null { + if (request.headers.get('Authorization')?.startsWith('Bearer ')) { + return null; + } + return HttpResponse.json( + { error: 'invalid_request', error_description: 'Missing or invalid Bearer token' }, + { status: 401 }, + ); +} + export const handlers = [ // Organizations endpoints http.get(`https://${apiHost}/v1/organizations/:organizationId`, ({ params }) => { @@ -87,38 +98,70 @@ export const handlers = [ return HttpResponse.json(response); }), - // Highlights endpoints + // Highlights endpoints. These handlers intentionally enforce the documented + // API contract (Bearer auth, `bible_id` naming, enveloped POST body) so the + // client cannot drift from it without tests failing. http.get(`https://${apiHost}/v1/highlights`, ({ request }) => { + const authError = requireBearerAuth(request); + if (authError) return authError; + const url = new URL(request.url); - const bibleId = url.searchParams.get('version_id'); + const bibleId = url.searchParams.get('bible_id'); const passageId = url.searchParams.get('passage_id'); + if (!bibleId || !passageId) { + return HttpResponse.json( + { error: 'invalid_request', error_description: 'bible_id and passage_id are required' }, + { status: 400 }, + ); + } - const highlights: Collection = { + return HttpResponse.json({ data: [ - { - version_id: bibleId ? Number(bibleId) : 111, - passage_id: passageId || 'MAT.1.1', - color: 'fffe00', - }, - { - version_id: bibleId ? Number(bibleId) : 111, - passage_id: passageId || 'MAT.1.2', - color: '5dff79', - }, + { bible_id: Number(bibleId), passage_id: `${passageId}.1`, color: 'fffe00' }, + { bible_id: Number(bibleId), passage_id: `${passageId}.2`, color: '5dff79' }, ], - next_page_token: null, - }; - - return HttpResponse.json(highlights); + }); }), http.post(`https://${apiHost}/v1/highlights`, async ({ request }) => { - const body = (await request.json()) as Highlight; + const authError = requireBearerAuth(request); + if (authError) return authError; + + const body = (await request.json()) as { + request_id?: string; + highlight?: { bible_id?: number; passage_id?: string; color?: string }; + }; + if ( + !body.request_id || + !body.highlight?.bible_id || + !body.highlight.passage_id || + !body.highlight.color + ) { + return HttpResponse.json( + { + error: 'invalid_request', + error_description: + 'Body must be { request_id, highlight: { bible_id, passage_id, color } }', + }, + { status: 400 }, + ); + } - return HttpResponse.json(body, { status: 201 }); + return HttpResponse.json(body.highlight, { status: 201 }); }), - http.delete(`https://${apiHost}/v1/highlights/:passageId`, () => { + http.delete(`https://${apiHost}/v1/highlights/:passageId`, ({ request }) => { + const authError = requireBearerAuth(request); + if (authError) return authError; + + const url = new URL(request.url); + if (!url.searchParams.get('bible_id')) { + return HttpResponse.json( + { error: 'invalid_request', error_description: 'bible_id is required' }, + { status: 400 }, + ); + } + return new HttpResponse(null, { status: 204 }); }), diff --git a/packages/core/src/__tests__/highlights.test.ts b/packages/core/src/__tests__/highlights.test.ts index 0a42dfbd..75ac41f2 100644 --- a/packages/core/src/__tests__/highlights.test.ts +++ b/packages/core/src/__tests__/highlights.test.ts @@ -1,484 +1,170 @@ -import { describe, it, expect, beforeEach, vi, afterEach } from 'vitest'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { http, HttpResponse } from 'msw'; import { ApiClient } from '../client'; import { HighlightsClient } from '../highlights'; -import { YouVersionPlatformConfiguration } from '../YouVersionPlatformConfiguration'; +import { server } from './setup'; -describe.skip('HighlightsClient', () => { +const apiHost = process.env.YVP_API_HOST; + +describe('HighlightsClient', () => { let apiClient: ApiClient; let highlightsClient: HighlightsClient; beforeEach(() => { apiClient = new ApiClient({ - apiHost: process.env.YVP_API_HOST || '', + apiHost, appKey: 'test-app', installationId: 'test-installation', }); highlightsClient = new HighlightsClient(apiClient); - // Set a default token for tests that don't explicitly pass one - YouVersionPlatformConfiguration.saveAuthData('test-token', null, null); }); afterEach(() => { - // Clean up token after each test - YouVersionPlatformConfiguration.saveAuthData(null, null, null); - vi.clearAllMocks(); // Reset all mocked calls between tests + vi.restoreAllMocks(); + vi.unstubAllGlobals(); }); describe('getHighlights', () => { - it('should fetch highlights with no options', async () => { - const highlights = await highlightsClient.getHighlights(); + it('fetches highlights with bible_id/passage_id params and maps the wire response', async () => { + const highlights = await highlightsClient.getHighlights( + { version_id: 111, passage_id: 'MAT.1' }, + 'test-token', + ); + // The MSW handler 401s without a Bearer header and 400s without + // bible_id/passage_id, so a successful response also proves the request + // contract. The wire `bible_id` must come back as `version_id`. expect(highlights.data).toHaveLength(2); expect(highlights.data[0]).toEqual({ version_id: 111, passage_id: 'MAT.1.1', color: 'fffe00', }); + expect(highlights.next_page_token).toBeNull(); }); - it('should fetch highlights with version_id option', async () => { - const highlights = await highlightsClient.getHighlights({ version_id: 1 }); - - expect(highlights.data).toHaveLength(2); - expect(highlights.data[0]?.version_id).toBe(1); - }); - - it('should fetch highlights with passage_id option', async () => { - const highlights = await highlightsClient.getHighlights({ passage_id: 'JHN.3.16' }); - - expect(highlights.data).toHaveLength(2); - expect(highlights.data[0]?.passage_id).toBe('JHN.3.16'); - }); - - it('should fetch highlights with both options', async () => { - const highlights = await highlightsClient.getHighlights({ - version_id: 1, - passage_id: 'JHN.3.16', - }); - - expect(highlights.data).toHaveLength(2); - expect(highlights.data[0]?.version_id).toBe(1); - expect(highlights.data[0]?.passage_id).toBe('JHN.3.16'); - }); - - it('should throw an error for invalid version_id', async () => { - await expect(highlightsClient.getHighlights({ version_id: 0 })).rejects.toThrow( - 'Version ID must be a positive integer', - ); - await expect(highlightsClient.getHighlights({ version_id: -1 })).rejects.toThrow( - 'Version ID must be a positive integer', - ); - await expect(highlightsClient.getHighlights({ version_id: 1.5 })).rejects.toThrow(); - }); - - it('should throw an error for invalid passage_id', async () => { - await expect(highlightsClient.getHighlights({ passage_id: '' })).rejects.toThrow( - 'Passage ID must be a non-empty string', - ); - await expect(highlightsClient.getHighlights({ passage_id: ' ' })).rejects.toThrow( - 'Passage ID must be a non-empty string', - ); - }); - - it('should include lat parameter when token provided explicitly', async () => { + it('sends the token as an Authorization: Bearer header, not a query param', async () => { const fetchSpy = vi.spyOn(global, 'fetch'); - const highlights = await highlightsClient.getHighlights({ version_id: 1 }, 'explicit-token'); + await highlightsClient.getHighlights({ version_id: 111, passage_id: 'MAT.1' }, 'my-token'); - expect(fetchSpy).toHaveBeenCalledWith( - expect.stringContaining('lat=explicit-token'), - expect.any(Object), - ); - expect(highlights.data).toHaveLength(2); - expect(highlights.data[0]?.version_id).toBe(1); - - fetchSpy.mockRestore(); - }); - - it('should include lat parameter when token auto-retrieved from config', async () => { - YouVersionPlatformConfiguration.saveAuthData('config-token', null, null); - const fetchSpy = vi.spyOn(global, 'fetch'); - - const highlights = await highlightsClient.getHighlights({ version_id: 1 }); - - expect(fetchSpy).toHaveBeenCalledWith( - expect.stringContaining('lat=config-token'), - expect.any(Object), - ); - expect(highlights.data).toHaveLength(2); - expect(highlights.data[0]?.version_id).toBe(1); - - fetchSpy.mockRestore(); + const [url, init] = fetchSpy.mock.calls[0] as [string, RequestInit]; + expect(url).not.toContain('lat='); + expect(url).toContain('bible_id=111'); + expect(url).toContain('passage_id=MAT.1'); + expect((init.headers as Record).Authorization).toBe('Bearer my-token'); }); - it('should throw an error when no token is available', async () => { - YouVersionPlatformConfiguration.saveAuthData(null, null, null); - - await expect(highlightsClient.getHighlights({ version_id: 1 })).rejects.toThrow( + it('throws a helpful error when no token is available', async () => { + await expect( + highlightsClient.getHighlights({ version_id: 111, passage_id: 'MAT.1' }), + ).rejects.toThrow( 'Authentication required. Please provide a token or sign in before accessing highlights.', ); }); - it('should use explicit token over config token', async () => { - YouVersionPlatformConfiguration.saveAuthData('config-token', null, null); - const fetchSpy = vi.spyOn(global, 'fetch'); - const highlights = await highlightsClient.getHighlights({ version_id: 1 }, 'explicit-token'); - - expect(fetchSpy).toHaveBeenCalledWith( - expect.stringContaining('lat=explicit-token'), - expect.any(Object), - ); - expect(highlights.data).toHaveLength(2); - expect(highlights.data[0]?.version_id).toBe(1); - - fetchSpy.mockRestore(); - }); - }); - - describe('createHighlight', () => { - it('should create a highlight', async () => { - const highlight = await highlightsClient.createHighlight({ - version_id: 111, - passage_id: 'MAT.1.1', - color: 'fffe00', - }); - - expect(highlight).toEqual({ - version_id: 111, - passage_id: 'MAT.1.1', - color: 'fffe00', - }); - }); - - it('should throw an error for invalid version_id', async () => { - await expect( - highlightsClient.createHighlight({ - version_id: 0, - passage_id: 'MAT.1.1', - color: 'fffe00', - }), - ).rejects.toThrow('Version ID must be a positive integer'); - + it('validates version_id and passage_id before making a request', async () => { await expect( - highlightsClient.createHighlight({ - version_id: -1, - passage_id: 'MAT.1.1', - color: 'fffe00', - }), + highlightsClient.getHighlights({ version_id: 0, passage_id: 'MAT.1' }, 'token'), ).rejects.toThrow('Version ID must be a positive integer'); - }); - - it('should throw an error for invalid passage_id', async () => { - await expect( - highlightsClient.createHighlight({ - version_id: 111, - passage_id: '', - color: 'fffe00', - }), - ).rejects.toThrow('Passage ID must be a non-empty string'); await expect( - highlightsClient.createHighlight({ - version_id: 111, - passage_id: ' ', - color: 'fffe00', - }), + highlightsClient.getHighlights({ version_id: 111, passage_id: ' ' }, 'token'), ).rejects.toThrow('Passage ID must be a non-empty string'); }); - it('should throw an error for invalid color', async () => { - await expect( - highlightsClient.createHighlight({ - version_id: 111, - passage_id: 'MAT.1.1', - color: 'invalid', - }), - ).rejects.toThrow('Color must be a 6-character hex string without #'); - - await expect( - highlightsClient.createHighlight({ - version_id: 111, - passage_id: 'MAT.1.1', - color: '#fffe00', - }), - ).rejects.toThrow('Color must be a 6-character hex string without #'); - - await expect( - highlightsClient.createHighlight({ - version_id: 111, - passage_id: 'MAT.1.1', - color: 'fff', - }), - ).rejects.toThrow('Color must be a 6-character hex string without #'); + it('throws a helpful error when the API response is malformed', async () => { + server.use( + http.get(`https://${apiHost}/v1/highlights`, () => + HttpResponse.json({ data: [{ wrong_shape: true }] }), + ), + ); await expect( - highlightsClient.createHighlight({ - version_id: 111, - passage_id: 'MAT.1.1', - color: 'fffe00a', - }), - ).rejects.toThrow('Color must be a 6-character hex string without #'); + highlightsClient.getHighlights({ version_id: 111, passage_id: 'MAT.1' }, 'token'), + ).rejects.toThrow('Unexpected highlights API response'); }); + }); - it('should accept valid hex colors', async () => { - const validColors = ['fffe00', '5dff79', '00d6ff', 'FFC66F', 'ff95ef']; - - for (const color of validColors) { - const highlight = await highlightsClient.createHighlight({ - version_id: 111, - passage_id: 'MAT.1.1', - color, - }); - - expect(highlight.color).toBe(color); - } - }); - - it('should accept passage ranges in passage_id', async () => { - const highlight = await highlightsClient.createHighlight({ - version_id: 111, - passage_id: 'MAT.1.1-5', - color: 'fffe00', - }); - - expect(highlight.passage_id).toBe('MAT.1.1-5'); - }); + describe('createHighlight', () => { + it('POSTs the enveloped body ({ request_id, highlight: { bible_id, ... } })', async () => { + let capturedBody: unknown; + server.use( + http.post(`https://${apiHost}/v1/highlights`, async ({ request }) => { + capturedBody = await request.json(); + const body = capturedBody as { highlight: Record }; + return HttpResponse.json(body.highlight, { status: 201 }); + }), + ); - it('should include lat parameter when token provided explicitly', async () => { - const fetchSpy = vi.spyOn(global, 'fetch'); const highlight = await highlightsClient.createHighlight( - { - version_id: 111, - passage_id: 'MAT.1.1', - color: 'fffe00', - }, - 'explicit-token', + { version_id: 111, passage_id: 'MAT.1.1', color: 'fffe00' }, + 'test-token', ); - expect(fetchSpy).toHaveBeenCalledWith( - expect.stringContaining('lat=explicit-token'), - expect.any(Object), - ); - expect(highlight).toEqual({ - version_id: 111, - passage_id: 'MAT.1.1', - color: 'fffe00', + expect(capturedBody).toEqual({ + request_id: expect.stringMatching(/.+/) as string, + highlight: { bible_id: 111, passage_id: 'MAT.1.1', color: 'fffe00' }, }); - - fetchSpy.mockRestore(); + expect(highlight).toEqual({ version_id: 111, passage_id: 'MAT.1.1', color: 'fffe00' }); }); - it('should include lat parameter when token auto-retrieved from config', async () => { - YouVersionPlatformConfiguration.saveAuthData('config-token', null, null); - const fetchSpy = vi.spyOn(global, 'fetch'); - const highlight = await highlightsClient.createHighlight({ - version_id: 111, - passage_id: 'MAT.1.1', - color: 'fffe00', - }); - - expect(fetchSpy).toHaveBeenCalledWith( - expect.stringContaining('lat=config-token'), - expect.any(Object), + it('creates a highlight against the contract-enforcing default handler', async () => { + const highlight = await highlightsClient.createHighlight( + { version_id: 111, passage_id: 'MAT.1.1', color: 'fffe00' }, + 'test-token', ); - expect(highlight).toEqual({ - version_id: 111, - passage_id: 'MAT.1.1', - color: 'fffe00', - }); - fetchSpy.mockRestore(); + expect(highlight).toEqual({ version_id: 111, passage_id: 'MAT.1.1', color: 'fffe00' }); }); - it('should throw an error when no token is available', async () => { - YouVersionPlatformConfiguration.saveAuthData(null, null, null); - + it('validates input before making a request', async () => { await expect( - highlightsClient.createHighlight({ - version_id: 111, - passage_id: 'MAT.1.1', - color: 'fffe00', - }), - ).rejects.toThrow( - 'Authentication required. Please provide a token or sign in before accessing highlights.', - ); - }); - - it('should use explicit token over config token', async () => { - YouVersionPlatformConfiguration.saveAuthData('config-token', null, null); - const fetchSpy = vi.spyOn(global, 'fetch'); - const highlight = await highlightsClient.createHighlight( - { - version_id: 111, - passage_id: 'MAT.1.1', - color: 'fffe00', - }, - 'explicit-token', - ); + highlightsClient.createHighlight( + { version_id: 0, passage_id: 'MAT.1.1', color: 'fffe00' }, + 'token', + ), + ).rejects.toThrow('Version ID must be a positive integer'); - expect(fetchSpy).toHaveBeenCalledWith( - expect.stringContaining('lat=explicit-token'), - expect.any(Object), - ); - expect(highlight).toEqual({ - version_id: 111, - passage_id: 'MAT.1.1', - color: 'fffe00', - }); + await expect( + highlightsClient.createHighlight( + { version_id: 111, passage_id: '', color: 'fffe00' }, + 'token', + ), + ).rejects.toThrow('Passage ID must be a non-empty string'); - fetchSpy.mockRestore(); + await expect( + highlightsClient.createHighlight( + { version_id: 111, passage_id: 'MAT.1.1', color: '#fffe00' }, + 'token', + ), + ).rejects.toThrow('Color must be a 6-character hex string without #'); }); }); describe('deleteHighlight', () => { - it('should delete a highlight', async () => { - let capturedStatus: number | undefined; - const originalFetch = global.fetch; - const fetchSpy = vi.spyOn(global, 'fetch').mockImplementation(async (...args) => { - const response = await originalFetch(...args); - capturedStatus = response.status; - return response; - }); - - await highlightsClient.deleteHighlight('MAT.1.1'); - - expect(fetchSpy).toHaveBeenCalledWith( - expect.stringContaining('lat=test-token'), - expect.any(Object), - ); - expect(capturedStatus).toBe(204); - - fetchSpy.mockRestore(); - }); - - it('should delete a highlight with version_id option', async () => { - let capturedStatus: number | undefined; - const originalFetch = global.fetch; - const fetchSpy = vi.spyOn(global, 'fetch').mockImplementation(async (...args) => { - const response = await originalFetch(...args); - capturedStatus = response.status; - return response; - }); - - await highlightsClient.deleteHighlight('MAT.1.1', { version_id: 111 }); - - expect(fetchSpy).toHaveBeenCalledWith( - expect.stringContaining('lat=test-token'), - expect.any(Object), - ); - expect(capturedStatus).toBe(204); - - fetchSpy.mockRestore(); - }); - - it('should throw an error for invalid passage_id', async () => { - await expect(highlightsClient.deleteHighlight('')).rejects.toThrow( - 'Passage ID must be a non-empty string', - ); - - await expect(highlightsClient.deleteHighlight(' ')).rejects.toThrow( - 'Passage ID must be a non-empty string', - ); - }); - - it('should throw an error for invalid version_id in options', async () => { - await expect(highlightsClient.deleteHighlight('MAT.1.1', { version_id: 0 })).rejects.toThrow( - 'Version ID must be a positive integer', - ); - - await expect(highlightsClient.deleteHighlight('MAT.1.1', { version_id: -1 })).rejects.toThrow( - 'Version ID must be a positive integer', - ); - }); - - it('should accept passage ranges in passage_id', async () => { - let capturedStatus: number | undefined; - const originalFetch = global.fetch; - const fetchSpy = vi.spyOn(global, 'fetch').mockImplementation(async (...args) => { - const response = await originalFetch(...args); - capturedStatus = response.status; - return response; - }); - - await highlightsClient.deleteHighlight('MAT.1.1-5'); - - expect(fetchSpy).toHaveBeenCalledWith( - expect.stringContaining('lat=test-token'), - expect.any(Object), - ); - expect(capturedStatus).toBe(204); - - fetchSpy.mockRestore(); - }); - - it('should include lat parameter when token provided explicitly', async () => { - let capturedStatus: number | undefined; - const originalFetch = global.fetch; - const fetchSpy = vi.spyOn(global, 'fetch').mockImplementation(async (...args) => { - const response = await originalFetch(...args); - capturedStatus = response.status; - return response; - }); - - await highlightsClient.deleteHighlight('MAT.1.1', undefined, 'explicit-token'); - - expect(fetchSpy).toHaveBeenCalledWith( - expect.stringContaining('lat=explicit-token'), - expect.any(Object), - ); - expect(capturedStatus).toBe(204); - - fetchSpy.mockRestore(); - }); - - it('should include lat parameter when token auto-retrieved from config', async () => { - YouVersionPlatformConfiguration.saveAuthData('config-token', null, null); - let capturedStatus: number | undefined; - const originalFetch = global.fetch; - const fetchSpy = vi.spyOn(global, 'fetch').mockImplementation(async (...args) => { - const response = await originalFetch(...args); - capturedStatus = response.status; - return response; - }); - - await highlightsClient.deleteHighlight('MAT.1.1'); - - expect(fetchSpy).toHaveBeenCalledWith( - expect.stringContaining('lat=config-token'), - expect.any(Object), - ); - expect(capturedStatus).toBe(204); - - fetchSpy.mockRestore(); - }); + it('DELETEs with a required bible_id param and Bearer auth', async () => { + const fetchSpy = vi.spyOn(global, 'fetch'); - it('should throw an error when no token is available', async () => { - YouVersionPlatformConfiguration.saveAuthData(null, null, null); + await expect( + highlightsClient.deleteHighlight('MAT.1.1', { version_id: 111 }, 'test-token'), + ).resolves.toBeUndefined(); - await expect(highlightsClient.deleteHighlight('MAT.1.1')).rejects.toThrow( - 'Authentication required. Please provide a token or sign in before accessing highlights.', - ); + const [url, init] = fetchSpy.mock.calls[0] as [string, RequestInit]; + expect(url).toContain('/v1/highlights/MAT.1.1'); + expect(url).toContain('bible_id=111'); + expect(url).not.toContain('lat='); + expect((init.headers as Record).Authorization).toBe('Bearer test-token'); }); - it('should use explicit token over config token', async () => { - YouVersionPlatformConfiguration.saveAuthData('config-token', null, null); - let capturedStatus: number | undefined; - const originalFetch = global.fetch; - const fetchSpy = vi.spyOn(global, 'fetch').mockImplementation(async (...args) => { - const response = await originalFetch(...args); - capturedStatus = response.status; - return response; - }); - - await highlightsClient.deleteHighlight('MAT.1.1', undefined, 'explicit-token'); - - expect(fetchSpy).toHaveBeenCalledWith( - expect.stringContaining('lat=explicit-token'), - expect.any(Object), - ); - expect(capturedStatus).toBe(204); + it('validates input before making a request', async () => { + await expect( + highlightsClient.deleteHighlight('', { version_id: 111 }, 'token'), + ).rejects.toThrow('Passage ID must be a non-empty string'); - fetchSpy.mockRestore(); + await expect( + highlightsClient.deleteHighlight('MAT.1.1', { version_id: 0 }, 'token'), + ).rejects.toThrow('Version ID must be a positive integer'); }); }); }); diff --git a/packages/core/src/client.ts b/packages/core/src/client.ts index 9f0091bd..3b6b3848 100644 --- a/packages/core/src/client.ts +++ b/packages/core/src/client.ts @@ -151,13 +151,20 @@ export class ApiClient { * @param path - The API endpoint path (relative to baseURL). * @param data - Optional request body data to send. * @param params - Optional query parameters to include in the request. + * @param headers - Optional headers merged over the default headers. * @returns A promise resolving to the response data of type T. */ - async post(path: string, data?: RequestData, params?: QueryParams): Promise { + async post( + path: string, + data?: RequestData, + params?: QueryParams, + headers?: RequestHeaders, + ): Promise { const url = `${this.baseURL}${path}${this.buildQueryString(params)}`; return this.request(url, { method: 'POST', body: data ? JSON.stringify(data) : undefined, + headers, }); } @@ -167,12 +174,14 @@ export class ApiClient { * @typeParam T - The expected response type. * @param path - The API endpoint path (relative to baseURL). * @param params - Optional query parameters to include in the request. + * @param headers - Optional headers merged over the default headers. * @returns A promise resolving to the response data of type T (may be empty for 204 responses). */ - async delete(path: string, params?: QueryParams): Promise { + async delete(path: string, params?: QueryParams, headers?: RequestHeaders): Promise { const url = `${this.baseURL}${path}${this.buildQueryString(params)}`; return this.request(url, { method: 'DELETE', + headers, }); } } diff --git a/packages/core/src/highlights.ts b/packages/core/src/highlights.ts index ebcd3962..c63dab70 100644 --- a/packages/core/src/highlights.ts +++ b/packages/core/src/highlights.ts @@ -2,25 +2,36 @@ import { z } from 'zod'; import type { ApiClient } from './client'; import type { Collection, Highlight, CreateHighlight } from './types'; import { YouVersionPlatformConfiguration } from './YouVersionPlatformConfiguration'; +import { + HighlightCollectionWireSchema, + HighlightWireSchema, + toHighlight, +} from './schemas/highlight'; /** * Options for getting highlights. + * The API requires both a Bible version and a passage scope. */ export type GetHighlightsOptions = { - version_id?: number; - passage_id?: string; + /** Bible version identifier (sent to the API as `bible_id`). */ + version_id: number; + /** Passage identifier in verse or chapter USFM format (e.g., "JHN.3" or "JHN.3.16"). */ + passage_id: string; }; /** * Options for deleting highlights. */ export type DeleteHighlightOptions = { - version_id?: number; + /** Bible version identifier (sent to the API as `bible_id`). */ + version_id: number; }; /** * Client for interacting with Highlights API endpoints. - * Note: All endpoints require OAuth authentication with appropriate scopes. + * Note: All endpoints require OAuth authentication with appropriate scopes + * (`read_highlights` / `write_highlights`), passed as an + * `Authorization: Bearer ` header. */ export class HighlightsClient { private client: ApiClient; @@ -49,7 +60,8 @@ export class HighlightsClient { if (lat) { return lat; } - const token = YouVersionPlatformConfiguration.accessToken; + const token = + typeof localStorage === 'undefined' ? null : YouVersionPlatformConfiguration.accessToken; if (!token) { throw new Error( 'Authentication required. Please provide a token or sign in before accessing highlights.', @@ -58,6 +70,10 @@ export class HighlightsClient { return token; } + private authHeaders(lat?: string): Record { + return { Authorization: `Bearer ${this.getAuthToken(lat)}` }; + } + private validateVersionId(value: number): void { try { this.versionIdSchema.parse(value); @@ -91,34 +107,46 @@ export class HighlightsClient { } } + /** + * Generates a UUID for idempotent create retries (the API requires a + * `request_id` on every create). + */ + private generateRequestId(): string { + if (typeof crypto !== 'undefined' && typeof crypto.randomUUID === 'function') { + return crypto.randomUUID(); + } + // Extremely old runtimes only; uniqueness (not randomness quality) is what matters here. + return `yvp-${Date.now().toString(16)}-${Math.random().toString(16).slice(2, 10)}`; + } + /** * Fetches a collection of highlights for a user. * The response will return a color per verse without ranges. * Requires OAuth with read_highlights scope. - * @param options Query parameters for filtering highlights. + * @param options Query parameters scoping the fetch. `version_id` and `passage_id` + * (verse or chapter USFM, e.g. "JHN.3") are both required by the API. * @param lat Optional long access token. If not provided, retrieves from YouVersionPlatformConfiguration. * @returns A collection of Highlight objects. */ - async getHighlights( - options?: GetHighlightsOptions, - lat?: string, - ): Promise> { - const token = this.getAuthToken(lat); - const params: Record = { - lat: token, - }; - - if (options?.version_id !== undefined) { - this.validateVersionId(options.version_id); - params.version_id = options.version_id; - } - - if (options?.passage_id !== undefined) { - this.validatePassageId(options.passage_id); - params.passage_id = options.passage_id; + async getHighlights(options: GetHighlightsOptions, lat?: string): Promise> { + this.validateVersionId(options.version_id); + this.validatePassageId(options.passage_id); + + const response = await this.client.get( + `/v1/highlights`, + { bible_id: options.version_id, passage_id: options.passage_id }, + this.authHeaders(lat), + ); + + const parsed = HighlightCollectionWireSchema.safeParse(response); + if (!parsed.success) { + throw new Error(`Unexpected highlights API response: ${parsed.error.message}`); } - return this.client.get>(`/v1/highlights`, params); + return { + data: parsed.data.data.map(toHighlight), + next_page_token: parsed.data.next_page_token ?? null, + }; } /** @@ -134,36 +162,48 @@ export class HighlightsClient { this.validatePassageId(data.passage_id); this.validateColor(data.color); - const token = this.getAuthToken(lat); + const response = await this.client.post( + `/v1/highlights`, + { + request_id: this.generateRequestId(), + highlight: { + bible_id: data.version_id, + passage_id: data.passage_id, + color: data.color, + }, + }, + undefined, + this.authHeaders(lat), + ); + + const parsed = HighlightWireSchema.safeParse(response); + if (!parsed.success) { + throw new Error(`Unexpected highlights API response: ${parsed.error.message}`); + } - return this.client.post(`/v1/highlights`, data, { lat: token }); + return toHighlight(parsed.data); } /** * Clears highlights for a passage. * Requires OAuth with write_highlights scope. * @param passageId The passage identifier (USFM format, e.g., "MAT.1.1" or "MAT.1.1-5"). - * @param options Optional query parameters including bible_id. + * @param options Query parameters; `version_id` is required by the API (sent as `bible_id`). * @param lat Optional long access token. If not provided, retrieves from YouVersionPlatformConfiguration. * @returns Promise that resolves when highlights are deleted (204 response). */ async deleteHighlight( passageId: string, - options?: DeleteHighlightOptions, + options: DeleteHighlightOptions, lat?: string, ): Promise { this.validatePassageId(passageId); + this.validateVersionId(options.version_id); - const token = this.getAuthToken(lat); - const params: Record = { - lat: token, - }; - - if (options?.version_id !== undefined) { - this.validateVersionId(options.version_id); - params.version_id = options.version_id; - } - - await this.client.delete(`/v1/highlights/${passageId}`, params); + await this.client.delete( + `/v1/highlights/${encodeURIComponent(passageId)}`, + { bible_id: options.version_id }, + this.authHeaders(lat), + ); } } diff --git a/packages/core/src/schemas/highlight.ts b/packages/core/src/schemas/highlight.ts index 2fa92ee4..bc94184e 100644 --- a/packages/core/src/schemas/highlight.ts +++ b/packages/core/src/schemas/highlight.ts @@ -1,23 +1,57 @@ import { z } from 'zod'; -const _HighlightSchema = z.object({ +const HEX_COLOR_REGEX = /^[0-9a-f]{6}$/; + +/** + * Wire format used by the highlights API (`/v1/highlights`), which names the + * Bible version field `bible_id`. The SDK exposes it as `version_id` for + * consistency with the rest of the public API (see {@link toHighlight}). + */ +export const HighlightWireSchema = z.object({ /** Bible version identifier */ + bible_id: z.number().int().positive(), + /** Passage identifier (e.g., "MAT.1.1") */ + passage_id: z.string(), + /** Hex color code (6 digits, no #) */ + color: z.string().regex(HEX_COLOR_REGEX), +}); + +export type HighlightWire = z.infer; + +export const HighlightCollectionWireSchema = z.object({ + data: z.array(HighlightWireSchema), + next_page_token: z.string().nullable().optional(), +}); + +export type HighlightCollectionWire = z.infer; + +const _HighlightSchema = z.object({ + /** Bible version identifier (sent to / received from the API as `bible_id`) */ version_id: z.number().int().positive(), /** Passage identifier (e.g., "MAT.1.1") */ passage_id: z.string(), /** Hex color code (6 digits, no #) */ - color: z.string().regex(/^[0-9a-f]{6}$/), + color: z.string().regex(HEX_COLOR_REGEX), }); export type Highlight = z.infer; +/** Maps an API wire highlight (`bible_id`) to the SDK shape (`version_id`). */ +export function toHighlight(wire: HighlightWire): Highlight { + return { + version_id: wire.bible_id, + passage_id: wire.passage_id, + color: wire.color, + }; +} + const _CreateHighlightSchema = z.object({ - /** Bible version identifier */ + /** Bible version identifier (sent to the API as `bible_id`) */ version_id: z.number().int().positive(), /** Passage identifier (e.g., "MAT.1.1") */ passage_id: z.string(), /** Hex color code (6 digits, no #) */ - color: z.string().regex(/^[0-9a-f]{6}$/), + color: z.string().regex(HEX_COLOR_REGEX), }); export type CreateHighlight = z.infer; diff --git a/packages/hooks/src/useHighlights.test.tsx b/packages/hooks/src/useHighlights.test.tsx index 985203e8..73b1dc64 100644 --- a/packages/hooks/src/useHighlights.test.tsx +++ b/packages/hooks/src/useHighlights.test.tsx @@ -7,6 +7,7 @@ import { HighlightsClient, ApiClient, type Collection, + type GetHighlightsOptions, type Highlight, type CreateHighlight, } from '@youversion/platform-core'; @@ -26,6 +27,8 @@ vi.mock('@youversion/platform-core', async () => { }); describe('useHighlights', () => { + const defaultOptions: GetHighlightsOptions = { version_id: 111, passage_id: 'MAT.1' }; + const mockHighlights: Collection = { data: [ { @@ -74,7 +77,7 @@ describe('useHighlights', () => { describe('context validation', () => { it('should throw error when context is not provided', () => { - expect(() => renderHook(() => useHighlights())).toThrow( + expect(() => renderHook(() => useHighlights(defaultOptions))).toThrow( 'YouVersion context not found. Make sure your component is wrapped with YouVersionProvider and an API key is provided.', ); }); @@ -83,7 +86,7 @@ describe('useHighlights', () => { describe('client creation', () => { it('should create HighlightsClient with correct ApiClient config', () => { const wrapper = createYVWrapper(); - renderHook(() => useHighlights(), { wrapper }); + renderHook(() => useHighlights(defaultOptions), { wrapper }); expect(ApiClient).toHaveBeenCalledWith({ appKey: 'test-app-key', @@ -93,7 +96,7 @@ describe('useHighlights', () => { it('should memoize HighlightsClient instance', () => { const wrapper = createYVWrapper(); - const { rerender } = renderHook(() => useHighlights(), { wrapper }); + const { rerender } = renderHook(() => useHighlights(defaultOptions), { wrapper }); rerender(); @@ -113,7 +116,7 @@ describe('useHighlights', () => { ); - const { rerender } = renderHook(() => useHighlights(), { wrapper }); + const { rerender } = renderHook(() => useHighlights(defaultOptions), { wrapper }); expect(HighlightsClient).toHaveBeenCalledTimes(1); @@ -126,9 +129,9 @@ describe('useHighlights', () => { }); describe('fetching highlights', () => { - it('should fetch highlights with no options', async () => { + it('should fetch highlights with the provided options', async () => { const wrapper = createYVWrapper(); - const { result } = renderHook(() => useHighlights(), { wrapper }); + const { result } = renderHook(() => useHighlights(defaultOptions), { wrapper }); expect(result.current.loading).toBe(true); expect(result.current.highlights).toBe(null); @@ -137,51 +140,7 @@ describe('useHighlights', () => { expect(result.current.loading).toBe(false); }); - expect.soft(mockGetHighlights).toHaveBeenCalledWith(undefined); - expect.soft(result.current.highlights).toEqual(mockHighlights); - }); - - it('should fetch highlights with version_id option', async () => { - const wrapper = createYVWrapper(); - const { result } = renderHook(() => useHighlights({ version_id: 111 }), { wrapper }); - - await waitFor(() => { - expect(result.current.loading).toBe(false); - }); - - expect.soft(mockGetHighlights).toHaveBeenCalledWith({ version_id: 111 }); - expect.soft(result.current.highlights).toEqual(mockHighlights); - }); - - it('should fetch highlights with passage_id option', async () => { - const wrapper = createYVWrapper(); - const { result } = renderHook(() => useHighlights({ passage_id: 'MAT.1.1' }), { wrapper }); - - await waitFor(() => { - expect(result.current.loading).toBe(false); - }); - - expect.soft(mockGetHighlights).toHaveBeenCalledWith({ passage_id: 'MAT.1.1' }); - expect.soft(result.current.highlights).toEqual(mockHighlights); - }); - - it('should fetch highlights with both options', async () => { - const wrapper = createYVWrapper(); - const { result } = renderHook( - () => useHighlights({ version_id: 111, passage_id: 'MAT.1.1' }), - { - wrapper, - }, - ); - - await waitFor(() => { - expect(result.current.loading).toBe(false); - }); - - expect.soft(mockGetHighlights).toHaveBeenCalledWith({ - version_id: 111, - passage_id: 'MAT.1.1', - }); + expect.soft(mockGetHighlights).toHaveBeenCalledWith(defaultOptions); expect.soft(result.current.highlights).toEqual(mockHighlights); }); @@ -189,7 +148,7 @@ describe('useHighlights', () => { const wrapper = createYVWrapper(); const { result, rerender } = renderHook(({ options }) => useHighlights(options), { wrapper, - initialProps: { options: { version_id: 111 } }, + initialProps: { options: defaultOptions }, }); await waitFor(() => { @@ -198,19 +157,22 @@ describe('useHighlights', () => { expect(mockGetHighlights).toHaveBeenCalledTimes(1); - rerender({ options: { version_id: 1 } }); + rerender({ options: { version_id: 1, passage_id: 'JHN.3' } }); await waitFor(() => { expect(result.current.loading).toBe(false); }); expect.soft(mockGetHighlights).toHaveBeenCalledTimes(2); - expect.soft(mockGetHighlights).toHaveBeenLastCalledWith({ version_id: 1 }); + expect.soft(mockGetHighlights).toHaveBeenLastCalledWith({ + version_id: 1, + passage_id: 'JHN.3', + }); }); it('should not fetch when enabled is false', async () => { const wrapper = createYVWrapper(); - const { result } = renderHook(() => useHighlights(undefined, { enabled: false }), { + const { result } = renderHook(() => useHighlights(defaultOptions, { enabled: false }), { wrapper, }); @@ -227,7 +189,7 @@ describe('useHighlights', () => { const error = new Error('Failed to fetch highlights'); mockGetHighlights.mockRejectedValueOnce(error); - const { result } = renderHook(() => useHighlights(), { wrapper }); + const { result } = renderHook(() => useHighlights(defaultOptions), { wrapper }); await waitFor(() => { expect(result.current.loading).toBe(false); @@ -239,7 +201,7 @@ describe('useHighlights', () => { it('should support manual refetch', async () => { const wrapper = createYVWrapper(); - const { result } = renderHook(() => useHighlights(), { wrapper }); + const { result } = renderHook(() => useHighlights(defaultOptions), { wrapper }); await waitFor(() => { expect(result.current.loading).toBe(false); @@ -258,7 +220,7 @@ describe('useHighlights', () => { describe('createHighlight mutation', () => { it('should create highlight and refetch', async () => { const wrapper = createYVWrapper(); - const { result } = renderHook(() => useHighlights(), { wrapper }); + const { result } = renderHook(() => useHighlights(defaultOptions), { wrapper }); await waitFor(() => { expect(result.current.loading).toBe(false); @@ -289,7 +251,7 @@ describe('useHighlights', () => { const error = new Error('Failed to create highlight'); mockCreateHighlight.mockRejectedValueOnce(error); - const { result } = renderHook(() => useHighlights(), { wrapper }); + const { result } = renderHook(() => useHighlights(defaultOptions), { wrapper }); await waitFor(() => { expect(result.current.loading).toBe(false); @@ -312,16 +274,16 @@ describe('useHighlights', () => { describe('deleteHighlight mutation', () => { it('should delete highlight and refetch', async () => { const wrapper = createYVWrapper(); - const { result } = renderHook(() => useHighlights(), { wrapper }); + const { result } = renderHook(() => useHighlights(defaultOptions), { wrapper }); await waitFor(() => { expect(result.current.loading).toBe(false); }); - const deletePromise = result.current.deleteHighlight('MAT.1.1'); + const deletePromise = result.current.deleteHighlight('MAT.1.1', { version_id: 111 }); await waitFor(() => { - expect(mockDeleteHighlight).toHaveBeenCalledWith('MAT.1.1', undefined); + expect(mockDeleteHighlight).toHaveBeenCalledWith('MAT.1.1', { version_id: 111 }); }); await deletePromise; @@ -331,37 +293,18 @@ describe('useHighlights', () => { }); }); - it('should delete highlight with options', async () => { - const wrapper = createYVWrapper(); - const { result } = renderHook(() => useHighlights(), { wrapper }); - - await waitFor(() => { - expect(result.current.loading).toBe(false); - }); - - const deletePromise = result.current.deleteHighlight('MAT.1.1', { version_id: 111 }); - - await waitFor(() => { - expect(mockDeleteHighlight).toHaveBeenCalledWith('MAT.1.1', { version_id: 111 }); - }); - - await deletePromise; - - expect(mockGetHighlights).toHaveBeenCalledTimes(2); - }); - it('should handle delete error', async () => { const wrapper = createYVWrapper(); const error = new Error('Failed to delete highlight'); mockDeleteHighlight.mockRejectedValueOnce(error); - const { result } = renderHook(() => useHighlights(), { wrapper }); + const { result } = renderHook(() => useHighlights(defaultOptions), { wrapper }); await waitFor(() => { expect(result.current.loading).toBe(false); }); - await expect(result.current.deleteHighlight('MAT.1.1')).rejects.toThrow( + await expect(result.current.deleteHighlight('MAT.1.1', { version_id: 111 })).rejects.toThrow( 'Failed to delete highlight', ); diff --git a/packages/hooks/src/useHighlights.ts b/packages/hooks/src/useHighlights.ts index 448cd399..d41af847 100644 --- a/packages/hooks/src/useHighlights.ts +++ b/packages/hooks/src/useHighlights.ts @@ -14,7 +14,7 @@ import { } from '@youversion/platform-core'; export function useHighlights( - options?: GetHighlightsOptions, + options: GetHighlightsOptions, apiOptions?: UseApiDataOptions, ): { highlights: Collection | null; @@ -22,7 +22,7 @@ export function useHighlights( error: Error | null; refetch: () => void; createHighlight: (data: CreateHighlight) => Promise; - deleteHighlight: (passageId: string, deleteOptions?: DeleteHighlightOptions) => Promise; + deleteHighlight: (passageId: string, deleteOptions: DeleteHighlightOptions) => Promise; } { const context = useContext(YouVersionContext); @@ -44,7 +44,7 @@ export function useHighlights( const { data, loading, error, refetch } = useApiData>( () => highlightsClient.getHighlights(options), - [highlightsClient, options?.version_id, options?.passage_id], + [highlightsClient, options.version_id, options.passage_id], { enabled: apiOptions?.enabled !== false, }, @@ -60,7 +60,7 @@ export function useHighlights( ); const deleteHighlight = useCallback( - async (passageId: string, deleteOptions?: DeleteHighlightOptions): Promise => { + async (passageId: string, deleteOptions: DeleteHighlightOptions): Promise => { await highlightsClient.deleteHighlight(passageId, deleteOptions); refetch(); }, From fa496e2be6521dcd6afcd645b59bc2ff854af7ed Mon Sep 17 00:00:00 2001 From: Cameron Pak Date: Wed, 8 Jul 2026 13:19:20 -0500 Subject: [PATCH 02/13] fix(core): accept uppercase hex colors from highlights API; clarify request_id - Wire/SDK color schema is now case-insensitive, matching the client's input validator. Previously a valid uppercase color (e.g. FFC66F) passed input validation on create but threw "Unexpected highlights API response" when the same value came back through HighlightWireSchema. Adds a round-trip test. - Clarify that request_id is a unique per-request id required by the API and does not by itself make caller-level retries idempotent (comment + changeset). Co-Authored-By: Claude Opus 4.8 --- .changeset/fix-highlights-api-contract.md | 2 +- packages/core/src/__tests__/highlights.test.ts | 9 +++++++++ packages/core/src/highlights.ts | 6 ++++-- packages/core/src/schemas/highlight.ts | 4 +++- 4 files changed, 17 insertions(+), 4 deletions(-) diff --git a/.changeset/fix-highlights-api-contract.md b/.changeset/fix-highlights-api-contract.md index 1a203994..9508b25c 100644 --- a/.changeset/fix-highlights-api-contract.md +++ b/.changeset/fix-highlights-api-contract.md @@ -7,6 +7,6 @@ Fix `HighlightsClient` to match the live highlights API contract. The client pre - Auth token is now sent as an `Authorization: Bearer ` header instead of a `lat` query parameter - Query/body fields now use the API's `bible_id` naming on the wire (the SDK keeps `version_id` in its public types and maps at the boundary) -- `createHighlight` now sends the required `{ request_id, highlight: { ... } }` envelope for idempotent retries +- `createHighlight` now sends the required `{ request_id, highlight: { ... } }` envelope (`request_id` is a unique per-request id the API requires) - `getHighlights` now requires `version_id` and `passage_id` (verse or chapter USFM), and `deleteHighlight` requires `version_id`, matching the API's required parameters; `useHighlights` options are updated accordingly - API responses are validated with Zod and mapped from the wire shape diff --git a/packages/core/src/__tests__/highlights.test.ts b/packages/core/src/__tests__/highlights.test.ts index 75ac41f2..f35bcbda 100644 --- a/packages/core/src/__tests__/highlights.test.ts +++ b/packages/core/src/__tests__/highlights.test.ts @@ -118,6 +118,15 @@ describe('HighlightsClient', () => { expect(highlight).toEqual({ version_id: 111, passage_id: 'MAT.1.1', color: 'fffe00' }); }); + it('accepts uppercase hex colors round-tripped through the wire schema', async () => { + const highlight = await highlightsClient.createHighlight( + { version_id: 111, passage_id: 'MAT.1.1', color: 'FFC66F' }, + 'test-token', + ); + + expect(highlight).toEqual({ version_id: 111, passage_id: 'MAT.1.1', color: 'FFC66F' }); + }); + it('validates input before making a request', async () => { await expect( highlightsClient.createHighlight( diff --git a/packages/core/src/highlights.ts b/packages/core/src/highlights.ts index c63dab70..8785def9 100644 --- a/packages/core/src/highlights.ts +++ b/packages/core/src/highlights.ts @@ -108,8 +108,10 @@ export class HighlightsClient { } /** - * Generates a UUID for idempotent create retries (the API requires a - * `request_id` on every create). + * Generates a unique `request_id` for a create call, which the API requires on + * every create. Note: a new id is minted per call, so this does not by itself + * make caller-level retries idempotent — reuse the same id across retries of a + * single logical create if end-to-end idempotency is needed. */ private generateRequestId(): string { if (typeof crypto !== 'undefined' && typeof crypto.randomUUID === 'function') { diff --git a/packages/core/src/schemas/highlight.ts b/packages/core/src/schemas/highlight.ts index bc94184e..a935b80c 100644 --- a/packages/core/src/schemas/highlight.ts +++ b/packages/core/src/schemas/highlight.ts @@ -1,6 +1,8 @@ import { z } from 'zod'; -const HEX_COLOR_REGEX = /^[0-9a-f]{6}$/; +// Case-insensitive: the API may echo colors in any case, and the client's input +// validator accepts both, so response parsing must not reject uppercase hex. +const HEX_COLOR_REGEX = /^[0-9a-f]{6}$/i; /** * Wire format used by the highlights API (`/v1/highlights`), which names the From cb8e8fc1d3717ef28d632c65b226a16b6ed423aa Mon Sep 17 00:00:00 2001 From: Cameron Pak Date: Wed, 8 Jul 2026 13:27:42 -0500 Subject: [PATCH 03/13] fix(core): handle empty highlights, normalize color, add recent-colors Verified HighlightsClient against the compiled OpenAPI spec. Follow-ups: - getHighlights: treat a 204 (no highlights for the passage, the common case) as an empty collection instead of throwing "Unexpected highlights API response". Adds a regression test. - createHighlight: normalize color to lowercase before sending; the API's color pattern is lowercase-only (^[0-9a-f]{6}$), so an uppercase input would otherwise be rejected server-side. - Add getRecentColors() for GET /v1/highlights/recent-colors (recent + default palette colors) and expose it from useHighlights for color pickers. Co-Authored-By: Claude Opus 4.8 --- .changeset/fix-highlights-api-contract.md | 3 + packages/core/src/__tests__/handlers.ts | 9 +++ .../core/src/__tests__/highlights.test.ts | 63 ++++++++++++++++++- packages/core/src/highlights.ts | 39 +++++++++++- packages/core/src/schemas/highlight.ts | 10 +++ packages/hooks/src/useHighlights.test.tsx | 17 +++++ packages/hooks/src/useHighlights.ts | 7 +++ 7 files changed, 145 insertions(+), 3 deletions(-) diff --git a/.changeset/fix-highlights-api-contract.md b/.changeset/fix-highlights-api-contract.md index 9508b25c..10fab280 100644 --- a/.changeset/fix-highlights-api-contract.md +++ b/.changeset/fix-highlights-api-contract.md @@ -9,4 +9,7 @@ Fix `HighlightsClient` to match the live highlights API contract. The client pre - Query/body fields now use the API's `bible_id` naming on the wire (the SDK keeps `version_id` in its public types and maps at the boundary) - `createHighlight` now sends the required `{ request_id, highlight: { ... } }` envelope (`request_id` is a unique per-request id the API requires) - `getHighlights` now requires `version_id` and `passage_id` (verse or chapter USFM), and `deleteHighlight` requires `version_id`, matching the API's required parameters; `useHighlights` options are updated accordingly +- `getHighlights` now treats a `204` (no highlights for the passage) as an empty collection instead of throwing +- `createHighlight` normalizes `color` to lowercase before sending, since the API accepts lowercase hex only +- Added `getRecentColors()` (`GET /v1/highlights/recent-colors`) to `HighlightsClient` and exposed it from `useHighlights` for building color pickers - API responses are validated with Zod and mapped from the wire shape diff --git a/packages/core/src/__tests__/handlers.ts b/packages/core/src/__tests__/handlers.ts index 6b64952b..9b89ab9a 100644 --- a/packages/core/src/__tests__/handlers.ts +++ b/packages/core/src/__tests__/handlers.ts @@ -123,6 +123,15 @@ export const handlers = [ }); }), + http.get(`https://${apiHost}/v1/highlights/recent-colors`, ({ request }) => { + const authError = requireBearerAuth(request); + if (authError) return authError; + + return HttpResponse.json({ + data: [{ color: 'fffe00' }, { color: '5dff79' }], + }); + }), + http.post(`https://${apiHost}/v1/highlights`, async ({ request }) => { const authError = requireBearerAuth(request); if (authError) return authError; diff --git a/packages/core/src/__tests__/highlights.test.ts b/packages/core/src/__tests__/highlights.test.ts index f35bcbda..22cfc6c1 100644 --- a/packages/core/src/__tests__/highlights.test.ts +++ b/packages/core/src/__tests__/highlights.test.ts @@ -73,6 +73,19 @@ describe('HighlightsClient', () => { ).rejects.toThrow('Passage ID must be a non-empty string'); }); + it('returns an empty collection when the API responds 204 (no highlights)', async () => { + server.use( + http.get(`https://${apiHost}/v1/highlights`, () => new HttpResponse(null, { status: 204 })), + ); + + const highlights = await highlightsClient.getHighlights( + { version_id: 111, passage_id: 'MAT.1' }, + 'test-token', + ); + + expect(highlights).toEqual({ data: [], next_page_token: null }); + }); + it('throws a helpful error when the API response is malformed', async () => { server.use( http.get(`https://${apiHost}/v1/highlights`, () => @@ -86,6 +99,43 @@ describe('HighlightsClient', () => { }); }); + describe('getRecentColors', () => { + it('returns the ordered list of hex colors with Bearer auth', async () => { + const fetchSpy = vi.spyOn(global, 'fetch'); + + const colors = await highlightsClient.getRecentColors('test-token'); + + expect(colors).toEqual(['fffe00', '5dff79']); + + const [url, init] = fetchSpy.mock.calls[0] as [string, RequestInit]; + expect(url).toContain('/v1/highlights/recent-colors'); + expect((init.headers as Record).Authorization).toBe('Bearer test-token'); + }); + + it('returns an empty list when the API responds 204', async () => { + server.use( + http.get( + `https://${apiHost}/v1/highlights/recent-colors`, + () => new HttpResponse(null, { status: 204 }), + ), + ); + + await expect(highlightsClient.getRecentColors('test-token')).resolves.toEqual([]); + }); + + it('throws a helpful error when the API response is malformed', async () => { + server.use( + http.get(`https://${apiHost}/v1/highlights/recent-colors`, () => + HttpResponse.json({ data: [{ not_a_color: true }] }), + ), + ); + + await expect(highlightsClient.getRecentColors('test-token')).rejects.toThrow( + 'Unexpected highlights API response', + ); + }); + }); + describe('createHighlight', () => { it('POSTs the enveloped body ({ request_id, highlight: { bible_id, ... } })', async () => { let capturedBody: unknown; @@ -118,13 +168,22 @@ describe('HighlightsClient', () => { expect(highlight).toEqual({ version_id: 111, passage_id: 'MAT.1.1', color: 'fffe00' }); }); - it('accepts uppercase hex colors round-tripped through the wire schema', async () => { + it('normalizes an uppercase color to lowercase on the wire (API is lowercase-only)', async () => { + let capturedBody: { highlight: { color: string } } | undefined; + server.use( + http.post(`https://${apiHost}/v1/highlights`, async ({ request }) => { + capturedBody = (await request.json()) as { highlight: { color: string } }; + return HttpResponse.json(capturedBody.highlight, { status: 201 }); + }), + ); + const highlight = await highlightsClient.createHighlight( { version_id: 111, passage_id: 'MAT.1.1', color: 'FFC66F' }, 'test-token', ); - expect(highlight).toEqual({ version_id: 111, passage_id: 'MAT.1.1', color: 'FFC66F' }); + expect(capturedBody?.highlight.color).toBe('ffc66f'); + expect(highlight).toEqual({ version_id: 111, passage_id: 'MAT.1.1', color: 'ffc66f' }); }); it('validates input before making a request', async () => { diff --git a/packages/core/src/highlights.ts b/packages/core/src/highlights.ts index 8785def9..67262268 100644 --- a/packages/core/src/highlights.ts +++ b/packages/core/src/highlights.ts @@ -5,6 +5,7 @@ import { YouVersionPlatformConfiguration } from './YouVersionPlatformConfigurati import { HighlightCollectionWireSchema, HighlightWireSchema, + RecentHighlightColorsWireSchema, toHighlight, } from './schemas/highlight'; @@ -140,6 +141,13 @@ export class HighlightsClient { this.authHeaders(lat), ); + // The API returns 204 (empty body) when the passage has no highlights, + // which is the common case. Treat it as an empty collection rather than a + // parse failure. + if (response === '' || response == null) { + return { data: [], next_page_token: null }; + } + const parsed = HighlightCollectionWireSchema.safeParse(response); if (!parsed.success) { throw new Error(`Unexpected highlights API response: ${parsed.error.message}`); @@ -151,6 +159,33 @@ export class HighlightsClient { }; } + /** + * Fetches the user's recently used highlight colors, followed by the default + * colors, as hex strings (no `#`). Useful for building a color picker. + * Requires OAuth with read_highlights scope. + * @param lat Optional long access token. If not provided, retrieves from YouVersionPlatformConfiguration. + * @returns An ordered list of hex color strings. + */ + async getRecentColors(lat?: string): Promise { + const response = await this.client.get( + `/v1/highlights/recent-colors`, + undefined, + this.authHeaders(lat), + ); + + // The API returns 204 (empty body) when there is nothing to return. + if (response === '' || response == null) { + return []; + } + + const parsed = RecentHighlightColorsWireSchema.safeParse(response); + if (!parsed.success) { + throw new Error(`Unexpected highlights API response: ${parsed.error.message}`); + } + + return parsed.data.data.map((entry) => entry.color); + } + /** * Creates or updates a highlight on a passage. * Verse ranges may be used in the passage_id attribute. @@ -171,7 +206,9 @@ export class HighlightsClient { highlight: { bible_id: data.version_id, passage_id: data.passage_id, - color: data.color, + // The API only accepts lowercase hex; normalize so a caller-supplied + // uppercase color doesn't get rejected server-side. + color: data.color.toLowerCase(), }, }, undefined, diff --git a/packages/core/src/schemas/highlight.ts b/packages/core/src/schemas/highlight.ts index a935b80c..298cc401 100644 --- a/packages/core/src/schemas/highlight.ts +++ b/packages/core/src/schemas/highlight.ts @@ -27,6 +27,16 @@ export const HighlightCollectionWireSchema = z.object({ export type HighlightCollectionWire = z.infer; +/** + * Wire format for `GET /v1/highlights/recent-colors`, a collection of hex color + * strings (recently used followed by default colors). + */ +export const RecentHighlightColorsWireSchema = z.object({ + data: z.array(z.object({ color: z.string().regex(HEX_COLOR_REGEX) })), +}); + +export type RecentHighlightColorsWire = z.infer; + const _HighlightSchema = z.object({ /** Bible version identifier (sent to / received from the API as `bible_id`) */ version_id: z.number().int().positive(), diff --git a/packages/hooks/src/useHighlights.test.tsx b/packages/hooks/src/useHighlights.test.tsx index 73b1dc64..7e8a456e 100644 --- a/packages/hooks/src/useHighlights.test.tsx +++ b/packages/hooks/src/useHighlights.test.tsx @@ -54,17 +54,20 @@ describe('useHighlights', () => { let mockGetHighlights: Mock; let mockCreateHighlight: Mock; let mockDeleteHighlight: Mock; + let mockGetRecentColors: Mock; beforeEach(() => { mockGetHighlights = vi.fn().mockResolvedValue(mockHighlights); mockCreateHighlight = vi.fn().mockResolvedValue(mockHighlight); mockDeleteHighlight = vi.fn().mockResolvedValue(undefined); + mockGetRecentColors = vi.fn().mockResolvedValue(['fffe00', '5dff79']); (HighlightsClient as unknown as ReturnType).mockImplementation(function () { return { getHighlights: mockGetHighlights, createHighlight: mockCreateHighlight, deleteHighlight: mockDeleteHighlight, + getRecentColors: mockGetRecentColors, }; }); @@ -311,4 +314,18 @@ describe('useHighlights', () => { expect(mockGetHighlights).toHaveBeenCalledTimes(1); }); }); + + describe('getRecentColors', () => { + it('delegates to the client and returns the color list', async () => { + const wrapper = createYVWrapper(); + const { result } = renderHook(() => useHighlights(defaultOptions), { wrapper }); + + await waitFor(() => { + expect(result.current.loading).toBe(false); + }); + + await expect(result.current.getRecentColors()).resolves.toEqual(['fffe00', '5dff79']); + expect(mockGetRecentColors).toHaveBeenCalledTimes(1); + }); + }); }); diff --git a/packages/hooks/src/useHighlights.ts b/packages/hooks/src/useHighlights.ts index d41af847..7337d0db 100644 --- a/packages/hooks/src/useHighlights.ts +++ b/packages/hooks/src/useHighlights.ts @@ -23,6 +23,7 @@ export function useHighlights( refetch: () => void; createHighlight: (data: CreateHighlight) => Promise; deleteHighlight: (passageId: string, deleteOptions: DeleteHighlightOptions) => Promise; + getRecentColors: () => Promise; } { const context = useContext(YouVersionContext); @@ -67,6 +68,11 @@ export function useHighlights( [highlightsClient, refetch], ); + const getRecentColors = useCallback( + (): Promise => highlightsClient.getRecentColors(), + [highlightsClient], + ); + return { highlights: data, loading, @@ -74,5 +80,6 @@ export function useHighlights( refetch, createHighlight, deleteHighlight, + getRecentColors, }; } From 36fe0e84fcb93f3296c704b9b6b1d261a8f07d36 Mon Sep 17 00:00:00 2001 From: Dustin Kelley Date: Wed, 8 Jul 2026 13:47:50 -0500 Subject: [PATCH 04/13] docs(core): update HighlightsClient documentation for OAuth permissions --- packages/core/src/highlights.ts | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/packages/core/src/highlights.ts b/packages/core/src/highlights.ts index 67262268..891f20de 100644 --- a/packages/core/src/highlights.ts +++ b/packages/core/src/highlights.ts @@ -30,9 +30,12 @@ export type DeleteHighlightOptions = { /** * Client for interacting with Highlights API endpoints. - * Note: All endpoints require OAuth authentication with appropriate scopes - * (`read_highlights` / `write_highlights`), passed as an - * `Authorization: Bearer ` header. + * Note: All endpoints require an OAuth access token passed as an + * `Authorization: Bearer ` header, and the user must have granted the + * `highlights` data-exchange permission at sign-in. That permission is + * requested via the `requested_permissions[]` authorize param (see + * `SignInWithYouVersionPermission`) — it is NOT an OIDC scope and never appears + * in the token's `scope`. A token without it yields 403/404 from these routes. */ export class HighlightsClient { private client: ApiClient; @@ -125,7 +128,7 @@ export class HighlightsClient { /** * Fetches a collection of highlights for a user. * The response will return a color per verse without ranges. - * Requires OAuth with read_highlights scope. + * Requires the `highlights` permission (see class note). * @param options Query parameters scoping the fetch. `version_id` and `passage_id` * (verse or chapter USFM, e.g. "JHN.3") are both required by the API. * @param lat Optional long access token. If not provided, retrieves from YouVersionPlatformConfiguration. @@ -162,7 +165,7 @@ export class HighlightsClient { /** * Fetches the user's recently used highlight colors, followed by the default * colors, as hex strings (no `#`). Useful for building a color picker. - * Requires OAuth with read_highlights scope. + * Requires the `highlights` permission (see class note). * @param lat Optional long access token. If not provided, retrieves from YouVersionPlatformConfiguration. * @returns An ordered list of hex color strings. */ @@ -189,7 +192,7 @@ export class HighlightsClient { /** * Creates or updates a highlight on a passage. * Verse ranges may be used in the passage_id attribute. - * Requires OAuth with write_highlights scope. + * Requires the `highlights` permission (see class note). * @param data The highlight data to create or update. * @param lat Optional long access token. If not provided, retrieves from YouVersionPlatformConfiguration. * @returns The created or updated Highlight object. @@ -225,7 +228,7 @@ export class HighlightsClient { /** * Clears highlights for a passage. - * Requires OAuth with write_highlights scope. + * Requires the `highlights` permission (see class note). * @param passageId The passage identifier (USFM format, e.g., "MAT.1.1" or "MAT.1.1-5"). * @param options Query parameters; `version_id` is required by the API (sent as `bible_id`). * @param lat Optional long access token. If not provided, retrieves from YouVersionPlatformConfiguration. From af72465e62d0d419da5db075c66e5524dd933df6 Mon Sep 17 00:00:00 2001 From: Dustin Kelley Date: Wed, 8 Jul 2026 14:59:28 -0500 Subject: [PATCH 05/13] feat(ui): integrate highlights functionality into BibleReader and Navbar --- examples/vite-react/src/components/navbar.tsx | 7 +- packages/hooks/src/context/index.ts | 1 + packages/ui/src/components/bible-reader.tsx | 77 ++++-------- packages/ui/src/lib/use-reader-highlights.ts | 118 ++++++++++++++++++ 4 files changed, 149 insertions(+), 54 deletions(-) create mode 100644 packages/ui/src/lib/use-reader-highlights.ts diff --git a/examples/vite-react/src/components/navbar.tsx b/examples/vite-react/src/components/navbar.tsx index a9c9ecb6..71a3f6c2 100644 --- a/examples/vite-react/src/components/navbar.tsx +++ b/examples/vite-react/src/components/navbar.tsx @@ -3,7 +3,11 @@ import { Menu } from 'lucide-react'; import { Button } from '@/components/ui/button'; import { Sheet, SheetContent, SheetHeader, SheetTitle, SheetTrigger } from '@/components/ui/sheet'; import { ModeToggle } from '@/components/mode-toggle'; -import { useYVAuth, YouVersionAuthButton } from '@youversion/platform-react-ui'; +import { + SignInWithYouVersionPermission, + useYVAuth, + YouVersionAuthButton, +} from '@youversion/platform-react-ui'; import type { Page } from '@/App'; const navItems: { label: string; page: Page }[] = [ @@ -88,6 +92,7 @@ export function Navbar({ currentPage, onNavigate }: NavbarProps) { size="short" onAuthError={(err) => console.error('Auth error:', err)} scopes={['profile', 'email']} + permissions={[SignInWithYouVersionPermission.highlights]} /> )} diff --git a/packages/hooks/src/context/index.ts b/packages/hooks/src/context/index.ts index e2183601..ddb69413 100644 --- a/packages/hooks/src/context/index.ts +++ b/packages/hooks/src/context/index.ts @@ -1,2 +1,3 @@ export * from './YouVersionContext'; export * from './YouVersionProvider'; +export * from './YouVersionAuthContext'; diff --git a/packages/ui/src/components/bible-reader.tsx b/packages/ui/src/components/bible-reader.tsx index 99ee9e07..0e219cb7 100644 --- a/packages/ui/src/components/bible-reader.tsx +++ b/packages/ui/src/components/bible-reader.tsx @@ -2,11 +2,16 @@ import i18n from '@/i18n'; import { useDelayedLoading } from '@/lib/use-delayed-loading'; +import { useReaderHighlights } from '@/lib/use-reader-highlights'; import { cn } from '@/lib/utils'; import { INTER_FONT, SOURCE_SERIF_FONT, type FontFamily } from '@/lib/verse-html-utils'; import { useControllableState } from '@radix-ui/react-use-controllable-state'; import type { BibleBook } from '@youversion/platform-core'; -import { DEFAULT_LICENSE_FREE_BIBLE_VERSION, getAdjacentChapter } from '@youversion/platform-core'; +import { + DEFAULT_LICENSE_FREE_BIBLE_VERSION, + getAdjacentChapter, + SignInWithYouVersionPermission, +} from '@youversion/platform-core'; import { useBooks, usePassage, @@ -503,40 +508,22 @@ function Content() { }, [book, chapter]); // ---- Verse selection + highlights ------------------------------------------ - // Selection is ephemeral; highlights persist to localStorage only for now - // (ADR-001) in the future `highlight` API shape: keyed by full passage_id USFM - // and scoped by versionId/bible_id (ADR-002). The reader DOM ref lets us anchor - // the popover and pull clean verse text for Copy / Share. + // Selection is ephemeral. Highlights sync via the Highlights API for signed-in + // readers (chapter-scoped, keyed by full passage_id USFM); when not signed in + // they are ephemeral session state (never persisted) — see useReaderHighlights. + // The reader DOM ref lets us anchor the popover and pull clean verse text for + // Copy / Share. const readerRef = useRef(null); const [selectedVerses, setSelectedVerses] = useState([]); const [popoverOpen, setPopoverOpen] = useState(false); const [anchorElement, setAnchorElement] = useState(null); const lastSelectionRef = useRef([]); - const [highlightStore, setHighlightStore] = useState>({}); - - const highlightsStorageKey = `youversion-platform:highlights:${versionId}`; - - // Clear the store synchronously (during render) the moment the version key - // changes. The load effect below runs *after* paint, so without this the - // previous version's highlights would paint over the new text for one frame — - // their `${book}.${chapter}.N` keys collide with the new version's verses. - const [loadedHighlightsKey, setLoadedHighlightsKey] = useState(highlightsStorageKey); - if (loadedHighlightsKey !== highlightsStorageKey) { - setLoadedHighlightsKey(highlightsStorageKey); - setHighlightStore({}); - } - // Load this version's highlights when the version changes (client-only). - useEffect(() => { - let data: Record = {}; - try { - const raw = localStorage.getItem(highlightsStorageKey); - if (raw) data = JSON.parse(raw) as Record; - } catch { - // Ignore (unavailable or malformed storage). - } - setHighlightStore(data); - }, [highlightsStorageKey]); + const { highlightsByPassageId, applyHighlight, removeHighlight } = useReaderHighlights({ + versionId, + book, + chapter, + }); // Navigating away (book/chapter/version) drops the selection — those verses no // longer exist on screen (ADR-007). @@ -551,13 +538,13 @@ function Content() { const chapterPrefix = `${book}.${chapter}.`; const highlightedVerses = useMemo(() => { const map: Record = {}; - for (const [passageId, color] of Object.entries(highlightStore)) { + for (const [passageId, color] of Object.entries(highlightsByPassageId)) { if (!passageId.startsWith(chapterPrefix)) continue; const verseNum = parseInt(passageId.slice(chapterPrefix.length), 10); if (verseNum) map[verseNum] = color; } return map; - }, [highlightStore, chapterPrefix]); + }, [highlightsByPassageId, chapterPrefix]); // Distinct colors present in the current selection → drives the X (remove) circles. const activeHighlights = useMemo( @@ -570,14 +557,6 @@ function Content() { [selectedVerses, highlightedVerses], ); - function persistHighlights(next: Record) { - try { - localStorage.setItem(highlightsStorageKey, JSON.stringify(next)); - } catch { - // Ignore (private mode / quota exceeded). - } - } - function closeAndClearSelection() { setPopoverOpen(false); setSelectedVerses([]); @@ -607,23 +586,12 @@ function Content() { } function handleHighlight(color: string) { - const next = { ...highlightStore }; - for (const verse of selectedVerses) { - next[`${book}.${chapter}.${verse}`] = color; - } - setHighlightStore(next); - persistHighlights(next); + applyHighlight(selectedVerses, color); closeAndClearSelection(); } function handleClearHighlight(color: string) { - const next = { ...highlightStore }; - for (const verse of selectedVerses) { - const passageId = `${book}.${chapter}.${verse}`; - if (next[passageId] === color) delete next[passageId]; - } - setHighlightStore(next); - persistHighlights(next); + removeHighlight(selectedVerses, color); // Multiple colors active → keep open so the user can remove others (AC 8a); // last color removed → dismiss (AC 8). @@ -844,7 +812,10 @@ function UserMenu() { if (onSignInPress) { onSignInPress(); } else { - void signIn({ scopes: ['profile'] }); + void signIn({ + scopes: ['profile'], + permissions: [SignInWithYouVersionPermission.highlights], + }); } }; const handleSignOut = () => { diff --git a/packages/ui/src/lib/use-reader-highlights.ts b/packages/ui/src/lib/use-reader-highlights.ts new file mode 100644 index 00000000..5a441549 --- /dev/null +++ b/packages/ui/src/lib/use-reader-highlights.ts @@ -0,0 +1,118 @@ +'use client'; + +import { useHighlights, YouVersionAuthContext } from '@youversion/platform-react-hooks'; +import { useCallback, useContext, useEffect, useState } from 'react'; + +type UseReaderHighlightsArgs = { + versionId: number; + book: string; + chapter: string; +}; + +type UseReaderHighlightsResult = { + /** Full passage_id (e.g. "JHN.3.16") -> 6-char lowercase hex, no `#`. */ + highlightsByPassageId: Record; + applyHighlight: (verses: number[], color: string) => void; + removeHighlight: (verses: number[], color: string) => void; +}; + +/** + * Encapsulates the reader's highlight state. When the user is signed in, + * highlights load from and persist to the Highlights API (chapter-scoped); when + * not signed in they are ephemeral session state (never persisted). + * + * Reads auth via the raw `YouVersionAuthContext` (null when no auth provider is + * mounted) rather than `useYVAuth()`, which throws — the reader must keep + * working for consumers who don't enable auth. + * + * Failure UX is intentionally out of scope: mutations apply optimistically and + * log on error (no revert/toast). The subsequent refetch inside `useHighlights` + * reconciles the store against the server (source of truth). + */ +export function useReaderHighlights({ + versionId, + book, + chapter, +}: UseReaderHighlightsArgs): UseReaderHighlightsResult { + const authContext = useContext(YouVersionAuthContext); + const isSignedIn = !!authContext?.userInfo; + + const chapterPassageId = `${book}.${chapter}`; + const chapterPrefix = `${chapterPassageId}.`; + + const { highlights, createHighlight, deleteHighlight } = useHighlights( + { version_id: versionId, passage_id: chapterPassageId }, + { enabled: isSignedIn }, + ); + + // Optimistic, client-side view of the current scope's highlights. + const [store, setStore] = useState>({}); + + // Clear synchronously (during render) the moment the scope key changes, so the + // previous version's colors for a shared passage_id (e.g. "JHN.3.16") can't + // paint over the new scope for one frame before the fetch resolves. + const scopeKey = `${versionId}:${chapterPassageId}`; + const [loadedScopeKey, setLoadedScopeKey] = useState(scopeKey); + if (loadedScopeKey !== scopeKey) { + setLoadedScopeKey(scopeKey); + setStore({}); + } + + // Server is source of truth when signed in. Rebuild the store from the fetched + // collection, filtered to the current scope so a stale collection (mid-refetch + // after a scope change) can never leak another version's/chapter's entries. + useEffect(() => { + if (!isSignedIn || !highlights) return; + const next: Record = {}; + for (const highlight of highlights.data) { + if (highlight.version_id !== versionId) continue; + if (!highlight.passage_id.startsWith(chapterPrefix)) continue; + next[highlight.passage_id] = highlight.color; + } + setStore(next); + }, [isSignedIn, highlights, versionId, chapterPrefix]); + + const applyHighlight = useCallback( + (verses: number[], color: string) => { + setStore((prev) => { + const next = { ...prev }; + for (const verse of verses) next[`${chapterPrefix}${verse}`] = color; + return next; + }); + if (!isSignedIn) return; + for (const verse of verses) { + createHighlight({ + version_id: versionId, + passage_id: `${chapterPrefix}${verse}`, + color, + }).catch(console.error); + } + }, + [isSignedIn, versionId, chapterPrefix, createHighlight], + ); + + const removeHighlight = useCallback( + (verses: number[], color: string) => { + // Compute the affected passages from current state before mutating, so the + // API calls (a side effect) stay out of the state updater — which React + // may invoke twice in StrictMode. + const cleared = verses + .map((verse) => `${chapterPrefix}${verse}`) + .filter((passageId) => store[passageId] === color); + if (cleared.length === 0) return; + + setStore((prev) => { + const next = { ...prev }; + for (const passageId of cleared) delete next[passageId]; + return next; + }); + if (!isSignedIn) return; + for (const passageId of cleared) { + deleteHighlight(passageId, { version_id: versionId }).catch(console.error); + } + }, + [store, isSignedIn, versionId, chapterPrefix, deleteHighlight], + ); + + return { highlightsByPassageId: store, applyHighlight, removeHighlight }; +} From 2ec9c36f1f52aa60e001ba54956937c910c5ae98 Mon Sep 17 00:00:00 2001 From: Dustin Kelley Date: Wed, 8 Jul 2026 15:06:47 -0500 Subject: [PATCH 06/13] feat(ui): integrate Highlights API into BibleReader for signed-in users --- .changeset/wire-highlights-api-to-reader.md | 13 ++ docs/adr/YPE-642-verse-action-popover.md | 34 +++++ .../ui/src/lib/use-reader-highlights.test.tsx | 117 ++++++++++++++++++ 3 files changed, 164 insertions(+) create mode 100644 .changeset/wire-highlights-api-to-reader.md create mode 100644 packages/ui/src/lib/use-reader-highlights.test.tsx diff --git a/.changeset/wire-highlights-api-to-reader.md b/.changeset/wire-highlights-api-to-reader.md new file mode 100644 index 00000000..f717b92d --- /dev/null +++ b/.changeset/wire-highlights-api-to-reader.md @@ -0,0 +1,13 @@ +--- +'@youversion/platform-core': minor +'@youversion/platform-react-hooks': minor +'@youversion/platform-react-ui': minor +--- + +Wire the Highlights API into the Bible reader, replacing the localStorage-only highlight store. + +- `BibleReader` now loads and saves verse highlights via the Highlights API for signed-in readers (chapter-scoped `GET`, optimistic `create`/`delete`), with the server as the source of truth. The previous `localStorage` persistence (`youversion-platform:highlights:`) is removed. +- The reader's sign-in (and the example navbar button) now request the `highlights` data-exchange permission so the token is granted highlights access. +- `@youversion/platform-react-hooks` now exports `YouVersionAuthContext`, enabling null-safe reads of signed-in state without `useYVAuth()` throwing when no auth provider is mounted. + +When not signed in, highlighting is currently ephemeral session state (not persisted); the dedicated not-signed-in / permission flow is a follow-up. diff --git a/docs/adr/YPE-642-verse-action-popover.md b/docs/adr/YPE-642-verse-action-popover.md index ec13acf8..ee8c8dcb 100644 --- a/docs/adr/YPE-642-verse-action-popover.md +++ b/docs/adr/YPE-642-verse-action-popover.md @@ -174,3 +174,37 @@ Selection is always enabled in BibleReader (no opt-out prop for now; YAGNI). (SSR) says prefer `useEffect`. Pre-existing; clean up while here. - #131 leftovers to handle: replace the old verse-selection Storybook story + remove demo highlight CSS; add a changeset. + +## As-built addendum — YPE-1034 (Highlights API wiring) + +**ADR-001 (localStorage only) is superseded.** The follow-up ticket it named +landed: highlights now sync via the Highlights API for signed-in readers, and +the localStorage store is removed entirely. + +- **Persistence:** `bible-reader.tsx` `Content` no longer owns a + localStorage-backed `highlightStore` / `persistHighlights`. The + `youversion-platform:highlights:` key is gone and is never written. + Highlight state now lives in the new `lib/use-reader-highlights.ts` hook. +- **Signed in:** `useReaderHighlights` fetches via `useHighlights` (chapter-scoped + `GET /v1/highlights`), and apply/remove call `createHighlight` / + `deleteHighlight` optimistically per verse. The server is the source of truth — + the store is rebuilt from the fetched collection, filtered to the current + `version_id` + chapter prefix so a stale mid-refetch collection can't leak + another scope's colors (this replaces ADR-001's version-keyed reset guard). +- **Signed out (interim):** highlights are ephemeral session state — applied to + the in-memory store, never persisted, no API calls. This is a placeholder: the + not-signed-in / permission state machine (a later ticket) will replace it with + a pending-highlight → sign-in flow that defers the highlight until the + `highlights` permission is granted. ADR-002's API-shaped model made this swap + mechanical, as intended. +- **Auth read:** `useReaderHighlights` reads sign-in state via the raw + `YouVersionAuthContext` (now exported from `@youversion/platform-react-hooks`) + rather than `useYVAuth()`, which throws when no auth provider is mounted — the + reader must keep working for consumers who don't enable auth. +- **Permission at sign-in:** the reader's `handleSignIn` (and the example navbar + button) request `permissions: ['highlights']` so the token is actually granted + highlights access. This is a data-exchange **permission** + (`requested_permissions[]`), not an OIDC scope — see the corrected + `HighlightsClient` docs. +- **Out of scope (unchanged):** failure/revert UX (optimistic + `console.error` + only) and `getRecentColors` (popover keeps its hardcoded colors). diff --git a/packages/ui/src/lib/use-reader-highlights.test.tsx b/packages/ui/src/lib/use-reader-highlights.test.tsx new file mode 100644 index 00000000..dc7fdb5e --- /dev/null +++ b/packages/ui/src/lib/use-reader-highlights.test.tsx @@ -0,0 +1,117 @@ +import { act, renderHook } from '@testing-library/react'; +import { createElement, type ContextType, type ReactNode } from 'react'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +// Mock only `useHighlights`; keep the real `YouVersionAuthContext` so we can +// drive signed-in state through a Provider. +const { useHighlightsMock, createHighlight, deleteHighlight } = vi.hoisted(() => ({ + useHighlightsMock: vi.fn(), + createHighlight: vi.fn(), + deleteHighlight: vi.fn(), +})); + +vi.mock('@youversion/platform-react-hooks', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, useHighlights: useHighlightsMock }; +}); + +import { YouVersionAuthContext } from '@youversion/platform-react-hooks'; +import { useReaderHighlights } from './use-reader-highlights'; + +type Highlight = { version_id: number; passage_id: string; color: string }; + +const signedIn = { + userInfo: { id: 'user-1' }, + setUserInfo: () => {}, + isLoading: false, + error: null, +} as unknown as ContextType; + +function wrapper({ children }: { children: ReactNode }) { + return createElement(YouVersionAuthContext.Provider, { value: signedIn }, children); +} + +function mockServerHighlights(data: Highlight[]) { + useHighlightsMock.mockReturnValue({ + highlights: { data, next_page_token: null }, + loading: false, + error: null, + refetch: vi.fn(), + createHighlight, + deleteHighlight, + getRecentColors: vi.fn(), + }); +} + +const JHN3 = { versionId: 111, book: 'JHN', chapter: '3' }; + +describe('useReaderHighlights (signed in)', () => { + beforeEach(() => { + vi.clearAllMocks(); + createHighlight.mockResolvedValue({}); + deleteHighlight.mockResolvedValue(undefined); + mockServerHighlights([]); + }); + + it('fetches highlights for the current version + chapter scope', () => { + renderHook(() => useReaderHighlights(JHN3), { wrapper }); + expect(useHighlightsMock).toHaveBeenCalledWith( + { version_id: 111, passage_id: 'JHN.3' }, + { enabled: true }, + ); + }); + + it('builds the map from server data, filtered to the current version + chapter', () => { + mockServerHighlights([ + { version_id: 111, passage_id: 'JHN.3.16', color: 'ffff00' }, + { version_id: 111, passage_id: 'JHN.3.17', color: 'aabbcc' }, + { version_id: 111, passage_id: 'GEN.1.1', color: 'ddeeff' }, // other chapter → excluded + { version_id: 999, passage_id: 'JHN.3.16', color: '000000' }, // other version → excluded + ]); + + const { result } = renderHook(() => useReaderHighlights(JHN3), { wrapper }); + + expect(result.current.highlightsByPassageId).toEqual({ + 'JHN.3.16': 'ffff00', + 'JHN.3.17': 'aabbcc', + }); + }); + + it('applyHighlight posts one createHighlight per verse with USFM passage_id and optimistically paints', () => { + const { result } = renderHook(() => useReaderHighlights(JHN3), { wrapper }); + + act(() => result.current.applyHighlight([16, 17], 'ffff00')); + + expect(createHighlight).toHaveBeenCalledTimes(2); + expect(createHighlight).toHaveBeenCalledWith({ + version_id: 111, + passage_id: 'JHN.3.16', + color: 'ffff00', + }); + expect(createHighlight).toHaveBeenCalledWith({ + version_id: 111, + passage_id: 'JHN.3.17', + color: 'ffff00', + }); + expect(result.current.highlightsByPassageId).toEqual({ + 'JHN.3.16': 'ffff00', + 'JHN.3.17': 'ffff00', + }); + }); + + it('removeHighlight deletes only passages matching the color and clears them from the map', () => { + mockServerHighlights([ + { version_id: 111, passage_id: 'JHN.3.16', color: 'ffff00' }, + { version_id: 111, passage_id: 'JHN.3.17', color: 'aabbcc' }, + ]); + + const { result } = renderHook(() => useReaderHighlights(JHN3), { wrapper }); + + // Remove ffff00 across both verses; only 3.16 matches that color. + act(() => result.current.removeHighlight([16, 17], 'ffff00')); + + expect(deleteHighlight).toHaveBeenCalledTimes(1); + expect(deleteHighlight).toHaveBeenCalledWith('JHN.3.16', { version_id: 111 }); + expect(result.current.highlightsByPassageId).toEqual({ 'JHN.3.17': 'aabbcc' }); + }); +}); From fae796217e3e672ec1e84a52614c40bb4dd68776 Mon Sep 17 00:00:00 2001 From: Dustin Kelley Date: Thu, 9 Jul 2026 09:24:00 -0500 Subject: [PATCH 07/13] refactor(ui): rename state variables in useReaderHighlights for clarity --- packages/ui/src/lib/use-reader-highlights.ts | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/packages/ui/src/lib/use-reader-highlights.ts b/packages/ui/src/lib/use-reader-highlights.ts index 5a441549..5a842b92 100644 --- a/packages/ui/src/lib/use-reader-highlights.ts +++ b/packages/ui/src/lib/use-reader-highlights.ts @@ -46,7 +46,7 @@ export function useReaderHighlights({ ); // Optimistic, client-side view of the current scope's highlights. - const [store, setStore] = useState>({}); + const [optimisticStore, setOptimisticStore] = useState>({}); // Clear synchronously (during render) the moment the scope key changes, so the // previous version's colors for a shared passage_id (e.g. "JHN.3.16") can't @@ -55,7 +55,7 @@ export function useReaderHighlights({ const [loadedScopeKey, setLoadedScopeKey] = useState(scopeKey); if (loadedScopeKey !== scopeKey) { setLoadedScopeKey(scopeKey); - setStore({}); + setOptimisticStore({}); } // Server is source of truth when signed in. Rebuild the store from the fetched @@ -69,12 +69,12 @@ export function useReaderHighlights({ if (!highlight.passage_id.startsWith(chapterPrefix)) continue; next[highlight.passage_id] = highlight.color; } - setStore(next); + setOptimisticStore(next); }, [isSignedIn, highlights, versionId, chapterPrefix]); const applyHighlight = useCallback( (verses: number[], color: string) => { - setStore((prev) => { + setOptimisticStore((prev) => { const next = { ...prev }; for (const verse of verses) next[`${chapterPrefix}${verse}`] = color; return next; @@ -98,10 +98,10 @@ export function useReaderHighlights({ // may invoke twice in StrictMode. const cleared = verses .map((verse) => `${chapterPrefix}${verse}`) - .filter((passageId) => store[passageId] === color); + .filter((passageId) => optimisticStore[passageId] === color); if (cleared.length === 0) return; - setStore((prev) => { + setOptimisticStore((prev) => { const next = { ...prev }; for (const passageId of cleared) delete next[passageId]; return next; @@ -111,8 +111,8 @@ export function useReaderHighlights({ deleteHighlight(passageId, { version_id: versionId }).catch(console.error); } }, - [store, isSignedIn, versionId, chapterPrefix, deleteHighlight], + [optimisticStore, isSignedIn, versionId, chapterPrefix, deleteHighlight], ); - return { highlightsByPassageId: store, applyHighlight, removeHighlight }; + return { highlightsByPassageId: optimisticStore, applyHighlight, removeHighlight }; } From 298484c562a8dad078002173ca36fe810ac868fe Mon Sep 17 00:00:00 2001 From: Dustin Kelley Date: Thu, 9 Jul 2026 09:58:06 -0500 Subject: [PATCH 08/13] fix(ui): clear reader highlights on sign-out and user switch Reset optimistic highlight state when auth identity changes so server-backed highlights are not shown after sign-out or account switch. Co-authored-by: Cursor --- .../ui/src/lib/use-reader-highlights.test.tsx | 55 +++++++++++++++++++ packages/ui/src/lib/use-reader-highlights.ts | 8 ++- 2 files changed, 61 insertions(+), 2 deletions(-) diff --git a/packages/ui/src/lib/use-reader-highlights.test.tsx b/packages/ui/src/lib/use-reader-highlights.test.tsx index dc7fdb5e..b39e2029 100644 --- a/packages/ui/src/lib/use-reader-highlights.test.tsx +++ b/packages/ui/src/lib/use-reader-highlights.test.tsx @@ -115,3 +115,58 @@ describe('useReaderHighlights (signed in)', () => { expect(result.current.highlightsByPassageId).toEqual({ 'JHN.3.17': 'aabbcc' }); }); }); + +describe('useReaderHighlights (auth transitions)', () => { + function authValue(userId: string | null): ContextType { + return { + userInfo: userId ? { id: userId } : null, + setUserInfo: () => {}, + isLoading: false, + error: null, + } as unknown as ContextType; + } + + // Wrapper reads the current auth from a mutable ref so a `rerender` can flip + // the signed-in user (or sign out) without remounting the hook. + function renderWithAuth(initialUserId: string | null) { + let auth = authValue(initialUserId); + const dynamicWrapper = ({ children }: { children: ReactNode }) => + createElement(YouVersionAuthContext.Provider, { value: auth }, children); + const utils = renderHook(() => useReaderHighlights(JHN3), { wrapper: dynamicWrapper }); + const setUser = (userId: string | null) => { + auth = authValue(userId); + utils.rerender(); + }; + return { ...utils, setUser }; + } + + beforeEach(() => { + vi.clearAllMocks(); + createHighlight.mockResolvedValue({}); + deleteHighlight.mockResolvedValue(undefined); + }); + + it('clears the store when the reader signs out (userId -> null)', () => { + mockServerHighlights([{ version_id: 111, passage_id: 'JHN.3.16', color: 'ffff00' }]); + + const { result, setUser } = renderWithAuth('user-1'); + expect(result.current.highlightsByPassageId).toEqual({ 'JHN.3.16': 'ffff00' }); + + act(() => setUser(null)); + + expect(result.current.highlightsByPassageId).toEqual({}); + }); + + it("clears the previous user's highlights when a different user signs in", () => { + mockServerHighlights([{ version_id: 111, passage_id: 'JHN.3.16', color: 'ffff00' }]); + + const { result, setUser } = renderWithAuth('user-1'); + expect(result.current.highlightsByPassageId).toEqual({ 'JHN.3.16': 'ffff00' }); + + // user-2 signs in but the (mocked) fetch hasn't resolved for them yet. + mockServerHighlights([]); + act(() => setUser('user-2')); + + expect(result.current.highlightsByPassageId).toEqual({}); + }); +}); diff --git a/packages/ui/src/lib/use-reader-highlights.ts b/packages/ui/src/lib/use-reader-highlights.ts index 5a842b92..56c94b94 100644 --- a/packages/ui/src/lib/use-reader-highlights.ts +++ b/packages/ui/src/lib/use-reader-highlights.ts @@ -35,6 +35,7 @@ export function useReaderHighlights({ chapter, }: UseReaderHighlightsArgs): UseReaderHighlightsResult { const authContext = useContext(YouVersionAuthContext); + const userId = authContext?.userInfo?.id ?? null; const isSignedIn = !!authContext?.userInfo; const chapterPassageId = `${book}.${chapter}`; @@ -50,8 +51,11 @@ export function useReaderHighlights({ // Clear synchronously (during render) the moment the scope key changes, so the // previous version's colors for a shared passage_id (e.g. "JHN.3.16") can't - // paint over the new scope for one frame before the fetch resolves. - const scopeKey = `${versionId}:${chapterPassageId}`; + // paint over the new scope for one frame before the fetch resolves. The user + // identity is part of the key: signing out (userId -> null) or switching users + // resets the store immediately, so one reader can't keep painting another + // user's server-backed highlights until the next navigation/remount. + const scopeKey = `${userId ?? 'signed-out'}:${versionId}:${chapterPassageId}`; const [loadedScopeKey, setLoadedScopeKey] = useState(scopeKey); if (loadedScopeKey !== scopeKey) { setLoadedScopeKey(scopeKey); From 99cf3c06ccf17f4b03fde312f83b866dc08d4306 Mon Sep 17 00:00:00 2001 From: Dustin Kelley Date: Thu, 9 Jul 2026 09:59:28 -0500 Subject: [PATCH 09/13] fix(ui): normalize highlight colors to lowercase for consistency --- packages/ui/src/lib/use-reader-highlights.test.tsx | 13 +++++++++++++ packages/ui/src/lib/use-reader-highlights.ts | 4 ++-- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/packages/ui/src/lib/use-reader-highlights.test.tsx b/packages/ui/src/lib/use-reader-highlights.test.tsx index b39e2029..67c2905c 100644 --- a/packages/ui/src/lib/use-reader-highlights.test.tsx +++ b/packages/ui/src/lib/use-reader-highlights.test.tsx @@ -77,6 +77,19 @@ describe('useReaderHighlights (signed in)', () => { }); }); + it('normalizes server highlight colors to lowercase so removeHighlight matches popover palette', () => { + mockServerHighlights([{ version_id: 111, passage_id: 'JHN.3.16', color: 'FFFF00' }]); + + const { result } = renderHook(() => useReaderHighlights(JHN3), { wrapper }); + expect(result.current.highlightsByPassageId).toEqual({ 'JHN.3.16': 'ffff00' }); + + // Popover emits lowercase palette values; removal must match the normalized store. + act(() => result.current.removeHighlight([16], 'ffff00')); + + expect(deleteHighlight).toHaveBeenCalledWith('JHN.3.16', { version_id: 111 }); + expect(result.current.highlightsByPassageId).toEqual({}); + }); + it('applyHighlight posts one createHighlight per verse with USFM passage_id and optimistically paints', () => { const { result } = renderHook(() => useReaderHighlights(JHN3), { wrapper }); diff --git a/packages/ui/src/lib/use-reader-highlights.ts b/packages/ui/src/lib/use-reader-highlights.ts index 56c94b94..39816e7e 100644 --- a/packages/ui/src/lib/use-reader-highlights.ts +++ b/packages/ui/src/lib/use-reader-highlights.ts @@ -35,7 +35,7 @@ export function useReaderHighlights({ chapter, }: UseReaderHighlightsArgs): UseReaderHighlightsResult { const authContext = useContext(YouVersionAuthContext); - const userId = authContext?.userInfo?.id ?? null; + const userId = authContext?.userInfo?.userId ?? null; const isSignedIn = !!authContext?.userInfo; const chapterPassageId = `${book}.${chapter}`; @@ -71,7 +71,7 @@ export function useReaderHighlights({ for (const highlight of highlights.data) { if (highlight.version_id !== versionId) continue; if (!highlight.passage_id.startsWith(chapterPrefix)) continue; - next[highlight.passage_id] = highlight.color; + next[highlight.passage_id] = highlight.color.toLowerCase(); } setOptimisticStore(next); }, [isSignedIn, highlights, versionId, chapterPrefix]); From 586b78b508e74a205848f7b6cdf599cbb8dea63e Mon Sep 17 00:00:00 2001 From: Dustin Kelley Date: Thu, 9 Jul 2026 10:09:15 -0500 Subject: [PATCH 10/13] test(ui): use userId in useReaderHighlights auth mock to match production shape The test mocked auth `userInfo` with an `id` field, but the hook reads `userInfo.userId` (the production shape). This made the scope key never change on sign-out, so the "clears the store when the reader signs out" test failed. Align the mock with production so the scope key resets. Co-authored-by: Cursor --- packages/ui/src/lib/use-reader-highlights.test.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/ui/src/lib/use-reader-highlights.test.tsx b/packages/ui/src/lib/use-reader-highlights.test.tsx index 67c2905c..dcdd8f43 100644 --- a/packages/ui/src/lib/use-reader-highlights.test.tsx +++ b/packages/ui/src/lib/use-reader-highlights.test.tsx @@ -21,7 +21,7 @@ import { useReaderHighlights } from './use-reader-highlights'; type Highlight = { version_id: number; passage_id: string; color: string }; const signedIn = { - userInfo: { id: 'user-1' }, + userInfo: { userId: 'user-1' }, setUserInfo: () => {}, isLoading: false, error: null, @@ -132,7 +132,7 @@ describe('useReaderHighlights (signed in)', () => { describe('useReaderHighlights (auth transitions)', () => { function authValue(userId: string | null): ContextType { return { - userInfo: userId ? { id: userId } : null, + userInfo: userId ? { userId } : null, setUserInfo: () => {}, isLoading: false, error: null, From 7860b4490e24917ae4150bb3479d7664fd8bad35 Mon Sep 17 00:00:00 2001 From: Dustin Kelley Date: Thu, 9 Jul 2026 11:23:31 -0500 Subject: [PATCH 11/13] fix(ui): ensure optimistic store sync on highlight mutation failures --- packages/ui/src/lib/use-reader-highlights.ts | 26 ++++++++++++++------ 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/packages/ui/src/lib/use-reader-highlights.ts b/packages/ui/src/lib/use-reader-highlights.ts index 39816e7e..d7f49435 100644 --- a/packages/ui/src/lib/use-reader-highlights.ts +++ b/packages/ui/src/lib/use-reader-highlights.ts @@ -26,8 +26,10 @@ type UseReaderHighlightsResult = { * working for consumers who don't enable auth. * * Failure UX is intentionally out of scope: mutations apply optimistically and - * log on error (no revert/toast). The subsequent refetch inside `useHighlights` - * reconciles the store against the server (source of truth). + * log on error (no revert/toast). Reconciliation against the server (source of + * truth) happens via refetch — `useHighlights` refetches on a successful + * mutation, and on failure we refetch here so the optimistic store can't stay + * permanently out of sync until the next navigation. */ export function useReaderHighlights({ versionId, @@ -41,7 +43,7 @@ export function useReaderHighlights({ const chapterPassageId = `${book}.${chapter}`; const chapterPrefix = `${chapterPassageId}.`; - const { highlights, createHighlight, deleteHighlight } = useHighlights( + const { highlights, createHighlight, deleteHighlight, refetch } = useHighlights( { version_id: versionId, passage_id: chapterPassageId }, { enabled: isSignedIn }, ); @@ -89,10 +91,15 @@ export function useReaderHighlights({ version_id: versionId, passage_id: `${chapterPrefix}${verse}`, color, - }).catch(console.error); + }).catch((error) => { + console.error(error); + // Success already refetches inside `useHighlights`; on failure we + // reconcile the optimistic store against the server here. + refetch(); + }); } }, - [isSignedIn, versionId, chapterPrefix, createHighlight], + [isSignedIn, versionId, chapterPrefix, createHighlight, refetch], ); const removeHighlight = useCallback( @@ -112,10 +119,15 @@ export function useReaderHighlights({ }); if (!isSignedIn) return; for (const passageId of cleared) { - deleteHighlight(passageId, { version_id: versionId }).catch(console.error); + deleteHighlight(passageId, { version_id: versionId }).catch((error) => { + console.error(error); + // Success already refetches inside `useHighlights`; on failure we + // reconcile the optimistic store against the server here. + refetch(); + }); } }, - [optimisticStore, isSignedIn, versionId, chapterPrefix, deleteHighlight], + [optimisticStore, isSignedIn, versionId, chapterPrefix, deleteHighlight, refetch], ); return { highlightsByPassageId: optimisticStore, applyHighlight, removeHighlight }; From 67870989da88229ed2a9ca4137813d0b99de04f7 Mon Sep 17 00:00:00 2001 From: Dustin Kelley Date: Thu, 9 Jul 2026 11:25:18 -0500 Subject: [PATCH 12/13] test(ui): add tests for useReaderHighlights behavior when signed out --- .../ui/src/lib/use-reader-highlights.test.tsx | 39 +++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/packages/ui/src/lib/use-reader-highlights.test.tsx b/packages/ui/src/lib/use-reader-highlights.test.tsx index dcdd8f43..5cdd1f9f 100644 --- a/packages/ui/src/lib/use-reader-highlights.test.tsx +++ b/packages/ui/src/lib/use-reader-highlights.test.tsx @@ -183,3 +183,42 @@ describe('useReaderHighlights (auth transitions)', () => { expect(result.current.highlightsByPassageId).toEqual({}); }); }); + +describe('useReaderHighlights (signed out)', () => { + // No wrapper: `useContext(YouVersionAuthContext)` is null, so `isSignedIn` is + // false and highlights stay ephemeral (never persisted to the API). + beforeEach(() => { + vi.clearAllMocks(); + mockServerHighlights([]); + }); + + it('disables the fetch when there is no signed-in user', () => { + renderHook(() => useReaderHighlights(JHN3)); + expect(useHighlightsMock).toHaveBeenCalledWith( + { version_id: 111, passage_id: 'JHN.3' }, + { enabled: false }, + ); + }); + + it('applyHighlight paints ephemerally without calling createHighlight', () => { + const { result } = renderHook(() => useReaderHighlights(JHN3)); + + act(() => result.current.applyHighlight([16, 17], 'ffff00')); + + expect(createHighlight).not.toHaveBeenCalled(); + expect(result.current.highlightsByPassageId).toEqual({ + 'JHN.3.16': 'ffff00', + 'JHN.3.17': 'ffff00', + }); + }); + + it('removeHighlight clears ephemerally without calling deleteHighlight', () => { + const { result } = renderHook(() => useReaderHighlights(JHN3)); + + act(() => result.current.applyHighlight([16], 'ffff00')); + act(() => result.current.removeHighlight([16], 'ffff00')); + + expect(deleteHighlight).not.toHaveBeenCalled(); + expect(result.current.highlightsByPassageId).toEqual({}); + }); +}); From 131f1cf28b8ad40b1a4fd61d61d6be5dd538f353 Mon Sep 17 00:00:00 2001 From: Dustin Kelley Date: Thu, 9 Jul 2026 12:24:00 -0500 Subject: [PATCH 13/13] fix(ui): user1 highlights from rendering on user2 sign in --- .../ui/src/lib/use-reader-highlights.test.tsx | 18 ++++++++++++++++-- packages/ui/src/lib/use-reader-highlights.ts | 5 ++++- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/packages/ui/src/lib/use-reader-highlights.test.tsx b/packages/ui/src/lib/use-reader-highlights.test.tsx index 5cdd1f9f..32129966 100644 --- a/packages/ui/src/lib/use-reader-highlights.test.tsx +++ b/packages/ui/src/lib/use-reader-highlights.test.tsx @@ -176,12 +176,26 @@ describe('useReaderHighlights (auth transitions)', () => { const { result, setUser } = renderWithAuth('user-1'); expect(result.current.highlightsByPassageId).toEqual({ 'JHN.3.16': 'ffff00' }); - // user-2 signs in but the (mocked) fetch hasn't resolved for them yet. - mockServerHighlights([]); + // user-2 signs in but the fetch hasn't refetched for them: `useHighlights` + // still returns user-1's collection (same reference). It must NOT be painted. act(() => setUser('user-2')); expect(result.current.highlightsByPassageId).toEqual({}); }); + + it("adopts the new user's highlights once their fetch resolves", () => { + mockServerHighlights([{ version_id: 111, passage_id: 'JHN.3.16', color: 'ffff00' }]); + + const { result, setUser } = renderWithAuth('user-1'); + act(() => setUser('user-2')); + expect(result.current.highlightsByPassageId).toEqual({}); + + // Fresh collection for user-2 (new reference) is trusted and painted. + mockServerHighlights([{ version_id: 111, passage_id: 'JHN.3.19', color: 'aabbcc' }]); + act(() => setUser('user-2')); + + expect(result.current.highlightsByPassageId).toEqual({ 'JHN.3.19': 'aabbcc' }); + }); }); describe('useReaderHighlights (signed out)', () => { diff --git a/packages/ui/src/lib/use-reader-highlights.ts b/packages/ui/src/lib/use-reader-highlights.ts index d7f49435..a1dd2792 100644 --- a/packages/ui/src/lib/use-reader-highlights.ts +++ b/packages/ui/src/lib/use-reader-highlights.ts @@ -59,9 +59,11 @@ export function useReaderHighlights({ // user's server-backed highlights until the next navigation/remount. const scopeKey = `${userId ?? 'signed-out'}:${versionId}:${chapterPassageId}`; const [loadedScopeKey, setLoadedScopeKey] = useState(scopeKey); + const [staleHighlights, setStaleHighlights] = useState(null); if (loadedScopeKey !== scopeKey) { setLoadedScopeKey(scopeKey); setOptimisticStore({}); + setStaleHighlights(highlights); } // Server is source of truth when signed in. Rebuild the store from the fetched @@ -69,6 +71,7 @@ export function useReaderHighlights({ // after a scope change) can never leak another version's/chapter's entries. useEffect(() => { if (!isSignedIn || !highlights) return; + if (highlights === staleHighlights) return; const next: Record = {}; for (const highlight of highlights.data) { if (highlight.version_id !== versionId) continue; @@ -76,7 +79,7 @@ export function useReaderHighlights({ next[highlight.passage_id] = highlight.color.toLowerCase(); } setOptimisticStore(next); - }, [isSignedIn, highlights, versionId, chapterPrefix]); + }, [isSignedIn, highlights, staleHighlights, versionId, chapterPrefix]); const applyHighlight = useCallback( (verses: number[], color: string) => {