Add confirmation modal for adaptor changes with credentials #4446
Add confirmation modal for adaptor changes with credentials #4446
Conversation
Show confirmation modal when users select a different adaptor. The modal warns that credentials will be reset and gives users a chance to cancel. Created useAdaptorChangeConfirmation hook to share logic between FullScreenIDE and JobForm. Confirmation only appears when the job has credentials and the user selects a different adaptor package. Also fixed AlertDialog z-index to sit above other modals.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4446 +/- ##
==========================================
+ Coverage 89.36% 89.38% +0.01%
==========================================
Files 425 425
Lines 20187 20187
==========================================
+ Hits 18041 18045 +4
+ Misses 2146 2142 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
doc-han
left a comment
doc-han
left a comment
There was a problem hiding this comment.
I initially expected issue #4395 to require less code than it did.
This PR addresses two things:
- Fixes the reported bug
- Introduces a confirmation modal when switching adaptors (new feature)
Since this changes the user experience, it may be worth a UX review. @theroinaochieng.
I'm wondering whether 2. could've been a feature PR instead of being added to a bug fix. The new confirmation modal increases the number of clicks users have to be doing whenever they're switching adaptors with credentials(the default for production work).
| <AlertDialog | ||
| isOpen={isAdaptorChangeConfirmationOpen} | ||
| onClose={handleCloseConfirmation} | ||
| onConfirm={handleConfirmAdaptorChange} | ||
| title="Change Adaptor?" | ||
| description="Warning: Changing adaptors will reset the credential for this step. Are you sure you want to continue?" | ||
| confirmLabel="Continue" | ||
| cancelLabel="Cancel" | ||
| variant="primary" | ||
| /> |
There was a problem hiding this comment.
We now have this <AlertDialog /> being placed in two different places. I'm wondering wouldn't it have been best to have it in the <ConfigureAdaptor /> component so that wherever adaptor modal is used, we automatically have this <AlertDialog /> ?
There was a problem hiding this comment.
Thank you for your feedback 🙏🏻.
I don't think adding the alert dialog to ConfigureAdaptorModal component will work because:
- It opens after the adaptor has already changed - it's for selecting the version/credential for an adaptor
- The confirmation needs to happen before we commit to the new adaptor change, not after (ie from
AdaptorSelectionModal)
I also don't think it makes sense to put it into AdaptorSelectionModal because:
WorkflowDiagramuses it to create new jobs (which don't have credentials and don't need confirmation)- It would force
WorkflowDiagramto passjob/updateJob/setIsConfigureModalOpenetc props it doesn't need - Nested modals feel a bit messy
Instead, I created a composite AdaptorSelector component that bundles AdaptorSelectionModal + AlertDialog + confirmation hook. I also fixed the ESC problem! What do you think?
| <AlertDialog | ||
| isOpen={isAdaptorChangeConfirmationOpen} | ||
| onClose={handleCloseConfirmation} | ||
| onConfirm={handleConfirmAdaptorChange} | ||
| title="Change Adaptor?" | ||
| description="Warning: Changing adaptors will reset the credential for this step. Are you sure you want to continue?" | ||
| confirmLabel="Continue" | ||
| cancelLabel="Cancel" | ||
| variant="primary" | ||
| /> |
There was a problem hiding this comment.
Being called at two different location. could go into adaptor modal
Update all 21 test names in useAdaptorChangeConfirmation tests to use "should" prefix for better readability and consistency with common testing conventions (e.g., "shows confirmation" → "should show confirmation").
The AlertDialog for confirming adaptor changes was duplicated in FullScreenIDE and JobForm. Extract this into a composite AdaptorSelector component that bundles AdaptorSelectionModal, AlertDialog, and the confirmation hook. Fixes ESC key handling by adding a higher-priority keyboard shortcut (110 vs 100) so pressing ESC on the confirmation dialog closes it instead of the picker behind it. Removes ~70 lines of duplicated code.
2 participants
Originally raised in #4413 but I missed adding this functionality to FullScreenIDE, so started the approach again from scratch with this information.
Description
Fixes credential handling when changing job adaptors. When users select a different adaptor, a confirmation modal now warns that credentials will be reset.
Key changes:
project_credential_idandkeychain_credential_idare reset on confirmationuseAdaptorChangeConfirmationhook to share logic betweenFullScreenIDEandJobFormImprovements over PR #4413:
Closes #4395
Validation steps
Setup:
1. Confirmation appears when changing adaptors WITH credentials:
2. No confirmation when changing adaptors WITHOUT credentials:
3. No confirmation for same adaptor
Set job to HTTP adaptor with credential
Click "Configure" → "Change" → Select HTTP again
✓ No confirmation, configure modal reopens
4. No confirmation when changing versions:
5. Cancel preserves state:
Repeat the same tests for FullScreenIDE - Click "Code" and then change the adaptor from there
Additional notes for the reviewer
This is consistent with existing behaviour for delete/reset confirmations throughout Inspector. If we want to
change this so Escape only closes the confirmation dialog (leaving the parent modal open), that should be
handled in a separate issue as it would require reworking how AlertDialog integrates with the keyboard
shortcut priority system across the application.
AI Usage
Please disclose whether you've used AI anywhere in this PR (it's cool, we just
want to know!):
You can read more details in our
Responsible AI Policy
Pre-submission checklist
/reviewwith Claude Code)
(e.g.,
:owner,:admin,:editor,:viewer)