Skip to content

fix: resolve emoji reactions query and mutations for quote/vote actions - #442

Open
AegisX-dev wants to merge 3 commits into
QuoteVote:mainfrom
AegisX-dev:feat/rc1-016-fix-emoji-rendering
Open

fix: resolve emoji reactions query and mutations for quote/vote actions#442
AegisX-dev wants to merge 3 commits into
QuoteVote:mainfrom
AegisX-dev:feat/rc1-016-fix-emoji-rendering

Conversation

@AegisX-dev

Copy link
Copy Markdown
Contributor

Summary

Fixes RC1-016: Resolves emoji reaction rendering, query handling, and mutation persistence for quote and vote activity records.

Changes Made

Backend (quotevote-backend)

  • Resolvers: Created reactionResolver.ts to handle:
  • actionReactions query
  • addActionReaction mutation
  • updateActionReaction mutation
  • Schema & Server: Added mutation definitions (addActionReaction, updateActionReaction) to server.ts and registered reactionResolver.
  • Unit Tests: Created reactionResolver.test.ts covering query and mutation execution paths under auth guard conditions.

Frontend (quotevote-frontend)

  • State Guard: Added optional chaining in CommentReactions.tsx when reading user ID to prevent crashes in unauthenticated or empty store states.
  • Event Propagation: Stopped click event bubbling on reaction component container to prevent unexpected card selection expansion when toggling reactions.

Verification & Testing

  • Ran backend test suite: 99 passed / 99 total
  • Ran frontend test suite: 159 passed / 159 total
  • Verified reactions render, update, and persist correctly across action types.

@vercel

vercel Bot commented Jul 31, 2026

Copy link
Copy Markdown

@AegisX-dev is attempting to deploy a commit to the Louis Girifalco's projects Team on Vercel.

A member of the Team first needs to authorize it.

@motirebuma
motirebuma self-requested a review August 1, 2026 21:19

@motirebuma motirebuma left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hey @AegisX-dev - the code base for the reaction system is looking good overall. The resolver structure, auth guards, and tests are all in order. A few security and polish items before merge.

What's good

  1. Auth guards - both mutations check context.user._id and throw UNAUTHENTICATED with appropriate GraphQL error extensions. Tested both authenticated and unauthorized cases.

  2. Frontend - optional chaining on state.user.data?.id prevents crash when unauthenticated, and stopPropagation() on the reaction container fixes the card expansion bug. Both are simple one-liners.

  3. Tests - good coverage of query, create, update, and auth rejection scenarios.

Items to address

  1. addActionReaction trusts client-provided userId. The mutation takes userId from args.reaction.userId and passes it to Reaction.create() directly. An authenticated user could add a reaction on another user's behalf. Use context.user._id instead:
const rxn = await Reaction.create({
userId: context.user._id, // insecure to take userId from args
actionId: args.reaction.actionId,
emoji: args.reaction.emoji
});
  1. updateActionReaction needs ownership guard. Any authenticated user could update any reaction by _id. Before updating, verify that the reaction belongs to the current user:
const existing = await Reaction.findById(args._id).lean();
if (!existing || existing.userId.toString() !== context.user._id.toString()) {
throw new GraphQLError('Not authorized', { extensions: { code: 'FORBIDDEN' } });
}
  1. Missing deleteActionReaction mutation. Currently, users can add and update reactions, but not delete them. Need a mutation to remove a reaction.

  2. No guard in addActionReaction against multiple reactions from the same user. A user could click "React" multiple times and create multiple identical reactions. Consider using findOneAndUpdate(..., { upsert: true }) on (userId, actionId) index, or add a unique index on these fields.

Nit

The Reaction model already defines a static findByActionId method. The query resolver should use Reaction.findByActionId(args.actionId) rather than Reaction.find({ actionId: args.actionId }) - that way, if the implementation of findByActionId() changes in the future, the resolver will continue to work.

Thanks @AegisX-dev

@flyblackbox @AegisX-dev

@motirebuma
motirebuma self-requested a review August 2, 2026 19:48

@motirebuma motirebuma left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hi @AegisX-dev, superb! All of my comments are addressed, and the implementation looks good.

What is good

Security issues

addActionReaction makes use of context.user._id rather than the given userId in the arguments. updateActionReaction, as well as the new deleteActionReaction, now have ownership guarding, returning FORBIDDEN if the user is not the one who created the reaction.

Duplicate guard

The new addActionReaction uses findOneAndUpdate(..., { upsert: true }) on (userId, actionId) combo, and a { userId: 1, actionId: 1 } unique index on the model. A belt and suspenders approach.

Delete mutation

The new delete mutation has an auth guard, ownership check, a GraphQL schema mutation, and TypeScript type definition. The frontend also has the DELETE_ACTION_REACTION mutation.

Other type fixes

The static methods now return QueryWithHelpers<...> rather than Promise, so that .lean() can be chained on them. Good catch!

Model statics usage

Using Reaction.findByActionId() static method rather than the raw find().

Test coverage

All the tests are present and cover the happy path, ownership check rejection (FORBIDDEN), not-found, delete success, and the upsert in addActionReaction correctly uses context.user._id even if another userId is passed in arguments.

Frontend fixes

Optional chaining on the user store and stopPropagation() on the reaction container.

No issues.

thansk @AegisX-dev

@flyblackbox @AegisX-dev

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.

2 participants