[cueweb/docs] Add Services (Facility Service Defaults) page (CueCommander parity) - #2415
Conversation
…parity) Wire the existing CueCommander "Services" menu item to a new /services page mirroring CueGUI's "Facility Service Defaults" tab (ServiceDialog): a left pane listing facility default services and a right-pane edit form. (In CueGUI the Services tab is the Facility Service Defaults tab, so the page lives at /services where the sidebar item already points.) - Form fields match CueGUI: Name, Threadable, Min/Max Threads (100 = 1 thread), Min Memory MB, Min Gpu Memory MB, Timeout, Timeout LLU, OOM Increase MB, predefined Tags (two-column order) plus a Custom Tags toggle. Memory converts MB<->KB (x1024); threads stored as cores*100. - Save validates (name length/charset, non-negative numbers, min <= max threads when max > 0, OOM > 0, tag charset) then shows a facility-wide confirmation before Create (new) or Update (existing). New / Del in the left pane (Del confirms first). - Proxy routes under app/api/service/: getdefaultservices, create, update, delete. Service type + getDefaultServices in get_utils; create/update/ deleteService in action_utils.
📝 WalkthroughWalkthroughThis PR adds complete facility-wide default service management to cueweb, including data types, four API route handlers (create, read, update, delete), client-side service utilities, an editable form component, and a page to list and manage services. ChangesFacility Service Defaults Management
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@cueweb/app/utils/get_utils.ts`:
- Around line 369-372: getDefaultServices currently converts non-array responses
(including null from accessGetApi on failures) into an empty list, hiding load
failures; change getDefaultServices (the exported async function) to check
whether the response is an array and throw an Error when it is not (mirroring
getHosts behavior) instead of returning [], so callers can handle request
failures distinctly from an empty service list.
In `@cueweb/components/ui/service-defaults-form.tsx`:
- Around line 159-179: The confirm flows call boolean-returning helpers
(createService/updateService and deleteService) but return normally on false,
which lets ConfirmDialog close; change these handlers so a failure propagates as
a thrown error instead of returning silently: in service-defaults-form.tsx's
handleConfirm check the result of createService/updateService (symbols:
createService, updateService, handleConfirm, onSaved) and if ok is false throw
an Error (or reject the promise) with a descriptive message so ConfirmDialog's
onConfirm sees an exception and keeps the modal open; do the same in
app/services/page.tsx for the delete flow (symbol: deleteService and the delete
handler) — throw when deleteService returns false and only call success
toast/onDeleted when the call returned true.
- Around line 117-137: The numeric validation currently allows fractional
numbers (e.g., 1.5) because it only checks Number.isFinite; update the
validation used for numericFields (the array with ["Min Threads", minThreads],
["Max Threads", maxThreads], ["Min Memory MB", minMemoryMb], ["Min Gpu Memory
MB", minGpuMemoryMb], ["Timeout", timeout], ["Timeout LLU", timeoutLlu], ["OOM
Increase MB", oomIncreaseMb]) to reject non-integer values where the field is
integer-backed by adding a Number.isInteger(x) requirement (and keep the
existing non-negative check), and also ensure the OOM check uses integer > 0
(Number.isInteger and x > 0); finally apply the same integer-enforcing change to
the other similar validation block referenced elsewhere (the other numeric
validation around timeout/KB fields) so fractional inputs are rejected
consistently for minCores/maxCores/timeout/timeoutLlu and KB-backed memory
fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4f78bf74-5205-4391-898d-657a5e9d09c9
📒 Files selected for processing (8)
cueweb/app/api/service/create/route.tscueweb/app/api/service/delete/route.tscueweb/app/api/service/getdefaultservices/route.tscueweb/app/api/service/update/route.tscueweb/app/services/page.tsxcueweb/app/utils/action_utils.tscueweb/app/utils/get_utils.tscueweb/components/ui/service-defaults-form.tsx
…ng them Address review feedback on the Facility Service Defaults page: - get_utils.ts: getDefaultServices now throws on a non-array response (mirroring getHosts) instead of collapsing a failed load into []. A backend outage reaches the page's catch/error state rather than falsely rendering "No services defined". - service-defaults-form.tsx: require non-negative integers for the numeric fields (Min/Max Threads, Min Memory, Min GPU Memory, Timeout, Timeout LLU, OOM Increase). They map to integer-backed proto fields and CueGUI uses integer spin boxes, so fractional input like 1.5 is now rejected up front. - service-defaults-form.tsx / services/page.tsx: throw from the confirm handlers when createService / updateService / deleteService return false, so ConfirmDialog keeps the modal open for retry instead of dismissing as if the action had succeeded. The error toast is still surfaced by the helpers.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cueweb/app/services/page.tsx (1)
41-45:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve a distinct load-failure state instead of falling back to empty data.
At Line 44, the initial fetch failure is still coerced to
[], so the UI can render "No services defined." (Lines 112-114) during backend outages. That reintroduces the same failure-masking behavior this PR is trying to eliminate.Suggested minimal fix
export default function FacilityServiceDefaultsPage() { const [services, setServices] = React.useState<Service[] | null>(null); + const [loadError, setLoadError] = React.useState<string | null>(null); @@ const load = React.useCallback(async (isCancelled?: () => boolean) => { try { const data = await getDefaultServices(); if (isCancelled?.()) return; + setLoadError(null); setServices(data); } catch (err) { if (isCancelled?.()) return; handleError(err, "Could not load services"); + setLoadError("Could not load services"); setServices((prev) => prev ?? []); } }, []); @@ - ) : sorted.length === 0 ? ( + ) : loadError ? ( + <p className="p-3 text-sm text-destructive">{loadError}</p> + ) : sorted.length === 0 ? ( <p className="p-3 text-sm text-muted-foreground">No services defined.</p> ) : (Also applies to: 112-114
🤖 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/services/page.tsx` around lines 41 - 45, In the catch block of the service loading function, the code is masking the load failure by defaulting to an empty array with the fallback `prev ?? []` in the setServices call. Instead of coercing the services state to an empty array when an error occurs, preserve a distinct failure state that allows the UI to differentiate between a load failure and genuinely empty services. Either keep the previous value unchanged or explicitly set an error/loading state so that the UI rendering logic can display appropriate error messaging instead of showing "No services defined" during backend outages.
🤖 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.
Outside diff comments:
In `@cueweb/app/services/page.tsx`:
- Around line 41-45: In the catch block of the service loading function, the
code is masking the load failure by defaulting to an empty array with the
fallback `prev ?? []` in the setServices call. Instead of coercing the services
state to an empty array when an error occurs, preserve a distinct failure state
that allows the UI to differentiate between a load failure and genuinely empty
services. Either keep the previous value unchanged or explicitly set an
error/loading state so that the UI rendering logic can display appropriate error
messaging instead of showing "No services defined" during backend outages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 43c6e3b7-6323-4c98-915c-8846687f791a
📒 Files selected for processing (3)
cueweb/app/services/page.tsxcueweb/app/utils/get_utils.tscueweb/components/ui/service-defaults-form.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- cueweb/components/ui/service-defaults-form.tsx
- User guide: Add user-facing Facility Service Defaults section with screenshots - Reference: Add technical entry (routes, RPCs, MB/KB and units, validation) - Developer guide: Add CueCommander-parity architecture section - Other guides: Add overview entry - Add CueCommander Facility Services screenshots (light + dark variants)
|
Ready for review! |
# Conflicts: # cueweb/app/utils/action_utils.ts # cueweb/app/utils/get_utils.ts # docs/_docs/developer-guide/cueweb-development.md # docs/_docs/other-guides/cueweb.md # docs/_docs/reference/cueweb.md # docs/_docs/user-guides/cueweb-user-guide.md
30199a8
into
AcademySoftwareFoundation:master
Related Issues
Main issue:
Issues related to this PR:
Summarize your change.
[cueweb] Add Services (Facility Service Defaults) page (CueCommander parity)
Wire the existing CueCommander "Services" menu item to a new /services page mirroring CueGUI's "Facility Service Defaults" tab (ServiceDialog): a left pane listing facility default services and a right-pane edit form. (In CueGUI the Services tab is the Facility Service Defaults tab, so the page lives at /services where the sidebar item already points.)
[cueweb/docs] Document Facility Service Defaults (Services) page