Feat/react posts emoji#556
Open
VongoSanDi wants to merge 8 commits into
Open
Conversation
📝 WalkthroughWalkthroughThis PR adds forum post reaction functionality (emoji reactions to posts). It introduces database tables, storage repository methods, HTTP PUT/DELETE handlers, error handling, integration tests, and SQLx query metadata for managing user reactions on forum posts with permission checks. ChangesForum Post Reaction Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| user_id INT NOT NULL REFERENCES users(id) ON DELETE CASCADE, | ||
| emoji TEXT NOT NULL, | ||
| created_at TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT NOW(), | ||
| updated_at TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT NOW(), |
Contributor
There was a problem hiding this comment.
I think that updated_at isn't needed here
Contributor
Author
There was a problem hiding this comment.
For me it's part of the combo created_at / updated_at, I agree update_at isn't really need and so is created_at no ?
Contributor
There was a problem hiding this comment.
created_at makes sense, it's just that we don't update reactions, we only create new ones (or remove them)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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.
Inline comments:
In `@backend/common/src/error/mod.rs`:
- Around line 347-348: The enum variant CouldNotFindForumPostReaction has the
wrong error text and isn't handled as a 404 in the HTTP-status mapping; update
the enum variant definition to #[error("could not find forum post reaction")]
CouldNotFindForumPostReaction(#[source] sqlx::Error) and then add that variant
to the match that maps errors to 404 (the error-to-status mapping function/match
around the 678+ region) so that CouldNotFindForumPostReaction returns 404
instead of falling through to 500.
In
`@backend/storage/.sqlx/query-fe8367af19eab2a0b0d0be3c828c0c9ee871b4a05e63de8d770a6d8050b1bdd8.json`:
- Line 3: The upsert CTE `upserted` can return zero rows when the emoji is
unchanged, causing the outer LEFT JOIN against `upserted up` to yield NULL
reaction fields; change the outer SELECT to join directly to
`forum_post_reactions` filtered by user_id = $2 (instead of LEFT JOIN upserted)
so the response always reflects the persisted reaction, and update the SELECT
projection to use `forum_post_reactions.id`, `.forum_post_id`, `.user_id`, and
`.emoji` (rather than `up.reaction_*`) while keeping the `upserted` CTE in place
for the write side-effect.
In `@backend/storage/migrations/fixtures/fixtures.sql`:
- Around line 232-233: The sequence public.forum_posts_id_seq must be advanced
to avoid collisions with seeded IDs (the fixtures insert rows up to id 10);
update the sequence value for forum_posts (public.forum_posts_id_seq) to at
least 10 (preferably set next value to 11) after the INSERTs so subsequent
inserts use unique IDs — locate the fixture INSERTs for forum_posts and
add/adjust a SETVAL/ALTER SEQUENCE call referencing public.forum_posts_id_seq to
set its next value accordingly.
In `@backend/storage/src/models/forum.rs`:
- Around line 335-338: The UserCreatedForumPostReaction struct's emoji field
currently accepts arbitrary input; add server-side validation by introducing
either a newtype (e.g., Emoji) with TryFrom<String> or a validate() method on
UserCreatedForumPostReaction that is invoked wherever instances are
created/parsed, enforcing: trim and reject empty/whitespace-only strings,
enforce a reasonable max length (e.g., limit to ~4–8 Unicode grapheme clusters
or a byte cap such as 64 bytes), and optionally run a simple emoji-only
check/allowlist or Unicode emoji detection; return a clear validation error
instead of persisting invalid emoji. Ensure the validation is used during
deserialization/creation paths so invalid payloads are rejected before DB
storage.
In `@backend/storage/src/repositories/forum_repository.rs`:
- Around line 1657-1659: The query is fetching a forum post but maps failures to
Error::CouldNotFindForumThread which is misleading; change the error mapping on
the .fetch_one(self.borrow()).await call to a post-specific variant (e.g.
Error::CouldNotFindForumPost) and if that variant doesn't exist, add a new
Error::CouldNotFindForumPost variant and use it in place of
Error::CouldNotFindForumThread so failures from this fetch are reported as post
lookup errors.
- Around line 1531-1565: The upsert CTE (upserted) can return no row when the ON
CONFLICT uses "DO UPDATE ... WHERE forum_post_reactions.emoji IS DISTINCT FROM
EXCLUDED.emoji", causing existing reactions to disappear from the result; remove
the WHERE clause so the UPDATE always returns a row and instead make updated_at
conditional so it only changes on real emoji changes (e.g., replace "DO UPDATE
SET emoji = EXCLUDED.emoji, updated_at = NOW() WHERE forum_post_reactions.emoji
IS DISTINCT FROM EXCLUDED.emoji" with an unconditional "DO UPDATE SET emoji =
EXCLUDED.emoji, updated_at = CASE WHEN forum_post_reactions.emoji IS DISTINCT
FROM EXCLUDED.emoji THEN NOW() ELSE forum_post_reactions.updated_at END" so the
upserted CTE always RETURNING the reaction row).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 81f2df4e-1ffc-433d-810c-a6c2e7c165f0
📒 Files selected for processing (28)
.gitignorebackend/api/src/api_doc.rsbackend/api/src/handlers/forum/delete_forum_post_reaction.rsbackend/api/src/handlers/forum/mod.rsbackend/api/src/handlers/forum/set_forum_post_reaction.rsbackend/api/tests/fixtures/with_test_forum_post.sqlbackend/api/tests/fixtures/with_test_forum_reaction.sqlbackend/api/tests/test_forum_post.rsbackend/common/src/error/mod.rsbackend/storage/.sqlx/query-07435792b9b25324453fb323c5edc0f0a3765f2b32969f229d8675f08ac0a006.jsonbackend/storage/.sqlx/query-43ca35fea526b25dffb69e86700c0d3a1bdebdb973df5059ff7eb3e9a933eb33.jsonbackend/storage/.sqlx/query-46d7eee133e0653d9f11ab67f5d6faec7050c9b4c6a8c78e2097015d3e0fb7fb.jsonbackend/storage/.sqlx/query-4edda78ffd766d9ec15eb015fe5b985755924b0f0b44d5cf9411059cfbc5c757.jsonbackend/storage/.sqlx/query-599587c7ce69b090843274603171c411af859ae256fc01eaf66af2aa2a922900.jsonbackend/storage/.sqlx/query-6c4a5f875d86d837493c8bf2a8e4f6fb93f4e50dce73ca6d07d42df32a80308a.jsonbackend/storage/.sqlx/query-7da73662a96a68e239d011598ace3bc5b287a82c5b0c34ce9543842a1bed0ea4.jsonbackend/storage/.sqlx/query-9b42210d7c01c72c57238e1b0984cbfee280f3d5dcc6592a58eec33562760504.jsonbackend/storage/.sqlx/query-af3da3d706d7dfb3a66c7bb2a3f9747f2717ea16a0a01de22630c87f42bec1b6.jsonbackend/storage/.sqlx/query-c45f235654a1b2aa8c849c5644443fe34ea7a4dd976fe6b4405e7b4a585a1325.jsonbackend/storage/.sqlx/query-c4e55538610671cfcf982e49c58425223e356c29912d19d4e62cf7b576637c0a.jsonbackend/storage/.sqlx/query-d94c7cf9c02a4f060345d02ac4bd2434069fc46d43e6f3e7e3618737c2dcd547.jsonbackend/storage/.sqlx/query-f4de9d3dad0a4229f75304798debf8ab4a602081f6e429658262bc9e58f7320b.jsonbackend/storage/.sqlx/query-f6d849721ff84614c129c14455d9a6adbe0ad29b7876963d5bd9015c0f73ba9d.jsonbackend/storage/.sqlx/query-fe8367af19eab2a0b0d0be3c828c0c9ee871b4a05e63de8d770a6d8050b1bdd8.jsonbackend/storage/migrations/20250312215600_initdb.sqlbackend/storage/migrations/fixtures/fixtures.sqlbackend/storage/src/models/forum.rsbackend/storage/src/repositories/forum_repository.rs
💤 Files with no reviewable changes (11)
- backend/storage/.sqlx/query-c45f235654a1b2aa8c849c5644443fe34ea7a4dd976fe6b4405e7b4a585a1325.json
- backend/storage/.sqlx/query-43ca35fea526b25dffb69e86700c0d3a1bdebdb973df5059ff7eb3e9a933eb33.json
- backend/storage/.sqlx/query-46d7eee133e0653d9f11ab67f5d6faec7050c9b4c6a8c78e2097015d3e0fb7fb.json
- backend/storage/.sqlx/query-7da73662a96a68e239d011598ace3bc5b287a82c5b0c34ce9543842a1bed0ea4.json
- backend/storage/.sqlx/query-599587c7ce69b090843274603171c411af859ae256fc01eaf66af2aa2a922900.json
- backend/storage/.sqlx/query-c4e55538610671cfcf982e49c58425223e356c29912d19d4e62cf7b576637c0a.json
- backend/storage/.sqlx/query-4edda78ffd766d9ec15eb015fe5b985755924b0f0b44d5cf9411059cfbc5c757.json
- backend/storage/.sqlx/query-6c4a5f875d86d837493c8bf2a8e4f6fb93f4e50dce73ca6d07d42df32a80308a.json
- backend/storage/.sqlx/query-f4de9d3dad0a4229f75304798debf8ab4a602081f6e429658262bc9e58f7320b.json
- backend/storage/.sqlx/query-d94c7cf9c02a4f060345d02ac4bd2434069fc46d43e6f3e7e3618737c2dcd547.json
- backend/storage/.sqlx/query-f6d849721ff84614c129c14455d9a6adbe0ad29b7876963d5bd9015c0f73ba9d.json
| @@ -0,0 +1,126 @@ | |||
| { | |||
| "db_name": "PostgreSQL", | |||
| "query": "\n WITH upserted AS (\n INSERT INTO forum_post_reactions (forum_post_id, user_id, emoji)\n VALUES ($1, $2, $3)\n ON CONFLICT (forum_post_id, user_id)\n DO UPDATE SET\n emoji = EXCLUDED.emoji,\n updated_at = NOW()\n WHERE forum_post_reactions.emoji IS DISTINCT FROM EXCLUDED.emoji\n RETURNING\n id AS reaction_id,\n forum_post_id AS reaction_forum_post_id,\n user_id AS reaction_user_id,\n emoji AS reaction_emoji\n )\n SELECT\n fp.id,\n fp.content,\n fp.created_at,\n fp.updated_at,\n fp.sticky,\n fp.locked,\n fp.forum_thread_id,\n u.id AS created_by_user_id,\n u.username AS created_by_user_username,\n u.class_name AS created_by_user_class_name,\n u.avatar AS created_by_user_avatar,\n u.banned AS created_by_user_banned,\n u.warned AS created_by_user_warned,\n u.custom_title AS created_by_user_custom_title,\n up.reaction_id AS \"reaction_id?\",\n up.reaction_forum_post_id AS \"reaction_forum_post_id?\",\n up.reaction_user_id AS \"reaction_user_id?\",\n up.reaction_emoji AS \"reaction_emoji?\"\n FROM forum_posts fp\n JOIN users u ON fp.created_by_id = u.id\n LEFT JOIN upserted up\n ON up.reaction_forum_post_id = fp.id\n WHERE fp.id = $1\n ", | |||
There was a problem hiding this comment.
Upsert returns reaction: None when the same emoji is re-submitted.
The PostgreSQL ON CONFLICT DO UPDATE WHERE <condition> clause behaves as DO NOTHING when the condition is false (i.e., the incoming emoji is the same as the stored one). In that case, the RETURNING clause emits zero rows. The CTE upserted is empty, the LEFT JOIN upserted up ON ... produces NULLs for all up.* columns, and ForumPostHierarchy.reaction is deserialized as None — even though the reaction still exists in the database.
Reproduce: call PUT with emoji "👍" twice; the second response has "reaction": null.
The fix is to have the outer SELECT join directly against the forum_post_reactions table for the reaction data (using $2 as the user filter), keeping the CTE purely as the write side-effect:
🐛 Proposed fix (change the LEFT JOIN in the outer SELECT)
- LEFT JOIN upserted up
- ON up.reaction_forum_post_id = fp.id
+ LEFT JOIN forum_post_reactions r
+ ON r.forum_post_id = fp.id AND r.user_id = $2
WHERE fp.id = $1and update the SELECT projection accordingly:
- up.reaction_id AS "reaction_id?",
- up.reaction_forum_post_id AS "reaction_forum_post_id?",
- up.reaction_user_id AS "reaction_user_id?",
- up.reaction_emoji AS "reaction_emoji?"
+ r.id AS "reaction_id?",
+ r.forum_post_id AS "reaction_forum_post_id?",
+ r.user_id AS "reaction_user_id?",
+ r.emoji AS "reaction_emoji?"The CTE still runs (write-side-effect CTEs execute even when unreferenced in PG 12+), updated_at is only touched when the emoji actually changes, and the response always reflects the current persisted reaction.
Note: This fix must be applied in
backend/storage/src/repositories/forum_repository.rs(the SQL source); the SQLx JSON artefact will be regenerated bycargo sqlx prepare.
🤖 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
`@backend/storage/.sqlx/query-fe8367af19eab2a0b0d0be3c828c0c9ee871b4a05e63de8d770a6d8050b1bdd8.json`
at line 3, The upsert CTE `upserted` can return zero rows when the emoji is
unchanged, causing the outer LEFT JOIN against `upserted up` to yield NULL
reaction fields; change the outer SELECT to join directly to
`forum_post_reactions` filtered by user_id = $2 (instead of LEFT JOIN upserted)
so the response always reflects the persisted reaction, and update the SELECT
projection to use `forum_post_reactions.id`, `.forum_post_id`, `.user_id`, and
`.emoji` (rather than `up.reaction_*`) while keeping the `upserted` CTE in place
for the write side-effect.
Comment on lines
+335
to
+338
| #[derive(Debug, Serialize, Deserialize, FromRow, ToSchema)] | ||
| pub struct UserCreatedForumPostReaction { | ||
| pub emoji: String, | ||
| } |
There was a problem hiding this comment.
emoji field accepts arbitrary input — add validation.
emoji: String has no constraints: empty strings, whitespace-only values, multi-kilobyte text, or HTML payloads are all accepted. At minimum, enforce:
- Non-empty / non-whitespace-only content.
- A reasonable max length (e.g., a handful of bytes sufficient for a single multi-codepoint emoji).
- Optionally, restrict to an allowlist of emoji codepoints or a validated emoji string.
Without server-side validation, the API is exposed to storing unexpected content that could cause display issues or, if the frontend ever renders this field without escaping, XSS.
💡 Sketch of a validation approach in the handler
// In set_forum_post_reaction.rs, before calling the repository:
+ let emoji = reaction.into_inner();
+ let emoji_str = emoji.emoji.trim();
+ if emoji_str.is_empty() || emoji_str.len() > 64 {
+ return Err(arcadia_common::error::Error::BadRequest(
+ "emoji must be a non-empty string of at most 64 bytes".into(),
+ ));
+ }🤖 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 `@backend/storage/src/models/forum.rs` around lines 335 - 338, The
UserCreatedForumPostReaction struct's emoji field currently accepts arbitrary
input; add server-side validation by introducing either a newtype (e.g., Emoji)
with TryFrom<String> or a validate() method on UserCreatedForumPostReaction that
is invoked wherever instances are created/parsed, enforcing: trim and reject
empty/whitespace-only strings, enforce a reasonable max length (e.g., limit to
~4–8 Unicode grapheme clusters or a byte cap such as 64 bytes), and optionally
run a simple emoji-only check/allowlist or Unicode emoji detection; return a
clear validation error instead of persisting invalid emoji. Ensure the
validation is used during deserialization/creation paths so invalid payloads are
rejected before DB storage.
| pub content: String, | ||
| pub sticky: bool, | ||
| pub locked: bool, | ||
| pub reaction: Option<ForumPostReaction>, |
Contributor
There was a problem hiding this comment.
this would rather be a Vec<ForumPostReaction> since multiple users can react on the same post.
|
|
||
| #[derive(Debug, Serialize, Deserialize, FromRow, ToSchema)] | ||
| pub struct UserCreatedForumPostReaction { | ||
| pub emoji: String, |
Contributor
There was a problem hiding this comment.
not sure if rust has some way of typing this as an emoji type, but otherwise we should do some validation to make sure only emojis can be used here
It wasn't included in the 404 match arm
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/common/src/error/mod.rs (1)
350-351:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWrong error message on
CouldNotFindForumPostReaction— still unresolved.The
#[error]text is still"could not find forum post", which duplicatesCouldNotFindForumPostand produces a misleading JSON error body for callers. The 404 status-mapping fix from the previous review round was applied correctly, but this text was not updated.🐛 Proposed fix
- #[error("could not find forum post")] + #[error("could not find forum post reaction")] CouldNotFindForumPostReaction(#[source] sqlx::Error),🤖 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 `@backend/common/src/error/mod.rs` around lines 350 - 351, The error enum variant CouldNotFindForumPostReaction has the wrong #[error(...)] text ("could not find forum post"); update its error message to a distinct, accurate string such as "could not find forum post reaction" so callers receive the correct JSON body; locate the enum variant CouldNotFindForumPostReaction and change the #[error("...")] attribute accordingly (keeping the #[source] sqlx::Error intact).
🧹 Nitpick comments (2)
backend/api/tests/test_forum_post.rs (2)
42-53: ⚡ Quick winAvoid order-coupled assertions in list responses.
These checks rely on fixed list ordering. Prefer selecting posts by identity (or reaction presence) so tests remain stable if default ordering changes.
Proposed refactor
- assert_eq!(posts.total_items, 2); - assert_eq!(posts.results[0].created_by.id, 100); - assert!(posts.results[0].reaction.is_some()); - assert_eq!(posts.results[0].reaction.as_ref().unwrap().emoji, "🥰"); - assert_eq!( - posts.results[0].reaction.as_ref().unwrap().forum_post_id, - 100 - ); - assert!(posts.results[1].reaction.is_none()); + assert_eq!(posts.total_items, 2); + let reacted_post = posts + .results + .iter() + .find(|p| p.reaction.is_some()) + .expect("expected one reacted post"); + assert_eq!(reacted_post.created_by.id, 100); + assert_eq!(reacted_post.reaction.as_ref().unwrap().emoji, "🥰"); + assert_eq!(reacted_post.reaction.as_ref().unwrap().forum_post_id, 100); + + let non_reacted_post = posts + .results + .iter() + .find(|p| p.reaction.is_none()) + .expect("expected one non-reacted post"); + assert_ne!(non_reacted_post.id, reacted_post.id);🤖 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 `@backend/api/tests/test_forum_post.rs` around lines 42 - 53, The test in backend/api/tests/test_forum_post.rs currently assumes a fixed ordering of posts (asserting posts.results[0] and posts.results[1]); change the assertions to locate posts by identity or reaction instead: use posts.results to find the item with created_by.id == 100 and assert that that found post has reaction.is_some() and that its reaction.emoji == "🥰" and reaction.forum_post_id == 100, and separately locate the other post (e.g., by a different id or by reaction.is_none()) and assert reaction.is_none(); this removes order coupling while keeping checks against posts.results, created_by.id, and reaction.
146-154: ⚡ Quick winAssert DB state is unchanged after invalid reaction payload.
The test verifies
400, but it should also confirm no reaction was created/updated for the target post.Proposed refactor
let resp = test::call_service(&service, req).await; assert_eq!(resp.status(), StatusCode::BAD_REQUEST); let body = test::read_body(resp).await; let body = String::from_utf8(body.to_vec()).unwrap(); assert!(body.contains("could not update forum post reaction")); + + let req_verify = test::TestRequest::get() + .uri("/api/forum/thread/posts?thread_id=103&page_size=10") + .insert_header(auth_header(&user.token)) + .to_request(); + let posts_after: PaginatedResults<ForumPostHierarchy> = + common::call_and_read_body_json_with_status(&service, req_verify, StatusCode::OK).await; + assert!(posts_after.results[0].reaction.is_none()); }🤖 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 `@backend/api/tests/test_forum_post.rs` around lines 146 - 154, The test currently only asserts a 400 response (using test::call_service and StatusCode::BAD_REQUEST) but must also verify the DB state is unchanged: after the existing assertions (resp and body via test::read_body), query the reactions for the target post (use the same test DB connection/pool fixture used elsewhere in this test suite) and assert that no reaction record was created or modified for that post/user (or that the reaction count remains the same / reaction value unchanged). Locate the test function in test_forum_post.rs and add a DB check using the repository/DAO function you already have (e.g., get_post_reactions, find_reaction_by_post_and_user, or direct query against the post_reactions table) to confirm no new/updated reaction exists for the target post.
🤖 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.
Inline comments:
In
`@backend/storage/.sqlx/query-428a678919b1a16a2614dee0d093279f3235e39a250ce5a3ca2ed260c8f71d54.json`:
- Line 3: The upsert uses a WHERE clause that prevents RETURNING from emitting a
row when the emoji is unchanged, causing the upserted CTE to be empty and
reaction_* fields to be NULL; fix by removing the WHERE clause from the ON
CONFLICT DO UPDATE in the SQL used by forum_repository (the INSERT INTO
forum_post_reactions ... ON CONFLICT (forum_post_id, user_id) DO UPDATE SET
emoji = EXCLUDED.emoji — remove the "WHERE forum_post_reactions.emoji IS
DISTINCT FROM EXCLUDED.emoji" guard so the UPDATE always runs and RETURNING
always yields a row), then regenerate the sqlx metadata (run sqlx prepare /
rebuild) so the JSON query file reflects the updated SQL.
---
Duplicate comments:
In `@backend/common/src/error/mod.rs`:
- Around line 350-351: The error enum variant CouldNotFindForumPostReaction has
the wrong #[error(...)] text ("could not find forum post"); update its error
message to a distinct, accurate string such as "could not find forum post
reaction" so callers receive the correct JSON body; locate the enum variant
CouldNotFindForumPostReaction and change the #[error("...")] attribute
accordingly (keeping the #[source] sqlx::Error intact).
---
Nitpick comments:
In `@backend/api/tests/test_forum_post.rs`:
- Around line 42-53: The test in backend/api/tests/test_forum_post.rs currently
assumes a fixed ordering of posts (asserting posts.results[0] and
posts.results[1]); change the assertions to locate posts by identity or reaction
instead: use posts.results to find the item with created_by.id == 100 and assert
that that found post has reaction.is_some() and that its reaction.emoji == "🥰"
and reaction.forum_post_id == 100, and separately locate the other post (e.g.,
by a different id or by reaction.is_none()) and assert reaction.is_none(); this
removes order coupling while keeping checks against posts.results,
created_by.id, and reaction.
- Around line 146-154: The test currently only asserts a 400 response (using
test::call_service and StatusCode::BAD_REQUEST) but must also verify the DB
state is unchanged: after the existing assertions (resp and body via
test::read_body), query the reactions for the target post (use the same test DB
connection/pool fixture used elsewhere in this test suite) and assert that no
reaction record was created or modified for that post/user (or that the reaction
count remains the same / reaction value unchanged). Locate the test function in
test_forum_post.rs and add a DB check using the repository/DAO function you
already have (e.g., get_post_reactions, find_reaction_by_post_and_user, or
direct query against the post_reactions table) to confirm no new/updated
reaction exists for the target post.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6f68fd00-4c4c-4ffc-a229-b188e6123d06
📒 Files selected for processing (9)
backend/api/tests/fixtures/with_test_forum_reaction.sqlbackend/api/tests/test_forum_post.rsbackend/common/src/error/mod.rsbackend/storage/.sqlx/query-428a678919b1a16a2614dee0d093279f3235e39a250ce5a3ca2ed260c8f71d54.jsonbackend/storage/.sqlx/query-9b42210d7c01c72c57238e1b0984cbfee280f3d5dcc6592a58eec33562760504.jsonbackend/storage/.sqlx/query-af3da3d706d7dfb3a66c7bb2a3f9747f2717ea16a0a01de22630c87f42bec1b6.jsonbackend/storage/migrations/20250312215600_initdb.sqlbackend/storage/migrations/fixtures/fixtures.sqlbackend/storage/src/repositories/forum_repository.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- backend/storage/.sqlx/query-9b42210d7c01c72c57238e1b0984cbfee280f3d5dcc6592a58eec33562760504.json
- backend/storage/migrations/20250312215600_initdb.sql
- backend/storage/.sqlx/query-af3da3d706d7dfb3a66c7bb2a3f9747f2717ea16a0a01de22630c87f42bec1b6.json
- backend/storage/src/repositories/forum_repository.rs
| @@ -0,0 +1,126 @@ | |||
| { | |||
| "db_name": "PostgreSQL", | |||
| "query": "\n WITH upserted AS (\n INSERT INTO forum_post_reactions (forum_post_id, user_id, emoji)\n VALUES ($1, $2, $3)\n ON CONFLICT (forum_post_id, user_id)\n DO UPDATE SET\n emoji = EXCLUDED.emoji\n WHERE forum_post_reactions.emoji IS DISTINCT FROM EXCLUDED.emoji\n RETURNING\n id AS reaction_id,\n forum_post_id AS reaction_forum_post_id,\n user_id AS reaction_user_id,\n emoji AS reaction_emoji\n )\n SELECT\n fp.id,\n fp.content,\n fp.created_at,\n fp.updated_at,\n fp.sticky,\n fp.locked,\n fp.forum_thread_id,\n u.id AS created_by_user_id,\n u.username AS created_by_user_username,\n u.class_name AS created_by_user_class_name,\n u.avatar AS created_by_user_avatar,\n u.banned AS created_by_user_banned,\n u.warned AS created_by_user_warned,\n u.custom_title AS created_by_user_custom_title,\n up.reaction_id AS \"reaction_id?\",\n up.reaction_forum_post_id AS \"reaction_forum_post_id?\",\n up.reaction_user_id AS \"reaction_user_id?\",\n up.reaction_emoji AS \"reaction_emoji?\"\n FROM forum_posts fp\n JOIN users u ON fp.created_by_id = u.id\n LEFT JOIN upserted up\n ON up.reaction_forum_post_id = fp.id\n WHERE fp.id = $1\n ", | |||
There was a problem hiding this comment.
No-op upsert silently returns reaction: null when the same emoji is resubmitted.
The DO UPDATE ... WHERE forum_post_reactions.emoji IS DISTINCT FROM EXCLUDED.emoji guard is the problem. When an ON CONFLICT DO UPDATE ... WHERE condition is not satisfied, the row will not be returned. So when a user PUTs the same emoji they already have:
- The conflict fires.
- The
WHERE ... IS DISTINCT FROMcondition isfalse→ no update runs. RETURNINGemits no row → theupsertedCTE is empty.- The outer
LEFT JOIN upserted upproduces NULLs for allreaction_*columns. - SQLx decodes those NULLs as
Option::None, and the JSON response contains"reaction": null.
The DB state is correct (the reaction is preserved), but the API response misrepresents it as absent — any client that trusts the response body will think the reaction was removed.
Simplest fix: drop the WHERE guard so the DO UPDATE always fires and RETURNING always produces a row. The cost is one extra write when the emoji hasn't changed, which is negligible.
Since this JSON is auto-generated by sqlx prepare, the fix must be applied to the SQL in the Rust source file (backend/storage/src/repositories/forum_repository.rs) and then regenerated.
🐛 Proposed fix (SQL diff, to be applied in the Rust source)
ON CONFLICT (forum_post_id, user_id)
DO UPDATE SET
- emoji = EXCLUDED.emoji
- WHERE forum_post_reactions.emoji IS DISTINCT FROM EXCLUDED.emoji
+ emoji = EXCLUDED.emoji
RETURNING📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "query": "\n WITH upserted AS (\n INSERT INTO forum_post_reactions (forum_post_id, user_id, emoji)\n VALUES ($1, $2, $3)\n ON CONFLICT (forum_post_id, user_id)\n DO UPDATE SET\n emoji = EXCLUDED.emoji\n WHERE forum_post_reactions.emoji IS DISTINCT FROM EXCLUDED.emoji\n RETURNING\n id AS reaction_id,\n forum_post_id AS reaction_forum_post_id,\n user_id AS reaction_user_id,\n emoji AS reaction_emoji\n )\n SELECT\n fp.id,\n fp.content,\n fp.created_at,\n fp.updated_at,\n fp.sticky,\n fp.locked,\n fp.forum_thread_id,\n u.id AS created_by_user_id,\n u.username AS created_by_user_username,\n u.class_name AS created_by_user_class_name,\n u.avatar AS created_by_user_avatar,\n u.banned AS created_by_user_banned,\n u.warned AS created_by_user_warned,\n u.custom_title AS created_by_user_custom_title,\n up.reaction_id AS \"reaction_id?\",\n up.reaction_forum_post_id AS \"reaction_forum_post_id?\",\n up.reaction_user_id AS \"reaction_user_id?\",\n up.reaction_emoji AS \"reaction_emoji?\"\n FROM forum_posts fp\n JOIN users u ON fp.created_by_id = u.id\n LEFT JOIN upserted up\n ON up.reaction_forum_post_id = fp.id\n WHERE fp.id = $1\n ", | |
| "query": "\n WITH upserted AS (\n INSERT INTO forum_post_reactions (forum_post_id, user_id, emoji)\n VALUES ($1, $2, $3)\n ON CONFLICT (forum_post_id, user_id)\n DO UPDATE SET\n emoji = EXCLUDED.emoji\n RETURNING\n id AS reaction_id,\n forum_post_id AS reaction_forum_post_id,\n user_id AS reaction_user_id,\n emoji AS reaction_emoji\n )\n SELECT\n fp.id,\n fp.content,\n fp.created_at,\n fp.updated_at,\n fp.sticky,\n fp.locked,\n fp.forum_thread_id,\n u.id AS created_by_user_id,\n u.username AS created_by_user_username,\n u.class_name AS created_by_user_class_name,\n u.avatar AS created_by_user_avatar,\n u.banned AS created_by_user_banned,\n u.warned AS created_by_user_warned,\n u.custom_title AS created_by_user_custom_title,\n up.reaction_id AS \"reaction_id?\",\n up.reaction_forum_post_id AS \"reaction_forum_post_id?\",\n up.reaction_user_id AS \"reaction_user_id?\",\n up.reaction_emoji AS \"reaction_emoji?\"\n FROM forum_posts fp\n JOIN users u ON fp.created_by_id = u.id\n LEFT JOIN upserted up\n ON up.reaction_forum_post_id = fp.id\n WHERE fp.id = $1\n ", |
🤖 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
`@backend/storage/.sqlx/query-428a678919b1a16a2614dee0d093279f3235e39a250ce5a3ca2ed260c8f71d54.json`
at line 3, The upsert uses a WHERE clause that prevents RETURNING from emitting
a row when the emoji is unchanged, causing the upserted CTE to be empty and
reaction_* fields to be NULL; fix by removing the WHERE clause from the ON
CONFLICT DO UPDATE in the SQL used by forum_repository (the INSERT INTO
forum_post_reactions ... ON CONFLICT (forum_post_id, user_id) DO UPDATE SET
emoji = EXCLUDED.emoji — remove the "WHERE forum_post_reactions.emoji IS
DISTINCT FROM EXCLUDED.emoji" guard so the UPDATE always runs and RETURNING
always yields a row), then regenerate the sqlx metadata (run sqlx prepare /
rebuild) so the JSON query file reflects the updated SQL.
FrenchGithubUser
force-pushed
the
main
branch
4 times, most recently
from
09383f1 to
c8d1750
Compare
FrenchGithubUser
force-pushed
the
main
branch
2 times, most recently
from
4c57b76 to
bf0ed97
Compare
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
2 participants
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.
[] DB:
[x] Create table forum_post_reactions
[] BACK:
[] Creation tests
[x] Test get the post with a reaction
[x] Test add in forum_post
[x] Test delete in in forum_post
[x] Test edit in forum_post
[] Creation handlers set_forum_post_reaction & delete_forum_post_reaction
[x] PUT /api/forum/post/{post_id}/reaction → add or replace the reaction to the post
[x] DELETE /api/forum/post/{post_id}/reaction → delete the user reaction from the post
Summary by CodeRabbit
Release Notes