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

All mutations need to have toast.error #2225

Closed
3 tasks done
harshsbhat opened this issue Oct 6, 2024 · 23 comments · Fixed by #2251
Closed
3 tasks done

All mutations need to have toast.error #2225

harshsbhat opened this issue Oct 6, 2024 · 23 comments · Fixed by #2251

Comments

@harshsbhat
Copy link
Contributor

harshsbhat commented Oct 6, 2024

Preliminary Checks

Reproduction / Replay Link (Optional)

No response

Issue Summary

I noticed there are multiple mutations where if they fail we are not logging a proper error message using the toast. Which means that the user won't get a success/error toast and the action won't take place either. I have listed the exact mutations here.

-createPermission
-updatePermission
-deletePermission
-deleteRole
-updateRole
-ratelimit.override.create
-ratelimit.override.update
-ratelimit.override.delete

-deleteNamespace
-updateNamespace
-workspace updateName 
-workspace.optIntoBeta

Steps to Reproduce

  1. Go to https://app.unkey.com/ratelimits/[namespaceId]/settings
  2. Rename the namespace
  3. It doesn't display the error in toast nor does it rename the namespace

Expected behavior

It should rename the namespace and display a proper toast message.

Other information

No response

Screenshots

image

Version info

- OS:
- Node:
- npm:
@harshsbhat harshsbhat added Bug Something isn't working Needs Approval Needs approval from Unkey labels Oct 6, 2024
Copy link

linear bot commented Oct 6, 2024

@harshsbhat
Copy link
Contributor Author

I plan to work on this if it gets approved. Please don't assign yourself

@chronark
Copy link
Collaborator

chronark commented Oct 6, 2024

I plan to work on this if it gets approved. Please don't assign yourself

don't try to reserve this please, it's unfair to others,
I'd appreciate if you'd wrap up your other issues first

@chronark
Copy link
Collaborator

chronark commented Oct 6, 2024

I can't reproduce it
https://share.cleanshot.com/s5LdYKQ0

@harshsbhat
Copy link
Contributor Author

I got the reason why it's happening. It's because I was trying to rename it to a name that already exists.

( I didn't know that if I use a name in SDK initialization without having a namespace it creates one automatically )

But the toast problem still exists then

@chronark
Copy link
Collaborator

chronark commented Oct 6, 2024

( I didn't know that if I use a name in SDK initialization without having a namespace it creates one automatically )

that's not the SDK, but the API where this happens.

OK so the issue is renaming when the new name collides with an existing one. We need to return a better error from trpc and make the toast work

also there seems to be no loading indicator on the save button

@chronark
Copy link
Collaborator

chronark commented Oct 6, 2024

/award 150

Copy link

oss-gg bot commented Oct 6, 2024

Awarding harshsbhat: 150 points 🕹️ Well done! Check out your new contribution on oss.gg/harshsbhat

@Vardhaman619
Copy link
Contributor

/assign

Copy link

oss-gg bot commented Oct 6, 2024

Assigned to @Vardhaman619! Please open a draft PR linking this issue within 48h ⚠️ If we can't detect a PR from you linking this issue in 48h, you'll be unassigned automatically 🕹️ Excited to have you ship this 🚀

@harshsbhat
Copy link
Contributor Author

/assign

Copy link

oss-gg bot commented Oct 6, 2024

Assigned to @harshsbhat! Please open a draft PR linking this issue within 48h ⚠️ If we can't detect a PR from you linking this issue in 48h, you'll be unassigned automatically 🕹️ Excited to have you ship this 🚀

@harshsbhat harshsbhat removed their assignment Oct 6, 2024
@harshsbhat
Copy link
Contributor Author

I have unassigned. Its open if anyone wants to take it

@AkshayBandi027
Copy link
Contributor

/assign

Copy link

oss-gg bot commented Oct 6, 2024

You already have an open issue assigned to you here. Once that's closed or unassigned, only then we recommend you to take up more.

@harshsbhat
Copy link
Contributor Author

@AkshayBandi027 I unassigned myself so that someone without any issue in hand can work on this. Please

@harshsbhat
Copy link
Contributor Author

/assign

Copy link

oss-gg bot commented Oct 6, 2024

Assigned to @harshsbhat! Please open a draft PR linking this issue within 48h ⚠️ If we can't detect a PR from you linking this issue in 48h, you'll be unassigned automatically 🕹️ Excited to have you ship this 🚀

@harshsbhat
Copy link
Contributor Author

assigning this as my work in the other PR is done and just needs a few reviews

@harshsbhat
Copy link
Contributor Author

harshsbhat commented Oct 7, 2024

@chronark as I was solving this. I noticed there are multiple mutations where if they fail we are not logging a proper error message using the toast. Which means that the user won't get a success/error toast and the action won't take place either. I have listed the exact mutations here. Wdyt? Should we update this issue to solve the following too?

-createPermission
-updatePermission
-deletePermission
-deleteRole
-updateRole
-ratelimit.override.create
-ratelimit.override.update
-ratelimit.override.delete

-deleteNamespace
-updateNamespace
-workspace updateName 
-workspace.optIntoBeta

@chronark
Copy link
Collaborator

chronark commented Oct 7, 2024

yes please

@harshsbhat harshsbhat changed the title Renaming namespace not working. ( toast is broken too ) All mutations need to have toast.error Oct 7, 2024
Copy link

oss-gg bot commented Oct 7, 2024

@Vardhaman619, Just a little reminder: Please open a draft PR linking this issue within 12 hours. If we can't detect a PR in 12h, you will be unassigned automatically.

Copy link

oss-gg bot commented Oct 9, 2024

@Vardhaman619, Just a little reminder: Please open a draft PR linking this issue within 12 hours. If we can't detect a PR in 12h, you will be unassigned automatically.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants