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.

Sign up for GitHub

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

Jump to bottom

updated error handling sytem with unified way to show user error messeges #275

Merged
merged 53 commits into from
Jan 9, 2025
Merged

updated error handling sytem with unified way to show user error messeges #275

merged 53 commits into from
Jan 9, 2025
+1,166 −321

Conversation

nitinawari
Copy link
Contributor

@nitinawari nitinawari commented Dec 26, 2024 •
edited
Loading

fixed #190

  • implemented unified way to show error to user with toast message
    404 page
    image

@nitinawari
updated error handling sytem with unified wayt to show user error mes…

Verified

This commit was signed with the committer’s verified signature.
mitchellolsthoorn Mitchell Olsthoorn
SSH Key Fingerprint: 3RAPR58BlEndU2xx+8n1/4JkNHy/ZecINVP1oif7sIU
Verified
Learn about vigilant mode
Loading
Loading status checks…
1f5793a 
…sege
@nitinawari
Copy link
Contributor Author

@arkid15r Sir, I have created a complete error-handling system to display user error messages in a unified way. Please let me know if I have missed anything, made any mistakes, or if there are any changes or deletions needed

Rajgupta36
Rajgupta36 suggested changes Dec 28, 2024
View reviewed changes
Copy link
Collaborator

@Rajgupta36 Rajgupta36 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

hiii @nitinawari can you update the code with latest test case changes

@nitinawari
Copy link
Contributor Author

"@Rajgupta36, I’m facing issues with some test cases, all of which are related to a single functionality. Should I update the code without passing these test cases now? Could you help me resolve this issue?"

@Rajgupta36
Copy link
Collaborator

Rajgupta36 commented Dec 29, 2024 •
edited
Loading

"@Rajgupta36, I’m facing issues with some test cases, all of which are related to a single functionality. Should I update the code without passing these test cases now? Could you help me resolve this issue?"

Could you share more details about the specific issues you're facing? maybe i could help .

@Rajgupta36
Copy link
Collaborator

it is about retry functionality when user reach 3 times retry attempt it shoud show the home button but it not working i am unable to debug i write proper functionality but what is missing i am unable to find out here is failed test cases

Can you share the debugging logs?

@nitinawari
Copy link
Contributor Author

@Rajgupta36 there is nothing in dubug log just a retry increament count but i have this test failure
image

@yashpandey06
Copy link
Contributor

@Rajgupta36 there is nothing in dubug log just a retry increament count but i have this test failure image

@nitinawari ...should i suggest something ?....first ..please lets try to to add some console.log ststement and check how many times useErrorhandler instantiated and how many times the instance of ErrorService (btw naming is wrong inyour case) is getting created and calling handlerror function.

@yashpandey06
Copy link
Contributor

yashpandey06 commented Dec 29, 2024 •
edited
Loading

as you have given maximum retry count as 3 ...i am quite sure its not 3 .

If you get the count (actual ) and update the test case and then try to run it.I hope it can help

@nitinawari
Copy link
Contributor Author

i think here race condition happening now looking into that part also what is meant bu this
" ErrorService (btw naming is wrong inyour case)"

@yashpandey06
Copy link
Contributor

i think here race condition happening now looking into that part also what is meant bu this " ErrorService (btw naming is wrong inyour case)"

@nitinawari i mean the file name is wrong right ..?

exactly ..that what was i thinking ...

You can try this :
make your service file singleton or use mutex .

@nitinawari
Copy link
Contributor Author

@yashpandey06 can you plz elaborate more. it is not a problem related to naming or instance it more like to retry attempt logic

@nitinawari
Copy link
Contributor Author

@arkid15r,
-yes, to add ShadCN to our project, we need to run some predefined commands, which will set up their configuration to add separate reusable components like Toast.tsx. Their naming conventions are predefined; we just need to import and use them.
https://ui.shadcn.com/docs/installation/vite

  • Additionally, our configuration is also working fine with it

@nitinawari
Copy link
Contributor Author

@Rajgupta36 @harsh3dev

@harsh3dev
Copy link
Collaborator

@arkid15r
The shadcn library adds some config files and It has its own naming convention for example: use-toast.tsx and I think we can use the normal import as we are using in the project no need of @.
Also I think we can change the naming of files according to our naming convention @Rajgupta36 can confirm.

@Rajgupta36
Copy link
Collaborator

image

can you make it little bit better.

@nitinawari
Copy link
Contributor Author

@Rajgupta36 ok i will use new york style and what about messege should i also update message

@Rajgupta36
Copy link
Collaborator

Rajgupta36 commented Jan 5, 2025 •
edited
Loading

@Rajgupta36 ok i will use new york style and what about messege should i also update message

no

nitinawari and others added 4 commits January 6, 2025 22:46
@nitinawari
Merge branch 'main' into error-handling-with-unified-error-messeging
Loading
Loading status checks…
769030a
@nitinawari
bug fixes
57fa0ed
@nitinawari
Merge remote-tracking branch 'origin/error-handling-with-unified-erro…
Loading
Loading status checks…
8881414 
…r-messeging' into error-handling-with-unified-error-messeging
@nitinawari
Fix: Rename files
Loading
Loading status checks…
320e4ba
@nitinawari
Copy link
Contributor Author

@arkid15r Sir, I have fixed the bugs as per your suggestions.
Initially, this PR was solely about error handling. However, we later decided to use ShadCN as our component library. Due to this decision, I needed to add a toast component to display user errors. As a result, this PR includes the ShadCN configuration.

This PR can be divided into two parts:

ShadCN Integration and Configuration

  • The configuration required to set up ShadCN has been added to the project.
  • We can easily customize every part of ShadCN, including naming conventions, styling, and other aspects.
    @Rajgupta36 @harsh3dev can you plz help us in this part to review

Error Handling in the Frontend

  • The core part of this PR handles errors in a unified manner.
  • Errors are displayed to the user using the toast component provided by ShadCN.
  • The focus should be on lib/ErrorWrapper and the API calls that have been rewritten for better error handling.

arkid15r
arkid15r reviewed Jan 7, 2025
View reviewed changes
Copy link
Collaborator

@arkid15r arkid15r left a comment

Choose a reason for hiding this comment

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

@nitinawari I'm going to review this PR, could you resolve the conflicts?

@Rajgupta36
Copy link
Collaborator

@nitinawari, could you please take a look at the requested changes . i added a shadcn config so you don't need this @ character for importing and exporting

nitinawari and others added 2 commits January 8, 2025 07:19
@nitinawari
Merge branch 'main' into error-handling-with-unified-error-messeging
Loading
Loading status checks…
744f35a
@nitinawari
resolve confict
Loading
Loading status checks…
556c005
@nitinawari
Copy link
Contributor Author

@arkid15r: Ready for review. I hope we can close this today. I have made the requested changes. If you find any bugs or need any improvements, I am happy to make the changes. I am available the whole day.

@nitinawari
shadcn aliases update
Loading
Loading status checks…
09fd8e8
@Rajgupta36
Copy link
Collaborator

looks good @nitinawari

@nitinawari nitinawari requested a review from arkid15r January 8, 2025 15:06
nitinawari and others added 3 commits January 9, 2025 01:47
@nitinawari
Merge branch 'main' into error-handling-with-unified-error-messeging
Loading
Loading status checks…
fe5b1ee
@nitinawari
resolve conflict
Loading
Loading status checks…
b55a2da
@arkid15r
Merge branch 'main' into pr/nitinawari/275
Loading
Loading status checks…
137b3f1
@arkid15r arkid15r enabled auto-merge January 9, 2025 01:07
arkid15r
arkid15r reviewed Jan 9, 2025
View reviewed changes
frontend/src/lib/hooks/useToast.ts
import type { ToastActionElement, ToastProps } from 'components/ui/Toast'

const TOAST_LIMIT = 1
const TOAST_REMOVE_DELAY_SECOND = 10 //in seconds
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is too long and we should have different delays for success/warning/error messages.

frontend/src/lib/hooks/useToast.ts
import type { ToastActionElement, ToastProps } from 'components/ui/Toast'

const TOAST_LIMIT = 1
const TOAST_REMOVE_DELAY_SECOND = 10 //in seconds
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const TOAST_REMOVE_DELAY_SECOND = 10 //in seconds
const TOAST_REMOVE_DELAY_IN_SECONDS = 10

frontend/src/lib/hooks/useToast.ts
Comment on lines +30 to +46
type Action =
| {
type: ActionType['ADD_TOAST']
toast: ToasterToast
}
| {
type: ActionType['UPDATE_TOAST']
toast: Partial<ToasterToast>
}
| {
type: ActionType['DISMISS_TOAST']
toastId?: ToasterToast['id']
}
| {
type: ActionType['REMOVE_TOAST']
toastId?: ToasterToast['id']
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you explain this syntax to me?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This functionality is useful for real-time updates to toasts but is not needed currently. For now, it has been disabled by removing the ActionType function, and it can be reintroduced later if required

arkid15r
arkid15r approved these changes Jan 9, 2025
View reviewed changes
Copy link
Collaborator

@arkid15r arkid15r left a comment

Choose a reason for hiding this comment

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

This still looks too big to me but now it's easier to understand.
A lot of work, thank you @nitinawari

I'll bump the estimated points for this task

Let's merge it 👍

@arkid15r arkid15r disabled auto-merge January 9, 2025 01:10
@arkid15r arkid15r merged commit ca265ef into OWASP:main Jan 9, 2025
12 checks passed
@nitinawari
Copy link
Contributor Author

@arkid15r Thank you for appreciating my work and increasing the points for this task! I'll continue to improve and refine it in the future. 😊

@Rajgupta36 Rajgupta36 mentioned this pull request Jan 9, 2025
Open
@nitinawari nitinawari deleted the error-handling-with-unified-error-messeging branch January 11, 2025 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@harsh3dev harsh3dev

@Rajgupta36 Rajgupta36

@arkid15r arkid15r

@kasya kasya

Assignees
No one assigned
Labels
frontend frontend-tests makefile
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Introduce a unified way to show user facing error messages on the FE
5 participants
@nitinawari @Rajgupta36 @yashpandey06 @arkid15r @harsh3dev