UI: Show clear permission toast for 403 errors on user actions#61588
UI: Show clear permission toast for 403 errors on user actions#61588Abhishekmishra2808 wants to merge 12 commits intoapache:mainfrom
Conversation
Abhishekmishra2808
requested review from
bbovenzi,
choo121600,
guan404ming,
pierrejeambrun,
ryanahamilton,
shubhamraj-git and
vatsrahul1001
as code owners
Abhishekmishra2808
force-pushed
the
fix-403-permission-error
branch
from
5c64e14 to
efaaccb
Compare
|
Static checks are failing, please take a look to setup local validations: https://github.com/apache/airflow/blob/main/contributing-docs/03a_contributors_quick_start_beginners.rst |
|
It would be helpful for the review if you could add a screenshot showing how the toast message looks😀 |
|
Instead of manually writing error messages in the UI, it would be better to make sure that fastapi is sending the correct error messages or that the UI is using the correct error message that is coming from the API. |
pierrejeambrun
left a comment
pierrejeambrun
left a comment
There was a problem hiding this comment.
Good idea, thanks for the PR.
Also we probably need to do some cleaning because in multiple places we still have the specific mutation onError hook that will also raise a toaster. We end up displaying two toasters.
Screen.Recording.2026-02-12.at.16.43.01.mov
…dling - Add isActive() method to toaster abstraction for consistency - Implement centralized 403 error handling in MutationCache - Use i18n translations instead of hard-coded strings - Update all mutation hooks to skip 403 errors (handled centrally) - Prevent duplicate toaster notifications for permission errors
|
Hi @pierrejeambrun, Please let me know if anything else needs improvement. |
- Extract error handling logic into shared utility function - Create createErrorToaster in airflow-core/src/airflow/ui/src/utils/errorHandling.ts - Function safely extracts HTTP status from error.status or error.response?.status - Skips 403 errors (handled by MutationCache) - Type-safe implementation with no 'any' types - Update useClearRun.ts to use new utility function - Export createErrorToaster from utils/index.ts This utility can be reused across 15+ files with similar error handling patterns.
- Fix object stringification error by providing default message - Refactor createErrorToaster to meet max-params rule (3 params max) - Sort object type properties alphabetically per perfectionist rule
|
@pierrejeambrun fixed all the changes suggested by you, could you please review it now? |
|
@pierrejeambrun Please re-run the CI and review this please. sorry for pinging you again |
|
@choo121600 @pierrejeambrun please check it now ! |
|
The i18n keys |
|
@choo121600 added them in the latest commit, please check it now ! |
| const status = | ||
| (error as unknown as { status?: number }).status ?? | ||
| (error as unknown as { response?: { status?: number } }).response?.status; |
There was a problem hiding this comment.
I feel like we should pull this out into a util instead of copy-pasting this code all over.
There was a problem hiding this comment.
@bbovenzi Nice Idea, Please check out the latest commit!
eladkal
added
type:bug-fix
|
addressed all the concern told by the reviewers. This PR is ready for the final review. |
Abhishekmishra2808
requested review from
bbovenzi,
choo121600 and
pierrejeambrun
pierrejeambrun
left a comment
pierrejeambrun
left a comment
There was a problem hiding this comment.
You introduced a new getErrorStatus and getErrorMessage that is only used in this PR while the entire codebase is still doing manual check everywhere else. It wouldn't be needed if you were using the createErrorToaster everywhere as requested above. No idea why the createErrorToaster is only used 3 times, and most of the time we still do that creation manually.
This is taking a lot of back and forth and energy for something that isn't suppose to. Please make sure you address properly and spend the time to make the code as maintainable and readable as possible before asking for a final review.
| const status = | ||
| (error as { status?: number }).status ?? (error as { response?: { status?: number } }).response?.status; | ||
|
|
There was a problem hiding this comment.
Why not use getStatus? Same for other places.
|
Common function was merged, can you re-use it in this PR? |
6 participants
Description
Fixes #61460
This PR improves the user experience when an action fails due to insufficient permissions.
A global React Query
MutationCacheerror handler now intercepts HTTP 403 (Forbidden) responses and displays a clear toast message:The toast is deduplicated to prevent spam when multiple mutations fail simultaneously. Existing retry behavior and error propagation remain unchanged.
Scope is intentionally limited to mutations (user actions) to avoid noisy background query refresh toasts.
This provides clearer feedback for protected UI actions (such as triggering DAGs or editing resources) with minimal risk and no behavioral side effects.
AFTER