Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add notifications when there is an error in the service #264

Merged
merged 8 commits into from
Aug 3, 2024

Conversation

wet6123
Copy link
Contributor

@wet6123 wet6123 commented Aug 2, 2024

What this PR does / why we need it:

The currently implemented error exceptions don't have a detailed reason for the error, so users can't figure out why the error occurred. To solve this problem, I've added a detailed reason for each error exception and JWT AuthGuard. I've also modified the snackbar menu on the frontend to output the detailed reason.

Before modification
스크린샷 2024-07-25 오후 9 12 24

After modification
스크린샷 2024-07-31 오후 8 52 06

Which issue(s) this PR fixes:

Fixes #196

Special notes for your reviewer:

  • You can check if the JWT token does not exist or expires.
  • Added detailed error reasons to the exception that exists in the services.
  • It would be nice to add error branches in the future to provide more detailed error handling.

Does this PR introduce a user-facing change?:

Users can see why the error occurred.

Additional documentation:


Checklist:

  • Added relevant tests or not required
  • Didn't break anything

Summary by CodeRabbit

  • New Features

    • Enhanced JWT authentication with improved error handling and token verification.
    • Introduced a new CustomError interface in the frontend for better error handling.
  • Bug Fixes

    • Improved clarity of error messages across multiple services, providing more detailed exceptions for unauthorized, not found, and conflict errors.
  • Documentation

    • Updated error handling logic to prioritize specific feedback in the frontend.

@CLAassistant
Copy link

CLAassistant commented Aug 2, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

coderabbitai bot commented Aug 2, 2024

Walkthrough

The recent updates across multiple services and hooks focus on enhancing error handling by providing clearer and more descriptive exceptions. Improvements include structured error messages that offer additional context, such as causes and descriptions for various exceptions. Additionally, the JWT authentication logic in the JwtAuthGuard has been refined, enhancing security and user feedback. Collectively, these changes promote a more robust and user-friendly experience within the application.

Changes

Files Change Summary
backend/src/auth/jwt.guard.ts Enhanced JWT authentication with improved error handling and specific exceptions for missing or invalid tokens.
backend/src/documents/documents.service.ts, backend/src/files/files.service.ts, backend/src/intelligence/intelligence.service.ts, backend/src/users/users.service.ts, backend/src/workspace-documents/workspace-documents.service.ts, backend/src/workspace-users/workspace-users.service.ts, backend/src/workspaces/workspaces.service.ts Improved error handling across multiple services by providing detailed context in exceptions, including parameters like cause and description.
frontend/src/hooks/useErrorHandler.ts Introduced CustomError interface to enhance error message handling, prioritizing specific error details from responses.

Assessment against linked issues

Objective Addressed Explanation
Add notification for access denial (Issue #196) Changes improve error messages but do not implement notifications for users.

A rabbit's delight, so bright and spry,
With clearer whispers, we now know why.
When pages are locked, a message will say,
“Not for you, friend, go find another way!” 🐇✨


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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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 as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Outside diff range, codebase verification and nitpick comments (1)
backend/src/auth/jwt.guard.ts (1)

1-11: Ensure the jsonwebtoken package is installed.

The jsonwebtoken package is not listed in the dependencies of the backend or frontend package.json files. Please add it to the dependencies section in the appropriate package.json file.

  • backend/package.json
  • frontend/package.json
Analysis chain

Ensure the jsonwebtoken package is installed.

The new import for jsonwebtoken requires the package to be installed. Verify that it is included in the dependencies.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that `jsonwebtoken` is included in the dependencies.

# Test: Check for `jsonwebtoken` in package.json. Expect: It should be listed as a dependency.
rg --files-with-matches 'jsonwebtoken' package.json

Length of output: 104


Script:

#!/bin/bash
# Description: Locate the package.json file and verify that `jsonwebtoken` is included in the dependencies.

# Step 1: Locate the package.json file
PACKAGE_JSON_PATH=$(fd package.json)

# Step 2: Check for `jsonwebtoken` in the located package.json file
if [[ -n "$PACKAGE_JSON_PATH" ]]; then
  rg --files-with-matches 'jsonwebtoken' "$PACKAGE_JSON_PATH"
else
  echo "package.json not found"
fi

Length of output: 304


Script:

#!/bin/bash
# Description: Verify that `jsonwebtoken` is included in the dependencies of each package.json file.

# Step 1: Locate all package.json files
fd package.json --exec rg --files-with-matches 'jsonwebtoken' {}

Length of output: 66


Script:

#!/bin/bash
# Description: Print the contents of each package.json file to manually verify the presence of `jsonwebtoken`.

# Step 1: Locate all package.json files
fd package.json --exec cat {}

Length of output: 5452

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between e184d24 and 304f21b.

Files selected for processing (9)
  • backend/src/auth/jwt.guard.ts (2 hunks)
  • backend/src/documents/documents.service.ts (2 hunks)
  • backend/src/files/files.service.ts (3 hunks)
  • backend/src/intelligence/intelligence.service.ts (1 hunks)
  • backend/src/users/users.service.ts (1 hunks)
  • backend/src/workspace-documents/workspace-documents.service.ts (4 hunks)
  • backend/src/workspace-users/workspace-users.service.ts (1 hunks)
  • backend/src/workspaces/workspaces.service.ts (5 hunks)
  • frontend/src/hooks/useErrorHandler.ts (1 hunks)
Additional comments not posted (24)
frontend/src/hooks/useErrorHandler.ts (2)

4-12: The CustomError interface is well-defined.

This interface extends the built-in Error type and adds necessary properties for enhanced error handling.


17-21: The useErrorHandler function improvements are approved.

The function now provides more specific feedback based on the response structure, enhancing error handling.

backend/src/workspace-users/workspace-users.service.ts (1)

24-28: Enhanced error handling is approved.

The NotFoundException now includes a custom error message and additional details, improving clarity and user feedback.

backend/src/documents/documents.service.ts (2)

34-37: Enhanced UnauthorizedException handling is approved.

The UnauthorizedException now includes a custom error message and additional details, improving clarity and user feedback.


49-52: Enhanced NotFoundException handling is approved.

The NotFoundException now includes a custom error message and additional details, improving clarity and user feedback.

backend/src/auth/jwt.guard.ts (3)

15-18: LGTM! Ensure ConfigService is correctly configured.

The ConfigService is correctly injected. Ensure that it is properly configured in the application module.


31-44: LGTM! Verify the correctness of token retrieval and verification.

The new logic for token retrieval and verification looks good. Ensure that the token is correctly retrieved from the headers and verified using the secret key.


45-61: LGTM! Verify the error handling logic.

The error handling logic distinguishes between different error types and provides clear feedback. Ensure that the error messages are correctly displayed to the user.

backend/src/users/users.service.ts (1)

77-80: LGTM! The enhanced error handling improves clarity.

The new error handling logic provides a clear and informative message when a nickname conflict occurs.

backend/src/intelligence/intelligence.service.ts (1)

28-32: LGTM! The enhanced error handling improves clarity.

The new error handling logic provides a clear and informative message when a feature is not found.

backend/src/files/files.service.ts (4)

49-52: Enhanced UnauthorizedException message.

The detailed message and structured error object improve clarity. This change is approved.


56-59: Enhanced UnprocessableEntityException message.

The detailed message and structured error object improve clarity. This change is approved.


84-87: Enhanced NotFoundException message.

The detailed message and structured error object improve clarity. This change is approved.


104-107: Enhanced BadRequestException message.

The detailed message and structured error object improve clarity. This change is approved.

backend/src/workspaces/workspaces.service.ts (5)

28-31: Enhanced ConflictException message.

The detailed message and structured error object improve clarity. This change is approved.


69-72: Enhanced NotFoundException message.

The detailed message and structured error object improve clarity. This change is approved.


123-127: Enhanced NotFoundException message.

The detailed message and structured error object improve clarity. This change is approved.


165-168: Enhanced UnauthorizedException message.

The detailed message and structured error object improve clarity. This change is approved.


178-181: Enhanced NotFoundException message.

The detailed message and structured error object improve clarity. This change is approved.

backend/src/workspace-documents/workspace-documents.service.ts (5)

29-33: Enhanced NotFoundException message.

The detailed message and structured error object improve clarity. This change is approved.


59-63: Enhanced NotFoundException message.

The detailed message and structured error object improve clarity. This change is approved.


129-133: Enhanced NotFoundException message.

The detailed message and structured error object improve clarity. This change is approved.


161-165: Enhanced NotFoundException message.

The detailed message and structured error object improve clarity. This change is approved.


122-127: Improved readability and maintainability.

Storing the return value in a variable before returning improves readability and maintainability. This change is approved.

@wet6123 wet6123 changed the title Feat/error notification Add notifications when there is an error in the service Aug 2, 2024
@krapie krapie requested review from devleejb and krapie August 2, 2024 12:20
@krapie krapie added the enhancement 🌟 New feature or request label Aug 2, 2024
@krapie krapie removed their request for review August 2, 2024 13:34
Copy link
Member

@devleejb devleejb left a comment

Choose a reason for hiding this comment

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

I have few questions.

Q1)
In constructor of NestJS HTTP error, I found that we can modify JSON response body by overriding objectOrError.

Should we use second parameter descriptionOrOptions?

constructor(objectOrError?: string | object | any, descriptionOrOptions?: string | HttpExceptionOptions);

By default, the JSON response body contains two properties:

  • statusCode: this will be the value 404.
  • message: the string 'Not Found' by default; override this by supplying a string in the objectOrError parameter.

Q2)
It seems that the cause field in descriptionOrOptions are all new Error(). Should we pass the field?

backend/src/auth/jwt.guard.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 304f21b and a66bd78.

Files selected for processing (9)
  • backend/src/auth/jwt.guard.ts (2 hunks)
  • backend/src/documents/documents.service.ts (1 hunks)
  • backend/src/files/files.service.ts (2 hunks)
  • backend/src/intelligence/intelligence.service.ts (1 hunks)
  • backend/src/users/users.service.ts (1 hunks)
  • backend/src/workspace-documents/workspace-documents.service.ts (4 hunks)
  • backend/src/workspace-users/workspace-users.service.ts (1 hunks)
  • backend/src/workspaces/workspaces.service.ts (3 hunks)
  • frontend/src/hooks/useErrorHandler.ts (1 hunks)
Files skipped from review as they are similar to previous changes (9)
  • backend/src/auth/jwt.guard.ts
  • backend/src/documents/documents.service.ts
  • backend/src/files/files.service.ts
  • backend/src/intelligence/intelligence.service.ts
  • backend/src/users/users.service.ts
  • backend/src/workspace-documents/workspace-documents.service.ts
  • backend/src/workspace-users/workspace-users.service.ts
  • backend/src/workspaces/workspaces.service.ts
  • frontend/src/hooks/useErrorHandler.ts

@wet6123
Copy link
Contributor Author

wet6123 commented Aug 3, 2024

Thank you asking, @devleejb

A1)
I didn't want to lose the value of the message, so I didn't overwrite it. But I found that if we put the string at the objectOrError, the value of the message moved to the error, and the string value was saved in the message. I think this is better than the old way. Thanks for the advice!!

A2)
We don't have to. Totally my mistake. Thanks for letting me know!

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between a66bd78 and b0d8ebe.

Files selected for processing (1)
  • frontend/src/hooks/useErrorHandler.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
  • frontend/src/hooks/useErrorHandler.ts

@wet6123 wet6123 requested a review from devleejb August 3, 2024 07:37
Copy link
Member

@devleejb devleejb left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution.

@devleejb devleejb merged commit 6b26087 into yorkie-team:main Aug 3, 2024
3 checks passed
minai621 pushed a commit that referenced this pull request Nov 5, 2024
* Add detail error descriptions

* Fix exception occurrence

* Update JwtAuthGuard to handle token expiration and invalid token errors

* Update useErrorHandler to handle custom error responses

* Update error handling and exception messages

* Update error handler to handle custom error response

* Update JwtAuthGuard to handle exception with handleRequest

* Remove console.error in useErrorHandler
@coderabbitai coderabbitai bot mentioned this pull request Nov 8, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement 🌟 New feature or request
Projects
Status: In review
Status: Done
Development

Successfully merging this pull request may close these issues.

Add Notification when Accessing Pages such as Documents without Permission
4 participants