UI: Confirm before running scripts#33679
Conversation
jacobshandling
had a problem deploying
to
Docker Hub
GitHub Actions
Error
jacobshandling
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #33679 +/- ##
==========================================
- Coverage 64.00% 64.00% -0.01%
==========================================
Files 2067 2068 +1
Lines 207188 207209 +21
Branches 6847 6766 -81
==========================================
+ Hits 132608 132617 +9
- Misses 64147 64159 +12
Partials 10433 10433
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jacobshandling
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
jacobshandling
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
jacobshandling
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
jacobshandling
force-pushed
the
28711-confirm-before-script-run
branch
from
b13d676 to
93d6e36
Compare
jacobshandling
had a problem deploying
to
Docker Hub
GitHub Actions
Error
jacobshandling
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
jacobshandling
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
sgress454
left a comment
sgress454
left a comment
There was a problem hiding this comment.
LGTM, tested 👍
It's worth siccing Coderabbit on this to catch any inconsistencies with the renaming and shuffling around of vars, in case there's any cases I missed.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughAdds a confirmation step to running scripts from the host details flow by introducing a ConfirmRunScriptModal and refactoring modal orchestration. Updates RunScriptModal and ScriptDetailsModal APIs and control flow to delegate run actions via callbacks. Renames DeleteScriptModal’s completion prop (onDone → afterDelete). Adjusts related table config and tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant SMG as ScriptModalGroup
participant RSM as RunScriptModal
participant SDM as ScriptDetailsModal
participant CRM as ConfirmRunScriptModal
participant API as Scripts API (via callback)
U->>SMG: Open "Run script" actions
SMG->>RSM: Show scripts (hostTeamId, loading flags)
U->>RSM: Select "Run" on a script
RSM-->>SMG: onClickRun(script)
note right of SMG: New step: confirmation before execution
SMG->>CRM: Open confirm modal (scriptName, hostName, isRunningScript)
alt User confirms
U->>CRM: Click "Run"
CRM-->>SMG: onConfirmRunScript()
SMG->>API: Execute run (set isRunningScript)
API-->>SMG: Result (success/error)
SMG->>CRM: Close modal
SMG->>RSM: Refresh/reflect state
else User cancels
U->>CRM: Click "Cancel"
CRM-->>SMG: onCancel()
SMG->>RSM: Return to previous modal
end
U->>RSM: Click a script name
RSM-->>SMG: onClickViewScript(scriptDetails)
SMG->>SDM: Open details (with onClickRun → confirmation)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
frontend/pages/hosts/details/HostDetailsPage/modals/RunScriptModal/ScriptsTableConfig.tsx (1)
108-110: Tiny cleanup: avoid unnecessary template literal.- <span className={`script-info-text`}> + <span className="script-info-text"> {cellProps.row.original.name} </span>frontend/pages/hosts/details/HostDetailsPage/modals/ConfirmRunScriptModal/ConfirmRunScriptModal.tsx (1)
34-53: Form wrapper not needed; use a div.There’s no submit. Dropping the form avoids enter‑key submits and simplifies semantics.
- <form className={`${baseClass}__form`}> + <div className={`${baseClass}__form`}> ... - </form> + </div>frontend/pages/hosts/details/HostDetailsPage/modals/ScriptModalGroup/ScriptModalGroup.tsx (4)
103-110: Use boolean for isScriptContentError and drop magic fallback ID.
- error is an Error object; pass a boolean to isScriptContentError to match the name/intent.
- With enabled guarding, fallback
?? 1isn’t needed.} = useQuery<string, Error>( - ["scriptContent", selectedScript?.script_id], - () => scriptsAPI.downloadScript(selectedScript?.script_id ?? 1), + ["scriptContent", selectedScript?.script_id], + () => scriptsAPI.downloadScript(selectedScript!.script_id), { refetchOnWindowFocus: false, enabled: !!selectedScript && currentModal === ModalGroupOption.ViewScriptDetails, } );And when passing to the child:
- isScriptContentError={isSelectedScriptContentError} + isScriptContentError={!!isSelectedScriptContentError}
112-116: goBack safety fallback.If previousModal is ever null, default to Run to avoid setting null currentModal.
- const goBack = useCallback(() => { - setCurrentModal(previousModal); - setPreviousModal(null); - }, [previousModal]); + const goBack = useCallback(() => { + setCurrentModal(previousModal ?? ModalGroupOption.Run); + setPreviousModal(null); + }, [previousModal]);
117-140: Guard against rapid double‑click on “Run”.Early‑return if already running to prevent duplicate submissions.
const onConfirmRunScript = useCallback(async () => { - // will always be truthy at this point - if (selectedScript) { + if (isRunningScript) return; + if (selectedScript) { try { setIsRunningScript(true); await scriptsAPI.runScript({ host_id: host.id, // will be defined when this is being called script_id: selectedScript.script_id, }); ... } finally { setIsRunningScript(false); setSelectedScript(null); setCurrentModal(ModalGroupOption.Run); } } - }, [host.id, refetchHostScripts, renderFlash, selectedScript]); + }, [host.id, isRunningScript, refetchHostScripts, renderFlash, selectedScript]);If Button already disables on isLoading, this is belt‑and‑suspenders.
142-149: Rename typo: onClikRunBeforeConfirmation → onClickRunBeforeConfirmation.Improves readability and searchability.
- const onClikRunBeforeConfirmation = useCallback( + const onClickRunBeforeConfirmation = useCallback( (script: IHostScript) => { setPreviousModal(currentModal); setCurrentModal(ModalGroupOption.ConfirmRun); setSelectedScript(script); }, [currentModal] );And update call sites in this file.
frontend/pages/hosts/details/HostDetailsPage/modals/RunScriptModal/RunScriptModal.tsx (1)
78-80: Add setPage to callback deps.State setters are stable, but adding it silences lint and is clearer.
- const onQueryChange = useCallback(({ pageIndex }: ITableQueryData) => { - setPage(pageIndex); - }, []); + const onQueryChange = useCallback(({ pageIndex }: ITableQueryData) => { + setPage(pageIndex); + }, [setPage]);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
changes/28711-confirm-script-runs(1 hunks)frontend/pages/ManageControlsPage/Scripts/cards/ScriptBatchProgress/ScriptBatchProgress.tests.tsx(2 hunks)frontend/pages/ManageControlsPage/Scripts/cards/ScriptLibrary/ScriptLibrary.tsx(1 hunks)frontend/pages/ManageControlsPage/Scripts/components/DeleteScriptModal/DeleteScriptModal.tsx(2 hunks)frontend/pages/ManageControlsPage/Scripts/components/ScriptListItem/ScriptListItem.tests.tsx(6 hunks)frontend/pages/hosts/components/ScriptDetailsModal/ScriptDetailsModal.tsx(5 hunks)frontend/pages/hosts/details/HostDetailsPage/modals/ConfirmRunScriptModal/ConfirmRunScriptModal.tsx(1 hunks)frontend/pages/hosts/details/HostDetailsPage/modals/ConfirmRunScriptModal/index.ts(1 hunks)frontend/pages/hosts/details/HostDetailsPage/modals/RunScriptModal/RunScriptModal.tsx(5 hunks)frontend/pages/hosts/details/HostDetailsPage/modals/RunScriptModal/ScriptsTableConfig.tsx(3 hunks)frontend/pages/hosts/details/HostDetailsPage/modals/ScriptModalGroup/ScriptModalGroup.tsx(4 hunks)
🔇 Additional comments (23)
frontend/pages/ManageControlsPage/Scripts/components/ScriptListItem/ScriptListItem.tests.tsx (5)
1-2: LGTM!The addition of
renderWithSetupalongside the existingrenderimport is appropriate for migrating interaction tests to use user-event.
58-70: LGTM!The test has been correctly converted to use async user interactions with proper async/await patterns.
72-86: LGTM!The test correctly validates click handler isolation using async user interactions.
88-102: LGTM!The test correctly validates the delete handler with proper async user interactions.
104-118: LGTM!The test correctly validates the edit handler with proper async user interactions.
frontend/pages/ManageControlsPage/Scripts/cards/ScriptBatchProgress/ScriptBatchProgress.tests.tsx (2)
62-62: LGTM: Test date updated to push scheduled time further into the future.The change from year 2099 to 9999 ensures the test consistently exercises the "many years in the future" rendering path, reducing brittleness as time advances.
201-201: LGTM: Regex updated to accommodate relative time phrasing variation.The updated pattern accepts both "about" and "over" in the relative time string, making the test more resilient to changes in the date-formatting library's output.
frontend/pages/ManageControlsPage/Scripts/components/DeleteScriptModal/DeleteScriptModal.tsx (1)
18-18: LGTM: Prop renamed for clarity.The prop rename from
onDonetoafterDeleteimproves semantic clarity and intention-revealing API design. The change is consistently applied across the interface, destructuring, and invocation.Also applies to: 26-26, 48-48
frontend/pages/ManageControlsPage/Scripts/cards/ScriptLibrary/ScriptLibrary.tsx (1)
186-186: LGTM: Usage updated to match DeleteScriptModal API change.The prop usage correctly reflects the rename from
onDonetoafterDeletein DeleteScriptModal's public interface.frontend/pages/hosts/components/ScriptDetailsModal/ScriptDetailsModal.tsx (5)
40-41: LGTM: Public API expanded with delegation callbacks.The new optional props
onCloseandonClickRunimprove composability by allowing parent components to control exit behavior and script execution flow. The JSDoc comment clearly explains the dual-purpose exit handling.Also applies to: 45-45, 61-63
48-48: LGTM: Type signature expanded to allow null.Adding
| nulltoselectedScriptDetailsimproves type safety and flexibility for cases where script details may not be immediately available.
287-287: LGTM: Exit behavior now supports dual-purpose close/cancel.The
onExithandler correctly delegates toonClosewhen provided, falling back toonCancelotherwise. This allows parent components to distinguish between "go back" and "close" semantics.
292-292: LGTM: Footer rendering now conditional.The footer is now only rendered when
shouldShowFooteris true, eliminating unnecessary rendering when script details are unavailable.
153-172: Resolved:onClickRunis always passed whenshowHostScriptActionsis true
InScriptModalGroup.tsx, the<ScriptDetailsModal … />invocation at lines 207–216 includes bothshowHostScriptActionsandonClickRun={onClikRunBeforeConfirmation}, ensuring the “run” action callback is never undefined.changes/28711-confirm-script-runs (1)
1-2: LGTM: Changelog entry clearly documents the feature.The changelog entry concisely describes the addition of confirmation steps to the two script run flows, aligning with the PR objectives.
frontend/pages/hosts/details/HostDetailsPage/modals/ConfirmRunScriptModal/index.ts (1)
1-1: Barrel re‑export looks good.Keeps imports clean and aligns with local component default export.
frontend/pages/hosts/details/HostDetailsPage/modals/RunScriptModal/ScriptsTableConfig.tsx (2)
54-64: Confirm role gating for “Run” action.Observers (global/team) currently have run permission. If this is unintended, tighten to admin/maintainer only.
82-88: Approve onClickViewScript signature change. All call sites now use the single-argument form; no stale two-argument invocations remain.frontend/pages/hosts/details/HostDetailsPage/modals/ConfirmRunScriptModal/ConfirmRunScriptModal.tsx (1)
28-33: LGTM on modal wiring and loading flags.Title, isLoading/isHidden, and close behavior look correct.
frontend/pages/hosts/details/HostDetailsPage/modals/ScriptModalGroup/ScriptModalGroup.tsx (2)
177-190: Confirm modal wiring matches UX spec.Cancel returns to origin; confirm resets to table; loading state flows down. Good.
236-247: Run details modal back‑nav logic looks correct.Transitions respect previous modal (details vs run list).
frontend/pages/hosts/details/HostDetailsPage/modals/RunScriptModal/RunScriptModal.tsx (2)
105-112: Confirm Enter‑key behavior.
onEnter={onClose}means pressing Enter closes the modal. Validate this is intended for this modal (no text inputs); otherwise consider removing to avoid accidental closure.
116-145: Loading/empty/error states are well handled.Spinner during initial load, error fallback, and empty state copy read well.
2 participants
Related issue: Resolves #28711 and #33685
Checklist for submitter
If some of the following don't apply, delete the relevant line.
Summary by CodeRabbit