Skip to content

feat(web): loading icon and error modal on actions + better support for Safari#823

Open
krish-nvidia wants to merge 5 commits intoNVIDIA:mainfrom
krish-nvidia:web-action-handling
Open

feat(web): loading icon and error modal on actions + better support for Safari#823
krish-nvidia wants to merge 5 commits intoNVIDIA:mainfrom
krish-nvidia:web-action-handling

Conversation

@krish-nvidia
Copy link
Copy Markdown
Contributor

@krish-nvidia krish-nvidia commented Apr 6, 2026

Description

This PR replaces the old inline status bubbles with a dropdown modal from the top for all web admin actions:

  • Spinner on submit so it's obvious an action is in progress
  • Success modal from top of the screen (auto-dismisses)
  • Error modal on failure with a copy button, instead of dumping to a raw error page

It also:

  • Extended action status tracking to SetFirstBootOrder, DisableSecureBoot, EnableLockdown, DisableLockdown
  • Actions that returned raw HTTP errors now redirect back and show the error in the overlay
  • Added -webkit-backdrop-filter prefixes and a body.preload class to fix Safari transition flashes
Screenshots

Loading Icon (Disabled Buttons)

Loading Icons

Success

Success

Error

Error

Type of Change

  • Add - New feature or capability
  • Change - Changes in existing functionality
  • Fix - Bug fixes
  • Remove - Removed features or deprecated functionality
  • Internal - Internal changes (refactoring, tests, docs, etc.)

Related Issues (Optional)

Breaking Changes

  • This PR contains breaking changes

Testing

  • Unit tests added/updated
  • Integration tests added/updated
  • Manual testing performed
  • No testing required (docs, internal refactor, etc.)

Additional Notes

…or Safari

Signed-off-by: Krish Dandiwala <kdandiwala@nvidia.com>
Signed-off-by: Krish Dandiwala <kdandiwala@nvidia.com>
Signed-off-by: Krish Dandiwala <kdandiwala@nvidia.com>
Signed-off-by: Krish Dandiwala <kdandiwala@nvidia.com>
@krish-nvidia krish-nvidia requested a review from poroh April 6, 2026 15:19
@krish-nvidia krish-nvidia self-assigned this Apr 6, 2026
@krish-nvidia krish-nvidia requested a review from a team as a code owner April 6, 2026 15:19
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 6, 2026

🔐 TruffleHog Secret Scan

No secrets or credentials found!

Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉

🔗 View scan details

🕐 Last updated: 2026-04-06 15:21:29 UTC | Commit: 11943c5

@kensimon
Copy link
Copy Markdown
Contributor

kensimon commented Apr 6, 2026

TBH I'm not a huge fan of fullscreen modals that stop you from interacting with the page while a thing is happening. Especially when it stays up for several seconds after the operation is complete. I'm all about cleaning up the javascript to avoid repeating this logic, but is there a way we could keep the spinner scoped to the button, so that the rest of the page is responsive?

image

@krish-nvidia
Copy link
Copy Markdown
Contributor Author

It was an intentional decision to gray out the whole page when an action is submitted. We don't want the user to be clicking off the page or attempting to issue another action when one is already pending to avoid conflicts. Most actions should finish within a couple seconds but I can make it so the check mark goes away a bit quicker on a success. The error modal is already dismissible immediately via clicking off of it or the dismiss button.

@krish-nvidia krish-nvidia requested a review from nvlitagaki April 6, 2026 17:13
@kensimon
Copy link
Copy Markdown
Contributor

kensimon commented Apr 6, 2026

We don't want the user to be clicking off the page or attempting to issue another action when one is already pending to avoid conflicts

Why not? I can always close the tab, and that constitutes clicking off the page. Why do we care if they do other stuff? Also the "avoid conflicts" part is moot because other users may be issuing commands too, conflicts are stuff we have to deal with. The existing approach already disables the button itself to avoid accidental double-clicks, etc... I really don't think we need to block the page here.

Copy link
Copy Markdown
Contributor

@poroh poroh left a comment

Choose a reason for hiding this comment

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

IMO. It looks better from visual perspective but little bit more restrictive from UX perspective.

@krish-nvidia
Copy link
Copy Markdown
Contributor Author

The existing approach already disables the button itself to avoid accidental double-clicks

To clarify, the previous implementation did the same thing: on submit, it disabled all links, inputs, and selects across the entire page (document.querySelectorAll('a, input, select, #json-link').forEach(el => el.classList.add('disabled'))), not just the button itself. So the page was already fully blocked, it just wasn't visually obvious so the overlay makes that existing behavior explicit.

Why do we care if they do other stuff?

These are operations that change machine state, like power cycling, resetting a BMC, or manipulating the BIOS. I'm no UX engineer, but blocking the page for those few seconds feels like a pretty standard pattern (like payment processing screens) which signals something important is happening. It reduces the chance of someone clicking something else mid-flight and potentially leaving things in a half-baked state.

@krish-nvidia
Copy link
Copy Markdown
Contributor Author

Just had a conversation with Leah, our UI expert, and I've been convinced to leave the spinner next to the button and not block the whole page. Will update the PR shortly.

@kensimon
Copy link
Copy Markdown
Contributor

kensimon commented Apr 6, 2026

feels like a pretty standard pattern

It's a pretty standard pattern because it's lazy. It means nobody wants to go and figure out what pieces of the UI are safe to use and which ones aren't, so they just block the whole page and let the user sit and wait. It's not something we should be moving towards if our existing UI works without it.

To clarify, the previous implementation did the same thing: on submit, it disabled all links, inputs, and selects across the entire page (document.querySelectorAll('a, input, select, #json-link').forEach(el => el.classList.add('disabled'))), not just the button itself.

I wasn't aware we did this... it seems a recent change from a few weeks ago. It's a trivial fix:

diff --git a/crates/api/src/web/action_status.rs b/crates/api/src/web/action_status.rs
index b851eca625..62b6bf7ac8 100644
--- a/crates/api/src/web/action_status.rs
+++ b/crates/api/src/web/action_status.rs
@@ -26,8 +26,8 @@ use itertools::Itertools;
 //  3. Shows spinner
 pub fn action_spinner_script(id: &str) -> String {
     const HANDLER: &str = r#"function() {
-  document.querySelectorAll('a, input, select, #json-link').forEach(el => el.classList.add('disabled'));
   const actionCell = this.closest('.action-cell');
+  actionCell.querySelectorAll('a, input, select, #json-link').forEach(el => el.classList.add('disabled'));
   if (actionCell) {
      actionCell.querySelector('.action-spinner')?.style.setProperty('visibility', 'visible');
      actionCell.querySelector('#action-cell-status')?.style.setProperty('visibility', 'hidden');

To be clear, I like everything else about this change, especially having the error message be placed into a common location. I just don't think we need to be disabling the whole page while an operation happens.

Signed-off-by: Krish Dandiwala <kdandiwala@nvidia.com>
@poroh
Copy link
Copy Markdown
Contributor

poroh commented Apr 6, 2026

diff --git a/crates/api/src/web/action_status.rs b/crates/api/src/web/action_status.rs
index b851eca625..62b6bf7ac8 100644
--- a/crates/api/src/web/action_status.rs
+++ b/crates/api/src/web/action_status.rs
@@ -26,8 +26,8 @@ use itertools::Itertools;
 //  3. Shows spinner
 pub fn action_spinner_script(id: &str) -> String {
     const HANDLER: &str = r#"function() {
-  document.querySelectorAll('a, input, select, #json-link').forEach(el => el.classList.add('disabled'));
   const actionCell = this.closest('.action-cell');
+  actionCell.querySelectorAll('a, input, select, #json-link').forEach(el => el.classList.add('disabled'));
   if (actionCell) {
      actionCell.querySelector('.action-spinner')?.style.setProperty('visibility', 'visible');
      actionCell.querySelector('#action-cell-status')?.style.setProperty('visibility', 'hidden');

I tried it and decided not to go this way because with current design we neither can handle multiple simultaneous actions correctly, nor save user work on selecting next action. This is why I disabled everything. I agree that the best way is to fix these issues but it requires switching paradigm from form POST to Ajax. Without this paradigm shift I believe that we should either block all navigation (as before this PR) or block screen as in initial version of this PR.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants