feat(inline-annotations): opt-in agentation-style pin annotations#7
feat(inline-annotations): opt-in agentation-style pin annotations#7olyaiy wants to merge 1 commit into
Conversation
Inspired by benjitaylor/agentation, this adds a sticky "annotation mode"
gated behind a new `inlineAnnotations: { enabled: true }` config flag so
existing consumers see no behavior change.
When enabled, clicking the trigger from an empty draft puts the widget
into annotation mode: the element picker stays sticky, clicking any
element on the host page opens an inline comment popup anchored to that
element, and submitting drops a numbered pin marker. Pins persist across
reloads via the existing localStorage draft round, and are cleared on
successful submit.
Implementation notes:
- `FeedbackPin` lives on each `FeedbackRoundItem` so persistence and
draft round transport flow through unchanged.
- New `src/pin-positioning.ts` holds pure helpers (anchor math, popup
placement, shadow-DOM-aware `deepElementFromPoint`, css-selector
re-resolution) with 15 unit tests.
- A requestAnimationFrame ticker + ResizeObserver + capture-phase scroll
listener keep pins glued to their target element through scroll and
reflow; scroll vs. fixed sub-layers handle position: fixed ancestors.
- Two-step Escape: inside popup -> closes popup; in mode -> exits mode.
- Adds CSS using existing theme tokens so light/dark modes work.
- Adds a Playwright spec covering add/edit/delete/escape flows; the
fixture toggles the feature via `?inlineAnnotations=true` so unrelated
e2e specs are unaffected.
![obvious-autobuild-staging[bot]](https://avatars.githubusercontent.com/u/31830338?s=60&v=4){kind=small}
There was a problem hiding this comment.
Obvious Code Review
Verdict: COMMENT — Medium findings (no blockers)
Summary
- 🔴 Blocker: 0
- 🟠 High: 0
- 🟡 Medium: 2
- 💡 Suggestion: 3
- 🔵 Nit: 2
Medium Findings
M1 — isElementFixed treats position: sticky as fixed (src/pin-positioning.ts:84)
Sticky elements behave like relative until stuck. Recording yPx in viewport-space for a pre-stuck sticky element causes the pin to visually drift from the annotated element after the user scrolls. Fix: remove sticky from the fixed check.
M2 — Pins inside aria-hidden="true" container (src/index.ts, renderPinLayer())
Pin <button> elements have aria-label but are wrapped in aria-hidden="true" divs. They are invisible to screen readers and not keyboard-reachable. WCAG 2.1 SC 4.1.2 violation. Fix: remove aria-hidden="true" from both pin layer divs.
Also Noted
- S1:
shouldEnterAnnotationModeOnTrigger()entry conditions undocumented on public config type - S2: Full
shadowRoot.innerHTMLreplace on fixed↔scroll pin transition blurs active textarea - S3:
isElementFixedunit tests only cover thenullguard — no sticky coverage
Clean Areas
XSS/injection (all escapeHtml), async safety in handleAnnotationPick, storage parsing, MAX_ROUND_ITEMS guards, ticker/listener lifecycle cleanup, deepElementFromPoint loop safety, popup placement edge cases, E2E coverage.
| return true; | ||
| } | ||
| } | ||
| const parent = current.parentElement; |
There was a problem hiding this comment.
Medium — sticky is treated as fixed, causing pin drift for pre-stuck elements.
getComputedStyle().position returns "sticky" regardless of whether the element is currently stuck. A sticky nav bar scrolls with the document until its threshold — clicking it before sticking stores yPx in viewport space, then the pin drifts after scroll.
// Fix: remove sticky from the check
if (position === "fixed") {
return true;
}If sticky stuck-state detection is needed in future, use IntersectionObserver against a sentinel — it's the only reliable signal.
| .querySelector('[data-inline-popup-submit="true"]') | ||
| ?.addEventListener("click", (event) => { | ||
| event.stopPropagation(); | ||
| this.handleInlineAnnotationSubmit(); |
There was a problem hiding this comment.
Medium — Pin buttons are inside aria-hidden="true", making them invisible to screen readers and unreachable by keyboard.
The individual <button> elements have aria-label="Edit annotation N: ..." but the parent <div aria-hidden="true"> removes all descendants from the accessibility tree, including those buttons. WCAG 2.1 SC 4.1.2.
// Fix: remove aria-hidden from both pin layer divs
<div class="obv-pin-layer obv-pin-layer-scroll">The pointer-events: none CSS on the layer already handles event isolation — aria-hidden is not needed for that.
1 participant
Summary
benjitaylor/agentation: clicking the trigger from an empty draft enters a sticky "annotation mode" where users click any page element, leave a comment in an anchored popup, and the SDK drops a numbered pin marker. Pins persist in the draft round and clear on submit.inlineAnnotations: { enabled: true }config flag so existing consumers see zero behavior change. Default is off.src/pin-positioning.tswith unit-test coverage. Shadow-DOM-awaredeepElementFromPointso the picker works inside web components on the host page.What's new
pin?: FeedbackPinfield onFeedbackRoundItem(round-trips through existinglocalStoragedraft persistence).annotationModeActive/inlineAnnotationOpen/editingPinItemId/pendingPinAnchorprivate fields + render+bind methods for the annotation shell, inline popup, and pin layer (scroll vs. fixed sub-layers forposition: fixedancestors).ResizeObserver+ capture-phasescrolllistener re-resolve pins viacssSelectorwhile annotation mode is active, with a movement threshold to avoid useless work.--obv-feedback-*tokens so light/dark mode work automatically.Test plan
bun run test:typecheckbun run test:unit— 38 pass (15 new forpin-positioning)bun run build— IIFE + ESM cleanbun run test:e2e— 16 pass; new spec covers add / edit / delete / Escape flows. Fixture gates the feature via?inlineAnnotations=trueso unrelated specs are untouched.examples/vanilla/index.html(flag now enabled in that example).