refactor: allow secondary rundowns#2086
Open
cpvalente wants to merge 7 commits into
Open
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR enables multi-rundown editing by refactoring both client and server to support explicit rundown ID scoping. The client gains ID-based rundown fetching with scoped selection UI, while the server makes all mutations require explicit ChangesClient-side multi-rundown scoping and UI
Server-side multi-rundown mutation scoping
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
cpvalente
force-pushed
the
cuesheet-rundown
branch
4 times, most recently
from
ceb387a to
b2691be
Compare
alex-Arc
force-pushed
the
cuesheet-rundown
branch
3 times, most recently
from
a82c78a to
088ca1a
Compare
alex-Arc
force-pushed
the
cuesheet-rundown
branch
3 times, most recently
from
b392900 to
9a2f65e
Compare
![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: 7
🧹 Nitpick comments (2)
apps/client/src/views/cuesheet/cuesheet-table/CuesheetTable.tsx (1)
228-232: 💤 Low valueConsider simplifying the loading check.
The
!flatRundowncheck is essentially dead code given thatRundownSource.flatRundownis typed asExtendedEntry[](always an array, never undefined). Thestatus === 'pending'check alone should suffice. However, this is a minor defensive coding pattern that doesn't cause harm.🤖 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 `@apps/client/src/views/cuesheet/cuesheet-table/CuesheetTable.tsx` around lines 228 - 232, The loading check uses a redundant !flatRundown condition; since RundownSource.flatRundown is typed as ExtendedEntry[] (always an array), simplify isLoading to only check status === 'pending' by updating the isLoading assignment (referencing isLoading, flatRundown, and status) and keep the existing early return to EmptyPage unchanged.apps/client/src/views/cuesheet/CuesheetTableWrapper.tsx (1)
58-68: 💤 Low valueUnnecessary fragment wrapper around single element.
The fragment wrapper
<>...</>is redundant when there's only a single child element.♻️ Suggested simplification
insertElement={ - <> - <RundownSelect - cuesheetMode={cuesheetMode} - selectedRundownId={selectedRundownId} - loadedRundownId={loadedRundownId} - setSelectedRundownId={setSelectedRundownId} - projectRundowns={projectRundowns} - /> - </> + <RundownSelect + cuesheetMode={cuesheetMode} + selectedRundownId={selectedRundownId} + loadedRundownId={loadedRundownId} + setSelectedRundownId={setSelectedRundownId} + projectRundowns={projectRundowns} + /> }🤖 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 `@apps/client/src/views/cuesheet/CuesheetTableWrapper.tsx` around lines 58 - 68, The JSX fragment wrapping the single RundownSelect is redundant; in CuesheetTableWrapper replace the insertElement prop value that currently uses <>...</> with a direct JSX element (set insertElement={<RundownSelect ... />}), keeping all props (cuesheetMode, selectedRundownId, loadedRundownId, setSelectedRundownId, projectRundowns) unchanged and leaving RundownSelect as the sole child.
🤖 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 `@apps/client/src/common/hooks/useEntryAction.ts`:
- Around line 73-75: The helper useScopedEntryActions currently maps a null
rundownId to an empty string which creates an explicit scoped key and bypasses
the fallback path in useEntryActionsForRundown; update useScopedEntryActions so
it forwards undefined (e.g. pass rundownId ?? undefined or conditional if null)
instead of '' and keep the parameter types compatible with
useEntryActionsForRundown (useScopedEntryActions(rundownId: string | null) =>
useEntryActionsForRundown(rundownId ?? undefined)) so the loaded-rundown
fallback path is preserved.
In `@apps/client/src/features/rundown/useEventSelection.ts`:
- Around line 140-142: The selection lookup in useEventSelection is hard-coded
to PROJECT_RUNDOWNS.loaded which can produce incorrect range selections for
background/secondary rundowns; instead accept or read an active rundownId
supplied by CuesheetTable (e.g., pass/set an active rundownId in
useEventSelection from CuesheetTable) and use that ID when calling
getRundownQueryKey(rundownId) and when computing shift/ctrl range logic; replace
usages of getQueryData<ProjectRundownsList>(PROJECT_RUNDOWNS)?.loaded with the
active rundownId parameter/state and ensure all selection computation in
useEventSelection uses that ID.
In
`@apps/client/src/views/cuesheet/__tests__/useCuesheetRundownSelection.test.ts`:
- Around line 10-16: Update the tests to match the actual
resolveSelectedRundownId signature: call
resolveSelectedRundownId(storedSelectedRundownId, availableRundownIds) (i.e.,
pass the Set as the second argument, not the loaded id) and change expectations
so the missing/invalid stored selection expects the FOLLOW_LOADED_RUNDOWN_ID
constant (use FOLLOW_LOADED_RUNDOWN_ID in assertions) instead of the string
'loaded'; keep resolveSelectedRundownId and FOLLOW_LOADED_RUNDOWN_ID as the
referenced symbols when changing the test cases.
In
`@apps/client/src/views/cuesheet/cuesheet-table/cuesheet-table-settings/CuesheetTableHeaderToolbar.tsx`:
- Line 70: The data-background-rundown attribute on Toolbar.Root is currently
set with !modeControls?.isCurrentRundown which evaluates to true when
modeControls is undefined; change the logic so the attribute is true only when
modeControls exists and isCurrentRundown is false (e.g., use modeControls ?
!modeControls.isCurrentRundown : false or Boolean(modeControls &&
!modeControls.isCurrentRundown)); update the Toolbar.Root prop (in
CuesheetTableHeaderToolbar.tsx) referencing modeControls and isCurrentRundown to
implement this explicit check so editor context doesn't show BACKGROUND EDIT
styling.
In
`@apps/client/src/views/cuesheet/cuesheet-table/cuesheet-table-settings/CuesheetTableSettings.module.scss`:
- Around line 11-12: Remove the unexpected blank line immediately preceding the
transition declaration in CuesheetTableSettings.module.scss so the rule reads
without an empty line before "transition: background-color 0.2s ease-in-out;";
edit the block containing the transition property to eliminate the extra newline
so Stylelint no longer flags the empty line before the transition declaration.
In `@apps/client/src/views/cuesheet/useCuesheetRundownSelection.ts`:
- Around line 15-20: The function resolveSelectedRundownId is assuming
availableRundownIds is a Set which causes a TypeError in tests that pass arrays
or other iterables; update the function to accept a wider type (e.g.,
Set<string> | string[] | Iterable<string>) or normalize the input at the start
(convert availableRundownIds to a Set) before calling .has, then use
storedSelectedRundownId and FOLLOW_LOADED_RUNDOWN_ID as before; ensure the
normalization happens inside resolveSelectedRundownId so callers/tests need no
change.
In `@apps/server/src/api-data/rundown/rundown.dao.ts`:
- Around line 619-634: getProcessedRundown currently calls
getDataProvider().getRundown(rundownId) and immediately dereferences stored
(id/title/revision) which will throw if the rundown is missing; add an explicit
null/undefined check after calling getRundown and before calling processRundown,
and if missing throw a clear error (e.g. throw new Error(`Rundown not found:
${rundownId}`)) or return an appropriate error result so callers get a helpful
message; update getProcessedRundown to only call processRundown(stored, ...) and
access stored.id/title/revision after the existence check.
---
Nitpick comments:
In `@apps/client/src/views/cuesheet/cuesheet-table/CuesheetTable.tsx`:
- Around line 228-232: The loading check uses a redundant !flatRundown
condition; since RundownSource.flatRundown is typed as ExtendedEntry[] (always
an array), simplify isLoading to only check status === 'pending' by updating the
isLoading assignment (referencing isLoading, flatRundown, and status) and keep
the existing early return to EmptyPage unchanged.
In `@apps/client/src/views/cuesheet/CuesheetTableWrapper.tsx`:
- Around line 58-68: The JSX fragment wrapping the single RundownSelect is
redundant; in CuesheetTableWrapper replace the insertElement prop value that
currently uses <>...</> with a direct JSX element (set
insertElement={<RundownSelect ... />}), keeping all props (cuesheetMode,
selectedRundownId, loadedRundownId, setSelectedRundownId, projectRundowns)
unchanged and leaving RundownSelect as the sole child.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2526da59-bdf9-432d-8521-2af08435e251
⛔ Files ignored due to path filters (1)
packages/types/src/api/websocket/refetch.type.tsis excluded by none and included by none
📒 Files selected for processing (32)
apps/client/src/common/api/rundown.tsapps/client/src/common/hooks-query/useProjectRundowns.tsapps/client/src/common/hooks-query/useRundown.tsapps/client/src/common/hooks-query/useScopedRundown.tsapps/client/src/common/hooks/useEntryAction.tsapps/client/src/common/utils/socket.tsapps/client/src/features/app-settings/panel/manage-panel/ManageRundowns.tsxapps/client/src/features/rundown/RundownExport.tsxapps/client/src/features/rundown/entry-editor/CuesheetEventEditor.tsxapps/client/src/features/rundown/rundown-table/EditorTableSettings.tsxapps/client/src/features/rundown/rundown-table/RundownTable.tsxapps/client/src/features/rundown/useEventSelection.tsapps/client/src/views/cuesheet/CuesheetPage.module.scssapps/client/src/views/cuesheet/CuesheetPage.tsxapps/client/src/views/cuesheet/CuesheetTableWrapper.tsxapps/client/src/views/cuesheet/__tests__/useApplyCuesheetPolicy.test.tsapps/client/src/views/cuesheet/__tests__/useCuesheetRundownSelection.test.tsapps/client/src/views/cuesheet/cuesheet-edit-modal/EntryEditModal.tsxapps/client/src/views/cuesheet/cuesheet-table/CuesheetTable.tsxapps/client/src/views/cuesheet/cuesheet-table/cuesheet-table-elements/EditableImage.tsxapps/client/src/views/cuesheet/cuesheet-table/cuesheet-table-settings/CuesheetTableHeaderToolbar.tsxapps/client/src/views/cuesheet/cuesheet-table/cuesheet-table-settings/CuesheetTableSettings.module.scssapps/client/src/views/cuesheet/useApplyCuesheetPolicy.tsapps/client/src/views/cuesheet/useCuesheetRundownSelection.tsapps/server/src/api-data/rundown/__mocks__/rundown.mocks.tsapps/server/src/api-data/rundown/__tests__/rundown.dao.test.tsapps/server/src/api-data/rundown/rundown.dao.tsapps/server/src/api-data/rundown/rundown.parser.tsapps/server/src/api-data/rundown/rundown.router.tsapps/server/src/api-data/rundown/rundown.service.tsapps/server/src/api-data/rundown/rundown.validation.tsapps/server/src/api-integration/integration.controller.ts
💤 Files with no reviewable changes (2)
- apps/server/src/api-data/rundown/rundown.validation.ts
- apps/client/src/features/rundown/rundown-table/EditorTableSettings.tsx
Comment on lines
+73
to
+75
| export const useScopedEntryActions = (rundownId: string | null) => useEntryActionsForRundown(rundownId ?? ''); | ||
|
|
||
| function useEntryActionsForRundown(scopedRundownId: string | undefined) { |
Contributor
There was a problem hiding this comment.
Preserve fallback semantics for missing scoped rundown ID.
useScopedEntryActions currently converts null to '', which forces a scoped query key for an empty ID instead of using the loaded-rundown fallback path in useEntryActionsForRundown.
Suggested fix
-export const useScopedEntryActions = (rundownId: string | null) => useEntryActionsForRundown(rundownId ?? '');
+export const useScopedEntryActions = (rundownId: string | null) =>
+ useEntryActionsForRundown(rundownId ?? undefined);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export const useScopedEntryActions = (rundownId: string | null) => useEntryActionsForRundown(rundownId ?? ''); | |
| function useEntryActionsForRundown(scopedRundownId: string | undefined) { | |
| export const useScopedEntryActions = (rundownId: string | null) => | |
| useEntryActionsForRundown(rundownId ?? undefined); | |
| function useEntryActionsForRundown(scopedRundownId: string | undefined) { |
🤖 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 `@apps/client/src/common/hooks/useEntryAction.ts` around lines 73 - 75, The
helper useScopedEntryActions currently maps a null rundownId to an empty string
which creates an explicit scoped key and bypasses the fallback path in
useEntryActionsForRundown; update useScopedEntryActions so it forwards undefined
(e.g. pass rundownId ?? undefined or conditional if null) instead of '' and keep
the parameter types compatible with useEntryActionsForRundown
(useScopedEntryActions(rundownId: string | null) =>
useEntryActionsForRundown(rundownId ?? undefined)) so the loaded-rundown
fallback path is preserved.
Comment on lines
+140
to
+142
| const rundownId = ontimeQueryClient.getQueryData<ProjectRundownsList>(PROJECT_RUNDOWNS)?.loaded; | ||
| if (!rundownId) return undefined; | ||
| return ontimeQueryClient.getQueryData<Rundown>(getRundownQueryKey(rundownId)); |
Contributor
There was a problem hiding this comment.
Selection range logic is pinned to the loaded rundown, which can mis-select in background tables.
Line 140 hard-codes selection data to PROJECT_RUNDOWNS.loaded. But CuesheetTable is now source-driven, so shift/ctrl range logic can compute against a different rundown than the visible one when editing/viewing a secondary rundown.
Please scope selection lookup to the active table rundown (e.g., store/set active rundownId in useEventSelection from CuesheetTable and use that ID for getRundownQueryKey(...)).
🤖 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 `@apps/client/src/features/rundown/useEventSelection.ts` around lines 140 -
142, The selection lookup in useEventSelection is hard-coded to
PROJECT_RUNDOWNS.loaded which can produce incorrect range selections for
background/secondary rundowns; instead accept or read an active rundownId
supplied by CuesheetTable (e.g., pass/set an active rundownId in
useEventSelection from CuesheetTable) and use that ID when calling
getRundownQueryKey(rundownId) and when computing shift/ctrl range logic; replace
usages of getQueryData<ProjectRundownsList>(PROJECT_RUNDOWNS)?.loaded with the
active rundownId parameter/state and ensure all selection computation in
useEventSelection uses that ID.
| return ( | ||
| <Toolbar.Root className={style.tableSettings}> | ||
| <ViewSettings optionsStore={options} /> | ||
| <Toolbar.Root className={style.tableSettings} data-background-rundown={!modeControls?.isCurrentRundown}> |
Contributor
There was a problem hiding this comment.
Verify data-background-rundown attribute logic when modeControls is undefined.
When modeControls is undefined (editor context), !modeControls?.isCurrentRundown evaluates to !undefined → true, causing the toolbar to display the "BACKGROUND EDIT" styling. This seems unintended for the editor view.
🐛 Suggested fix
- <Toolbar.Root className={style.tableSettings} data-background-rundown={!modeControls?.isCurrentRundown}>
+ <Toolbar.Root className={style.tableSettings} data-background-rundown={modeControls && !modeControls.isCurrentRundown}>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <Toolbar.Root className={style.tableSettings} data-background-rundown={!modeControls?.isCurrentRundown}> | |
| <Toolbar.Root className={style.tableSettings} data-background-rundown={modeControls && !modeControls.isCurrentRundown}> |
🤖 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
`@apps/client/src/views/cuesheet/cuesheet-table/cuesheet-table-settings/CuesheetTableHeaderToolbar.tsx`
at line 70, The data-background-rundown attribute on Toolbar.Root is currently
set with !modeControls?.isCurrentRundown which evaluates to true when
modeControls is undefined; change the logic so the attribute is true only when
modeControls exists and isCurrentRundown is false (e.g., use modeControls ?
!modeControls.isCurrentRundown : false or Boolean(modeControls &&
!modeControls.isCurrentRundown)); update the Toolbar.Root prop (in
CuesheetTableHeaderToolbar.tsx) referencing modeControls and isCurrentRundown to
implement this explicit check so editor context doesn't show BACKGROUND EDIT
styling.
c9d4eb0 to
86d3449
Compare
ui: fix disable radio button
alex-Arc
force-pushed
the
cuesheet-rundown
branch
from
86d3449 to
9d63976
Compare
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
2 participants
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.
No description provided.