[cueweb] Job Management actions: min/max cores, batch confirmation, unbook (#2281, #2283, #2288)#2405
Conversation
User guide: add Set Min/Max Cores, Unbook, and multi-job batch confirmation subsections under Job Management Operations, each with light + dark screenshots. Developer guide: document the new job/proc RPCs, the /api/job/action and /api/proc/action proxy routes, the cueweb:cores-changed optimistic-update contract, and the batch confirmation policy.
📝 WalkthroughWalkthroughThis PR implements three interconnected job management features for CueWeb: setting job min/max cores through a dedicated dialog, unbooking procs with optional kill confirmation, and adding confirmation gates to multi-job toolbar operations. The backend exposes three REST routes that proxy to the OpenCue gateway; frontend dialogs drive user interactions via CustomEvents and dispatch success events for table patching or refresh. A new bulk operation flow in the Jobs dashboard toolbar gates destructive and multi-select actions with confirmation dialogs listing affected job names. ChangesJob Management Operations Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
ScreenshotsRendered from the screenshots committed in this PR ( Set Min/Max Cores
Multi-job batch confirmation
Unbook
|
|
Hi @ramonfigueiredo! I’ve spent some time adding mock data and testing this out, but before marking the PR ready for review, I’d like to double-check the scope for Unbook #2288. For reference, CueGUI’s cuegui/cuegui/UnbookDialog.py also exposes a few extra options:
I didn’t include allocation selection, an unbook amount limit, memory/runtime filters, or redirect-to-job/group for now, since they seemed to be outside the scope of the issue description. I looked into it a bit more, and it seems like none of those options should require backend work. The first three already map to existing ProcSearchCriteria fields, and the redirect option looks like it maps to the existing UnbookToGroup / UnbookToJob RPCs: Lines 389 to 422 in de826ac So I think it's gonna be as frontend only follow up. Does the scope of the PR match what you had in mind for #2288, or would you prefer to include any of those options in this PR instead? |
Hi @ttpss930141011 ! Thanks for digging into this and for testing with mock data. Yes, the current scope matches what I had in mind for #2288. My intention for this issue was to bring the existing unbook functionality to CueWeb with the core options, not necessarily to achieve full feature parity with CueGUI in the same PR. I agree with your assessment that allocation selection, amount limits, memory/runtime filters, and redirect-to-job/group appear to be frontend-only enhancements, since they already map to existing ProcSearchCriteria fields and the existing UnbookToGroup / UnbookToJob RPCs. Those would make good follow-up improvements and can be tackled separately. So I'd prefer to keep this PR focused and avoid expanding the scope. Once this lands, we can open additional issues (or a single parity issue) for the remaining options. Thanks again for investigating this and confirming the backend support. |
|
Excellent job, @ttpss930141011 ! I will review your PR soon. Once it is ready for review, please change the status to Thanks |
ttpss930141011
requested review from
DiegoTavares,
lithorus and
ramonfigueiredo
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
cueweb/app/__tests__/api/utils/set-cores.test.ts (1)
21-31: 💤 Low valueRemove unused mocks.
The mocks for
getJobForLayer(lines 29-31) andtoastWarning(line 26) are not used bysetJobCoresor any of the tests. Removing them would improve maintainability by reducing noise in the test setup.♻️ Simplify mock setup
jest.mock('`@/app/utils/notify_utils`', () => ({ toastSuccess: jest.fn(), - toastWarning: jest.fn(), handleError: jest.fn(), })); -jest.mock('`@/app/utils/get_utils`', () => ({ - getJobForLayer: jest.fn(), -}));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cueweb/app/__tests__/api/utils/set-cores.test.ts` around lines 21 - 31, The test file registers unused mocks for getJobForLayer and toastWarning which aren’t used by the setJobCores tests; remove the unused mock entries from the jest.mock calls (delete getJobForLayer from the '`@/app/utils/get_utils`' mock and remove toastWarning from the '`@/app/utils/notify_utils`' mock) so only accessActionApi, toastSuccess, and handleError remain mocked for clarity and maintainability.cueweb/app/__tests__/api/utils/unbook.test.ts (1)
20-30: 💤 Low valueRemove unused mocks.
The mocks for
getJobForLayer(lines 28-30) andtoastWarning(line 25) are not used byunbookJobor any of the tests. Removing them would improve maintainability.♻️ Simplify mock setup
jest.mock('`@/app/utils/notify_utils`', () => ({ toastSuccess: jest.fn(), - toastWarning: jest.fn(), handleError: jest.fn(), })); -jest.mock('`@/app/utils/get_utils`', () => ({ - getJobForLayer: jest.fn(), -}));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cueweb/app/__tests__/api/utils/unbook.test.ts` around lines 20 - 30, Remove the unused mocks from the test setup: delete the jest.mock entry that stubs getJobForLayer and remove toastWarning from the notify_utils mock since neither getJobForLayer nor toastWarning are referenced by unbookJob or its tests; keep accessActionApi, toastSuccess, and handleError mocks intact so unbookJob-related behavior remains covered.cueweb/app/api/job/action/setmaxcores/route.ts (1)
22-25: 💤 Low valueOptional: method guard is redundant in Next.js 15 route handlers.
Next.js route handlers are method-specific by export name. The
method !== 'POST'check on line 23 will never trigger for aPOSTfunction export. Consider removing lines 22-25 to simplify.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cueweb/app/api/job/action/setmaxcores/route.ts` around lines 22 - 25, Remove the redundant HTTP method guard in the route handler: since this file is a Next.js route exporting a POST handler (exported POST function), delete the block that reads the request.method and returns a 405 when not 'POST' (the if (method !== 'POST') { ... } block). Keep the rest of the POST handler logic intact and rely on Next.js to dispatch only POST requests to the exported POST function.cueweb/app/api/proc/action/unbook/route.ts (1)
30-33: 💤 Low valueOptional: method guard is redundant in Next.js 15 route handlers.
The
method !== 'POST'check on line 31 will never trigger for aPOSTfunction export. Consider removing lines 30-33 for consistency with the framework's method-specific routing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cueweb/app/api/proc/action/unbook/route.ts` around lines 30 - 33, Remove the redundant runtime method guard: delete the lines that read and check request.method (the `const method = request.method;` and `if (method !== 'POST') { ... }` conditional) from the route handler so the file relies on the Next.js 15 export POST function routing; keep the exported POST handler (and its existing logic) intact and ensure no other code depends on the removed `method` variable.cueweb/app/api/job/action/setmincores/route.ts (1)
22-25: 💤 Low valueOptional: method guard is redundant in Next.js 15 route handlers.
Next.js route handlers are method-specific by export name: the
POSTfunction only receives POST requests. The explicitmethod !== 'POST'check on line 23 will never trigger. Consider removing lines 22-25 to simplify the code, or keep them as defensive programming if preferred.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cueweb/app/api/job/action/setmincores/route.ts` around lines 22 - 25, The explicit method guard using request.method and NextResponse in the route handler is redundant because Next.js route files dispatch by exported function name (POST), so remove the check block that reads request.method !== 'POST' and the 405 response; keep only the POST export implementation (e.g., the POST function in route.ts) and any remaining request handling logic, or if you prefer defensive coding, convert the block into a short comment explaining it's intentionally redundant.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cueweb/app/__tests__/api/utils/set-cores.test.ts`:
- Around line 21-31: The test file registers unused mocks for getJobForLayer and
toastWarning which aren’t used by the setJobCores tests; remove the unused mock
entries from the jest.mock calls (delete getJobForLayer from the
'`@/app/utils/get_utils`' mock and remove toastWarning from the
'`@/app/utils/notify_utils`' mock) so only accessActionApi, toastSuccess, and
handleError remain mocked for clarity and maintainability.
In `@cueweb/app/__tests__/api/utils/unbook.test.ts`:
- Around line 20-30: Remove the unused mocks from the test setup: delete the
jest.mock entry that stubs getJobForLayer and remove toastWarning from the
notify_utils mock since neither getJobForLayer nor toastWarning are referenced
by unbookJob or its tests; keep accessActionApi, toastSuccess, and handleError
mocks intact so unbookJob-related behavior remains covered.
In `@cueweb/app/api/job/action/setmaxcores/route.ts`:
- Around line 22-25: Remove the redundant HTTP method guard in the route
handler: since this file is a Next.js route exporting a POST handler (exported
POST function), delete the block that reads the request.method and returns a 405
when not 'POST' (the if (method !== 'POST') { ... } block). Keep the rest of the
POST handler logic intact and rely on Next.js to dispatch only POST requests to
the exported POST function.
In `@cueweb/app/api/job/action/setmincores/route.ts`:
- Around line 22-25: The explicit method guard using request.method and
NextResponse in the route handler is redundant because Next.js route files
dispatch by exported function name (POST), so remove the check block that reads
request.method !== 'POST' and the 405 response; keep only the POST export
implementation (e.g., the POST function in route.ts) and any remaining request
handling logic, or if you prefer defensive coding, convert the block into a
short comment explaining it's intentionally redundant.
In `@cueweb/app/api/proc/action/unbook/route.ts`:
- Around line 30-33: Remove the redundant runtime method guard: delete the lines
that read and check request.method (the `const method = request.method;` and `if
(method !== 'POST') { ... }` conditional) from the route handler so the file
relies on the Next.js 15 export POST function routing; keep the exported POST
handler (and its existing logic) intact and ensure no other code depends on the
removed `method` variable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f513c11f-8904-49bc-a097-e6ce95b3c449
⛔ Files ignored due to path filters (14)
docs/assets/images/cueweb/cueweb_cuetopia_monitor_jobs_batch_confirmation.pngis excluded by!**/*.pngdocs/assets/images/cueweb/cueweb_cuetopia_monitor_jobs_batch_confirmation_dark.pngis excluded by!**/*.pngdocs/assets/images/cueweb/cueweb_cuetopia_monitor_jobs_set_min_max_cores_confirmation.pngis excluded by!**/*.pngdocs/assets/images/cueweb/cueweb_cuetopia_monitor_jobs_set_min_max_cores_confirmation_dark.pngis excluded by!**/*.pngdocs/assets/images/cueweb/cueweb_cuetopia_monitor_jobs_set_min_max_cores_menu.pngis excluded by!**/*.pngdocs/assets/images/cueweb/cueweb_cuetopia_monitor_jobs_set_min_max_cores_menu_dark.pngis excluded by!**/*.pngdocs/assets/images/cueweb/cueweb_cuetopia_monitor_jobs_set_min_max_cores_window.pngis excluded by!**/*.pngdocs/assets/images/cueweb/cueweb_cuetopia_monitor_jobs_set_min_max_cores_window_dark.pngis excluded by!**/*.pngdocs/assets/images/cueweb/cueweb_cuetopia_monitor_jobs_unbook_kill_confirmation.pngis excluded by!**/*.pngdocs/assets/images/cueweb/cueweb_cuetopia_monitor_jobs_unbook_kill_confirmation_dark.pngis excluded by!**/*.pngdocs/assets/images/cueweb/cueweb_cuetopia_monitor_jobs_unbook_menu.pngis excluded by!**/*.pngdocs/assets/images/cueweb/cueweb_cuetopia_monitor_jobs_unbook_menu_dark.pngis excluded by!**/*.pngdocs/assets/images/cueweb/cueweb_cuetopia_monitor_jobs_unbook_window.pngis excluded by!**/*.pngdocs/assets/images/cueweb/cueweb_cuetopia_monitor_jobs_unbook_window_dark.pngis excluded by!**/*.png
📒 Files selected for processing (12)
cueweb/app/__tests__/api/utils/set-cores.test.tscueweb/app/__tests__/api/utils/unbook.test.tscueweb/app/api/job/action/setmaxcores/route.tscueweb/app/api/job/action/setmincores/route.tscueweb/app/api/proc/action/unbook/route.tscueweb/app/jobs/data-table.tsxcueweb/app/utils/action_utils.tscueweb/components/ui/context_menus/action-context-menu.tsxcueweb/components/ui/set-cores-dialog.tsxcueweb/components/ui/unbook-dialog.tsxdocs/_docs/developer-guide/cueweb-development.mddocs/_docs/user-guides/cueweb-user-guide.md
2 participants
Related Issues
#2281
#2283
#2288
Summarize your change.
Bundles three CueWeb Job Management actions into one PR, following the same
pattern as the host-actions work (#2397): context-menu / toolbar handler →
app/utils/action_utils.tshelper →app/api/.../route.tsproxy → REST gateway.Each ports the corresponding CueGUI behavior onto the Monitor Jobs page.
Set min/max cores (#2281)
range 0–50000, pre-filled with the job's current values.
min ≤ maxvalidation disables Apply and shows an inline error.SetMinCoresandSetMaxCores(the issue's "both updatedatomically"), with one success toast.
app/api/job/action/setmincores|setmaxcores/route.ts; helpersetJobCores.RequestCoresDialogis an email/SMTP composer — this builds the realSetMinCores/SetMaxCoresRPCs instead.Multi-job batch confirmation (#2283)
(reuses
components/ui/confirm-dialog.tsx).pause / unpause confirm only when ≥2 jobs are selected. Destructive actions use
destructive styling + CueGUI's kill warning text. Cancel dispatches no RPC.
app/jobs/data-table.tsx(no new API routes).Unbook job (#2288)
optional "Kill unbooked frames?" checkbox that adds a second kill-confirmation step
(mirroring CueGUI's
KillConfirmationDialog).procroute:app/api/proc/action/unbook/route.ts→ProcInterface.UnbookProcs, body{ r: { jobs: [...] }, kill }. HelperunbookJob./host.ProcInterface/UnbookProcs(host.protodeclares
package host;, so grpc-gateway useshost.as the prefix), verifiedagainst a running rest-gateway.
Documentation
confirmation, unbook) under the Monitor Jobs / Cuetopia section, each with screenshots.
/api/job/action/...and/api/proc/action/...proxy routes, thecueweb:cores-changedeventcontract, and the batch confirmation policy.
Tests
action_utilshelpers and the new routes.dialog exercised (cores +
min ≤ maxguard, batch pause/kill confirmations with thejob list, unbook + optional kill);
UnbookProcsreturns the expected proc count.npm run lintandtsc --noEmitclean.LLM usage disclosure
Claude (Opus) was used to explore the existing CueGUI patterns, draft unit tests and the
PR request, and write the documentation.
Summary by CodeRabbit
New Features
Documentation