fix: send refetch on all rundown edits#2098
Conversation
|
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 refactors rundown management by moving duplication logic to the service layer and introducing two new operations ( ChangesRundown Management Service Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
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 |
alex-Arc
force-pushed
the
pre-switch-refactor
branch
3 times, most recently
from
79797bb to
eb37924
Compare
alex-Arc
force-pushed
the
pre-switch-refactor
branch
from
eb37924 to
4d079bd
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apps/server/src/api-data/rundown/rundown.router.ts (1)
123-135: 💤 Low valueMinor: Status code 201 is unconventional for PATCH.
Line 129 returns status 201 (Created) for a PATCH operation that updates an existing rundown's title. HTTP status 200 (OK) or 204 (No Content) would be more semantically appropriate for updates. While not incorrect, 201 is typically reserved for operations that create new resources.
♻️ Optional fix to use conventional status code
- res.status(201).json({ loaded: getCurrentRundown().id, rundowns: normalisedToRundownArray(projectRundowns) }); + res.status(200).json({ loaded: getCurrentRundown().id, rundowns: normalisedToRundownArray(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/server/src/api-data/rundown/rundown.router.ts` around lines 123 - 135, The PATCH handler currently returns HTTP 201; change the response to a conventional update status: replace res.status(201).json(...) in the router.patch async callback with res.status(200).json(...) (or res.status(204).send() if you prefer no body) so renameRundown and the handler return a proper success code for updates; keep the existing body (loaded and rundowns) when using 200.apps/server/src/api-data/rundown/rundown.service.ts (2)
738-740: ⚖️ Poor tradeoffMinor: Redundant refetch when renaming the loaded rundown.
When renaming the currently loaded rundown,
initRundown(line 735) already schedulessendRefetch(RefetchKey.ProjectRundowns)at line 675, making the explicit call here at line 739 redundant. While harmless (refetch is idempotent), this could be optimized by conditionally scheduling the refetch only when not re-initializing.🤖 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/server/src/api-data/rundown/rundown.service.ts` around lines 738 - 740, The setImmediate call that always does sendRefetch(RefetchKey.ProjectRundowns) is redundant when initRundown already scheduled the same refetch; update the code around initRundown and the setImmediate block so you only schedule the extra refetch when not re-initializing the currently loaded rundown — e.g., detect the re-initialization/rename case (use the same flag or condition used by initRundown) and wrap the setImmediate(sendRefetch(RefetchKey.ProjectRundowns)) call in that conditional so it is skipped during initRundown-driven re-initialization.
732-735: 💤 Low valueMinor: Variable shadowing reduces clarity.
At line 733,
rundownis re-declared, shadowing the variable from line 725. While this works correctly (the second declaration fetches the persisted updated rundown), consider using a distinct name likeupdatedRundownto improve readability.♻️ Optional refactor for clarity
if (isCurrentRundown(id)) { - const rundown = dataProvider.getRundown(id); + const updatedRundown = dataProvider.getRundown(id); const customField = dataProvider.getCustomFields(); - await initRundown(rundown, customField); + await initRundown(updatedRundown, customField); }🤖 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/server/src/api-data/rundown/rundown.service.ts` around lines 732 - 735, The variable declared at the second retrieval shadows the earlier "rundown"; rename the second variable to a distinct name (e.g., updatedRundown) so the code reads: const updatedRundown = dataProvider.getRundown(id); const customField = dataProvider.getCustomFields(); await initRundown(updatedRundown, customField); keep the surrounding logic (isCurrentRundown check and use of dataProvider.getCustomFields and initRundown) unchanged.
🤖 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 `@apps/server/src/api-data/rundown/rundown.router.ts`:
- Around line 123-135: The PATCH handler currently returns HTTP 201; change the
response to a conventional update status: replace res.status(201).json(...) in
the router.patch async callback with res.status(200).json(...) (or
res.status(204).send() if you prefer no body) so renameRundown and the handler
return a proper success code for updates; keep the existing body (loaded and
rundowns) when using 200.
In `@apps/server/src/api-data/rundown/rundown.service.ts`:
- Around line 738-740: The setImmediate call that always does
sendRefetch(RefetchKey.ProjectRundowns) is redundant when initRundown already
scheduled the same refetch; update the code around initRundown and the
setImmediate block so you only schedule the extra refetch when not
re-initializing the currently loaded rundown — e.g., detect the
re-initialization/rename case (use the same flag or condition used by
initRundown) and wrap the setImmediate(sendRefetch(RefetchKey.ProjectRundowns))
call in that conditional so it is skipped during initRundown-driven
re-initialization.
- Around line 732-735: The variable declared at the second retrieval shadows the
earlier "rundown"; rename the second variable to a distinct name (e.g.,
updatedRundown) so the code reads: const updatedRundown =
dataProvider.getRundown(id); const customField = dataProvider.getCustomFields();
await initRundown(updatedRundown, customField); keep the surrounding logic
(isCurrentRundown check and use of dataProvider.getCustomFields and initRundown)
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8f0b5014-f87e-43dd-99b1-38a81ec89555
⛔ Files ignored due to path filters (1)
e2e/tests/features/202-cuesheet.spec.tsis excluded by none and included by none
📒 Files selected for processing (6)
apps/server/src/api-data/rundown/__tests__/rundown.utils.test.tsapps/server/src/api-data/rundown/rundown.router.tsapps/server/src/api-data/rundown/rundown.service.tsapps/server/src/api-data/rundown/rundown.utils.tsapps/server/src/api-data/rundown/rundown.validation.tsapps/server/src/classes/data-provider/DataProvider.ts
💤 Files with no reviewable changes (2)
- apps/server/src/api-data/rundown/tests/rundown.utils.test.ts
- apps/server/src/api-data/rundown/rundown.utils.ts
alex-Arc
force-pushed
the
pre-switch-refactor
branch
from
56312d5 to
1f98635
Compare
| newRundown.title = `Copy of ${rundown.title}`; | ||
| newRundown.revision = 0; | ||
|
|
||
| const newProjectRundowns = await dataProvider.setRundown(newRundownId, newRundown); |
There was a problem hiding this comment.
question: would we benefit from a thin abstraction like addRundown
There was a problem hiding this comment.
in the dataProvider?
There was a problem hiding this comment.
I was thinking in the service. that would keep the logic of the service functions at a higher level
There was a problem hiding this comment.
hmm what would be it's responsibility?
we have makeNewRundown for blank ones
There was a problem hiding this comment.
It would be to clarify that we are adding a rundown to the project, doesn't matter if it is empty or not
Right now, all the methods need to know how to add a rundown to the projects
This could make maintenance a bit tricky, like if we ever want to kick off side effects we need to change many things
| initRundown(rundown, customField); | ||
| } else { | ||
| setImmediate(() => { | ||
| sendRefetch(RefetchKey.ProjectRundowns); |
There was a problem hiding this comment.
question: should we not refetch if the rundown is renamed?
There was a problem hiding this comment.
yes either it is done as part of initRundown
or we do it manually if it is a background rundown
Co-authored-by: Carlos Valente <34649812+cpvalente@users.noreply.github.com>
2 participants
ensures that all edits to rundown data result in a refetch event
this is done by refactoring all the routes to use the rundown.service instead of doing it inplace