[cuebot] Complete the gRPC response in ShowInterface.SetCommentEmail#2409
Open
ramonfigueiredo wants to merge 6 commits into
Open
Conversation
Replicates the CueGUI CueCommander Shows window in CueWeb, on top of the existing Create Show modal.
- Stats table: the /shows page now uses the shared SimpleDataTable with a new isShowsTable variant and CueGUI's columns - Show Name, Cores Run (reserved_cores), Frames Run (running_frames), Frames Pending (pending_frames), Jobs (pending_jobs) - loaded from GetActiveShows and auto-refreshing every 30s. Numeric columns sort by their underlying value.
- Row context menu (ShowContextMenu): Show Properties and Create Subscription.
- Show Properties dialog (ShowDialog parity), four tabs: Settings (default max/min cores, comment notification email), Booking (enable booking / enable dispatch), Statistics (read-only show_stats), Raw Show Data. Save calls only the setters whose value changed, then refreshes the table.
- Create Subscription dialog (SubscriptionCreator parity): Show + Alloc dropdowns, Size (100), Burst (110).
- Create Show dialog gains the per-allocation Subscriptions section (checkbox + Size + Burst per allocation), creating the show then a subscription on each checked allocation.
- New proxy routes: show/getactiveshows, allocation/getall, and show/action/ {enablebooking,enabledispatching,setdefaultmaxcores,setdefaultmincores, setcommentemail,createsubscription}. All validate the body (400 on malformed JSON / missing fields) and propagate the gateway's real HTTP status. createsubscription maps Cuebot's duplicate-key error to a short, user-facing "show already has a subscription on that allocation" message.
- get_utils: widen the shared Show type (default cores, comment email, extended show_stats) and add an Allocation type plus getActiveShows / getAllocations; action_utils: enableShowBooking/Dispatching, setShowDefaultMax/MinCores, setShowCommentEmail, createShowSubscription, and the row dispatchers; show-action-events.ts holds the shared dialog event contract.
- Tests: Jest coverage for the show action helpers and the show/allocation get_utils helpers.
Replicates the CueGUI CueCommander Shows window in CueWeb, on top of the existing Create Show modal.
- Stats table: the /shows page now uses the shared SimpleDataTable with a new isShowsTable variant and CueGUI's columns - Show Name, Cores Run (reserved_cores), Frames Run (running_frames), Frames Pending (pending_frames), Jobs (pending_jobs) - loaded from GetActiveShows and auto-refreshing every 30s. Numeric columns sort by their underlying value.
- Row context menu (ShowContextMenu): Show Properties and Create Subscription.
- Show Properties dialog (ShowDialog parity), four tabs: Settings (default max/min cores, comment notification email), Booking (enable booking / enable dispatch), Statistics (read-only show_stats), Raw Show Data. Save calls only the setters whose value changed, then refreshes the table.
- Create Subscription dialog (SubscriptionCreator parity): Show + Alloc dropdowns, Size (100), Burst (110).
- Create Show dialog gains the per-allocation Subscriptions section (checkbox + Size + Burst per allocation), creating the show then a subscription on each checked allocation.
- New proxy routes: show/getactiveshows, allocation/getall, and show/action/ {enablebooking,enabledispatching,setdefaultmaxcores,setdefaultmincores, setcommentemail,createsubscription}. All validate the body (400 on malformed JSON / missing fields) and propagate the gateway's real HTTP status. createsubscription maps Cuebot's duplicate-key error to a short, user-facing "show already has a subscription on that allocation" message.
- get_utils: widen the shared Show type (default cores, comment email, extended show_stats) and add an Allocation type plus getActiveShows / getAllocations; action_utils: enableShowBooking/Dispatching, setShowDefaultMax/MinCores, setShowCommentEmail, createShowSubscription, and the row dispatchers; show-action-events.ts holds the shared dialog event contract.
- Tests: Jest coverage for the show action helpers and the show/allocation get_utils helpers.
Documents CueWeb's CueCommander Shows window (stats table, Create Show, Show Properties, Create Subscription) across the CueWeb docs. - User guide (cueweb-user-guide.md): new Shows section with the stats table, a Show columns table, Create Show (with subscriptions), the row actions menu, the four Show Properties tabs (Settings / Booking / Statistics / Raw Show Data), and Create Subscription - each with a light screenshot. - Overview (other-guides/cueweb.md): add a Shows feature entry (stats table, Create Show with per-allocation subscriptions, Show Properties and Create Subscription row actions). - Reference (reference/cueweb.md): add a Shows behavior section plus Show Properties / Create Subscription subsections, list the new show RPCs and /api/show/... + /api/allocation/getall proxy routes (with body-validation and duplicate-error notes), and mark the Shows route implemented in the roadmap. - Developer guide (cueweb-development.md): note the SimpleDataTable isShowsTable flag / ShowContextMenu, and add a Shows window section (file layout, stats table, CustomEvent dance, the three dialogs and their validation, action helpers/routes). - Add Shows screenshots (light + dark) for the page, columns, search, menu, Create Show, Create Subscription, and the four Show Properties tabs.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds a complete Shows UI to CueWeb (listing, create, properties, subscriptions, table/columns, routes, helpers) and fixes a Cuebot gRPC bug by calling responseObserver.onCompleted() in ManageShow#setCommentEmail; tests and docs included. ChangesShows Window (CueCommander Parity)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
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 |
Collaborator
Author
|
This PR was created on top of PR: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cueweb/app/api/show/action/setcommentemail/route.ts (1)
40-44: ⚡ Quick winShared pattern: eliminate unnecessary JSON parsing and re-wrapping across action routes.
All three action routes (
setcommentemail,setdefaultmaxcores,setdefaultmincores) parsehandleRoute'sNextResponse, extract data, and re-wrap it in an identical structure. SincehandleRoutealready returns the correctly formatted response ({ data: ... }on success,{ error: ... }on failure), these routes can simply return its response directly, removing redundant overhead.♻️ Apply this refactor to all three routes
const body = JSON.stringify(jsonBody); - const response = await handleRoute(method, endpoint, body, true); - const responseData = await response.json(); - - if (!response.ok) return NextResponse.json({ error: responseData.error }, { status: response.status }); - return NextResponse.json({ data: responseData.data }, { status: response.status }); + return await handleRoute(method, endpoint, body, true);🤖 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/show/action/setcommentemail/route.ts` around lines 40 - 44, The route currently calls handleRoute(method, endpoint, body, true), awaits response.json(), and re-wraps that payload into a new NextResponse.json; instead, return the original response directly to avoid redundant parsing and re-wrapping—replace the block in setcommentemail's route handler that does const response = await handleRoute(...); const responseData = await response.json(); if (!response.ok) ... return NextResponse.json(...) with a single return of the awaited response (same change should be applied identically in the setdefaultmaxcores and setdefaultmincores route handlers) so handleRoute's existing { data: ... } / { error: ... } response shape is preserved.
🤖 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/components/ui/show-properties-dialog.tsx`:
- Around line 105-109: Validation currently treats empty strings as zero because
Number("") === 0; before converting maxCores/minCores to numbers (where
nextMax/nextMin are computed) and in the two other validation paths that parse
those inputs, explicitly reject empty values by checking value.trim() === "" and
show the existing toastWarning (or appropriate error) instead of parsing; then
only call Number(...) after the non-empty check and continue the existing
finite/non-negative checks (update places referencing maxCores, minCores,
nextMax, nextMin accordingly).
---
Nitpick comments:
In `@cueweb/app/api/show/action/setcommentemail/route.ts`:
- Around line 40-44: The route currently calls handleRoute(method, endpoint,
body, true), awaits response.json(), and re-wraps that payload into a new
NextResponse.json; instead, return the original response directly to avoid
redundant parsing and re-wrapping—replace the block in setcommentemail's route
handler that does const response = await handleRoute(...); const responseData =
await response.json(); if (!response.ok) ... return NextResponse.json(...) with
a single return of the awaited response (same change should be applied
identically in the setdefaultmaxcores and setdefaultmincores route handlers) so
handleRoute's existing { data: ... } / { error: ... } response shape is
preserved.
🪄 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: 1c140201-7d85-4ea8-8bbd-ce74182a9793
⛔ Files ignored due to path filters (24)
docs/assets/images/cueweb/cueweb_cuecommander_shows.pngis excluded by!**/*.pngdocs/assets/images/cueweb/cueweb_cuecommander_shows_columns.pngis excluded by!**/*.pngdocs/assets/images/cueweb/cueweb_cuecommander_shows_columns_dark.pngis excluded by!**/*.pngdocs/assets/images/cueweb/cueweb_cuecommander_shows_create_new_show.pngis excluded by!**/*.pngdocs/assets/images/cueweb/cueweb_cuecommander_shows_create_new_show_dark.pngis excluded by!**/*.pngdocs/assets/images/cueweb/cueweb_cuecommander_shows_create_subscription_menu.pngis excluded by!**/*.pngdocs/assets/images/cueweb/cueweb_cuecommander_shows_create_subscription_menu_dark.pngis excluded by!**/*.pngdocs/assets/images/cueweb/cueweb_cuecommander_shows_create_subscription_window.pngis excluded by!**/*.pngdocs/assets/images/cueweb/cueweb_cuecommander_shows_create_subscription_window_dark.pngis excluded by!**/*.pngdocs/assets/images/cueweb/cueweb_cuecommander_shows_dark.pngis excluded by!**/*.pngdocs/assets/images/cueweb/cueweb_cuecommander_shows_menu.pngis excluded by!**/*.pngdocs/assets/images/cueweb/cueweb_cuecommander_shows_menu_dark.pngis excluded by!**/*.pngdocs/assets/images/cueweb/cueweb_cuecommander_shows_properties_menu.pngis excluded by!**/*.pngdocs/assets/images/cueweb/cueweb_cuecommander_shows_properties_menu_booking_tab.pngis excluded by!**/*.pngdocs/assets/images/cueweb/cueweb_cuecommander_shows_properties_menu_booking_tab_dark.pngis excluded by!**/*.pngdocs/assets/images/cueweb/cueweb_cuecommander_shows_properties_menu_dark.pngis excluded by!**/*.pngdocs/assets/images/cueweb/cueweb_cuecommander_shows_properties_menu_raw_show_data_tab.pngis excluded by!**/*.pngdocs/assets/images/cueweb/cueweb_cuecommander_shows_properties_menu_raw_show_data_tab_dark.pngis excluded by!**/*.pngdocs/assets/images/cueweb/cueweb_cuecommander_shows_properties_menu_settings_tab.pngis excluded by!**/*.pngdocs/assets/images/cueweb/cueweb_cuecommander_shows_properties_menu_settings_tab_dark.pngis excluded by!**/*.pngdocs/assets/images/cueweb/cueweb_cuecommander_shows_properties_menu_statistics_tab.pngis excluded by!**/*.pngdocs/assets/images/cueweb/cueweb_cuecommander_shows_properties_menu_statistics_tab_dark.pngis excluded by!**/*.pngdocs/assets/images/cueweb/cueweb_cuecommander_shows_search.pngis excluded by!**/*.pngdocs/assets/images/cueweb/cueweb_cuecommander_shows_search_dark.pngis excluded by!**/*.png
📒 Files selected for processing (27)
cuebot/src/main/java/com/imageworks/spcue/servant/ManageShow.javacuebot/src/test/java/com/imageworks/spcue/test/servant/ManageShowTests.javacueweb/app/__tests__/api/utils/show_action_utils.test.tscueweb/app/__tests__/utils/show_get_utils.test.tscueweb/app/api/allocation/getall/route.tscueweb/app/api/show/action/createsubscription/route.tscueweb/app/api/show/action/enablebooking/route.tscueweb/app/api/show/action/enabledispatching/route.tscueweb/app/api/show/action/setcommentemail/route.tscueweb/app/api/show/action/setdefaultmaxcores/route.tscueweb/app/api/show/action/setdefaultmincores/route.tscueweb/app/api/show/getactiveshows/route.tscueweb/app/shows/page.tsxcueweb/app/shows/show-columns.tsxcueweb/app/shows/shows-client.tsxcueweb/app/utils/action_utils.tscueweb/app/utils/get_utils.tscueweb/components/ui/context_menus/action-context-menu.tsxcueweb/components/ui/create-show-dialog.tsxcueweb/components/ui/create-subscription-dialog.tsxcueweb/components/ui/show-action-events.tscueweb/components/ui/show-properties-dialog.tsxcueweb/components/ui/simple-data-table.tsxdocs/_docs/developer-guide/cueweb-development.mddocs/_docs/other-guides/cueweb.mddocs/_docs/reference/cueweb.mddocs/_docs/user-guides/cueweb-user-guide.md
ManageShow.setCommentEmail called responseObserver.onNext(...) but never responseObserver.onCompleted(), so the gRPC stream was never closed and callers hung until they timed out (the email was still written to the database). Every other setter in this servant closes the stream. Add the missing onCompleted() so the RPC returns promptly, and add a ManageShowTests.testSetCommentEmail regression test - using a recording StreamObserver that asserts onCompleted() was called (the existing FakeStreamObserver has empty methods and wouldn't catch this) and that the comment email is persisted. Fixes the hang in CueGUI's and CueWeb's Show Properties "Comment Notification Email" field.
ramonfigueiredo
force-pushed
the
cuebot-fix-setcommentemail-hang
branch
from
ed142fb to
1a9194b
Compare
Number("") and Number(" ") return 0, so a blank numeric field passed the finite/non-negative checks as a valid 0 and could silently persist an unintended zero default or subscription value. Reject empty/whitespace input explicitly before parsing in all three paths:
- Show Properties dialog: default max/min cores.
- Create Subscription dialog: Size / Burst.
- Create Show dialog: each checked allocation's Size / Burst.
Each blocks submit with the message it already used (toast or inline error); the existing NaN/negative checks still cover the other invalid cases.
Collaborator
Author
|
@DiegoTavares / @lithorus For this PR you only need to review this commit: 1a9194b The rest is from the PR: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related Issues
Summarize your change.
ManageShow.setCommentEmail called responseObserver.onNext(...) but never responseObserver.onCompleted(), so the gRPC stream was never closed and callers hung until they timed out (the email was still written to the database). Every other setter in this servant closes the stream.
Add the missing onCompleted() so the RPC returns promptly, and add a ManageShowTests.testSetCommentEmail regression test - using a recording StreamObserver that asserts onCompleted() was called (the existing FakeStreamObserver has empty methods and wouldn't catch this) and that the comment email is persisted.
Fixes the hang in CueGUI's and CueWeb's Show Properties "Comment Notification Email" field.