Open
Conversation
- Add nullable notes String to Prisma Block model and migration - Add notes to VideoBlock in api-journeys federation schema - Add notes to VideoBlockUpdateInput and VideoBlock type in api-journeys-modern - Add unit tests for notes read, update, and clear - Regenerate GraphQL schemas and api-journeys client Made-with: Cursor
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
WalkthroughAdds an optional Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant APIGateway as API_Gateway
participant JourneysAPI as Journeys_Service
participant DB as Database
Client->>APIGateway: videoBlockUpdate(input: { id, notes })
APIGateway->>JourneysAPI: Forward mutation (includes notes)
JourneysAPI->>DB: UPDATE Block SET notes = <value> WHERE id = <id>
DB-->>JourneysAPI: OK / updated row
JourneysAPI-->>APIGateway: Return updated block { id, notes, ... }
APIGateway-->>Client: Return updated block with notes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ 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 |
|
View your CI Pipeline Execution ↗ for commit 8f0bda9
☁️ Nx Cloud last updated this comment at |
Contributor
|
The latest updates on your projects.
|
Contributor
|
The latest updates on your projects.
|
Contributor
|
The latest updates on your projects.
|
Contributor
|
The latest updates on your projects.
|
Contributor
|
The latest updates on your projects.
|
Contributor
|
The latest updates on your projects.
|
Contributor
|
The latest updates on your projects.
|
![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)
apis/api-journeys-modern/schema.graphql (1)
2471-2471: Consider create/update symmetry fornotes.If clients need to set notes on initial block creation, adding
notestoVideoBlockCreateInputwould avoid a follow-up update mutation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apis/api-journeys-modern/schema.graphql` at line 2471, The schema currently exposes notes on the VideoBlock type but not in the creation input; add the notes field to VideoBlockCreateInput so clients can set notes on initial creation (mirroring VideoBlockUpdateInput/notes) and ensure the server resolver/validation that handles VideoBlockCreateInput accepts and persists notes in the same way as updates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apis/api-journeys-modern/src/schema/block/video/videoBlockUpdate.mutation.spec.ts`:
- Around line 200-235: Decide whether clearing notes should persist as an empty
string or as null, then update the test and mocks to match that contract: if the
semantic is "unset", change the mocked transaction return
(tx.block.update.mockResolvedValue) to have notes: null and update the assertion
expect(...videoBlockUpdate.notes).toBeNull() and the
expect(tx.block.update).toHaveBeenCalledWith(...) data expectation to
expect.objectContaining({ notes: null }); if the semantic is to store an empty
string, ensure the resolver/service normalizes input to '' (or leave as-is) and
keep the mocked return and assertions as notes: '' and expect(...).toBe('') so
the test, the tx.mockResolvedValue, and the called-with data are consistent with
the chosen behavior.
---
Nitpick comments:
In `@apis/api-journeys-modern/schema.graphql`:
- Line 2471: The schema currently exposes notes on the VideoBlock type but not
in the creation input; add the notes field to VideoBlockCreateInput so clients
can set notes on initial creation (mirroring VideoBlockUpdateInput/notes) and
ensure the server resolver/validation that handles VideoBlockCreateInput accepts
and persists notes in the same way as updates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c045d544-6765-4805-873d-42d0bdd0b8e1
⛔ Files ignored due to path filters (1)
apis/api-journeys/src/app/__generated__/graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (8)
apis/api-gateway/schema.graphqlapis/api-journeys-modern/schema.graphqlapis/api-journeys-modern/src/schema/block/video/inputs/videoBlockUpdateInput.tsapis/api-journeys-modern/src/schema/block/video/video.tsapis/api-journeys-modern/src/schema/block/video/videoBlockUpdate.mutation.spec.tsapis/api-journeys/src/app/modules/block/video/video.graphqllibs/prisma/journeys/db/migrations/20260305200417_add_video_block_notes/migration.sqllibs/prisma/journeys/db/schema.prisma
apis/api-journeys-modern/src/schema/block/video/videoBlockUpdate.mutation.spec.ts
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apis/api-journeys-modern/src/schema/block/video/videoBlockUpdate.mutation.spec.ts (1)
192-194: Extract a shared response type instead of repeating inline assertions.The same inline cast appears twice; a local type alias will reduce duplication and improve readability.
♻️ Suggested refactor
+ type VideoBlockUpdateResult = { + data?: { videoBlockUpdate: { notes: string | null } } + } + it('updates and returns notes when notes is set', async () => { @@ - expect( - (result as { data?: { videoBlockUpdate: { notes: string } } }).data - ?.videoBlockUpdate.notes - ).toBe('test trailer note') + expect((result as VideoBlockUpdateResult).data?.videoBlockUpdate.notes).toBe( + 'test trailer note' + ) @@ it('clears notes when notes is set to empty string', async () => { @@ - expect( - (result as { data?: { videoBlockUpdate: { notes: string } } }).data - ?.videoBlockUpdate.notes - ).toBe('') + expect((result as VideoBlockUpdateResult).data?.videoBlockUpdate.notes).toBe( + '' + )As per coding guidelines, "Define a type if possible."
Also applies to: 232-234
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apis/api-journeys-modern/src/schema/block/video/videoBlockUpdate.mutation.spec.ts` around lines 192 - 194, Extract a shared type alias for the mutation response and reuse it instead of repeating the inline cast; e.g. declare a type like VideoBlockUpdateResponse = { data?: { videoBlockUpdate: { notes: string } } } near the top of the spec and replace both occurrences of (result as { data?: { videoBlockUpdate: { notes: string } } }) with (result as VideoBlockUpdateResponse) so assertions that access .data?.videoBlockUpdate.notes for the videoBlockUpdate mutation use the shared alias.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apis/api-journeys-modern/src/schema/block/video/videoBlockUpdate.mutation.spec.ts`:
- Around line 192-194: Extract a shared type alias for the mutation response and
reuse it instead of repeating the inline cast; e.g. declare a type like
VideoBlockUpdateResponse = { data?: { videoBlockUpdate: { notes: string } } }
near the top of the spec and replace both occurrences of (result as { data?: {
videoBlockUpdate: { notes: string } } }) with (result as
VideoBlockUpdateResponse) so assertions that access
.data?.videoBlockUpdate.notes for the videoBlockUpdate mutation use the shared
alias.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9aace2dc-38a1-4670-a12d-92135fbc127a
⛔ Files ignored due to path filters (2)
apps/journeys-admin/__generated__/globalTypes.tsis excluded by!**/__generated__/**libs/shared/gql/src/__generated__/graphql-env.d.tsis excluded by!**/__generated__/**
📒 Files selected for processing (1)
apis/api-journeys-modern/src/schema/block/video/videoBlockUpdate.mutation.spec.ts
- Add notes to VideoBlockCreateInput for create/update symmetry - Extract VideoBlockUpdateResult type in spec to reduce duplication Made-with: Cursor
Contributor
Author
Review feedback addressed (87d0ab1)Fixed:
Not changed:
|
|
I see you added the "on stage" label, I'll get this merged to the stage branch! |
1 similar comment
|
I see you added the "on stage" label, I'll get this merged to the stage branch! |
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.
Summary
Adds a nullable
notesString field to VideoBlock for the template customization flow so publishers can describe the purpose of each video (e.g. "trailer", "intro") for template adapters.Changes
notes String?on Block model + migration (20260305200417_add_video_block_notes)notes: String @shareableon VideoBlock in federation schemanoteson VideoBlockUpdateInput and VideoBlock Pothos type; unit tests for read/update/clearQA
notesexists, returnsnullby defaultvideoBlockUpdatewithnotes: "test trailer note"→ re-query confirms valuevideoBlockUpdatewithnotes: ""→ value clearsCloses NES-1316
Made with Cursor
Summary by CodeRabbit
New Features
Tests
Chores