From a1012edaf1a2eb6c9ca39241fbf002b5a58436ec Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Thu, 8 Oct 2026 00:54:22 -0700 Subject: [PATCH] Inbox mobile S1 follow-up: the review's items A markdown link goes to the shell only for a trusted tap; a file opened at one mark scrolls to it once the marks are painted; 6.2's diff box fits its diff; the agent page's fetch targets the live Inbox (no-cors, so only the CSP refuses it); the pin label comments say "Button"; inbox-e2e runs on apps/hook/package.json. Contract section 5: an HTML page's pins and selections are page-reported, checked by the shell in M2. --- .github/workflows/inbox-e2e.yml | 2 ++ adr/implementation/inbox-mobile.md | 2 ++ packages/core/inbox-surface-bridge.ts | 2 +- packages/inbox/surface/SurfaceApp.tsx | 2 ++ packages/inbox/surface/SurfaceDocument.tsx | 14 ++++++-- packages/inbox/surface/surface.css | 17 ++++++++++ packages/ui/types.ts | 2 +- tests/e2e/surface.spec.ts | 39 +++++++++++++++++++--- 8 files changed, 71 insertions(+), 9 deletions(-) diff --git a/.github/workflows/inbox-e2e.yml b/.github/workflows/inbox-e2e.yml index abb80646b4..245a37fa51 100644 --- a/.github/workflows/inbox-e2e.yml +++ b/.github/workflows/inbox-e2e.yml @@ -39,6 +39,7 @@ on: - 'packages/server/guide/**' - 'packages/core/guide*' - 'apps/hook/server/index.ts' + - 'apps/hook/package.json' - 'apps/hook/server/inbox*' - 'build/**' - 'package.json' @@ -70,6 +71,7 @@ on: - 'packages/server/guide/**' - 'packages/core/guide*' - 'apps/hook/server/index.ts' + - 'apps/hook/package.json' - 'apps/hook/server/inbox*' - 'build/**' - 'package.json' diff --git a/adr/implementation/inbox-mobile.md b/adr/implementation/inbox-mobile.md index 4e1b5ab5c0..e9770330aa 100644 --- a/adr/implementation/inbox-mobile.md +++ b/adr/implementation/inbox-mobile.md @@ -337,6 +337,8 @@ Annotations are Plannotator's `Annotation` (`packages/ui/types.ts`) inside the I **Links in an agent's HTML page are not a bridge message (S1 review, 2026-10-08).** The `link` message's only source is the main frame's own rendered markdown, whose taps the surface sees itself. A link in an agent's page is a navigation the shell decides: the page's script can forge any message the viewer's frame sends, and a tap inside that frame never reaches the main frame (proved in WebKit), so the surface cannot tell a real tap from a forged one. In M2 the shell takes it in `WKNavigationDelegate.decidePolicyFor` with `navigationType == .linkActivated` on the agent's frame, a real-tap signal the page cannot forge, cancels the in-frame load and opens the URL in `SFSafariViewController`. Note for M2: today Plannotator's HTML viewer bridge cancels every non-fragment link click inside the frame (`preventDefault`) and reports it as a frame message, so no navigation reaches the delegate until a ui seam lets the frame navigate. +**Pins and selections from an agent's HTML page are page-reported (S1 follow-up, 2026-10-08).** For the same reason, the surface cannot tell a pin or a text selection the person made in an agent's page from one the page's script forged (the viewer's bridge runs in the page's own realm): a forged one reaches the shell as an ordinary `pin` or `selection`, with a label the page chose. The surface cannot drop them without dropping every pin (4.3). In M2 the shell takes a `pin`, `selection` or `draft` from an HTML attachment only within about a second of its own touch on the web view (a gesture recognizer that observes touches without cancelling them); markdown and diagrams are drawn in the main frame from the surface's own events, so their drafts need no such check. Links in the surface's own markdown go out only for a real tap (`isTrusted`). + `open_section` and `section` were added by S1 (2026-10-08): 6.1 and 6.2 put the guide's title and its close or back button in the shell's bar, and Continue, Previous, Reviewed and Next in the document, so each side has to tell the other where the guide is. A `selection` with both fields null follows only a selection draft; a pin or a part whose draft closes sends nothing. A `pin`'s `label` is the element's name as Plannotator's pinpoint names it ("Button", a heading's words), and `selector` is the draft's `htmlAnchor.selector`. A `draft`'s `label` is the quote's first words for a block and "Pick a host (node E)" for a diagram part; `kind` is `block` for a whole-diagram comment too. The surface is `apps/hook/dist/surface.html`; its CSP is `default-src 'none'` with `connect-src 'none'`, and lets an HTML page's own folder load from `plannotator-asset:` (script, style, image, font, media, frame and `base-uri`), since the page's frame inherits the policy. A draft is an `Annotation` with an empty `text`. The shell fills the person's words into `text`, saves `{ attachment_id, version, annotation }` through the door (7.17), and answers with `commit_annotation`. Edit (4.3) is the same save with the same annotation id and a new key. HTML-asset tokens live in the Inbox's memory, so after the Inbox restarts an asset answers 404; the shell then fetches `view` again. diff --git a/packages/core/inbox-surface-bridge.ts b/packages/core/inbox-surface-bridge.ts index f4570e7e59..365821bd3c 100644 --- a/packages/core/inbox-surface-bridge.ts +++ b/packages/core/inbox-surface-bridge.ts @@ -143,7 +143,7 @@ export interface SurfaceSelection { export interface SurfacePin { v: 1; type: "pin"; - /** `label` is the element as the sheet names it ("div.btnx"); `selector` finds it again. */ + /** `label` is the element as the sheet names it (Plannotator's pinpoint label: "Button", a heading's words); `selector` finds it again. */ target: { label: string; selector: string }; draft: SurfaceAnnotation; } diff --git a/packages/inbox/surface/SurfaceApp.tsx b/packages/inbox/surface/SurfaceApp.tsx index f69dfeef84..21ef1a619a 100644 --- a/packages/inbox/surface/SurfaceApp.tsx +++ b/packages/inbox/surface/SurfaceApp.tsx @@ -115,6 +115,8 @@ function Surface() { // has nowhere to go without a file of its own, so it does nothing. useEffect(() => { const onClick = (event: MouseEvent) => { + // A person's tap only: a scripted click on the document is not one. + if (!event.isTrusted) return; const link = (event.target as Element | null)?.closest?.('a[href]'); if (!link || event.defaultPrevented) return; const href = link.getAttribute('href') ?? ''; diff --git a/packages/inbox/surface/SurfaceDocument.tsx b/packages/inbox/surface/SurfaceDocument.tsx index e2053e9688..e6a8cc1d2e 100644 --- a/packages/inbox/surface/SurfaceDocument.tsx +++ b/packages/inbox/surface/SurfaceDocument.tsx @@ -69,13 +69,21 @@ export const SurfaceDocument = forwardRef(function const blocks = useMemo(() => attachmentBlocks(attachment.kind, open.text, attachment.name), [attachment.kind, attachment.name, open.text]); // Paint the stored highlights once the document is drawn; later ones arrive through commit. + // The shell's "3 annotations" sheet opens a file at one mark (`focus`): + // once the marks are painted, that one is brought to the middle of the screen. const painted = useRef(false); useEffect(() => { if (!isText || painted.current) return; - painted.current = true; - const id = requestAnimationFrame(() => viewerRef.current?.applySharedAnnotations(annotations.filter(annotationOwnsHighlight))); + const id = requestAnimationFrame(() => { + painted.current = true; + viewerRef.current?.applySharedAnnotations(annotations.filter(annotationOwnsHighlight)); + if (open.focus) { + const mark = document.querySelector(`[data-highlight-id="${CSS.escape(open.focus)}"], [data-bind-id="${CSS.escape(open.focus)}"]`); + mark?.scrollIntoView({ block: 'center' }); + } + }); return () => cancelAnimationFrame(id); - }, [isText, annotations]); + }, [isText, annotations, open.focus]); const dropDraft = () => { draft.current?.cancel(); diff --git a/packages/inbox/surface/surface.css b/packages/inbox/surface/surface.css index d189aca2cd..82b52bdde7 100644 --- a/packages/inbox/surface/surface.css +++ b/packages/inbox/surface/surface.css @@ -319,3 +319,20 @@ transition: none; } } + +/* One section per screen: a file's diff box takes the height its diff has, + up to most of the screen, instead of the desktop's line-count estimate, + which leaves a blank band under a short diff at a phone's width. A longer + diff still scrolls inside its box, where the renderer draws what is in + view. */ +.sf-gcard [data-guide-code-view-mounted] { + height: auto !important; +} +.sf-gcard [data-guide-code-view-mounted] > div { + height: auto; +} +.sf-gcard [data-guide-code-view-mounted] > div > div { + height: auto; + max-height: 70vh; + contain: none; +} diff --git a/packages/ui/types.ts b/packages/ui/types.ts index a54e274e99..f6615746c7 100644 --- a/packages/ui/types.ts +++ b/packages/ui/types.ts @@ -537,7 +537,7 @@ export interface HostDraft { * tap, Comment mode, an HTML pin). */ intent: 'selection' | 'compose'; - /** What it is on, as a composer names it ("div.btnx"). Absent for text. */ + /** What it is on, as a composer names it (Plannotator's pinpoint label: "Button", a heading's words). Absent for text. */ label?: string; /** * Drop the draft: its pending highlight or pin goes. The host calls it when diff --git a/tests/e2e/surface.spec.ts b/tests/e2e/surface.spec.ts index 90db6ac84e..008a27f4a9 100644 --- a/tests/e2e/surface.spec.ts +++ b/tests/e2e/surface.spec.ts @@ -131,7 +131,8 @@ const TICKET_PAGE = ` parent.postMessage({ type: 'plannotator-bridge-link-click', href: 'https://evil.example/forged' }, '*'); parent.postMessage({ type: 'plannotator-bridge-link-click', href: 'mailto:evil@evil.example' }, '*'); document.getElementById('forged').textContent = 'forged'; - fetch('http://127.0.0.1:1/').then(function () { out.push('fetch: reached'); }, function () { out.push('fetch: refused'); }).then(function () { + // The live Inbox, no-cors: only the inherited CSP can refuse it. + fetch('__INBOX_THREADS__', { mode: 'no-cors' }).then(function () { out.push('fetch: reached'); }, function () { out.push('fetch: refused'); }).then(function () { document.getElementById('probe').textContent = out.join(' | '); }); @@ -167,6 +168,8 @@ interface World { inbox: ShellMessage[]; attachments: { plan: any; ticket: any; flow: any }; guide: { thread_id: string; message_id: string }; + /** The comment the markdown test saved, for the focus test. */ + savedMarkId: string; } let world: World; @@ -195,7 +198,7 @@ async function inboxJson(path: string, init?: { method: string; body: unknown }) } /** What the shell does to open a file: read its view through the door (here, the window's route) and hand it over. */ -async function openAttachment(attachment: any): Promise { +async function openAttachment(attachment: any, focus: string | null = null): Promise { const view = await inboxJson(`/api/inbox/attachments/${attachment.id}/view`); const html = view.html === null ? null : String(view.html).replace(/ m.main && m.message.type === 'link')).toEqual([]); + world.savedMarkId = again.draft.id; }); test('HTML: a pin steps to its parent and back to its child, is saved and drawn; Interact and Annotate switch', async () => { @@ -609,6 +619,13 @@ test('a guided review: sections, a tick, Continue, one section with Reviewed; th expect(await next('section', from)).toMatchObject({ message_id: world.guide.message_id, section: 1, sections: 2 }); await expect(page.locator('[data-section-page="1"]')).toBeVisible(); await expect(page.locator('[data-section-page="1"] .sf-gnavbtn', { hasText: '01' })).toBeVisible(); + // The diff box fits its diff: no blank band under a short diff at a phone's width. + await expect(page.locator('[data-section-page="1"] diffs-container').first()).toBeVisible(); + const slack = await page.evaluate(() => { + const box = document.querySelector('[data-section-page="1"] [data-guide-code-view-mounted]')!; + return box.getBoundingClientRect().height - box.querySelector('diffs-container')!.getBoundingClientRect().height; + }); + expect(slack).toBeLessThan(60); await shot('09-guide-section'); from = mark(); @@ -625,6 +642,20 @@ test('a guided review: sections, a tick, Continue, one section with Reviewed; th expect(message.guide_reviewed).toEqual([true, true]); }); +test("a file opened at one of its marks (the shell's annotations sheet) scrolls that mark into view", async () => { + const { page } = world; + // Large text, so the mark starts well below the first screen. + await appearance('light', 2.5); + await openAttachment(world.attachments.plan, world.savedMarkId); + const markEl = page.locator(`[data-surface-attachment] [data-highlight-id="${world.savedMarkId}"]`).first(); + await expect(markEl).toBeVisible(); + await expect.poll(() => page.evaluate(() => document.querySelector('.sf-docscroll')!.scrollTop)).toBeGreaterThan(0); + const box = (await markEl.boundingBox())!; + expect(box.y).toBeGreaterThan(0); + expect(box.y + box.height).toBeLessThan(852); + await appearance('light', 1); +}); + test('theme and text size apply as the shell sends them', async () => { const { page } = world; await openAttachment(world.attachments.plan);