chore: remove journey provider context wrapper from journey cards #8586
Conversation
Ur-imazing
added
effort: 2
priority: soon
type: chore
|
I see you added the "on stage" label, I'll get this merged to the stage branch! |
…-wrapping-for-individual-journey
WalkthroughRemoved per-item JourneyProvider wrappers; components now accept explicit Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant JourneyCard as JourneyCard
participant DefaultMenu as DefaultMenu
participant DuplicateItem as DuplicateJourneyMenuItem
participant CopyDialog as CopyToTeamDialog
participant API as GraphQL_API
User->>JourneyCard: open card menu
JourneyCard->>DefaultMenu: render menu (pass journey prop)
User->>DefaultMenu: click "Duplicate" / "Copy"
DefaultMenu->>DuplicateItem: invoke Duplicate action (journey forwarded)
DuplicateItem->>CopyDialog: open CopyToTeamDialog (journey prop)
CopyDialog->>API: duplicateJourney mutation (uses journey.id)
API-->>CopyDialog: mutation result
CopyDialog-->>User: show success / trigger navigation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 4ae5fe8
☁️ Nx Cloud last updated this comment at |
|
The latest updates on your projects.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
apps/journeys-admin/src/components/JourneyList/ArchivedJourneyList/ArchivedJourneyList.tsx (1)
151-155: Redundantkeyprop onJourneyCard.The
keyis already specified on the parentGridcomponent (line 148). ThekeyonJourneyCardis unnecessary since React only needs the key on the outermost element in the map iteration.Suggested fix
- <JourneyCard - key={journey.id} - journey={journey} - refetch={refetch} - /> + <JourneyCard journey={journey} refetch={refetch} />apps/journeys-admin/src/components/TemplateList/TrashedTemplateList/TrashedTemplateList.tsx (1)
153-157: Redundantkeyprop onJourneyCard.The
keyis already specified on the parentGridcomponent (line 150). Remove the duplicate key fromJourneyCard.Suggested fix
- <JourneyCard - key={journey.id} - journey={journey} - refetch={refetch} - /> + <JourneyCard journey={journey} refetch={refetch} />apps/journeys-admin/src/components/JourneyList/TrashedJourneyList/TrashedJourneyList.tsx (1)
163-167: Redundantkeyprop onJourneyCard.The
keyis already specified on the parentGridcomponent (line 160). Remove the duplicate key fromJourneyCard.Suggested fix
- <JourneyCard - key={journey.id} - journey={journey} - refetch={refetch} - /> + <JourneyCard journey={journey} refetch={refetch} />apps/journeys-admin/src/components/TemplateList/ActiveTemplateList/ActiveTemplateList.tsx (1)
139-143: Redundantkeyprop onJourneyCard.The
keyis already specified on the parentGridcomponent (line 136). Remove the duplicate key fromJourneyCard.Suggested fix
- <JourneyCard - key={journey.id} - journey={journey} - refetch={refetch} - /> + <JourneyCard journey={journey} refetch={refetch} />apps/journeys-admin/src/components/JourneyList/ActiveJourneyList/ActivePriorityList/ActivePriorityList.tsx (1)
88-93: LGTM with minor nit: redundantkeyprop.The variant handling is correct—casting
key as JourneyCardVariantworks sinceallActiveJourneysis keyed byJourneyCardVariantenum values. The direct rendering withoutJourneyProvidersimplifies the component hierarchy.Minor: The
keyonJourneyCard(line 89) is redundant sinceGrid(line 87) already haskey={journey.id}.Optional: Remove redundant key
<JourneyCard - key={journey.id} journey={journey} refetch={refetch} variant={key as JourneyCardVariant} />apps/journeys-admin/src/components/JourneyList/JourneyListContent/JourneyListContent.tsx (1)
586-590: Remove duplicatekeyprop.The
keyis already set on the parentGridelement (line 574). ThekeyonJourneyCardis redundant.Suggested fix
- <JourneyCard - key={journey.id} - journey={journey} - refetch={refetch} - /> + <JourneyCard journey={journey} refetch={refetch} />apps/journeys-admin/src/components/TemplateList/ArchivedTemplateList/ArchivedTemplateList.tsx (1)
140-144: Remove duplicatekeyprop.Same issue as
JourneyListContent.tsx— thekeyis already on the parentGrid(line 137).Suggested fix
- <JourneyCard - key={journey.id} - journey={journey} - refetch={refetch} - /> + <JourneyCard journey={journey} refetch={refetch} />
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
apps/journeys-admin/src/components/JourneyList/ActiveJourneyList/ActivePriorityList/ActivePriorityList.tsxapps/journeys-admin/src/components/JourneyList/ArchivedJourneyList/ArchivedJourneyList.tsxapps/journeys-admin/src/components/JourneyList/JourneyListContent/JourneyListContent.tsxapps/journeys-admin/src/components/JourneyList/TrashedJourneyList/TrashedJourneyList.tsxapps/journeys-admin/src/components/TemplateList/ActiveTemplateList/ActiveTemplateList.tsxapps/journeys-admin/src/components/TemplateList/ArchivedTemplateList/ArchivedTemplateList.tsxapps/journeys-admin/src/components/TemplateList/TrashedTemplateList/TrashedTemplateList.tsx
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/base.mdc)
**/*.{ts,tsx,js,jsx}: Use early returns whenever possible to make the code more readable.
Use descriptive variable and function/const names.
Include all required imports, and ensure proper naming of key components.
Files:
apps/journeys-admin/src/components/TemplateList/ArchivedTemplateList/ArchivedTemplateList.tsxapps/journeys-admin/src/components/TemplateList/TrashedTemplateList/TrashedTemplateList.tsxapps/journeys-admin/src/components/JourneyList/ActiveJourneyList/ActivePriorityList/ActivePriorityList.tsxapps/journeys-admin/src/components/JourneyList/TrashedJourneyList/TrashedJourneyList.tsxapps/journeys-admin/src/components/JourneyList/JourneyListContent/JourneyListContent.tsxapps/journeys-admin/src/components/JourneyList/ArchivedJourneyList/ArchivedJourneyList.tsxapps/journeys-admin/src/components/TemplateList/ActiveTemplateList/ActiveTemplateList.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/base.mdc)
Define a type if possible.
Files:
apps/journeys-admin/src/components/TemplateList/ArchivedTemplateList/ArchivedTemplateList.tsxapps/journeys-admin/src/components/TemplateList/TrashedTemplateList/TrashedTemplateList.tsxapps/journeys-admin/src/components/JourneyList/ActiveJourneyList/ActivePriorityList/ActivePriorityList.tsxapps/journeys-admin/src/components/JourneyList/TrashedJourneyList/TrashedJourneyList.tsxapps/journeys-admin/src/components/JourneyList/JourneyListContent/JourneyListContent.tsxapps/journeys-admin/src/components/JourneyList/ArchivedJourneyList/ArchivedJourneyList.tsxapps/journeys-admin/src/components/TemplateList/ActiveTemplateList/ActiveTemplateList.tsx
apps/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/apps.mdc)
apps/**/*.{js,jsx,ts,tsx}: Always use MUI over HTML elements; avoid using CSS or tags.
Use descriptive variable and function/const names. Also, event functions should be named with a “handle” prefix, like “handleClick” for onClick and “handleKeyDown” for onKeyDown.
Implement accessibility features on elements. For example, a tag should have a tabindex=“0”, aria-label, on:click, and on:keydown, and similar attributes.
Files:
apps/journeys-admin/src/components/TemplateList/ArchivedTemplateList/ArchivedTemplateList.tsxapps/journeys-admin/src/components/TemplateList/TrashedTemplateList/TrashedTemplateList.tsxapps/journeys-admin/src/components/JourneyList/ActiveJourneyList/ActivePriorityList/ActivePriorityList.tsxapps/journeys-admin/src/components/JourneyList/TrashedJourneyList/TrashedJourneyList.tsxapps/journeys-admin/src/components/JourneyList/JourneyListContent/JourneyListContent.tsxapps/journeys-admin/src/components/JourneyList/ArchivedJourneyList/ArchivedJourneyList.tsxapps/journeys-admin/src/components/TemplateList/ActiveTemplateList/ActiveTemplateList.tsx
🧠 Learnings (7)
📚 Learning: 2025-12-19T04:58:24.460Z
Learnt from: CR
Repo: JesusFilm/core PR: 0
File: apps/watch/AGENTS.md:0-0
Timestamp: 2025-12-19T04:58:24.460Z
Learning: Applies to apps/watch/src/**/*.{ts,tsx} : Preserve existing contracts in components like `VideoBlock` which depend on the full provider stack, video.js, and mux metadata so autoplay, subtitles, and analytics remain intact.
Applied to files:
apps/journeys-admin/src/components/TemplateList/ArchivedTemplateList/ArchivedTemplateList.tsxapps/journeys-admin/src/components/JourneyList/ActiveJourneyList/ActivePriorityList/ActivePriorityList.tsxapps/journeys-admin/src/components/JourneyList/JourneyListContent/JourneyListContent.tsxapps/journeys-admin/src/components/JourneyList/ArchivedJourneyList/ArchivedJourneyList.tsxapps/journeys-admin/src/components/TemplateList/ActiveTemplateList/ActiveTemplateList.tsx
📚 Learning: 2025-11-11T23:22:02.196Z
Learnt from: mikeallisonJS
Repo: JesusFilm/core PR: 8156
File: apis/api-journeys-modern/src/lib/google/googleAuth.ts:0-0
Timestamp: 2025-11-11T23:22:02.196Z
Learning: In apis/api-journeys-modern, use the validated `env` object from `../../env` instead of accessing `process.env` directly for environment variables that are defined in env.ts (e.g., GOOGLE_CLIENT_ID, GOOGLE_CLIENT_SECRET, INTEGRATION_ACCESS_KEY_ENCRYPTION_SECRET). This eliminates the need for runtime validation checks since Zod validates them at application startup.
Applied to files:
apps/journeys-admin/src/components/JourneyList/ActiveJourneyList/ActivePriorityList/ActivePriorityList.tsxapps/journeys-admin/src/components/JourneyList/JourneyListContent/JourneyListContent.tsx
📚 Learning: 2025-04-29T04:56:45.588Z
Learnt from: Ur-imazing
Repo: JesusFilm/core PR: 6382
File: apps/journeys-admin/src/components/JourneyList/ArchivedJourneyList/ArchivedJourneyList.spec.tsx:149-150
Timestamp: 2025-04-29T04:56:45.588Z
Learning: The trailing "00" characters in journey card text content assertions are intentional in the JourneyList component tests.
Applied to files:
apps/journeys-admin/src/components/JourneyList/JourneyListContent/JourneyListContent.tsx
📚 Learning: 2025-09-16T04:10:28.624Z
Learnt from: jaco-brink
Repo: JesusFilm/core PR: 7679
File: apis/api-gateway/schema.graphql:0-0
Timestamp: 2025-09-16T04:10:28.624Z
Learning: In PR #7679 (showAssistant field), the showAssistant field was intentionally excluded from JourneyUpdateInput because it will be controlled through direct database operations rather than through GraphQL mutations, keeping it out of the public API surface while still being queryable.
Applied to files:
apps/journeys-admin/src/components/JourneyList/JourneyListContent/JourneyListContent.tsx
📚 Learning: 2025-12-19T04:58:24.460Z
Learnt from: CR
Repo: JesusFilm/core PR: 0
File: apps/watch/AGENTS.md:0-0
Timestamp: 2025-12-19T04:58:24.460Z
Learning: Applies to apps/watch/src/**/*.tsx : Use optional props with sensible defaults, include className prop for extensibility, and use ReactNode for children and content slots. Name interfaces with component name prefix (e.g., `ComponentNameProps`).
Applied to files:
apps/journeys-admin/src/components/JourneyList/JourneyListContent/JourneyListContent.tsx
📚 Learning: 2025-09-08T22:56:21.606Z
Learnt from: mikeallisonJS
Repo: JesusFilm/core PR: 7498
File: apis/api-journeys-modern/src/schema/action/emailAction/blockUpdateEmailAction.mutation.ts:26-36
Timestamp: 2025-09-08T22:56:21.606Z
Learning: In the action mutations migration (PR #7498), the `journeyId` parameter is intentionally kept in blockUpdate*Action mutations even when unused in the resolver, to maintain API compatibility during the migration from the old "api-journeys" endpoint to the modern "api-journeys-modern" endpoint.
Applied to files:
apps/journeys-admin/src/components/JourneyList/JourneyListContent/JourneyListContent.tsx
📚 Learning: 2025-12-19T04:58:24.460Z
Learnt from: CR
Repo: JesusFilm/core PR: 0
File: apps/watch/AGENTS.md:0-0
Timestamp: 2025-12-19T04:58:24.460Z
Learning: Applies to apps/watch/src/**/*.{ts,tsx} : Do not introduce new MUI usage; migrate existing MUI to shadcn/Tailwind. New shadcn work should coexist with core shells/wrappers still using MUI until explicit migration tasks retire them.
Applied to files:
apps/journeys-admin/src/components/TemplateList/ActiveTemplateList/ActiveTemplateList.tsx
🧬 Code graph analysis (5)
apps/journeys-admin/src/components/TemplateList/ArchivedTemplateList/ArchivedTemplateList.tsx (1)
apps/journeys-admin/src/components/JourneyList/JourneyCard/JourneyCard.tsx (1)
JourneyCard(53-383)
apps/journeys-admin/src/components/TemplateList/TrashedTemplateList/TrashedTemplateList.tsx (1)
apps/journeys-admin/src/components/JourneyList/JourneyCard/JourneyCard.tsx (1)
JourneyCard(53-383)
apps/journeys-admin/src/components/JourneyList/ActiveJourneyList/ActivePriorityList/ActivePriorityList.tsx (2)
apps/journeys-admin/src/components/JourneyList/JourneyCard/JourneyCard.tsx (1)
JourneyCard(53-383)apps/journeys-admin/src/components/JourneyList/ActiveJourneyList/ActivePriorityList/ActiveJourneyListData.ts (1)
journey(93-97)
apps/journeys-admin/src/components/JourneyList/JourneyListContent/JourneyListContent.tsx (1)
apps/journeys-admin/src/components/JourneyList/JourneyCard/JourneyCard.tsx (1)
JourneyCard(53-383)
apps/journeys-admin/src/components/TemplateList/ActiveTemplateList/ActiveTemplateList.tsx (1)
apps/journeys-admin/src/components/JourneyList/JourneyCard/JourneyCard.tsx (1)
JourneyCard(53-383)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy Preview (journeys-admin, 8586/merge, pull_request, 22)
🔇 Additional comments (1)
apps/journeys-admin/src/components/JourneyList/JourneyListContent/JourneyListContent.tsx (1)
19-22: LGTM!Import cleanup is appropriate — removed unused
JourneyProvider,JourneyFields, and narrowed to only the required exports.
…-wrapping-for-individual-journey
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@apps/journeys-admin/src/components/JourneyList/ArchivedJourneyList/ArchivedJourneyList.tsx`:
- Around line 170-174: The JourneyCard in ArchivedJourneyList is given a
redundant key prop; remove the duplicate key={journey.id} from the JourneyCard
component (leave the key on the parent Grid item) so only the Grid retains the
unique key, updating the JSX where JourneyCard is rendered inside
ArchivedJourneyList/Grid.
apps/journeys-admin/src/components/JourneyList/ArchivedJourneyList/ArchivedJourneyList.tsx
Show resolved
Hide resolved
…-wrapping-for-individual-journey
github-actions
bot
requested a deployment
to
Preview - journeys-admin
…enuItem components
github-actions
bot
requested a deployment
to
Preview - journeys-admin
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@apps/journeys-admin/src/components/Editor/Toolbar/Items/CreateTemplateItem/CreateTemplateItem.tsx`:
- Around line 66-68: The code in CreateTemplateItem (teamId: globalPublish ?
'jfp-team' : (journeyData.team?.id ?? '')) can pass an empty string when
globalPublish is false and journeyData.team?.id is undefined; update the
CreateTemplateItem handler (the place that builds the mutation payload and calls
the create template mutation) to explicitly validate journeyData.team?.id when
globalPublish is false and either throw or surface a clear error/validation
message instead of sending an empty string, e.g., check if (!globalPublish &&
!journeyData.team?.id) then throw or return with a user-facing error before
calling the mutation so the mutation always receives a valid teamId.
In
`@apps/journeys-admin/src/components/JourneyList/JourneyCard/JourneyCardMenu/DuplicateJourneyMenuItem/DuplicateJourneyMenuItem.spec.tsx`:
- Around line 588-602: Remove the leftover debug console.log statements that
print dialogButtons and footerButtons in the DuplicateJourneyMenuItem.spec.tsx
test; specifically delete the console.log(...) calls that reference
dialogButtons.map(...) and footerButtons.map(...), leaving the surrounding test
logic intact (the queries using getByTestId('CopyToTeamDialog'), within(...) and
queryByTestId('dialog-actions')/getAllByRole('button')). Also remove the
identical debug console.log calls elsewhere in the same spec file where
dialogButtons/footerButtons are logged so no debug output remains.
🧹 Nitpick comments (1)
apps/journeys-admin/src/components/JourneyList/JourneyCard/JourneyCardMenu/DuplicateJourneyMenuItem/DuplicateJourneyMenuItem.spec.tsx (1)
170-256: Consider parameterized tests to reduce duplication.The "with journey from props" tests largely duplicate their context-based counterparts. You could use
describe.eachorit.eachto parameterize these test cases, reducing code duplication and making it easier to maintain both variants.♻️ Example approach using describe.each
describe.each([ { name: 'with JourneyProvider context', journey: undefined, wrapper: (children: React.ReactNode) => ( <JourneyProvider value={{ journey: contextJourney, variant: 'admin' }}> {children} </JourneyProvider> ), journeyId: 'journeyId', teamId: 'teamId' }, { name: 'with journey prop', journey: defaultJourney, wrapper: (children: React.ReactNode) => <>{children}</>, journeyId: 'journey-id', teamId: 'team1.id' } ])('$name', ({ journey, wrapper, journeyId, teamId }) => { it('should duplicate a journey on menu card click', async () => { // Use journeyId, teamId, and journey in mocks and render }) // ... other tests })Also applies to: 455-630, 717-780, 895-986
...journeys-admin/src/components/Editor/Toolbar/Items/CreateTemplateItem/CreateTemplateItem.tsx
Show resolved
Hide resolved
...yList/JourneyCard/JourneyCardMenu/DuplicateJourneyMenuItem/DuplicateJourneyMenuItem.spec.tsx
Outdated
Show resolved
Hide resolved
…or-individual-journey' of https://github.com/JesusFilm/core into urimchae/nes-316-substitute-journey-provider-wrapping-for-individual-journey
This comment has been minimized.
This comment has been minimized.
…-wrapping-for-individual-journey
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/journeys/ui/src/components/TemplateView/CreateJourneyButton/CreateJourneyButton.tsx (1)
313-313: Boolean null-check is always truthy.
openTeamDialogis a boolean state initialized withuseState(false). The conditionopenTeamDialog != nullis alwaystruefor booleans. This should simply beopenTeamDialogto conditionally render the dialog.Proposed fix
- {openTeamDialog != null && ( + {openTeamDialog && (
🤖 Fix all issues with AI agents
In
`@libs/journeys/ui/src/components/TemplateView/CreateJourneyButton/CreateJourneyButton.tsx`:
- Line 334: Remove the redundant double-cast on the journey prop: instead of
passing journey={journeyData as unknown as JourneyFields} pass
journey={journeyData} because journeyData is already typed as JourneyFields |
undefined and CopyToTeamDialog accepts an optional journey?: JourneyFields;
update the JSX where CopyToTeamDialog (or the component receiving the prop) is
called so it uses journey={journeyData} and remove the unnecessary casts to
improve clarity.
🧹 Nitpick comments (1)
apps/journeys-admin/src/components/JourneyList/JourneyCard/JourneyCardMenu/DuplicateJourneyMenuItem/DuplicateJourneyMenuItem.spec.tsx (1)
418-445: Fail fast when no primary dialog action is found.If none of the fallback paths locate a clickable action, the test silently proceeds and later times out. Consider throwing an explicit error to make failures clearer.
♻️ Suggested tweak
} else { // Last resort: click the last button that's not the close button const actionButtons = dialogButtons.filter( (btn) => !btn.getAttribute('data-testid')?.includes('close') ) if (actionButtons.length > 0) { fireEvent.click(actionButtons[actionButtons.length - 1]) + } else { + throw new Error( + 'Unable to locate a primary action button in CopyToTeamDialog' + ) } } }
| : undefined | ||
| } | ||
| isTranslating={translationVariables != null} | ||
| journey={journeyData as unknown as JourneyFields} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find CopyToTeamDialog component and check its props type definition
fd "CopyToTeamDialog" libs/journeys/ui/src --type f | head -20Repository: JesusFilm/core
Length of output: 280
🏁 Script executed:
#!/bin/bash
# Check the CreateJourneyButton file and find journeyData type and context
cat -n libs/journeys/ui/src/components/TemplateView/CreateJourneyButton/CreateJourneyButton.tsx | sed -n '1,50p'Repository: JesusFilm/core
Length of output: 2245
🏁 Script executed:
#!/bin/bash
# Look at the DynamicCopyToTeamDialog component (which wraps CopyToTeamDialog)
fd "DynamicCopyToTeamDialog" libs/journeys/ui/src --type fRepository: JesusFilm/core
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Get the CopyToTeamDialog component and check its props type
cat -n libs/journeys/ui/src/components/CopyToTeamDialog/CopyToTeamDialog.tsx | head -80Repository: JesusFilm/core
Length of output: 3890
🏁 Script executed:
#!/bin/bash
# Look for journeyData variable definition in CreateJourneyButton around line 334
cat -n libs/journeys/ui/src/components/TemplateView/CreateJourneyButton/CreateJourneyButton.tsx | sed -n '300,360p'Repository: JesusFilm/core
Length of output: 1486
🏁 Script executed:
#!/bin/bash
# Look for where journeyData comes from in the file
rg "journeyData" libs/journeys/ui/src/components/TemplateView/CreateJourneyButton/CreateJourneyButton.tsx -n -B 2 -A 2Repository: JesusFilm/core
Length of output: 2374
🏁 Script executed:
#!/bin/bash
# Check useJourney hook return type to confirm journeyFromContext typing
fd "JourneyProvider" libs/journeys/ui/src/libs --type dRepository: JesusFilm/core
Length of output: 100
🏁 Script executed:
#!/bin/bash
# Search for useJourney hook definition and return type
rg "export.*useJourney|return.*journey" libs/journeys/ui/src/libs/JourneyProvider -n -A 5 | head -40Repository: JesusFilm/core
Length of output: 1300
🏁 Script executed:
#!/bin/bash
# Get the JourneyProviderContext type definition
cat -n libs/journeys/ui/src/libs/JourneyProvider/JourneyProvider.tsx | head -50Repository: JesusFilm/core
Length of output: 1106
Remove unnecessary type cast that adds no value.
The cast journeyData as unknown as JourneyFields is redundant. Since journeyData is JourneyFields | undefined and CopyToTeamDialog accepts journey?: JourneyFields (an optional prop), TypeScript allows passing the value without any cast. Remove it to improve code clarity:
- journey={journeyData as unknown as JourneyFields}
+ journey={journeyData}🤖 Prompt for AI Agents
In
`@libs/journeys/ui/src/components/TemplateView/CreateJourneyButton/CreateJourneyButton.tsx`
at line 334, Remove the redundant double-cast on the journey prop: instead of
passing journey={journeyData as unknown as JourneyFields} pass
journey={journeyData} because journeyData is already typed as JourneyFields |
undefined and CopyToTeamDialog accepts an optional journey?: JourneyFields;
update the JSX where CopyToTeamDialog (or the component receiving the prop) is
called so it uses journey={journeyData} and remove the unnecessary casts to
improve clarity.
There was a problem hiding this comment.
remove the redundant typecast
There was a problem hiding this comment.
@csiyang, great! Thank you for confirming. The double cast pattern as unknown as is typically used as an escape hatch when TypeScript prevents direct conversion between unrelated types, but in this case it's unnecessary since journeyData (typed as JourneyFields | undefined) is already compatible with the optional prop journey?: JourneyFields.
🧠 Learnings used
Learnt from: mikeallisonJS
Repo: JesusFilm/core PR: 7629
File: apis/api-journeys-modern/src/schema/event/utils.ts:43-60
Timestamp: 2025-09-29T23:03:36.840Z
Learning: In the JesusFilm/core repository, do not recommend using Prisma's `upsert` operation for `JourneyVisitor` creation in `apis/api-journeys-modern/src/schema/event/utils.ts` as it is not race condition safe for this use case. The current `findUnique` then `create` pattern is the preferred approach.
Learnt from: mikeallisonJS
Repo: JesusFilm/core PR: 7486
File: apis/api-journeys-modern/schema.graphql:1068-1074
Timestamp: 2025-08-20T21:51:25.797Z
Learning: Before suggesting type changes during API migrations, always verify the existing implementation to ensure consistency is maintained. The migration to api-journeys-modern should preserve the same field types as the original api-journeys schema.
Learnt from: mikeallisonJS
Repo: JesusFilm/core PR: 7486
File: apis/api-journeys-modern/schema.graphql:199-205
Timestamp: 2025-08-20T23:32:18.117Z
Learning: The ChatButton.link field in api-journeys is actually nullable (String), not non-nullable (String!) as previously learned. When migrating GraphQL types from api-journeys to api-journeys-modern, the ChatButton.link field should remain nullable to match the existing api-journeys schema.
Learnt from: mikeallisonJS
Repo: JesusFilm/core PR: 8156
File: apis/api-journeys-modern/src/lib/google/googleAuth.ts:0-0
Timestamp: 2025-11-11T23:22:02.196Z
Learning: In apis/api-journeys-modern, use the validated `env` object from `../../env` instead of accessing `process.env` directly for environment variables that are defined in env.ts (e.g., GOOGLE_CLIENT_ID, GOOGLE_CLIENT_SECRET, INTEGRATION_ACCESS_KEY_ENCRYPTION_SECRET). This eliminates the need for runtime validation checks since Zod validates them at application startup.
Learnt from: mikeallisonJS
Repo: JesusFilm/core PR: 7486
File: apis/api-journeys-modern/schema.graphql:1068-1074
Timestamp: 2025-08-20T21:51:25.797Z
Learning: During the refactor to migrate types to modern, maintain consistency with existing api-journeys schema types, including PlausibleStatsAggregateValue.change as Int to match the original implementation.
Learnt from: jaco-brink
Repo: JesusFilm/core PR: 7679
File: apis/api-gateway/schema.graphql:0-0
Timestamp: 2025-09-16T04:10:28.624Z
Learning: In PR `#7679` (showAssistant field), the showAssistant field was intentionally excluded from JourneyUpdateInput because it will be controlled through direct database operations rather than through GraphQL mutations, keeping it out of the public API surface while still being queryable.
Learnt from: mikeallisonJS
Repo: JesusFilm/core PR: 7498
File: apis/api-journeys-modern/src/schema/action/emailAction/blockUpdateEmailAction.mutation.ts:26-36
Timestamp: 2025-09-08T22:56:21.606Z
Learning: In the action mutations migration (PR `#7498`), the `journeyId` parameter is intentionally kept in blockUpdate*Action mutations even when unused in the resolver, to maintain API compatibility during the migration from the old "api-journeys" endpoint to the modern "api-journeys-modern" endpoint.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| const result = jest.fn(() => { | ||
| return { | ||
| data: { | ||
| journeyTemplate: { | ||
| id: 'templateId', | ||
| template: true | ||
| } | ||
| } | ||
| } | ||
| }) |
There was a problem hiding this comment.
Just a nit, but you don't really need a return statement here
| const result = jest.fn(() => { | |
| return { | |
| data: { | |
| journeyTemplate: { | |
| id: 'templateId', | |
| template: true | |
| } | |
| } | |
| } | |
| }) | |
| const result = jest.fn(() => ({ | |
| data: { | |
| journeyTemplate: { | |
| id: 'templateId', | |
| template: true | |
| } | |
| } | |
| })) |
| <TemplateActionButton | ||
| variant="menu-item" | ||
| handleCloseMenu={handleCloseMenu} | ||
| journey={journey as unknown as JourneyFields} |
There was a problem hiding this comment.
This typecast should be unnecessary. Even if it was, its not good practice to type it as unknown since this makes us loose all the proper type checks for and child components.
| journey={journey as unknown as JourneyFields} | |
| journey={journey} |
| setOpen(false) | ||
| }} | ||
| submitAction={handleDuplicateJourney} | ||
| journey={journey as unknown as JourneyFields} |
There was a problem hiding this comment.
I think this will cause a type failure. There's often a reason why types mismatch, we shouldn't just ignore it. You should try find out why there are different types here and make, and either update types to use the same one, or just use a subset of the type you need.
I know we already using this typecast, I don't remember why it was done like this, but this is a good opportunity to clean it up if possible.
Suggested change
journey={journey as unknown as JourneyFields}
journey={journey}
| expect(getByText('Journey Duplicated')).toBeInTheDocument() | ||
| }) | ||
|
|
||
| it('should call refetchTemplateStats when duplicating a journey with fromTemplateId with journey from props', async () => { |
There was a problem hiding this comment.
This test is not needed, you haven't made any changes to how refetch is running.
| ) | ||
| }) | ||
|
|
||
| it('should close copy to dialog with journey from props', async () => { |
There was a problem hiding this comment.
Not needed, the onClose function has not been changed
| } | ||
| }, | ||
| result: jest.fn(() => ({ | ||
| describe.each([ |
There was a problem hiding this comment.
This is a good idea, but just a little hard read/ understand.
| : undefined | ||
| } | ||
| isTranslating={translationVariables != null} | ||
| journey={journeyData as unknown as JourneyFields} |
There was a problem hiding this comment.
remove the redundant typecast
| variant={variant} | ||
| signedIn={signedIn} | ||
| openTeamDialogOnSignIn={openTeamDialogOnSignIn} | ||
| journey={journeyData as unknown as JourneyFields} |
There was a problem hiding this comment.
| journey={journeyData as unknown as JourneyFields} | |
| journey={journeyData} |
| <UseThisTemplateButton | ||
| variant={variant} | ||
| signedIn={signedIn} | ||
| journey={journeyData as unknown as JourneyFields} |
There was a problem hiding this comment.
| journey={journeyData as unknown as JourneyFields} | |
| journey={journeyData} |
| describe.each([ | ||
| ['context journey', undefined], | ||
| ['prop journey', { id: 'journeyId' } as unknown as JourneyFields] | ||
| ])('(%s)', (_, defaultJourney) => { |
There was a problem hiding this comment.
Update these so when tests run the titles match the pattern of how our current tests are structured. Double check this for other places you've used this
| ])('(%s)', (_, defaultJourney) => { | |
| ])((%s', (_, defaultJourney) => { |
Summary by CodeRabbit
Refactor
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.