Skip to content

feat: author attribution for shared reviews#131

Merged
tomasz-tomczyk merged 1 commit intomainfrom
share-author-attribution
Apr 27, 2026
Merged

feat: author attribution for shared reviews#131
tomasz-tomczyk merged 1 commit intomainfrom
share-author-attribution

Conversation

@tomasz-tomczyk
Copy link
Copy Markdown
Owner

@tomasz-tomczyk tomasz-tomczyk commented Apr 27, 2026

Summary

  • Replaces the "imported" placeholder author with proper attribution: a cosmetic author_display_name and a verified user_id (UUID FK to users.id).
  • Server treats payload-supplied user_id as untrusted intent — only the bearer token decides what gets stamped. Existing comments matched by external_id keep their original user_id on roundtrip (preserves foreign-author attribution).
  • Demo export filter swapped from author_identity == "imported" to a configurable allowlist of comment IDs (DEMO_COMMENT_IDS env var).
  • LiveView demo filter now also keeps current_user_id-matched comments so authed visitors see their own comments.
  • Migrations: new comments.user_id nullable FK (ON DELETE SET NULL) + partial unique index on (review_id, external_id).
  • Test-only POST /api/test/seed-user (gated by Mix.env() in [:test, :dev]) for integration tests.
  • /api/auth/whoami returns id so the CLI can cache it.

Review

  • Code review: passed (elixir-expert fresh-eyes pass + targeted fixes)
  • Parity audit: N/A (no review-page files touched)

Test plan

  • mix precommit — 460/460 pass
  • mix test test/crit/reviews_test.exs test/crit_web/live/review_live_test.exs — 130/130 pass
  • Full crit↔crit-web share integration suite (./scripts/e2e-share.sh) — all 24 attribution + share-sync tests pass
  • Set DEMO_COMMENT_IDS fly secret post-deploy

See also: tomasz-tomczyk/crit#371 (CLI side)

🤖 Generated with Claude Code

Add proper author attribution when sharing a review from crit to crit-web.
Replaces the placeholder "imported" author with two complementary signals:

- author_display_name (cosmetic) flows through from CLI config / git user /
  GitHub / "Anonymous" fallback.
- user_id (verified identity) is stamped on comments only when the bearer
  token verifies, never trusted from the payload.

Server attribution rules:
- Existing comment matched by external_id keeps its user_id (preserves
  foreign attribution on roundtrip).
- Anon request -> user_id = NULL regardless of payload.
- Authed request: payload user_id matching current_user wins; anything
  else (including spoof attempts) drops to NULL.

Other changes:
- comments.user_id nullable FK to users.id (ON DELETE SET NULL)
- unique index on (review_id, external_id) WHERE external_id IS NOT NULL
- demo export filter switched to a configurable allowlist of comment IDs
  (DEMO_COMMENT_IDS env var) — replaces the legacy author_identity = "imported"
- /api/auth/whoami returns id so the CLI can cache it
- test-only POST /api/test/seed-user for integration tests
- demo review filter (LiveView) now also keeps current_user's own comments

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 27, 2026

Codecov Report

❌ Patch coverage is 77.22772% with 23 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.80%. Comparing base (36ba322) to head (1029bbf).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
lib/crit/reviews.ex 81.25% 12 Missing ⚠️
lib/crit_web/controllers/api_controller.ex 18.18% 9 Missing ⚠️
lib/crit_web/live/review_live.ex 95.83% 1 Missing ⚠️
lib/crit_web/router.ex 0.00% 1 Missing ⚠️

❌ Your patch status has failed because the patch coverage (77.22%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #131      +/-   ##
==========================================
- Coverage   76.11%   75.80%   -0.31%     
==========================================
  Files          52       52              
  Lines        1524     1579      +55     
==========================================
+ Hits         1160     1197      +37     
- Misses        364      382      +18     
Flag Coverage Δ
unit 75.80% <77.22%> (-0.31%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@tomasz-tomczyk tomasz-tomczyk merged commit 27f7d6a into main Apr 27, 2026
3 of 4 checks passed
@tomasz-tomczyk tomasz-tomczyk deleted the share-author-attribution branch April 27, 2026 08:54
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