add author to reacts and add delete react operation #189
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
|
Caution Review failedThe pull request is closed. 📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce a new feature for managing reactions to posts, specifically enabling users to add and remove reactions. The implementation includes updates to various methods and interfaces across multiple files, enhancing the structure and functionality of the application. Key modifications involve the removal of old reaction management structures and the introduction of a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Client
participant Reactor
participant Database
User->>Client: Request to remove reaction
Client->>Reactor: Call PostReactRemove
Reactor->>Database: Remove reaction by ID
Database-->>Reactor: Confirmation of removal
Reactor-->>Client: Success response
Client-->>User: Notify removal success
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Southclaws
force-pushed
the
reaction-rework
branch
from
d9fa815 to
e8ec05b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (8)
web/src/api/openapi-schema/react.ts (1)
15-15: LGTM: New author property addedThe addition of the
authorproperty to theReactinterface is correct and aligns with the PR objective. It's appropriately typed and marked as optional.Consider adding a JSDoc comment to describe the purpose of this new property, for example:
/** The user who created this reaction */ author?: ProfileReference;app/resources/react/db.go (2)
26-33: LGTM with a minor suggestion.The new
Getmethod is well-implemented and consistent with the codebase's error handling practices. However, consider adding a validation check forreactIDbefore querying the database to prevent unnecessary database calls for invalid IDs.Consider adding a validation check for
reactID:func (d *database) Get(ctx context.Context, reactID ReactID) (*React, error) { + if !reactID.IsValid() { + return nil, fault.New("invalid react ID", fctx.With(ctx), ftag.With(ftag.InvalidArgument)) + } r, err := d.db.React.Get(ctx, xid.ID(reactID)) if err != nil { return nil, fault.Wrap(err, fctx.With(ctx), ftag.With(ftag.Internal)) } return FromModel(r), nil }Note: This assumes the existence of an
IsValidmethod forReactID. If it doesn't exist, you may need to implement it or use an appropriate validation method.
58-66: LGTM with a security suggestion.The changes to the
Removemethod are good improvements:
- The simplified return type (
errorinstead of(*React, error)) is more appropriate for a delete operation.- The implementation is now more straightforward and consistent with error handling practices.
However, there's room for a security enhancement:
Consider adding a check to ensure the react belongs to the given
accountIDbefore deletion:func (d *database) Remove(ctx context.Context, accountID account.AccountID, reactID ReactID) error { + // Verify the react belongs to the account before deletion + react, err := d.db.React.Query(). + Where(react.ID(xid.ID(reactID)), react.AccountID(xid.ID(accountID))). + Only(ctx) + if err != nil { + if ent.IsNotFound(err) { + return fault.New("react not found or doesn't belong to the account", fctx.With(ctx), ftag.With(ftag.NotFound)) + } + return fault.Wrap(err, fctx.With(ctx), ftag.With(ftag.Internal)) + } + err := d.db.React. - DeleteOneID(xid.ID(reactID)). + DeleteOne(react). Exec(ctx) if err != nil { return fault.Wrap(err, fctx.With(ctx), ftag.With(ftag.Internal)) } return nil }This change ensures that a user can only delete their own reactions, preventing potential security issues where a user might delete reactions belonging to others.
web/src/api/openapi-server/posts.ts (1)
134-146: LGTM: New functionpostReactRemoveis well-implemented with a minor suggestion.The function is correctly implemented, following the established pattern for API-calling functions in this file. It properly uses the
getPostReactRemoveUrlfunction, sets the correct HTTP method, and handles the optionaloptionsparameter.For consistency with other functions in the file, consider adding a JSDoc comment describing the function's purpose. For example:
/** * Remove a reaction from a post. */ export const postReactRemove = async ( // ... rest of the function );web/src/api/openapi-client/posts.ts (1)
288-318: LGTM with a minor suggestion: Well-implemented custom hookThe
usePostReactRemovehook is correctly implemented usinguseSWRMutation. It properly utilizes the mutation key and fetcher functions, and the return value includes theswrKeyand spreads the query object. The type parameters and options are consistent with other hooks in the file.Consider extracting the type definition for the
optionsparameter into a separate type alias to improve readability. For example:type UsePostReactRemoveOptions<TError> = { swr?: SWRMutationConfiguration< Awaited<ReturnType<typeof postReactRemove>>, TError, string, Arguments, Awaited<ReturnType<typeof postReactRemove>> > & { swrKey?: string }; }; export const usePostReactRemove = <TError = ...>( postId: string, reactId: string, options?: UsePostReactRemoveOptions<TError> ) => { // ... rest of the implementation };This change would make the hook's signature more concise and easier to read.
app/transports/http/bindings/openapi_rbac/openapi_rbac_gen.go (1)
223-224: LGTM: New case for PostReactRemove added.The addition of the
PostReactRemovecase in theGetOperationPermissionfunction is correct and necessary to support the new operation for removing post reactions.For consistency with other cases, consider combining the two lines into a single line:
- case "PostReactRemove": - return optable.PostReactRemove() + case "PostReactRemove": return optable.PostReactRemove()This change would make the code more consistent with the style used in other cases of the switch statement.
app/transports/http/bindings/reacts.go (1)
41-41: Use consistent receiver names for methodsThe receiver in
PostReactRemoveis namedh, whereas inPostReactAddit isp. For consistency and readability, use the same receiver name across all methods of theReactsstruct.Consider renaming the receiver in
PostReactRemove:-func (h *Reacts) PostReactRemove(ctx context.Context, request openapi.PostReactRemoveRequestObject) (openapi.PostReactRemoveResponseObject, error) { +func (p *Reacts) PostReactRemove(ctx context.Context, request openapi.PostReactRemoveRequestObject) (openapi.PostReactRemoveResponseObject, error) {app/transports/http/openapi/server_gen.go (1)
23864-24112: Consider externalizing large embedded dataThe large embedded data starting at line 23864 significantly increases the file size and can impact readability and maintainability. Consider externalizing the Swagger specification into a separate file and using
go:embedto include it at compile time. This approach keeps the codebase cleaner and improves version control efficiency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
api/openapi.yamlis excluded by!**/*.yaml
📒 Files selected for processing (16)
- app/resources/react/db.go (3 hunks)
- app/resources/react/repo.go (1 hunks)
- app/services/react/service.go (0 hunks)
- app/services/reaction/reactions.go (1 hunks)
- app/services/services.go (2 hunks)
- app/transports/http/bindings/openapi_rbac/mapping.go (1 hunks)
- app/transports/http/bindings/openapi_rbac/openapi_rbac_gen.go (2 hunks)
- app/transports/http/bindings/reacts.go (2 hunks)
- app/transports/http/bindings/utils.go (0 hunks)
- app/transports/http/openapi/server_gen.go (16 hunks)
- web/src/api/openapi-client/posts.ts (1 hunks)
- web/src/api/openapi-schema/index.ts (1 hunks)
- web/src/api/openapi-schema/react.ts (1 hunks)
- web/src/api/openapi-schema/reactEmoji.ts (1 hunks)
- web/src/api/openapi-schema/reactInitialProps.ts (1 hunks)
- web/src/api/openapi-server/posts.ts (1 hunks)
💤 Files not reviewed due to no reviewable changes (2)
- app/services/react/service.go
- app/transports/http/bindings/utils.go
✅ Files skipped from review due to trivial changes (2)
- app/services/services.go
- web/src/api/openapi-schema/reactEmoji.ts
🧰 Additional context used
🪛 Biome
web/src/api/openapi-server/posts.ts
[error] 126-126: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
🔇 Additional comments (24)
app/resources/react/repo.go (2)
12-12: LGTM: NewGetmethod addedThe new
Getmethod is a good addition to theRepositoryinterface. It follows consistent patterns with other methods, including context usage and error handling. This method will allow for retrieving individual React objects, which aligns well with the PR objective of managing reactions.
14-14: Approve change, but verify impact:Removemethod signature updatedThe updated
Removemethod signature, which now only returns anerror, is a good simplification. This change aligns well with common practices for deletion operations. However, it's important to ensure that this change doesn't break existing code that might have been relying on the previously returned*Reactobject.To verify the impact of this change, please run the following script:
If the script returns any results, please review those occurrences to ensure they are updated to handle the new method signature.
✅ Verification successful
Change Verified:
Removemethod signature updated without breaking existing codeThe update to the
Removemethod signature inapp/resources/react/repo.go, which now solely returns anerror, has been thoroughly verified. The search revealed no existing code relies on the previous return type of*React. This change simplifies the method's contract and aligns with standard practices for deletion operations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any uses of the Remove method that expect a *React return value # Search for Remove method calls in the codebase rg --type go 'Remove\s*\([^)]*\)' -g '!app/resources/react/repo.go'Length of output: 9441
web/src/api/openapi-schema/reactInitialProps.ts (3)
16-16: Improved type safety withReactEmojiThe change to use
ReactEmojiinstead ofstringfor theemojiproperty is a positive improvement. This likely provides better type safety and consistency in how emojis are represented throughout the application. It may also enable better tooling support and reduce the chances of invalid emoji inputs.
Line range hint
1-9: Caution: This is a generated fileAs indicated by the comment at the top of the file, this is generated code. Direct modifications to this file are discouraged as they may be overwritten in the future.
To ensure that these changes are coming from the correct source, please run the following script:
#!/bin/bash # Check if there's a configuration file for orval that might be responsible for generating this file fd -e yml -e yaml -e json orvalIf a configuration file is found, please review it to ensure that the changes to
ReactInitialPropsare correctly specified there.Consider documenting the process for regenerating this file in your project's README or development guidelines to ensure all team members are aware of the correct procedure for making changes to generated files.
16-16:⚠️ Potential issueBreaking change:
emojiis now required and of typeReactEmojiThis change from
emoji?: string;toemoji: ReactEmoji;introduces two significant modifications:
- The
emojiproperty is now required, which may break existing code that doesn't always provide an emoji.- The type has changed from
stringtoReactEmoji, which likely provides more structure but may require updates in how emojis are handled throughout the application.Please ensure that all places using
ReactInitialPropsare updated to accommodate these changes. This might involve:
- Always providing an emoji when creating reactions
- Updating any code that expects
emojito be a stringTo verify the impact of this change, run the following script:
web/src/api/openapi-schema/react.ts (3)
11-11: LGTM: New import for ProfileReferenceThe addition of the
ProfileReferenceimport is correct and necessary for the newauthorproperty in theReactinterface.
12-12: LGTM: New import for ReactEmojiThe addition of the
ReactEmojiimport is correct and necessary for updating the type of theemojiproperty in theReactinterface.
Line range hint
1-18: Important: Verify changes in the source of truthThis file is generated by orval, as indicated by the comment at the top. Ensure that the changes made here are reflected in the correct source of truth (e.g., API specification) rather than directly in this generated file. This will prevent the changes from being overwritten in future generations.
Please confirm that these changes have been made in the appropriate source file or configuration for orval.
web/src/api/openapi-server/posts.ts (2)
125-128: LGTM: New typepostReactRemoveResponseis well-defined.The structure of the new response type is consistent with other response types in the file. The use of
voidfor thedataproperty is appropriate for a DELETE operation that doesn't return data.Note: The static analysis tool's suggestion to use
undefinedinstead ofvoidcan be ignored in this context. In TypeScript,voidis commonly used to indicate that a function doesn't return a value, which is the correct usage here.🧰 Tools
🪛 Biome
[error] 126-126: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
130-132: LGTM: New functiongetPostReactRemoveUrlis well-implemented.The function follows the established pattern for URL-generating functions in this file. It correctly incorporates both
postIdandreactIdinto the URL, providing a clear and focused implementation.web/src/api/openapi-client/posts.ts (5)
260-265: LGTM: Correct implementation ofpostReactRemoveThe
postReactRemovefunction is well-implemented. It correctly uses thefetcherto send a DELETE request to the appropriate endpoint, constructed using thepostIdandreactIdparameters.
267-274: LGTM: Well-implemented mutation fetcherThe
getPostReactRemoveMutationFetcherfunction is correctly implemented. It creates a closure overpostIdandreactId, and returns a function that matches the expected signature for SWR mutation fetchers. This implementation is consistent with other mutation fetchers in the file.
275-278: LGTM: Correct mutation key generationThe
getPostReactRemoveMutationKeyfunction is properly implemented. It correctly constructs the key string usingpostIdandreactId. The use ofas constensures type safety, and the key format is consistent with other mutation keys in the file.
280-286: LGTM: Appropriate type definitionsThe
PostReactRemoveMutationResultandPostReactRemoveMutationErrortypes are correctly defined. They accurately represent the expected result and possible error types for thepostReactRemoveoperation, maintaining consistency with other mutation types in the file.
257-318: Summary: Successful implementation of reaction removal functionalityThe changes in this file successfully implement the functionality to remove reactions from posts. The new
postReactRemovefunction and its associated utility functions (getPostReactRemoveMutationFetcher,getPostReactRemoveMutationKey) and custom hook (usePostReactRemove) are well-implemented and consistent with the existing patterns in the file.These additions enhance the API client's capabilities, allowing users to remove reactions from posts. The implementation is type-safe and follows the established conventions for mutation functions and hooks in this codebase.
Overall, these changes effectively meet the PR objectives and integrate seamlessly with the existing code structure.
app/transports/http/bindings/openapi_rbac/openapi_rbac_gen.go (1)
67-67: LGTM: New method for removing post reactions added.The addition of
PostReactRemove() (bool, *rbac.Permission)to theOperationPermissionsinterface is consistent with the PR objective and follows the established pattern for permission methods. This change supports the new feature for removing reactions from posts.web/src/api/openapi-schema/index.ts (1)
267-267: LGTM: New export added correctly.The new export statement
export * from "./reactEmoji";has been added correctly, following the existing pattern in the file. This addition aligns with the PR objectives of adding functionality related to reactions.As this file appears to be auto-generated, let's verify that the change was made by the generation process:
If the script indicates that the file is auto-generated and there are no manual changes, we can be confident that the addition was made through the proper generation process.
✅ Verification successful
Verified: The new export statement was added through the auto-generation process, and no manual changes were detected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the file is marked as auto-generated and if there are any manual changes. # Test 1: Check if the file is marked as auto-generated if grep -q "Generated by orval" web/src/api/openapi-schema/index.ts; then echo "File is marked as auto-generated." else echo "Warning: File is not marked as auto-generated." fi # Test 2: Check for any manual changes (assuming git is used) git diff --exit-code web/src/api/openapi-schema/index.ts || echo "Warning: Manual changes detected in the auto-generated file."Length of output: 202
app/transports/http/bindings/reacts.go (1)
31-31: Ensure reactions are correctly associated with users after removingaccountIDThe removal of
accountIDfrom thePostReactAddmethod may affect how reactions are linked to users. Verify that theAddmethod inp.reactordoes not requireaccountIDfor associating reactions with user accounts.Run the following script to check if the
Addmethod requiresaccountID:app/services/reaction/reactions.go (1)
39-51: Add method implementation is correctThe
Addmethod correctly retrieves the account ID from the session context, handles errors appropriately, and uses thereactWriterto add a reaction. Error wrapping with context provides helpful debugging information.app/transports/http/openapi/server_gen.go (5)
2250-2251: LGTMThe introduction of
ReactIDParamas a type alias forIdentifierimproves code clarity and consistency with other parameter types.
3179-3180: LGTMAdding the
PostReactRemovemethod to theClientInterfacealigns with the new functionality for removing reactions and follows the existing interface patterns.
7845-7883: LGTMThe
NewPostReactRemoveRequestfunction correctly constructs the DELETE request with the required path parameterspost_idandreact_id.
16714-16737: Verify security scopes forPostReactRemoveIn the
PostReactRemovemethod ofServerInterfaceWrapper, the security scopes are set to an empty list (ctx.Set(BrowserScopes, []string{})). Confirm that this operation does not require any authentication scopes. If it should be secured, ensure the appropriate scopes are specified.Run the following script to check the defined security requirements for this endpoint:
#!/bin/bash # Description: Search for security definitions associated with `PostReactRemove`. # Expectation: Identify if any security mechanisms are defined for this operation. rg --type go -A 5 'func \(w \*ServerInterfaceWrapper\) PostReactRemove' app/transports/http/openapi/
4446-4456: Verify error handling inPostReactRemovemethodThe
PostReactRemovemethod in theClientstruct appears correctly implemented. Ensure that callers of this method handle errors appropriately, especially since network operations can fail or return unexpected responses.Run the following script to check for proper error handling where
PostReactRemoveis used:✅ Verification successful
Error handling in
PostReactRemovemethod is properly implemented. All usages handle errors appropriately.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all usages of `PostReactRemove` handle errors. # Expectation: Each call to `PostReactRemove` should check for errors. rg --type go 'PostReactRemove\(' -A 5Length of output: 734
Script:
#!/bin/bash # Description: Find all invocations of `PostReactRemove` and check if errors are handled. # Search for all instances where `PostReactRemove` is called. rg --type go 'PostReactRemove\s*\(' # Additionally, search for error handling patterns following the function call. rg --type go 'PostReactRemove\s*\([^)]*\)\s*(?:,)?\s*err\s*?:='Length of output: 1878
Script:
#!/bin/bash # Description: Identify invocations of `PostReactRemove` that do not handle errors. # Find all lines where `PostReactRemove` is called. rg --type go 'PostReactRemove\s*\(' > all_calls.txt # Find lines where `PostReactRemove`'s error is handled (assigned to err). rg --type go 'PostReactRemove\s*\([^)]*\)\s*,\s*err\s*:=' > handled_calls.txt # Extract unhandled calls by excluding handled ones from all calls. grep -F -v -f handled_calls.txt all_calls.txt > unhandled_calls.txt # Display unhandled calls, if any. cat unhandled_calls.txtLength of output: 1946
Southclaws
force-pushed
the
reaction-rework
branch
from
e8ec05b to
e6b023b
Compare
No description provided.