[cueweb] Add per-show group tree with drag-to-reparent#2404
Conversation
Signed-off-by: Michael Vallido <vallido.michael@gmail.com>
…ansion Signed-off-by: Michael Vallido <vallido.michael@gmail.com>
# Conflicts: # cueweb/app/utils/action_utils.ts # cueweb/app/utils/get_utils.ts
# Conflicts: # cueweb/app/utils/get_utils.ts
Drag-and-drop library backing the group tree's reparent interactions. Signed-off-by: Michael Vallido <vallido.michael@gmail.com>
Override the base tsconfig's jsx:preserve so .tsx tests compile under ts-jest. Tests only; production uses Next's compiler. Signed-off-by: Michael Vallido <vallido.michael@gmail.com>
Move getState into app/utils/job_state.ts (re-exported from jobs/columns) and add getJobStateDotColor; add formatGroupDefaults. Pure, unit-tested helpers the group-tree rows reuse without importing the JSX-heavy columns module. Signed-off-by: Michael Vallido <vallido.michael@gmail.com>
Tree view of show -> groups -> subgroups -> jobs at /shows/[showName], with drag to reparent jobs/groups (dnd-kit). Optimistic updates with rollback, serialized reparents to avoid parentage cycles, cycle-safe traversal, whole-region drop targets with highlight, rolled-up stats, memoized rows. The show page renders full-width with a Refresh button and a stable-chrome loading skeleton. Implements AcademySoftwareFoundation#2305. Signed-off-by: Michael Vallido <vallido.michael@gmail.com>
Full-width landing list at /shows of all shows (active first, then alphabetical) with a Refresh button, linking into each show's group tree. sortShows is pure and unit-tested; the page's load+refresh behavior is covered. Signed-off-by: Michael Vallido <vallido.michael@gmail.com>
# Conflicts: # cueweb/app/utils/action_utils.ts
A successful reparent's refetch (getShowGroups) returns [] when the request fails, since accessGetApi swallows errors. setGroups([]) then blanked the tree to "No groups in this show." despite the move succeeding. A show always returns its root group, so [] only means the refetch failed — guard the write to keep the optimistic state in both the group and job persist paths. Add group-tree.test.tsx covering the orchestrator (AcademySoftwareFoundation#2305 acceptance criteria): optimistic reparent + refetch, rollback on failure, the blank-out guard, and the URL expand-state round-trip — paths that had no tests. Remove the unused getSubgroups helper and /api/group/getgroups route; the tree loads the whole show at once via getShowGroups. Signed-off-by: Michael Vallido <vallido.michael@gmail.com>
📝 WalkthroughWalkthroughAdds a client-side GroupTree with DnD reparenting: proxy routes, client helpers, tree build/rollup, DnD validation and immutable updates, optimistic persistence with rollback, UI components/context, show-page integration, tests, and docs. ChangesGroup Tree Feature Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
cueweb/app/utils/job_state.ts (1)
21-39: 💤 Low valueConsider removing optional chaining since
jobparameter is typed as non-nullable.The function signature accepts
job: Job, notjob: Job | null | undefined, but uses optional chaining (job?.state,job?.isPaused, etc.) throughout. If the type contract guarantees a non-nullJob, the optional chaining adds defensive overhead without benefit. Ifjobcan actually be null at runtime, update the signature tojob: Job | null | undefinedfor accuracy.🤖 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/utils/job_state.ts` around lines 21 - 39, getState currently uses optional chaining on the non-nullable job parameter; remove the unnecessary "?" usages and access properties directly (use job.state, job.isPaused, job.jobStats.deadFrames, job.jobStats.dependFrames, job.jobStats.pendingFrames, job.jobStats.runningFrames) so the function matches its Job signature — if job can actually be null/undefined instead, update the signature to job: Job | null | undefined and add an explicit null-check at the top of getState.cueweb/app/utils/action_utils.ts (1)
219-235: 💤 Low valueConsider adding empty-array guards for consistency.
Functions like
addHostTags(line 284) andremoveHostTags(line 292) check for empty arrays and returnfalseearly to avoid unnecessary API calls. The reparent functions would benefit from the same guard.♻️ Suggested refactor
export async function reparentGroups(newParentId: string, groupIds: string[]) { + if (groupIds.length === 0) return false; const endpoint = "/api/group/action/reparentgroups"; const body = JSON.stringify({ group: { id: newParentId }, groups: { groups: groupIds.map(id => ({ id })) }, }); return performAction(endpoint, [body], `Reparented ${groupIds.length} group(s)`); } export async function reparentJobs(newParentId: string, jobIds: string[]) { + if (jobIds.length === 0) return false; const endpoint = "/api/group/action/reparentjobs"; const body = JSON.stringify({ group: { id: newParentId }, jobs: { jobs: jobIds.map(id => ({ id })) }, }); return performAction(endpoint, [body], `Reparented ${jobIds.length} job(s)`); }🤖 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/utils/action_utils.ts` around lines 219 - 235, The reparentGroups and reparentJobs functions make API calls even when given empty arrays; add the same empty-array guard used by addHostTags/removeHostTags so they return false immediately if groupIds or jobIds is empty to avoid unnecessary performAction calls. Modify reparentGroups (function name) to check if groupIds.length === 0 and return false, and similarly modify reparentJobs (function name) to check jobIds.length === 0 and return false before constructing the body and calling performAction.
🤖 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/api/show/getgroups/route.ts`:
- Around line 36-37: The handler is re-wrapping the upstream response and
embedding a non-existent responseData.status in the JSON body while losing the
original HTTP status, causing all errors to appear as 200; update the branches
that call NextResponse.json(...) (the checks around response.ok and the success
return) to stop relying on responseData.status and instead return the upstream
HTTP status via NextResponse.json(body, { status: response.status }), include
the actual error payload (responseData.error) in the body for failures, and
ensure any reference to responseData.status is removed; check usages around
NextResponse.json, response.ok, responseData and the handleRoute serialization
to make sure the real HTTP status is preserved.
In `@cueweb/app/shows/`[showName]/page.tsx:
- Line 34: The code is using React's use() hook (const { showName } =
use(params);) which requires React 19+, but the project is pinned to React 18;
replace the use(params) call by reading params directly (e.g., const { showName
} = params) or otherwise await/unwrap any promise before the component boundary
so you don't rely on React's use(); update the statement that destructures
showName from params and remove the use() import/usage.
In `@cueweb/app/utils/get_utils.ts`:
- Around line 301-306: The function findShowByName returns the raw accessGetApi
result which can be undefined, violating the declared Promise<Show | null>
contract; update findShowByName to call accessGetApi(ENDPOINT, body), check the
response and explicitly return null when undefined (or when the value is not a
valid Show), otherwise return the response cast/typed as Show so callers always
receive either a Show or null; reference the findShowByName function and
accessGetApi call when making this change.
In `@cueweb/components/group-tree/group-node.tsx`:
- Around line 162-188: The CollapsibleTrigger is wrapping a non-interactive div
which prevents keyboard users from focusing or toggling groups; update the
trigger child (the element using setRowRef and containing the folder/label) to
be keyboard-accessible by making it an interactive element (preferably a
<button>-like element) or adding tabIndex={0}, role="button" and an onKeyDown
handler that calls onToggle(node.group.id, nextState) for Enter/Space, and
ensure the existing onMouseEnter/onFocus still call
requestJobsFor(node.group.id) and that click/stopPropagation behavior (the
handleRef/grip button) remains unchanged so drag handle still functions.
---
Nitpick comments:
In `@cueweb/app/utils/action_utils.ts`:
- Around line 219-235: The reparentGroups and reparentJobs functions make API
calls even when given empty arrays; add the same empty-array guard used by
addHostTags/removeHostTags so they return false immediately if groupIds or
jobIds is empty to avoid unnecessary performAction calls. Modify reparentGroups
(function name) to check if groupIds.length === 0 and return false, and
similarly modify reparentJobs (function name) to check jobIds.length === 0 and
return false before constructing the body and calling performAction.
In `@cueweb/app/utils/job_state.ts`:
- Around line 21-39: getState currently uses optional chaining on the
non-nullable job parameter; remove the unnecessary "?" usages and access
properties directly (use job.state, job.isPaused, job.jobStats.deadFrames,
job.jobStats.dependFrames, job.jobStats.pendingFrames,
job.jobStats.runningFrames) so the function matches its Job signature — if job
can actually be null/undefined instead, update the signature to job: Job | null
| undefined and add an explicit null-check at the top of getState.
🪄 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: d4f8e222-d03b-41d9-a9c0-e96f5ba86ded
⛔ Files ignored due to path filters (1)
cueweb/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (34)
cueweb/app/__tests__/api/utils/action_utils.test.tscueweb/app/__tests__/api/utils/get_utils.test.tscueweb/app/__tests__/api/utils/group_defaults.test.tscueweb/app/__tests__/api/utils/job_state.test.tscueweb/app/__tests__/components/group-tree/build-tree.test.tscueweb/app/__tests__/components/group-tree/dnd-helpers.test.tscueweb/app/__tests__/components/group-tree/expanded-param.test.tscueweb/app/__tests__/components/group-tree/group-tree.test.tsxcueweb/app/__tests__/components/group-tree/memoization.test.tsxcueweb/app/__tests__/shows/shows-page.test.tsxcueweb/app/__tests__/shows/sort-shows.test.tscueweb/app/api/group/action/reparentgroups/route.tscueweb/app/api/group/action/reparentjobs/route.tscueweb/app/api/group/getjobs/route.tscueweb/app/api/show/findshow/route.tscueweb/app/api/show/getgroups/route.tscueweb/app/jobs/columns.tsxcueweb/app/shows/[showName]/page.tsxcueweb/app/shows/page.tsxcueweb/app/shows/sort-shows.tscueweb/app/utils/action_utils.tscueweb/app/utils/get_utils.tscueweb/app/utils/group_defaults.tscueweb/app/utils/job_state.tscueweb/components/group-tree/build-tree.tscueweb/components/group-tree/dnd-helpers.tscueweb/components/group-tree/drag-preview.tsxcueweb/components/group-tree/expanded-param.tscueweb/components/group-tree/group-node.tsxcueweb/components/group-tree/group-tree-context.tsxcueweb/components/group-tree/group-tree.tsxcueweb/components/group-tree/job-leaf.tsxcueweb/jest.config.jscueweb/package.json
|
Hi @mvallido, When you have a chance, please resolve the merge conflicts and rebase/merge the latest Once everything is ready for review, please change the PR status from Draft to Open, and I'll review it as soon as possible. Thanks! |
…ger a11y - Routes return the real upstream status (2nd arg) instead of 200, drop the bogus responseData.status. - findShowByName returns `response ?? null` per its Show | null contract. - Expand/collapse trigger is now a focusable <button> (keyboard-accessible), with the `group` class on the trigger and stats as a <span> for valid markup.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cueweb/app/api/group/getjobs/route.ts (2)
22-25: ⚡ Quick winDead code: method check is redundant.
Since this handler is exported as
POST, Next.js guaranteesrequest.methodwill always be'POST'. The check on line 23 can never trigger.♻️ Proposed simplification
export async function POST(request: NextRequest) { const endpoint = "/job.GroupInterface/GetJobs"; - const method = request.method; - if (method !== 'POST') { - return NextResponse.json({ error: 'Invalid method. Only POST is allowed.' }, { status: 405 }); - } - const body = JSON.stringify(await request.json()); + const jsonBody = await request.json(); + const body = JSON.stringify(jsonBody); - const jsonBody = JSON.parse(body); if (!jsonBody || typeof jsonBody !== 'object' || !jsonBody.group?.id) { return NextResponse.json({ error: 'Invalid request body' }, { status: 400 }); } - const response = await handleRoute(method, endpoint, body); + const response = await handleRoute('POST', endpoint, body);🤖 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/group/getjobs/route.ts` around lines 22 - 25, The method check using request.method in the exported POST handler is redundant and unreachable because Next.js only invokes this file for POST requests; remove the conditional block that reads request.method and the early 405 return. Locate the exported POST function in route.ts (the handler that accepts Request) and delete the lines referencing request.method and the NextResponse 405 branch so the handler directly proceeds with request handling logic.
27-28: ⚡ Quick winRedundant parse/stringify cycle.
Lines 27-28 parse the request JSON, stringify it, then immediately parse it again. This is inefficient and unclear.
♻️ Clearer ordering
- const body = JSON.stringify(await request.json()); - const jsonBody = JSON.parse(body); + const jsonBody = await request.json(); + const body = JSON.stringify(jsonBody); if (!jsonBody || typeof jsonBody !== 'object' || !jsonBody.group?.id) { return NextResponse.json({ error: 'Invalid request body' }, { status: 400 }); }🤖 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/group/getjobs/route.ts` around lines 27 - 28, The code performs an unnecessary JSON stringify/parse cycle for the request body: replace the two lines that create body and jsonBody with a single await request.json() call (assign to jsonBody) and remove the intermediate body variable; update any references to body to use jsonBody instead so functions in this module (e.g., the route handler that uses jsonBody) consume the parsed object directly.
🤖 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/api/group/getjobs/route.ts`:
- Line 36: The current non-2xx branch returns NextResponse.json({ error:
responseData.error }, { status: response.status }) which can produce error:
undefined; update the handler in route.ts so when response.ok is false you
return a deterministic error string (e.g., responseData.error ||
response.statusText || "Unknown error") instead of possibly undefined. Locate
the code that builds the error response (the NextResponse.json call when
response.ok is false) and change it to use that fallback so consumers checking
if (res.error) reliably detect errors.
---
Nitpick comments:
In `@cueweb/app/api/group/getjobs/route.ts`:
- Around line 22-25: The method check using request.method in the exported POST
handler is redundant and unreachable because Next.js only invokes this file for
POST requests; remove the conditional block that reads request.method and the
early 405 return. Locate the exported POST function in route.ts (the handler
that accepts Request) and delete the lines referencing request.method and the
NextResponse 405 branch so the handler directly proceeds with request handling
logic.
- Around line 27-28: The code performs an unnecessary JSON stringify/parse cycle
for the request body: replace the two lines that create body and jsonBody with a
single await request.json() call (assign to jsonBody) and remove the
intermediate body variable; update any references to body to use jsonBody
instead so functions in this module (e.g., the route handler that uses jsonBody)
consume the parsed object directly.
🪄 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: 8330e4e5-2b1c-4074-9062-759af08a0785
📒 Files selected for processing (7)
cueweb/app/api/group/action/reparentgroups/route.tscueweb/app/api/group/action/reparentjobs/route.tscueweb/app/api/group/getjobs/route.tscueweb/app/api/show/findshow/route.tscueweb/app/api/show/getgroups/route.tscueweb/app/utils/get_utils.tscueweb/components/group-tree/group-node.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- cueweb/app/api/show/findshow/route.ts
- cueweb/app/api/group/action/reparentgroups/route.ts
- cueweb/app/api/group/action/reparentjobs/route.ts
- cueweb/app/api/show/getgroups/route.ts
- cueweb/app/utils/get_utils.ts
- cueweb/components/group-tree/group-node.tsx
Resolve cueweb conflicts:
- /shows index: adopt upstream's ShowsClient table (create-show dialog,
sortable columns, action menu); drop the simple list + sort-shows.
- /shows/[showName]: keep the group-tree page; switch its show lookup to
the canonical findShow() in show_utils so it works with upstream's
findshow route (notFound/{show} shape).
- findshow route: take upstream (JSON-parse guard, name trim, notFound).
- get_utils: drop the now-redundant findShowByName (superseded by
show_utils.findShow); keep the upstream Show-type comment.
|
Note: This PR has a conflict with one of my recently merged PR: |
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/api/group/getjobs/route.ts (1)
27-30:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle malformed JSON body with a 400 instead of throwing.
Line 27 can throw on invalid JSON, which currently turns a client request error into a server 500. Wrap JSON parsing in
try/catchand return a 400 on parse failure.Proposed fix
- const body = JSON.stringify(await request.json()); - const jsonBody = JSON.parse(body); + let jsonBody: unknown; + try { + jsonBody = await request.json(); + } catch { + return NextResponse.json({ error: 'Invalid JSON body' }, { status: 400 }); + } + const body = JSON.stringify(jsonBody); if (!jsonBody || typeof jsonBody !== 'object' || !jsonBody.group?.id) { return NextResponse.json({ error: 'Invalid request body' }, { status: 400 }); }🤖 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/group/getjobs/route.ts` around lines 27 - 30, The current parsing flow does JSON.stringify(await request.json()) then JSON.parse(body) which can throw and propagate as a 500; replace this with a protected parse: wrap the await request.json() call in a try/catch, return NextResponse.json({ error: 'Invalid JSON' }, { status: 400 }) on parse failure, then validate the resulting jsonBody object (check typeof === 'object' and jsonBody.group?.id) and return the existing 400 invalid body response if validation fails; update references to body/jsonBody accordingly (symbols: request.json(), jsonBody, NextResponse.json).
🤖 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/api/group/getjobs/route.ts`:
- Around line 27-30: The current parsing flow does JSON.stringify(await
request.json()) then JSON.parse(body) which can throw and propagate as a 500;
replace this with a protected parse: wrap the await request.json() call in a
try/catch, return NextResponse.json({ error: 'Invalid JSON' }, { status: 400 })
on parse failure, then validate the resulting jsonBody object (check typeof ===
'object' and jsonBody.group?.id) and return the existing 400 invalid body
response if validation fails; update references to body/jsonBody accordingly
(symbols: request.json(), jsonBody, NextResponse.json).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cf2de66f-ab6f-4b5d-b44e-9b4118d57cfa
📒 Files selected for processing (9)
cueweb/app/__tests__/api/utils/get_utils.test.tscueweb/app/api/group/action/reparentgroups/route.tscueweb/app/api/group/action/reparentjobs/route.tscueweb/app/api/group/getjobs/route.tscueweb/app/api/show/getgroups/route.tscueweb/app/shows/[showName]/page.tsxcueweb/app/utils/action_utils.tscueweb/app/utils/get_utils.tscueweb/components/group-tree/group-node.tsx
💤 Files with no reviewable changes (1)
- cueweb/app/tests/api/utils/get_utils.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- cueweb/app/utils/action_utils.ts
- cueweb/app/api/show/getgroups/route.ts
- cueweb/app/shows/[showName]/page.tsx
- cueweb/app/api/group/action/reparentjobs/route.ts
- cueweb/components/group-tree/group-node.tsx
- cueweb/app/api/group/action/reparentgroups/route.ts
|
Conflicts fixed! |
- Add a "Show detail page (group tree)" section to the CueWeb user guide and reference docs covering the /shows/[showName] page - Document expand/collapse with URL-persisted state, per-job progress/dead/state indicators, and drag-to-reparent - Add the group-tree RPCs and proxy routes (GetGroups, group GetJobs, ReparentGroups, ReparentJobs) to the reference tables - Add light and dark screenshots of the group-tree page
|
Thanks @ramonfigueiredo! I'd flipped it back to draft to work through the CodeRabbit feedback but haven't had the time to revisit it. I should've left a comment. Thank you for jumping in to resolve the conflicts and clean it up. |
|
@mvallido screenshots (v1) ScreenshotsShows page
Drag-and-drop reparent
|
f8e6a0b
into
AcademySoftwareFoundation:master
|
Great job, @mvallido ! I did some improvements in your PR. PR approved and merged. Thanks! |




Related Issues
Summary
Adds a per-show group tree: a view of show -> groups -> subgroups -> jobs with drag-to-reparent - reached by clicking a show in the Shows table.
/shows/[showName]rendering the full group hierarchy down tojobs, with rolled-up stats (running / pending / dead) per group.
@dnd-kit/react): drag a group or job onto a target group to move it. Updates apply optimistically, roll back if the backend rejects them, and retain the optimistic result if a post-success refetch fails, the tree is never left blank./shows): each show name links into its group tree.Mirrors the reparent semantics of CueGUI's
GroupDialog.py/CueJobMonitorTree.py.This branch is merged up to latest
master. The/showsindex it links from is master's Shows management table (sortable columns, Create Show, Show Properties, Create Subscription); the group tree is the show detail page reached from it.Acceptance criteria
How it works
buildTreeFromGroupsassembles the flat group list into a tree and computes rolled-up stats. Orphaned/unreachable groups are dropped.reparentgroups/reparentjobs,getgroups/getjobs,findshow).findShow(app/utils/show_utils.ts), which the group-tree page and the Shows Create-Show dialog both use.Documentation
Testing
npx tsc --noEmit -p .— clean.npx jest— 21 suites / 171 tests passing.serialization guard, the blank-out-on-refetch-failure guard, URL expand-state round-trip
(init + write), row memoization, and the API wrappers.
Testing
next build, which type-checks) is clean, verified by rebuilding the CueWeb image.npx jest: re-run to refresh suite/test counts. Coverage includes tree-building, cycle prevention, optimistic-reparent + rollback, the in-flight serialization guard, the blank-out-on-refetch-failure guard, URL expand-state round-trip (init + write), row memoization, and the API wrappers. (The superseded simple-index testssort-shows.test.ts/shows-page.test.tsxand the redundantfindShowByNametest block were removed when master's Shows table replaced the simple index.)LLM usage disclosure
@mvallido
Co-authored-by: Michael Vallido vallido.michael@gmail.com
Co-authored-by: Ramon Figueiredo rfigueiredo@imageworks.com