fix(comments): return correct status codes for comment reactions

[martian56]

What

Closes #146. CommentHandler.AddReaction mapped every error to HTTP 409 and echoed the raw err.Error() back to the client, so a missing comment, a permission failure, or a DB error all looked like a duplicate-reaction conflict and leaked internal strings. RemoveReaction had the inverse problem: every failure became a generic 500.

How

• AddReaction now switches on the service's sentinel errors: ErrReactionExists → 409 ("Already reacted"), a not-found / no-access error → 404, everything else → 500 with a generic message. No more raw err.Error() leakage.
• RemoveReaction returns 404 for not-found / no-access and only 500 for genuinely unexpected failures.
• CommentService.AddReaction translates the unique-constraint violation to ErrReactionExists (mirroring the issue-reaction service) instead of returning the raw GORM error.
• Added a small commentAccessNotFound helper so both handlers share the same not-found mapping as the rest of the comment handlers.

Testing

New internal/handler/comment_reaction_test.go:

• First reaction → 201; the same reaction again → 409; reacting on a missing comment → 404 (was 409).
• Remove after add → 204; removing from a missing comment → 404 (was 500).

go test ./internal/handler ./internal/service green.

AI assistance

Produced with the help of Claude Code (Claude Opus 4.8). AI-assisted commits carry a Co-Authored-By trailer.

Summary by CodeRabbit

• Bug Fixes

  • Reaction actions now return clearer status codes: 404 Not Found for missing or inaccessible comments, 409 Conflict for duplicate reactions, and 500 only for unexpected failures.
  • Removing a reaction now correctly reports missing comments as 404 Not Found instead of a generic server error.

• Tests

  • Added coverage for adding and removing comment reactions, including success cases, duplicate reactions, and missing-comment responses.

[strix-security]

Strix Security Review

No security issues found.

_{Updated for e28de66.}

---

Reviewed by Strix
_{Re-run review · Configure security review settings}

[coderabbitai]

📝 Walkthrough

Walkthrough

Refactors comment reaction error handling: AddReaction and RemoveReaction handlers now classify errors into 409 (duplicate reaction), 404 (not-found/access), and 500 (other) responses instead of generic/conflict-only responses. CommentService.AddReaction detects duplicate reactions via gorm.ErrDuplicatedKey. New handler tests validate the status codes.

Changes

Reaction Error Handling

Layer / File(s)	Summary
Duplicate-key detection in service apps/api/internal/service/comment.go	Adds gorm import and uses errors.Is(err, gorm.ErrDuplicatedKey) in AddReaction to return ErrReactionExists on duplicate reactions.
Handler error mapping apps/api/internal/handler/comment.go	Adds commentAccessNotFound helper; AddReaction now returns 409/404/500 via a switch instead of leaking err.Error(); RemoveReaction adds a 404 branch before falling back to 500.
Handler tests apps/api/internal/handler/comment_reaction_test.go	Adds tests verifying 201/409/404 for AddReaction and 201/204/404 for RemoveReaction.

Estimated code review effort: 2 (Simple) | ~10 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant CommentHandler
  participant CommentService

  Client->>CommentHandler: POST AddReaction
  CommentHandler->>CommentService: AddReaction(userID, commentID, emoji)
  CommentService-->>CommentHandler: ErrReactionExists / not-found error / other error
  alt ErrReactionExists
    CommentHandler-->>Client: 409 Conflict
  else commentAccessNotFound(err)
    CommentHandler-->>Client: 404 Not Found
  else other error
    CommentHandler-->>Client: 500 Internal Server Error
  end

Loading

Suggested labels: API

Poem

A rabbit hopped through error trails,
Sorting 409s from 404 fails,
No more leaking secrets in the JSON night,
Duplicate keys now caught just right,
Tests confirm each hop is tight! 🐇✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title is concise, conventional, and accurately summarizes the main comment reaction status-code fix.
Description check	✅ Passed	The description covers the problem, solution, testing, and AI disclosure, but several template sections remain unfilled.
Linked Issues check	✅ Passed	The PR implements the requested #146 fixes: correct status mapping, no raw error leakage, duplicate-key translation, and tests.
Out of Scope Changes check	✅ Passed	The diff stays focused on comment reaction handling, service error translation, and tests, with no clear unrelated changes.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/comment-reaction-errors

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.2)

level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies"

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

apps/api/internal/handler/comment_reaction_test.go (1)

36-55: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Good coverage for the documented cases; consider also asserting the same-emoji conflict path for removal parity.

Coverage nicely mirrors AddReaction's tests (create/remove happy path + missing-comment 404). One gap: there's no test for removing a reaction that never existed on an otherwise-valid comment (e.g., calling DELETE with an emoji the user never reacted with) — if the store's Remove treats "no matching row" as a no-op success rather than an error, the current 404 branch here would never engage for that scenario, worth confirming this is intentional (idempotent removal) rather than an untested gap.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/api/internal/handler/comment_reaction_test.go` around lines 36 - 55, The
remove-reaction coverage in Comment_RemoveReaction_StatusCodes is missing the
case where the comment exists but the requested emoji reaction was never
created. Add an assertion in this test (or a nearby reaction removal test) that
calls DELETE on a valid comment with a non-existent emoji and verify the
intended behavior, using AddReaction/RemoveReaction and the reactions endpoint
helpers to confirm whether removal is intentionally idempotent or should return
an error.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@apps/api/internal/handler/comment_reaction_test.go`:
- Around line 36-55: The remove-reaction coverage in
Comment_RemoveReaction_StatusCodes is missing the case where the comment exists
but the requested emoji reaction was never created. Add an assertion in this
test (or a nearby reaction removal test) that calls DELETE on a valid comment
with a non-existent emoji and verify the intended behavior, using
AddReaction/RemoveReaction and the reactions endpoint helpers to confirm whether
removal is intentionally idempotent or should return an error.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: f9a87fd1-95f5-4c75-bb79-445977a524e2

📥 Commits

Reviewing files that changed from the base of the PR and between 22fe70e and e28de66.

📒 Files selected for processing (3)

• apps/api/internal/handler/comment.go
• apps/api/internal/handler/comment_reaction_test.go
• apps/api/internal/service/comment.go