fix(highlights): staging-test fixes — dialog button, overlay reconciliation, per-verse delete (YPE-1034, PR 4)

.changeset/fix-highlights-jam-issues.md

@@ -0,0 +1,15 @@
+---
+'@youversion/platform-core': patch
+'@youversion/platform-react-hooks': patch
+'@youversion/platform-react-ui': patch
+---
+
+Fix four highlights-stack bugs surfaced by a staging session, plus a fill fade-in.
+
+- **Invisible primary button (ui):** the `default` button variant paired `bg-background` with `text-primary-foreground`, resolving to white-on-white in the light theme (the highlight permission dialog's Continue button was invisible). It now uses the standard `bg-primary` / `text-primary-foreground` pairing. `YouVersionAuthButton` pins `bg-background` explicitly so its neutral brand surface is unchanged.
+- **Optimistic overlay dropped before the write is visible (ui):** after a successful highlight write the seam hook dropped the optimistic overlay as soon as ANY refetch landed, trusting it as server truth. Under read-after-write lag (staging slowness, prod read replicas) that GET often did not yet reflect the write, so a highlight flickered out and back on apply, or a removed highlight reappeared. The overlay is now retired only once a fetch actually reflects the write (apply → the verse shows the written color; remove → the verse no longer shows the removed color); until then the overlay wins. Reset paths (chapter/version change, sign-out) release any write the server never converges on.
+- **Refetch coalescing (hooks behavior change):** `useHighlights.createHighlight` / `deleteHighlight` no longer refetch on each call. The sole consumer (`useBibleReaderHighlights`) now issues a single refetch after each batch settles — success or failure — so highlighting verses `[2,3,5]` fires two POSTs but only one GET (previously two). Batches with partial failures settle per sub-write: succeeded writes reconcile to server truth, only the failed writes' verses revert.
+- **DELETE by range is unsupported (core + ui):** range passage-ids (e.g. `JHN.1.2-3`) returned a non-2xx from staging and threw. The remove path now sends one DELETE per verse (`JHN.1.2`, `JHN.1.3`); POST/apply still collapses to ranges, which works. The `HighlightsClient.deleteHighlight` docstring no longer claims range delete is supported (marked unverified pending API-team confirmation). Large removals issue more DELETEs but still coalesce to a single refetch.
+- **Highlight fade-in (core styles):** verse highlight fills now transition their background color (~250ms, disabled under `prefers-reduced-motion`) instead of popping in, matching the Bible app. The selection underline and layout are untouched.
+
+Known/accepted trade-off (pending product sign-off): the optimistic overlay holds the locally-written value until a fetch reflects that write. A concurrent change from another device to the same verse therefore renders stale until navigation or the next write on this client. This is the deliberate cost of eliminating the flicker/ghost bugs above; documented in code comments in `useBibleReaderHighlights`.

packages/core/src/highlights.ts

@@ -230,7 +230,13 @@ export class HighlightsClient {
   /**
    * 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").
+   *
+   * UNVERIFIED / likely unsupported: passing a verse RANGE (e.g. "MAT.1.1-5")
+   * returned a non-2xx from staging (observed with `JHN.1.2-3`), even though the
+   * API stores highlights per verse and POST accepts ranges. Until the API team
+   * confirms range delete, callers should send one DELETE per verse passage-id
+   * (e.g. "MAT.1.1"). See YPE-1034.
+   * @param passageId The passage identifier (single-verse USFM, e.g., "MAT.1.1").
    * @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).

packages/core/src/styles/bible-reader.css

@@ -123,6 +123,20 @@
     display: inline;
   }
 
+  /* Fade highlight fills in/out instead of popping. The fill is painted as an
+     inline `background-color` on `.yv-v[v]` (see verse.tsx); transitioning only
+     that property animates apply/remove without touching the selection
+     underline (text-decoration) and without any layout shift. */
+  & .yv-v {
+    transition: background-color 250ms ease;
+  }
+
+  @media (prefers-reduced-motion: reduce) {
+    & .yv-v {
+      transition: none;
+    }
+  }
+
   /* Only show pointer cursor when verses are selectable */
   &[data-selectable='true'] .yv-v,
   &[data-selectable='true'] .verse {

packages/hooks/src/useHighlights.test.tsx

@@ -221,13 +221,15 @@ describe('useHighlights', () => {
   });
 
   describe('createHighlight mutation', () => {
-    it('should create highlight and refetch', async () => {
+    it('should create highlight WITHOUT auto-refetching (callers coalesce refetches)', async () => {
       const wrapper = createYVWrapper();
       const { result } = renderHook(() => useHighlights(defaultOptions), { wrapper });
 
       await waitFor(() => {
         expect(result.current.loading).toBe(false);
       });
+      // The mount fetch.
+      expect(mockGetHighlights).toHaveBeenCalledTimes(1);
 
       const createData: CreateHighlight = {
         version_id: 111,
@@ -244,6 +246,13 @@ describe('useHighlights', () => {
       const created = await createPromise;
       expect(created).toEqual(mockHighlight);
 
+      // No implicit GET after the write — the mount fetch is still the only one.
+      // A batching caller issues one refetch() after the whole batch settles.
+      await Promise.resolve();
+      expect(mockGetHighlights).toHaveBeenCalledTimes(1);
+
+      // An explicit refetch still works and issues exactly one GET.
+      result.current.refetch();
       await waitFor(() => {
         expect(mockGetHighlights).toHaveBeenCalledTimes(2);
       });
@@ -275,13 +284,14 @@ describe('useHighlights', () => {
   });
 
   describe('deleteHighlight mutation', () => {
-    it('should delete highlight and refetch', async () => {
+    it('should delete highlight WITHOUT auto-refetching (callers coalesce refetches)', async () => {
       const wrapper = createYVWrapper();
       const { result } = renderHook(() => useHighlights(defaultOptions), { wrapper });
 
       await waitFor(() => {
         expect(result.current.loading).toBe(false);
       });
+      expect(mockGetHighlights).toHaveBeenCalledTimes(1);
 
       const deletePromise = result.current.deleteHighlight('MAT.1.1', { version_id: 111 });
 
@@ -291,9 +301,9 @@ describe('useHighlights', () => {
 
       await deletePromise;
 
-      await waitFor(() => {
-        expect(mockGetHighlights).toHaveBeenCalledTimes(2);
-      });
+      // No implicit GET after the delete.
+      await Promise.resolve();
+      expect(mockGetHighlights).toHaveBeenCalledTimes(1);
     });
 
     it('should handle delete error', async () => {

packages/hooks/src/useHighlights.ts

@@ -55,21 +55,21 @@ export function useHighlights(
     },
   );
 
+  // NOTE: these mutations intentionally do NOT auto-refetch. A single logical
+  // apply/remove can fan out into several writes (one per contiguous run for
+  // apply, one per verse for delete); auto-refetching per call would issue a
+  // GET per write. Callers coalesce instead — issue the batch, then `refetch()`
+  // once after it settles. The seam hook (`useBibleReaderHighlights`) is the
+  // sole consumer and does exactly that.
   const createHighlight = useCallback(
-    async (data: CreateHighlight): Promise<Highlight> => {
-      const result = await highlightsClient.createHighlight(data);
-      refetch();
-      return result;
-    },
-    [highlightsClient, refetch],
+    (data: CreateHighlight): Promise<Highlight> => highlightsClient.createHighlight(data),
+    [highlightsClient],
   );
 
   const deleteHighlight = useCallback(
-    async (passageId: string, deleteOptions: DeleteHighlightOptions): Promise<void> => {
-      await highlightsClient.deleteHighlight(passageId, deleteOptions);
-      refetch();
-    },
-    [highlightsClient, refetch],
+    (passageId: string, deleteOptions: DeleteHighlightOptions): Promise<void> =>
+      highlightsClient.deleteHighlight(passageId, deleteOptions),
+    [highlightsClient],
   );
 
   const getRecentColors = useCallback(

packages/ui/src/components/YouVersionAuthButton.tsx

@@ -198,6 +198,11 @@ export const YouVersionAuthButton = React.forwardRef<HTMLButtonElement, YouVersi
           data-yv-theme={theme}
           className={cn(
             'yv:shadow-none yv:p-3 yv:h-auto yv:w-fit',
+            // The YV brand button is a neutral surface (white in light, dark in
+            // dark) with its own text/logo color set below — pin the background
+            // explicitly so it doesn't inherit the `default` variant's
+            // `bg-primary`, which would render the logo unreadable.
+            'yv:bg-background yv:hover:bg-background/90',
             variant === 'outline' ? 'yv:border' : 'yv:border-none',
             theme === 'light' ? 'yv:text-black' : 'yv:text-white',
             className,
@@ -231,6 +236,9 @@ export const YouVersionAuthButton = React.forwardRef<HTMLButtonElement, YouVersi
         data-yv-theme={theme}
         className={cn(
           'yv:relative yv:shadow-none yv:w-fit',
+          // Pin the neutral brand surface so the button doesn't inherit the
+          // `default` variant's `bg-primary` (see the icon branch above).
+          'yv:bg-background yv:hover:bg-background/90',
           variant === 'outline' ? 'yv:border' : 'yv:border-none',
           theme === 'light' ? 'yv:text-black' : 'yv:text-white',
           className,

packages/ui/src/components/ui/button.test.tsx

@@ -0,0 +1,21 @@
+/**
+ * @vitest-environment jsdom
+ */
+import { render } from '@testing-library/react';
+import { describe, expect, it } from 'vitest';
+import { Button } from './button';
+
+describe('Button — default variant colors', () => {
+  it('paints the primary surface, not background-on-foreground (regression: invisible button)', () => {
+    // The `default` variant must pair `bg-primary` with `text-primary-foreground`.
+    // The previous `bg-background` + `text-primary-foreground` pairing resolved to
+    // white-on-white in the light theme, so the Continue button on the highlight
+    // permission dialog was invisible. Guard against regressing to that pairing.
+    const { getByRole } = render(<Button>Continue</Button>);
+    const className = getByRole('button').className;
+
+    expect(className).toContain('yv:bg-primary');
+    expect(className).toContain('yv:text-primary-foreground');
+    expect(className).not.toContain('yv:bg-background');
+  });
+});

packages/ui/src/components/ui/button.tsx

@@ -9,7 +9,7 @@ const buttonVariants = cva(
   {
     variants: {
       variant: {
-        default: 'yv:bg-background yv:text-primary-foreground yv:hover:bg-background/90',
+        default: 'yv:bg-primary yv:text-primary-foreground yv:hover:bg-primary/90',
         destructive:
           'yv:bg-destructive yv:text-white yv:hover:bg-destructive/90 yv:focus-visible:ring-destructive/20 yv:dark:focus-visible:ring-destructive/40 yv:dark:bg-destructive/60',
         outline: