vndangkhoa/open-design
1
0
Fork 0
mirror of https://github.com/nexu-io/open-design.git synced 2026-06-01 03:14:35 +07:00
Code Issues Projects Releases Packages Wiki Activity Actions 1

fix(web): surface empty-annotation state for Inspect/Picker (#890) (#1005)

Browse source 
When the agent emits an HTML artifact with no `data-od-id` /
`data-screen-label` annotations (a freeform PRD → HTML pass through a
Claude-Code-compatible CLI without going through a skill, for
example), the existing Inspect / Picker affordances no-oped silently:

- The bridge's click handler walks up to <html>, finds nothing tagged,
  and bails before emitting `od:comment-target` — by design, since
  posting a synthetic id here would change save-to-source semantics
  for inspect overrides (the persisted CSS keys off the same
  elementId).
- The host then sat at "Click any element with `data-od-id` to tune
  its style" — phrased as if the user just hadn't found the right
  element, when the page in fact had nothing matching at all.
- Picker mode (Tweaks → Picker) had no hint at all.

The bridge already broadcasts `od:comment-targets` with the full list
on every mode toggle and DOM mutation, but the host's existing
listener was gated on `boardMode` only — Inspect mode never learned
the artifact's annotation count.

Two surgical fixes:

1. `FileViewer.tsx`: a dedicated `od:comment-targets` listener that
   installs whenever Inspect OR Comments mode is active, mirroring
   the bridge's broadcast into `liveCommentTargets`. The
   comment-mode-only listener still owns its hover / click / pod
   events; this new listener only handles the targets list.
2. `FileViewer.tsx`: the inspect-empty-hint banner now dispatches on
   `liveCommentTargets.size === 0`. Empty: a clear "this artifact has
   no `data-od-id` annotations yet — ask the agent to add them"
   message that names the missing attribute. Populated: existing
   instructive copy. Mirrored across Inspect and Picker modes so the
   failure surface gives the same calibration signal in both.

Tests:

- `tests/runtime/srcdoc-bridge-empty-targets.test.ts` (3 cases): pin
  the bridge contract this fix depends on. Run the IIFE in jsdom and
  assert (a) `allTargets()` posts an empty list for unannotated DOM,
  (b) clicks on unannotated elements do NOT post `od:comment-target`
  (regression pin against future "synthetic id" fallbacks that would
  silently change save-to-source semantics), (c) clicks DO still
  resolve to an annotated ancestor when one exists.
- `tests/components/FileViewer.inspect-empty-hint.test.tsx` (3
  cases): pin the host dispatch — empty state in Inspect mode, the
  switch back to instructive copy when targets show up, and the
  mirrored affordance in Picker mode.

Out of scope (flagged in the design comment so it isn't lost):
- The follow-up scenario from #890 ("parent has data-od-id, target
  child does not → adjustments hit the parent") is a different bug
  that needs either synthetic-id fallback or a UI affordance to
  descend into the click target. Leaving that to a follow-up so this
  PR stays narrow.
- i18n: the existing inspect-empty-hint copy is hardcoded English;
  rolling it into the 17-locale Dict is a separate cleanup.
This commit is contained in:
Sid 2026-05-09 11:20:13 +08:00 committed by GitHub
parent 9ef136ced5
commit 78ae6feb59
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
3 changed files with 416 additions and 12 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

116
apps/web/src/components/FileViewer.tsx
View file

@ -3197,6 +3197,58 @@ function HtmlViewer({
win.postMessage({ type: 'od:inspect-mode', enabled: inspectMode }, '*');
}, [inspectMode, srcDoc]);
// Mirror the bridge's `od:comment-targets` broadcast into
// `liveCommentTargets` whenever EITHER Inspect or Comments mode is
// active. The boardMode-only useEffect below still handles its
// own comment-specific events (hover / click target / pod), but
// the targets list itself is mode-agnostic — it's just "which
// elements on the page carry data-od-id / data-screen-label".
// Without this listener Inspect mode never learns the artifact's
// annotation count, and the empty-state hint added for #890 would
// misfire (always firing in Inspect mode, even on annotated
// artifacts) because the comment-mode listener short-circuits on
// `!boardMode`. Issue #890.
useEffect(() => {
if (!inspectMode && !boardMode) {
setLiveCommentTargets((current) => (current.size > 0 ? new Map() : current));
return;
}
function onMessage(ev: MessageEvent) {
if (ev.source !== iframeRef.current?.contentWindow) return;
const data = ev.data as
| {
type?: string;
targets?: Array<Partial<PreviewCommentSnapshot>>;
}
| null;
if (data?.type !== 'od:comment-targets' || !Array.isArray(data.targets)) return;
const next = new Map<string, PreviewCommentSnapshot>();
data.targets.forEach((item) => {
const elementId = String(item?.elementId || '');
if (!elementId) return;
next.set(elementId, {
filePath: file.name,
elementId,
selector: String(item?.selector || ''),
label: String(item?.label || ''),
text: String(item?.text || ''),
position: {
x: clampBridgeCoordinate(item?.position?.x),
y: clampBridgeCoordinate(item?.position?.y),
width: clampBridgeCoordinate(item?.position?.width),
height: clampBridgeCoordinate(item?.position?.height),
},
htmlHint: String(item?.htmlHint || ''),
selectionKind: 'element',
memberCount: undefined,
});
});
setLiveCommentTargets(next);
}
window.addEventListener('message', onMessage);
return () => window.removeEventListener('message', onMessage);
}, [inspectMode, boardMode, file.name]);
useEffect(() => {
setActiveCommentTarget(null);
setHoveredCommentTarget(null);
@ -4629,19 +4681,59 @@ function HtmlViewer({
error={inspectError}
/>
) : null}
{inspectMode && openHintBox && !activeInspectTarget ? (
{/*
Hint banner for Inspect / Picker modes. The bridge in
`apps/web/src/runtime/srcdoc.ts` posts `od:comment-targets`
with every element annotated with `data-od-id` /
`data-screen-label`, so `liveCommentTargets.size` is the
authoritative annotation count for the current artifact.
Two states:
- "has targets": the existing copy ("Click any element with
`data-od-id` to tune its style.") for users who just don't
see the crosshair cursor.
- "no targets" (issue #890): a freeform-generated artifact
(e.g. PRD → HTML through a Claude-Code-compatible CLI
without a skill) ships zero `data-od-id` annotations. The
bridge's click handler walks up to <html>, finds nothing,
and bails — clicks no-op silently. The static copy made
this look broken; the empty-state copy explains what's
missing and how to fix it. Mirrored across Inspect and
Picker because the failure surface is identical.
*/}
{(inspectMode || (boardMode && boardTool === 'inspect'))
&& openHintBox
&& !activeInspectTarget
&& !activeCommentTarget ? (
<div className="inspect-empty-hint-container">
<div className="inspect-empty-hint" data-testid="inspect-empty-hint">
Click any element with <code>data-od-id</code> to tune its style.
</div>
<button
type="button"
title="Close Inspect Hint"
aria-label="Close Inspect Hint"
onClick={() => setOpenHintBox(false)}
className="orbit-artifact-ghost">
<Icon className='' name='close' size={12} />
</button>
{liveCommentTargets.size === 0 ? (
<div
className="inspect-empty-hint"
data-testid="inspect-empty-hint-no-targets"
>
This artifact has no <code>data-od-id</code>{' '}
annotations yet — ask the agent to add them to the
sections you want to{' '}
{inspectMode ? 'inspect' : 'comment on'}.
</div>
) : (
<div
className="inspect-empty-hint"
data-testid="inspect-empty-hint"
>
Click any element with <code>data-od-id</code> to{' '}
{inspectMode ? 'tune its style' : 'leave a comment'}.
</div>
)}
<button
type="button"
title="Close Inspect Hint"
aria-label="Close Inspect Hint"
onClick={() => setOpenHintBox(false)}
className="orbit-artifact-ghost"
>
<Icon className="" name="close" size={12} />
</button>
</div>
) : null}
</div>

161
apps/web/tests/components/FileViewer.inspect-empty-hint.test.tsx Normal file
View file

@ -0,0 +1,161 @@
// @vitest-environment jsdom
import { act, cleanup, fireEvent, render, screen } from '@testing-library/react';
import { afterEach, describe, expect, it } from 'vitest';
import { FileViewer } from '../../src/components/FileViewer';
import type { ProjectFile } from '../../src/types';
// Regression coverage for nexu-io/open-design#890. When the agent emits
// an HTML artifact with no `data-od-id` / `data-screen-label`
// annotations (a freeform PRD → HTML pass without going through a
// skill, for example), the existing inspect-empty-hint banner lied:
// it said "Click any element with `data-od-id` to tune its style"
// even though no element on the page carried that attribute. The user
// would click, the bridge's click handler would walk up to <html>,
// find nothing tagged, and silently bail — no UI feedback. This test
// pins the new dispatch:
//
// - liveCommentTargets.size === 0 → empty-state copy explaining
// what's missing and how to fix it.
// - liveCommentTargets.size > 0 → existing instruction copy.
//
// And mirrors the same affordance into Picker mode so both inspect
// surfaces give the user the same calibration signal.
function htmlFile(name = 'mock.html'): ProjectFile {
return {
name,
path: name,
type: 'file',
size: 1024,
mtime: 1710000000,
kind: 'html',
mime: 'text/html',
artifactManifest: {
version: 1,
kind: 'html',
title: 'Mock',
entry: name,
renderer: 'html',
exports: ['html'],
},
};
}
function postTargetsFromIframe(targets: Array<{ elementId: string }>): void {
// FileViewer's message handler keys off `ev.source ===
// iframeRef.current?.contentWindow` to defend against cross-frame
// chatter on the host. Find the host's iframe in the rendered DOM
// and dispatch a real MessageEvent with `source` set to its
// contentWindow so the handler accepts the payload.
const iframe = document.querySelector('iframe');
if (!iframe) throw new Error('iframe not in DOM yet');
const event = new MessageEvent('message', {
data: {
type: 'od:comment-targets',
targets: targets.map((t) => ({
elementId: t.elementId,
selector: `[data-od-id="${t.elementId}"]`,
label: 'div',
text: '',
position: { x: 0, y: 0, width: 100, height: 50 },
htmlHint: '',
})),
},
source: iframe.contentWindow,
});
window.dispatchEvent(event);
}
describe('FileViewer Inspect/Picker empty-annotation hint (#890)', () => {
afterEach(() => {
cleanup();
});
it('shows the empty-state copy in Inspect mode when the iframe reports zero annotated targets', async () => {
render(
<FileViewer
projectId="project-1"
file={htmlFile()}
liveHtml="<html><body><h1>Plain PRD with no data-od-id</h1></body></html>"
/>,
);
// Activate Inspect — the toolbar button has a stable test id from
// the surrounding viewer chrome.
fireEvent.click(screen.getByTestId('inspect-mode-toggle'));
// The bridge boot sequence ends with a `od:comment-targets` post
// carrying an empty array for unannotated artifacts (pinned in
// tests/runtime/srcdoc-bridge-empty-targets.test.ts). Simulate
// that broadcast and assert the host renders the empty-state
// copy instead of the instructive default.
await act(async () => {
postTargetsFromIframe([]);
});
expect(screen.queryByTestId('inspect-empty-hint-no-targets')).toBeTruthy();
expect(screen.queryByTestId('inspect-empty-hint')).toBeNull();
// The message must mention what's missing so the user knows what
// to ask the agent for. We assert on the attribute name verbatim
// — it's the exact token the agent has to add to the artifact.
expect(screen.getByTestId('inspect-empty-hint-no-targets').textContent ?? '')
.toMatch(/data-od-id/);
});
it('switches back to the instructive copy once the iframe reports at least one annotated target', async () => {
render(
<FileViewer
projectId="project-1"
file={htmlFile()}
liveHtml="<html><body><main data-od-id='hero'>Hero</main></body></html>"
/>,
);
fireEvent.click(screen.getByTestId('inspect-mode-toggle'));
await act(async () => {
postTargetsFromIframe([{ elementId: 'hero' }]);
});
// Existing copy survives — pinning that the new dispatch doesn't
// accidentally drop the long-standing affordance for users whose
// artifacts already ship annotations.
expect(screen.getByTestId('inspect-empty-hint')).toBeTruthy();
expect(screen.queryByTestId('inspect-empty-hint-no-targets')).toBeNull();
});
it('shows the empty-state copy in Picker mode when the iframe reports zero annotated targets', async () => {
// Picker mode (Comments → Picker tool) has the same failure
// surface as Inspect: clicking an unannotated element no-ops.
// This test mirrors the Inspect coverage above.
render(
<FileViewer
projectId="project-1"
file={htmlFile()}
liveHtml="<html><body><h1>No annotations</h1></body></html>"
/>,
);
// Tweaks mode boots with the Picker tool already selected
// (`boardTool` defaults to `'inspect'`), so the empty-state hint
// path fires the moment we enter Tweaks — no inner button click
// needed. The inner `comment-mode-toggle` only renders alongside
// its Pods sibling once Tweaks is on.
fireEvent.click(screen.getByTestId('board-mode-toggle'));
await act(async () => {
postTargetsFromIframe([]);
});
// The same testid surfaces the empty-state copy regardless of
// which inspect surface is active — keeps the i18n-free copy
// consolidated and makes future migration to translated strings
// a single edit instead of two.
expect(screen.queryByTestId('inspect-empty-hint-no-targets')).toBeTruthy();
// Copy is mode-aware so the action is concrete: in Picker mode
// the user is leaving comments, not tuning style.
expect(screen.getByTestId('inspect-empty-hint-no-targets').textContent ?? '')
.toMatch(/comment on/i);
});
});

151
apps/web/tests/runtime/srcdoc-bridge-empty-targets.test.ts Normal file
View file

@ -0,0 +1,151 @@
// @vitest-environment node
import { describe, expect, it, vi } from 'vitest';
import { JSDOM } from 'jsdom';
import { buildSrcdoc } from '../../src/runtime/srcdoc';
// Behavioral coverage for nexu-io/open-design#890. The Examples / Tweaks
// preview iframe runs the bridge script generated by `buildSrcdoc`. When
// the agent emits an artifact without `data-od-id` / `data-screen-label`
// (e.g. a freeform PRD → HTML pass through a Claude-Code-compatible CLI
// without a skill), the bridge must:
//
// 1. Still post `od:comment-targets` with `targets: []` so the host
// can detect the empty-annotation state and surface a clearer
// hint than "Click any element with `data-od-id` …" — without
// this, FileViewer's `liveCommentTargets` map never updates and
// the hint banner sticks at its instructive-default state even
// though there's nothing to click.
// 2. Drop click events on unannotated elements without posting an
// `od:comment-target` — the click handler walks up to <html>,
// finds nothing tagged, and must bail. Posting a synthetic id
// here would change save-to-source semantics for inspect
// overrides (the persisted CSS keys off the same elementId), so
// this test pins the no-fallback contract.
//
// The host-side hint switch lives in `apps/web/src/components/FileViewer.tsx`
// (search for `inspect-empty-hint-no-targets`); these tests pin the
// signal it depends on.
function extractBridgeScript(srcdoc: string): string {
// The bridge is wrapped in `<script data-od-selection-bridge>(function(){…})()</script>`.
const match = srcdoc.match(
/<script data-od-selection-bridge>([\s\S]*?)<\/script>/,
);
if (!match || !match[1]) {
throw new Error('selection bridge script not found in srcdoc');
}
return match[1];
}
function setupBridgeDom(bodyHtml: string, mode: 'inspect' | 'comment') {
const srcdoc = buildSrcdoc(`<!doctype html><html><body>${bodyHtml}</body></html>`, {
inspectBridge: mode === 'inspect',
commentBridge: mode === 'comment',
});
const script = extractBridgeScript(srcdoc);
// Build a fresh JSDOM that mirrors the iframe runtime: `window.parent`
// is what the bridge calls into, and we spy on its postMessage to
// capture the messages it would emit to the host.
const dom = new JSDOM(`<!doctype html><html><body>${bodyHtml}</body></html>`, {
runScripts: 'outside-only',
pretendToBeVisual: true,
});
const win = dom.window;
const parentPostMessage = vi.fn();
// jsdom defaults `window.parent` to `window` itself for top-level
// documents; replace it with a stub that has a spied postMessage so
// we can observe what the bridge would send to the embedding host.
Object.defineProperty(win, 'parent', {
configurable: true,
value: { postMessage: parentPostMessage },
});
// Run the bridge IIFE inside the jsdom window so its `document` /
// `window` refer to our DOM. We don't use `runScripts: 'dangerously'`
// — the IIFE is our trusted source, evaluated by `Function` in the
// jsdom realm.
const evaluate = new win.Function(script);
evaluate.call(win);
return { dom, win, parentPostMessage };
}
describe('selection bridge — empty annotation surface (#890)', () => {
it('posts od:comment-targets with an empty list when the artifact has no annotated elements', async () => {
const { win, parentPostMessage } = setupBridgeDom(
// PRD-style mockup: real DOM, zero `data-od-id` / `data-screen-label`.
'<header><h1>Acme PRD</h1></header><main><section><p>Goals</p></section></main>',
'inspect',
);
// The bridge schedules the initial postTargets via setTimeout(0)
// after enabling the mode. Drain microtasks + the timer so the
// message lands before we assert.
await new Promise<void>((resolve) => win.setTimeout(resolve, 10));
const targetMessages = parentPostMessage.mock.calls
.map((call) => call[0])
.filter((message) => message?.type === 'od:comment-targets');
expect(targetMessages.length).toBeGreaterThan(0);
// Every targets-broadcast must be an empty list — there's nothing
// annotated to enumerate. If a future change starts inventing
// synthetic ids in `allTargets()`, this assertion fires before
// the host's empty-state hint silently disappears.
for (const message of targetMessages) {
expect(message.targets).toEqual([]);
}
});
it('does not post od:comment-target when the user clicks an unannotated element', async () => {
const { win, parentPostMessage } = setupBridgeDom(
'<header><h1 id="title">Acme PRD</h1></header>',
'inspect',
);
// Wait for bridge boot.
await new Promise<void>((resolve) => win.setTimeout(resolve, 10));
parentPostMessage.mockClear();
// Click directly on the <h1> — it has no `data-od-id` / `data-screen-label`,
// and no ancestor does either. The bridge's `closestTarget()` walks up
// to <html> and returns null; the click handler must bail before
// emitting a comment-target message.
const target = win.document.getElementById('title');
expect(target).not.toBeNull();
target!.dispatchEvent(
new win.MouseEvent('click', { bubbles: true, cancelable: true }),
);
const clickMessages = parentPostMessage.mock.calls
.map((call) => call[0])
.filter((message) => message?.type === 'od:comment-target');
expect(clickMessages).toEqual([]);
});
it('still posts od:comment-target when the click traverses up to an annotated ancestor', async () => {
// Sanity check / contract pin: the no-op behavior above is specific
// to the no-annotation case. When ANY ancestor carries `data-od-id`,
// the click must still resolve to that ancestor — this is the
// happy path the gallery has shipped since the bridge landed.
const { win, parentPostMessage } = setupBridgeDom(
'<main data-od-id="hero"><h1 id="title">Hero</h1></main>',
'inspect',
);
await new Promise<void>((resolve) => win.setTimeout(resolve, 10));
parentPostMessage.mockClear();
win.document.getElementById('title')!.dispatchEvent(
new win.MouseEvent('click', { bubbles: true, cancelable: true }),
);
const clickMessages = parentPostMessage.mock.calls
.map((call) => call[0])
.filter((message) => message?.type === 'od:comment-target');
expect(clickMessages).toHaveLength(1);
expect(clickMessages[0].elementId).toBe('hero');
});
});