feat(preview): hosted preview review mode + shared review chrome#241
Merged
Conversation
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…t, csp) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…eview tests Task A4 threaded review_type/encoding through the API controller but left Crit.Reviews.create_review ignoring the :review_type opt and dropping per-file :encoding, so preview reviews were persisted as :files and base64 assets were never flagged for decode. Thread both through the context (defaulting review_type to "files" so the NOT NULL column keeps its default), extend review_fixture to build preview reviews, restore the original raw_controller tests, add preview-mode coverage, and update the x-frame-options assertion to SAMEORIGIN. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The share() op already POSTs msg.payload verbatim to /api/reviews, the same endpoint the CLI's direct transport uses (Rule 2: no relay-specific endpoint). This makes the proxy-auth relay path carry preview-mode payloads — review_type: "preview" plus files with encoding: "base64" — through unchanged, identical in shape to files-mode. Add a clarifying comment so a future refactor doesn't add field-filtering that would silently drop review_type/encoding and break preview relay. crit-web has no JS test runner, so the guard is the comment plus the Phase E Playwright/integration coverage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…dom comments Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds priv/repo/seed_assets/preview_demo/ — a self-contained landing card (index.html + style.css + app.js + logo.png) with no external fonts or CDNs, so it renders cleanly under the preview CSP. The PNG exercises the base64 snapshot path; app.js proves JS runs in the iframe. The seed block (token preview-demo-0000, idempotent) inserts a review_type: :preview review for the seed user with one snapshot per asset (text raw, logo.png Base64-encoded with encoding: "base64") and two DOM-anchored comments — an open one on h1.hero and a resolved one on #cta — matching real selectors in index.html. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds a self-contained Playwright spec for the PreviewMode hook (the review_type: "preview" surface): iframe load + in-frame content via frameLocator, a dom-anchored comment rendering as a panel card with a matching badge count, viewport toggle width change, open vs. resolved card rendering, reply-from-panel, and reload persistence. A createPreviewReview helper posts an inline html+css+js preview review with dom_anchor comments via /api/reviews, so the spec is self-contained and does not depend on the dev/test seed (the MIX_ENV=test Playwright webServer does not run seeds). Also fixes serialize_comment to include dom_anchor in the comment shape pushed to the client. The PreviewMode hook filters comments by dom_anchor to render pins and panel cards; without it no pre-existing dom-anchored comment rendered. Updates the serialize_comment shape test accordingly. The in-iframe Pin-to-create flow is marked test.fixme: the vendored agent scripts are injected as <script src="/preview-agent/...">, but "preview-agent" is not in CritWeb.static_paths(), so Plug.Static 404s them and the agent never posts agent-ready (Pin stays disabled). The fix (serve /preview-agent/*) belongs to the agent-vendoring task; the spec documents the verified root cause so the fixme can be removed once it lands. Verified: full backend suite (1012 tests, 0 failures) on a clean DB, and the preview spec (6 passed, 1 skipped) stable across repeated runs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mix ecto.reset under MIX_ENV=test was running seeds.exs (guarded Mix.env() in [:dev, :test]), inserting the preview demo review outside the test sandbox and breaking 'no reviews' assertions in overview/reviews tests. Demo seeds are a dev convenience; restrict to :dev. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Local-harness hardening (no product change): - BASE_URL used 'localhost'; macOS resolves that to IPv6 ::1 first while the Phoenix/Bandit test server binds IPv4 only. Switch to 127.0.0.1. - The managed webServer env block didn't forward DB_PORT (local Postgres maps 5433); forward it (default 5432 for CI). NOTE: preview.spec.ts still fails to connect locally on this machine — the pre-existing viewed.spec.ts fails identically, confirming an environmental webServer/networking issue, NOT a product defect. The feature is proven green by crit's TestShareSyncPreview integration test (boots real crit-web, round-trips assets+agent). preview.spec runs in CI (Linux). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
helpers.ts and comment-policy.spec.ts each define their own BASE_URL used for request.post/delete (createReview/createPreviewReview). These drive the API calls that were getting ECONNREFUSED ::1 on macOS — the config BASE_URL only covers page.goto. Align them to 127.0.0.1 too. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The raw controller injects <script src="/preview-agent/..."> into preview HTML, but "preview-agent" was never added to Plug.Static's allowlist (static_paths/0), so every agent script 404'd — the injected agent never booted, so Pin mode could never enable. Add preview-agent to static_paths. Verified: /preview-agent/crit-agent.js now 200 (was 404). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Plug.CSRFProtection raises InvalidCrossOriginRequestError (403) for a
cross-origin GET that returns a JavaScript content-type (its anti-`<script
src>` data-exfiltration guard). A preview iframe is sandboxed with an opaque
origin, so its request for the preview's own app.js is cross-origin and got
the 403 HTML error page — which the browser then refused to execute ('MIME
type text/html is not executable'). The raw endpoint is a read-only GET by
unguessable token with no CSRF surface, so it sets plug_skip_csrf_protection
(honoured by CSRFProtection's register_before_send check).
Regression test: a preview .js asset serves 200 text/javascript.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Three bugs were blocking the preview comment flow in a real browser (all found by manually driving the page, not just unit tests): 1. Comment submit silently dropped. openComposer() called closeComposer() AFTER pendingAnchor was set, and closeComposer() nulls pendingAnchor — so submitComposer() hit `if (!pendingAnchor) return` and never pushed the add_comment event. No event reached the server. Fix: set pendingAnchor after closing the prior composer, and capture it locally in submit. 2. Iframe was cross-origin. The src used data-base-url (Endpoint.url(), e.g. http://localhost:4000); when the page is browsed via another host alias (http://127.0.0.1:4000) the iframe became cross-origin and the agent<->hook postMessage channel (exact-origin-checked both ends) silently broke — killing selection, the composer, and comments. Fix: root-relative iframe src so it is always same-origin as the parent. 3. Marker overlay CSS never loaded. The vendored crit-agent.js fetches /agent-marker.css, but the preview CSP is connect-src 'none' (and crit-web serves it under /preview-agent/). Fix: inline the marker CSS as a <style> in the injected preview HTML, making the agent's failed fetch harmless. Also un-fixme the e2e "pin mode: clicking an element creates a comment" test — it now drives the real cross-frame path and passes (the static_paths + CSRF-skip fixes that unblocked it already landed). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…e-src The injected crit-agent fetches its overlay CSS from <origin>/agent-marker.css (hard-coded in the vendored agent; crit local serves it there too). crit-web only served it under /preview-agent/, so the fetch 404'd in the iframe console even after CSP allowed it. Add a root GET /agent-marker.css route and drop the earlier inline-<style> workaround. Also relax the preview CSP connect-src from 'none' to 'self' so the same-origin marker fetch is allowed (external egress still blocked), and relax frame-src 'none' -> 'self' ONLY when the dev code reloader is on, so Phoenix LiveReload's injected frame stops logging a CSP violation in dev (prod stays frame-src 'none'). Result: preview page console is clean (0 errors), verified in-browser. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… bring preview to parity Reworks preview mode to reuse crit-web's existing review chrome instead of rebuilding it, and lifts the shared pieces into dependency-injected modules consumed by both files mode and preview. - Extract assets/js/comments-panel.js + settings-panel.js as pure, adapter-driven modules used by both document-renderer.js (files) and preview-mode.js (preview). - Preview reuses the shared panel + cards (markdown bodies, resolve/reply/resolved styling) instead of its own; fixes the raw-textContent markdown bug. - Move viewport + Navigate/Pin toggles into the right-aligned header (#crit-preview-controls); hide TOC + comment-nav prev/next in preview. - Add All/Open/Resolved filter pills + Expand-all to the preview panel. - Settings overlay works in preview (theme + about + preview shortcuts); files mode keeps content-width + hide-resolved + full shortcut list. - Comment + reply edit/delete in the preview sidebar, gated to the author (identity passed via data attrs on #crit-preview-layout). - Fix visibility/comment-policy popover z-index so it renders above the open comments panel. - Update e2e/preview.spec.ts for the shared panel structure + filter pills. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Re-sharing a review from the local Crit binary in direct (non-proxy) mode does a cross-origin PUT /api/reviews/:token from the browser. That triggers a CORS preflight, but there was no `options "/reviews/:token"` route, so the preflight hit Phoenix.Router.NoRouteError — and even with a route, LocalhostCors only advertised "POST, DELETE, OPTIONS", omitting PUT. - Add options routes for /reviews/:token and /reviews/:token/comments so the preflight reaches the LocalhostCors plug instead of 404ing. - Add GET + PUT to access-control-allow-methods. Pre-existing gap in direct-mode re-share/pull; surfaced via preview sharing. Test: OPTIONS preflight returns 204 with PUT advertised for localhost/127.0.0.1. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Review follow-up: lock the security invariant that broadening allow-methods to PUT does not widen which origins are trusted — only the @localhost regex gate reflects CORS headers. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Owner
Author
|
Companion PR (local CLI side): tomasz-tomczyk/crit#626 |
3 tasks
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #241 +/- ##
==========================================
+ Coverage 82.27% 82.42% +0.14%
==========================================
Files 89 89
Lines 3047 3089 +42
==========================================
+ Hits 2507 2546 +39
- Misses 540 543 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The Playwright harness loads the server over http://127.0.0.1 (commit added that to dodge macOS resolving localhost to IPv6 ::1 while Bandit binds IPv4), but the endpoint :url host is "localhost" (config.exs) and config/test.exs left check_origin at its default of true. The 127.0.0.1 origin then failed the check, 403-ing the LiveView socket (WebSocket AND longpoll) — so the LiveView never connected, "init" was never pushed, and every review page stayed stuck on "Loading document...", timing out all 131 E2E specs. Disabling check_origin in test (no cross-origin risk there) lets the socket accept both 127.0.0.1 and localhost. Verified: full chromium E2E now 130/131 (the one remaining color-contrast failure on .crit-round-diff-btn pre-exists on origin/main — unrelated to this branch). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Hosted side of CRI-78 "shareable preview mode" — renders crit
previewsnapshots at/r/:tokenand brings the review chrome to parity with the local CLI.ReviewLivebranches forreview_type=preview, rendering the snapshot in an iframe with viewport toggles + a DOM pin panel. Raw controller serves preview assets (base64/mime, agent injection, CSP).review_typeon reviews,encodingon round snapshots,dom_anchorjsonb on comments; context threads them through create/upsert.share-receiverforwards preview payloads to/api/reviews(proxy-auth transport).LocalhostCorsnow allows PUT and the router declares per-token OPTIONS routes, so direct-mode re-share preflight no longer 404s.Review
/crit-review): SHIP IT, no blockers (elixir-expert, validated)mix precommit: 1015 tests, 0 failures; sobelow cleanTest plan
mix precommitgreen; preview e2e specs; CORS preflight tests (PUT advertised, non-localhost origin rejected).make e2e-sharepasses against this branch.🤖 Generated with Claude Code