Skip to content

Feature/implement chrome extension - #455

Merged
pikann merged 9 commits into
masterfrom
feature/implement-chrome-extension
Sep 3, 2026
Merged

Feature/implement chrome extension#455
pikann merged 9 commits into
masterfrom
feature/implement-chrome-extension

Conversation

@pikann

@pikann pikann commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds a Chrome extension that lets users comment directly on elements of a running environment's forwarded preview — the same way Vercel's preview toolbar works — plus full support for those comments across the rest of the platform: the web app, the agent chat composer, and MCP tools for agents.

  • Browser extension (apps/extension, new): a Figma-style on-page commenting UI — click an element on a forwarded preview page to pin a comment, with clustering/spiderfy for pins that land close together, a reply thread, Resolve/Reopen, Copy link, Open (jumps to the comment's detail page), and a Create dropdown (Create Task / Create Conversation). Captures a cropped screenshot, the console errors, and any failed network requests active at comment time so a human or agent has full context without reopening the page.
  • Backend (services/api): new page_annotations/page_annotation_comments schema, scoped to a specific port forward (not the whole environment, since one environment can have several). Full CRUD plus resolve/reopen/comment/screenshot-upload/create-task-from-annotation, new annotations.read/write/resolve permissions wired into RBAC, and a paca_port cookie (set on login/refresh) + same-hostname credentialed-CORS support so the extension's content script can call the API directly from the forwarded preview page with no separate auth step.
  • Web app (apps/web): a new Port Forward detail page (Overview/Comments tabs) and a dedicated Comment detail page. Pasting a copied comment link into a task/doc/comment editor renders a rich, live-fetching preview card instead of a plain link; pasting one into the agent chat composer auto-attaches it as context. The comment detail page gets the same Copy/Open/Create-dropdown actions as the extension.
  • MCP (apps/mcp): list_annotations/get_annotation tools so agents can read page comments directly.
  • agent-runner: a new annotation context-item type so an attached comment renders correctly in agent prompts.

Notable fixes along the way

  • A duplicate-tool-call-id crash (Duplicate key toolCallId-... in useResources) in conversation rendering — some ACP agents reuse short ids like call_1 across unrelated tool calls within one turn; conversion into thread messages now disambiguates reused ids instead of crashing.
  • Extension's Copy button now works on the (almost always plain-HTTP) forwarded preview page — falls back to document.execCommand("copy") since navigator.clipboard requires a secure context.
  • Squashed what had been two migrations for the same schema into a single 000048_add_page_annotations.sql.

Test plan

  • go build, go vet, go test ./...services/api
  • tsc -b, biome check, vite build, vitest runapps/web
  • tsc -b, biome check, npm run buildapps/extension
  • tsc --noEmit, biome checkapps/mcp
  • Manually verified in the extension: pin creation/clustering, Resolve/Reopen/Copy/Open/Create, screenshot capture, scroll behavior
  • Manually verified in the web app: pasting a comment link into a description/comment editor and into the agent composer, the attach-context flow, and Create Task/Create Conversation from the comment detail page

🤖 Generated with Claude Code

- Implemented AnnotationHandler for managing page annotations via HTTP endpoints.
- Added methods for listing, creating, resolving, reopening annotations, and adding comments.
- Introduced screenshot upload functionality for annotations.
- Created database migration for page_annotations and page_annotation_comments tables.
- Updated auth_handler to set a new paca_port cookie for cross-origin requests.
- Enhanced CORS middleware to support same-hostname credentialed access.
- Added tests for paca_port cookie behavior in auth_handler.
- Updated router to include annotation routes and permissions.
…fic port forwards

- Updated the annotation service to include portForwardID in relevant methods, ensuring annotations are tied to specific port forwards instead of the broader environment.
- Introduced new methods for checking port forward ownership and retrieving annotations based on port forward ID.
- Modified task description generation to include a canonical URL for annotations, enhancing the context provided in task descriptions.
- Updated HTTP handlers and DTOs to accommodate changes in the annotation structure, including new routes for managing annotations under specific port forwards.
- Added migration script to transition existing annotations to the new port forward ownership model, ensuring data integrity and proper indexing.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

This is a large, carefully-documented feature (new MV3 Chrome extension + a full annotation backend + web/MCP integration). The one gap I'd want closed before merge is authorization: the annotation screenshot path never binds a referenced file to its uploader/project, unlike every other file-serving path in the codebase. Details inline.

Reviewed changes

  • Chrome extension — new MV3 extension (apps/extension/): background worker (screenshot capture, per-tab failed-request buffer, active-state), isolated content script (dormant everywhere except same-hostname pages carrying the paca_port cookie), MAIN-world console hook, options popup, 4 separate Vite builds + manifest copy.
  • Annotation backend — migration 000048 (page_annotations + page_annotation_comments, indexes, annotations.* role backfill), domain/service/repository/handler/DTO stack, annotations.{read,write,resolve} permissions, port-forwards/resolve endpoint, GetPortForward service/handler/route, task-creation-from-annotation with a BlockNote URL description.
  • paca_port cookie + CORS — non-HttpOnly port cookie set at login/refresh/logout, plus a new same-hostname (port-ignoring) credentialed CORS branch in corsMiddleware.
  • Web app — port-forward detail page + Comments tab, comment detail page, BlockNote annotationCard block + paste/load handlers, conversation attach-context via ?annotationId=, i18n across 9 locales, ContextItemType = "annotation".
  • MCPlist_annotations / get_annotation tools wired into the server + tool permissions.
  • Unrelated scope — a conversation-to-thread-messages.ts fix disambiguating reused ACP tool_call_ids (Goose) rides along in commit c9c927b1.

ℹ️ Same-hostname credentialed CORS widens read access across the whole API

The new branch in corsMiddleware grants Access-Control-Allow-Credentials: true + exact-origin echo to any origin whose hostname equals the API's Host, for every endpoint, overriding an operator's CORS_ORIGINS allow-list. This is required for the extension (a forwarded preview is same-hostname, and its content script needs credentialed reads), and it's documented extensively. But it is a real broadening of the default-config surface: previously a same-hostname cross-origin page's JS could send requests with the user's SameSite cookies but could not read responses (no credentials on the * echo); now it can read any project data through any authenticated endpoint. For a single-user self-hosted instance this is fine — the preview is your own app. On a shared-host or multi-tenant deployment, any app served on the same hostname now has credentialed read access to the whole API. Worth confirming that broad scope is intended; if it ever needs narrowing, this branch is the place to do it.

ℹ️ Nitpicks

  • Unrelated fix bundled in: the conversation-to-thread-messages.ts tool_call_id disambiguation (Goose ACP duplicate-key crash, with a good regression test) is unrelated to annotations/extension. Legit fix, but it would be easier to review/revert as its own PR.
  • http:// hardcoded in the new "Open" buttons: environment-detail.tsx, port-forward-detail.tsx, and comment-detail-view.tsx all window.open(\http://${host}:${port}...`). If a deployment fronts forwarded previews over TLS (common: a proxy on PORT_FORWARD_HOST), this opens the plain-HTTP origin and fails. Consider deriving the scheme from the page's own protocol, mirroring how the extension's content script builds baseUrl`.
  • Extension swallows API errors on comment actions: submitComment and the resolve/reopen/reply/create-task handlers are fire-and-forget with no user-visible .catch — a 403 (Viewer with only annotations.read) or an expired session silently does nothing. The README points at DevTools [Paca] logs, but an inline error would be friendlier.
  • Orphaned pending screenshots: upload-url creates a pending files row + presigned PUT; if the user never submits the comment, the row and any uploaded object are never cleaned up. Low impact (matches the existing attachment flow), but worth noting.
Technical details
# Screenshot file ownership is never verified

## Affected sites
- services/api/internal/service/annotation/annotation_service.go:321-342 (`GetScreenshotURL`) — presigns whatever `StorageKey` the referenced `files` row has, guarded only by the annotation belonging to `projectID`.
- services/api/internal/service/annotation/annotation_service.go:140-190 (`Create`) — accepts an arbitrary `ScreenshotFileID` and `MarkScreenshotFileUploaded`s it.
- services/api/internal/service/annotation/annotation_service.go:304-315 (`CompleteScreenshotUpload`) — same, links any `fileID` to the annotation.

## Required outcome
A member of project A with `annotations.write` can create an annotation referencing any `files` row they know the ID of (a task/doc attachment visible in the same project, or — if an ID leaks — a file from another project sharing the bucket), then read its bytes via `GET .../screenshot-url`, because `GetScreenshotURL` binds only to the annotation's project, never to the file's owner/project/storage-key prefix.

The rest of the codebase deliberately binds files to their owner before serving bytes:
- task attachments resolve via the `task_attachments` row and check `a.TaskID != taskID` (attachment/service.go);
- doc files check `docFileKeyHasDocPrefix(f.StorageKey, docID)` (attachment/doc_service.go);
- avatars check `strings.HasPrefix(f.StorageKey, avatarOwnerPrefix(...)+"/")` (attachment/avatar_service.go).

## Suggested approach
In `GetScreenshotURL`, reject files whose storage key does not start with `annotations/` (the key `InitiateScreenshotUpload` writes) and ideally verify the file's `uploaded_by` equals the caller. In `Create`/`CompleteScreenshotUpload`, reject a `ScreenshotFileID` that isn't a pending screenshot file initiated by this caller for this project. At minimum, mirror the storage-key-prefix guard the doc/avatar services already use.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

Comment thread services/api/internal/service/annotation/annotation_service.go
pikann and others added 3 commits September 3, 2026 06:56
- golangci-lint: add missing doc comments on exported annotation
  repository/service methods, and switch httptest.NewRequest to
  NewRequestWithContext (noctx) in auth_handler_test.go/cors_test.go.
- E2E: task position tests used fixed, non-existent task UUIDs, but
  MoveTask/BulkMoveTasks now verify the task belongs to the project
  (0cf5129's ownership-check fix) — update them to create real tasks
  via the API instead of reverting that check.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
pullfrog flagged that Create/CompleteScreenshotUpload accepted an
arbitrary files.id as screenshot_file_id with no check that the
caller actually uploaded it — a project member with annotations.write
could point an annotation at any files row they knew the ID of (a
task attachment, doc, avatar, or another user's screenshot) and
GetScreenshotURL would presign a URL for it, mirroring the doc/avatar
guards' storage-key-prefix + uploader check that this path was
missing.

- verifyAnnotationScreenshotFile: rejects any file whose storage key
  isn't under annotations/ or wasn't uploaded by the acting user; used
  by Create and CompleteScreenshotUpload (which now also takes the
  caller's ID) before ever attaching a file.
- GetScreenshotURL re-checks the storage-key prefix (not uploader
  identity — annotations.write lets any project member complete an
  upload on someone else's annotation, so uploader != creator is a
  legitimate case there) as defense in depth.
- Also fixes every annotationdom error silently mapping to 500: none
  of them were wired into the presenter's error-code table.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found. The incremental delta (commit 6b321d8) is a focused security fix that fully closes the prior review's IMPORTANT finding, with meaningful regression tests.

Reviewed changes

  • Screenshot ownership verificationCreate and CompleteScreenshotUpload now resolve the referenced files row and reject any file whose storage key isn't under annotations/ or whose uploaded_by isn't the caller (verifyAnnotationScreenshotFile). The handler injects callerID(r) server-side for both CreatedBy and the new completedBy parameter, so identity can't be spoofed from the request body; the only Create/CompleteScreenshotUpload call sites are the handler (MCP is list/get only), so no bypass path exists.
  • Read-side defense in depthGetScreenshotURL re-checks the annotations/ prefix before presigning (deliberately skipping the uploader-identity half, since any project member may legitimately attach a screenshot — a documented, tested design choice).
  • Error surface + tests — new ANNOTATION_SCREENSHOT_MISMATCH code mapped to 404, and a new annotation_service_test.go covering foreign-file rejection, own-file success, wrong-prefix rejection, and the uploader≠creator-but-legitimate case; the rejection tests assert a rejected foreign file is never attached to the annotation.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

pikann and others added 3 commits September 3, 2026 08:34
Chrome Web Store requires a Privacy Policy URL before an item can be
published. Since this repo is public, a plain PRIVACY.md the store
links to via its GitHub blob URL needs no separate hosting.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…rror handling

- Introduced a new error code for task creation in progress to prevent duplicate tasks.
- Implemented a claim mechanism for task creation to ensure only one task can be created per annotation at a time.
- Updated the annotation repository to include a method for claiming task creation.
- Enhanced the service layer to handle transient failures during task ID assignment with retries.
- Added tests to cover scenarios for task creation conflicts and transient failures.
- Updated CORS middleware to restrict credentialed access to specific extension routes.
- Modified the database schema to include a timestamp for task creation claims.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Commit 0fd5229 is titled "docs: add a privacy policy page" but bundles a half-finished refactor that breaks the web app's CI: two new test files fail against the current implementation (one imports a symbol that doesn't exist, one asserts paste-handler behavior the code doesn't have), and a new paca-port.ts module is never wired in. web-pr-ci.yml runs bun run test + bun run build on any PR touching apps/web/**, so this turns the checks red.

Reviewed changes

  • Privacy policy — new apps/extension/PRIVACY.md for the Chrome Web Store listing.
  • Shared port-cookie module — new apps/extension/src/shared/paca-port.ts extracting the paca_port cookie read.
  • Web tests — new annotation-link.test.ts and blocknote-annotation-paste-handler.test.ts covering the annotation-link matcher and the BlockNote paste handler.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

Comment thread apps/web/src/lib/annotation-link.test.ts Outdated
Comment thread apps/extension/src/shared/paca-port.ts
Comment thread apps/extension/PRIVACY.md Outdated

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

The two commits since the last review (f7c1917, cb509951) are substantive and well-tested: they close the prior CAUTION's CI breakage, add task-creation idempotency, narrow the credentialed CORS surface to the extension's own routes, and improve extension error UX. All four threads from the last review are resolved, and go vet + the annotation-service and CORS-router tests pass locally.

Reviewed changes

  • Resolved the CAUTION CI failuresmatchAnnotationLinkOnly is now exported from apps/web/src/lib/annotation-link.ts and adopted by the BlockNote paste handler, comment-blocknote.tsx, and the composer's thread.tsx, so both previously-failing web test files assert behavior the code now implements (link-with-surrounding-text pastes fall through to the default handler instead of dropping the surrounding text).
  • Task-creation idempotencyCreateTaskFromAnnotation now atomically claims the annotation (task_creation_claimed_at column, ClaimTaskCreation repo method with a 2-minute TTL) before calling out to task creation, closing the retry-after-timeout duplicate-task window; SetTaskID gets a bounded in-process retry. New service tests cover claim-conflict rejection, transient-failure recovery, and single-task creation.
  • CORS scoped to extension routes — the same-hostname credentialed CORS branch is now gated on extensionCredentialedPathPattern (auth/refresh, port-forwards/resolve, the nested annotations subtree) instead of applying to every endpoint; negative tests confirm non-extension routes no longer receive Allow-Credentials. This closes the earlier informational concern about whole-API cross-origin read widening.
  • Extension error feedback — every comment action (submit/resolve/reopen/reply/create-task) now .catches into a dismissible toast with actionable messaging (401/403 mapped to session/permission text) instead of a silent [Paca] console entry.
  • Privacy posture + accurate PRIVACY.md — the MAIN-world console hook only installs when the paca_port cookie is present, the background discards a tab's failed-request buffer as soon as the tab is known to be a non-preview, the previously-dead shared/paca-port.ts module is now imported by both content scripts, and PRIVACY.md accurately describes the across-tabs buffer behavior.
  • Web preview URL scheme — hardcoded http:// replaced with a portForwardUrl() helper that inherits window.location.protocol, fixing the earlier nit about TLS-fronted deployments.
  • Smaller fixes — the MCP annotation client always sends page_size (default 20) so an omitted value can't fetch an entire project's annotations unbounded; DecodeAnnotationCursor validates the embedded ID is a real UUID (malformed cursor restarts cleanly instead of a 500); hydrateAll batch-fetches comments (kills a per-annotation N+1 on the extension's hot path); reused ACP tool_call_ids get FIFO-queued handlers with a parallel-call regression test.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@pikann
pikann merged commit b51e5fd into master Sep 3, 2026
11 checks passed
@pikann
pikann deleted the feature/implement-chrome-extension branch September 3, 2026 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant