UI improvements #56
UI improvements #56
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements UI improvements for the Torc dashboard, focusing on better resource presets, improved scheduler naming workflow, and enhanced execution plan visualization with scheduler allocation details.
Key Changes:
- Introduced a new "large" resource preset and adjusted "medium" preset values to provide better granularity for resource selection
- Added interactive scheduler naming with user prompt and validation during creation from jobs
- Refactored execution plan from client-side computation to server-side CLI command, now including scheduler allocations per stage
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| torc-dash/static/js/app-wizard.js | Added "large" resource preset (8 CPUs, 50g memory), reduced "medium" to 4 CPUs/10g; implemented user prompt for scheduler naming with cancellation handling |
| torc-dash/static/js/app-modals.js | Refactored execution plan to use server-side CLI command instead of client-side computation; added renderSchedulerAllocations() to display scheduler allocation details per stage |
| torc-dash/static/js/api.js | Added cliExecutionPlan() API method to fetch execution plan with scheduler allocations from server |
| torc-dash/src/main.rs | Added cli_execution_plan_handler route handler to execute "torc workflows execution-plan" command with JSON output format |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
torc-dash/static/js/app-wizard.js
Outdated
| if (!schedulerName) { | ||
| // User cancelled |
There was a problem hiding this comment.
The prompt cancellation check doesn't handle the case where the user enters only whitespace. When the user enters only spaces and clicks OK, schedulerName will be a non-null string (e.g., " "), pass the null check, but then schedulerName.trim() will be an empty string. This would create a scheduler with an empty name, which would likely fail validation later (as seen in line 147 where schedulers must have a name). Consider checking if the trimmed value is empty as well.
| if (!schedulerName) { | |
| // User cancelled | |
| if (!schedulerName || !schedulerName.trim()) { | |
| // User cancelled or entered only whitespace |
torc-dash/static/js/app-modals.js
Outdated
| const plan = JSON.parse(result.stdout); | ||
| content.innerHTML = this.renderExecutionPlan(plan); | ||
| } catch (error) { | ||
| content.innerHTML = `<div class="placeholder-message">Error loading execution plan: ${error.message}</div>`; |
There was a problem hiding this comment.
The error message is not escaped before being inserted into the HTML. While error.message from JavaScript exceptions is typically safe, for consistency with the rest of the codebase and to prevent potential XSS if the error message contains user-controlled data, this should use this.escapeHtml(error.message) like the other error handlers in this file (see lines 320, 334).
| content.innerHTML = `<div class="placeholder-message">Error loading execution plan: ${error.message}</div>`; | |
| content.innerHTML = `<div class="placeholder-message">Error loading execution plan: ${this.escapeHtml(error.message)}</div>`; |
| const schedulerId = ++this.wizardSchedulerIdCounter; | ||
| const schedulerName = `scheduler-${schedulerId}`; | ||
| const defaultName = `scheduler-${schedulerId}`; | ||
| const schedulerName = prompt('Enter a name for the new scheduler:', defaultName); |
There was a problem hiding this comment.
Using the native prompt() dialog can be problematic for user experience and accessibility. The dialog is modal and blocks all JavaScript execution, the styling is inconsistent across browsers and cannot be customized, and it doesn't support screen readers well. Consider using a custom modal dialog for a better user experience, consistent with other UI patterns in the application.
torc-dash/static/js/app-modals.js
Outdated
| remaining.delete(id); | ||
| }); | ||
| // Parse the JSON output | ||
| const plan = JSON.parse(result.stdout); |
There was a problem hiding this comment.
The JSON.parse() call can throw an exception if the stdout contains invalid JSON. This would be caught by the outer try-catch block, but the error message would be generic ("Error loading execution plan: ...") rather than indicating it was a JSON parsing error. Consider wrapping this in a try-catch block to provide a more specific error message, such as "Invalid JSON response from server".
| const plan = JSON.parse(result.stdout); | |
| let plan; | |
| try { | |
| plan = JSON.parse(result.stdout); | |
| } catch (parseError) { | |
| content.innerHTML = `<div class="placeholder-message">Invalid JSON response from server: ${this.escapeHtml(parseError.message)}</div>`; | |
| return; | |
| } |
No description provided.