feat: add journey list view toggle#9159
Open
edmonday wants to merge 2 commits into
Open
Conversation
Contributor
WalkthroughThis PR adds a grid/list view toggle to the journeys list interface with URL-persisted display mode. A new ChangesGrid/List View Display Toggle
Sequence DiagramsequenceDiagram
participant User as User
participant JLV as JourneyListView
participant DC as DisplayControl
participant Router as Next Router
participant JLC as JourneyListContent
participant JLR as JourneyListRows
User->>DC: Click grid/list toggle
DC->>JLV: Call setDisplay(newMode)
JLV->>Router: router.push({query: {view: newMode}})
Router->>JLV: URL updated
JLV->>JLC: Pass display prop (grid|list)
JLC->>JLC: Compute listRows with variants
alt display === 'list'
JLC->>JLR: Render with computed rows
JLR->>JLR: Render responsive layout<br/>(table/list based on screen size)
JLR->>User: Display journeys in list view
else display === 'grid'
JLC->>User: Render existing grid/priority view
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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 docstrings
🧪 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 871ba6f
☁️ Nx Cloud last updated this comment at |
Contributor
|
The latest updates on your projects.
|
edmonday
force-pushed
the
edmonday/core-9158-journey-list
branch
from
871ba6f to
809edc4
Compare
Contributor
![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: 3
🧹 Nitpick comments (8)
apps/journeys-admin/src/components/JourneyList/JourneyListRow/JourneyListRow.tsx (6)
144-144: 💤 Low valueDiscarded state read; consider
useRefor removing entirely.
hasOpenDialog's read value is never used (const [, setHasOpenDialog]). IfJourneyCardMenuonly writes the value, switching touseRef(or a no-op callback) avoids unnecessary re-renders ofJourneyListRowwhenever the menu opens/closes a dialog.🤖 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/journeys-admin/src/components/JourneyList/JourneyListRow/JourneyListRow.tsx` at line 144, The state declaration const [, setHasOpenDialog] = useState(false) in JourneyListRow is only written to (never read), causing unnecessary re-renders; replace it with a ref or remove it: either change to a mutable ref (useRef(false)) named hasOpenDialogRef and update hasOpenDialogRef.current from JourneyCardMenu, or remove the state and pass a no-op setter to JourneyCardMenu if the dialog-open signal isn't needed, updating all references to setHasOpenDialog accordingly.
417-435: ⚡ Quick winDuplicate owner-lookup logic between
OwnerAvatarandOwnerCell.Both components run the exact same
journey.userJourneys?.find(...)and__typenameguard. Extract a shared helper (e.g.,getAuthenticatedOwner(journey)) so the lookup, role, and type narrowing live in one place.Also applies to: 490-517
🤖 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/journeys-admin/src/components/JourneyList/JourneyListRow/JourneyListRow.tsx` around lines 417 - 435, Extract the duplicate owner-lookup into a single helper function (e.g., getAuthenticatedOwner(journey): UserJourney | undefined) that performs journey.userJourneys?.find(...) and the AuthenticatedUser __typename check and returns the matched UserJourney or undefined; replace the inline lookup in OwnerAvatar and OwnerCell with calls to getAuthenticatedOwner(journey) and use the returned object for role/type-safe access (owner.role, owner.user) so the find logic and type narrowing live in one place.
248-253: ⚡ Quick win
TemplateBreakdownAnalyticsDialogis rendered for every row, even non-templates.The dialog can only be opened from the template-only
ActivityCellbranch (isTemplateCard), but it's mounted into every desktop row. While the dialog is dynamically imported withssr: false, the wrapper element is still rendered in the DOM for every journey row. Wrap it in theisTemplateCardcondition so it's only present when actually reachable.♻️ Proposed fix
- <TemplateBreakdownAnalyticsDialog - journeyId={journey.id} - open={breakdownDialogOpen} - handleClose={() => setBreakdownDialogOpen(false)} - /> + {isTemplateCard && ( + <TemplateBreakdownAnalyticsDialog + journeyId={journey.id} + open={breakdownDialogOpen} + handleClose={() => setBreakdownDialogOpen(false)} + /> + )}🤖 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/journeys-admin/src/components/JourneyList/JourneyListRow/JourneyListRow.tsx` around lines 248 - 253, The TemplateBreakdownAnalyticsDialog is being rendered for every JourneyListRow even when not a template; update the JSX in JourneyListRow (where TemplateBreakdownAnalyticsDialog, breakdownDialogOpen and setBreakdownDialogOpen are used) to conditionally render the dialog only when isTemplateCard is true (the same condition used by the template branch in ActivityCell) so the component is only mounted for template rows and passes journey.id; keep the existing props (open and handleClose) unchanged.
54-66: ⚖️ Poor tradeoffFile/component naming inconsistency.
The directory and file are named
JourneyListRow(singular), but the primary exported component isJourneyListRows(plural). Per the coding guidelines, each component lives in its own PascalCase directory withComponentName.tsxmatching the component name. The currently un-exported helper at line 135 is also calledJourneyListRow, which adds further ambiguity.Consider renaming the directory and file to
JourneyListRows(matching the public export) and renaming the internal helper (e.g.,JourneyListDesktopRow) to mirrorJourneyListMobileRow.As per coding guidelines: "Every component must live in its own PascalCase directory with structure: ComponentName.tsx (implementation)".
🤖 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/journeys-admin/src/components/JourneyList/JourneyListRow/JourneyListRow.tsx` around lines 54 - 66, The exported component JourneyListRows and the file/directory named JourneyListRow are inconsistent; rename the directory and primary file to JourneyListRows (so the component file is JourneyListRows.tsx) and update any imports accordingly, and rename the internal helper currently named JourneyListRow (the un-exported helper at line ~135) to a clearer name such as JourneyListDesktopRow to match the existing JourneyListMobileRow; ensure all references (props interfaces JourneyListRowItem / JourneyListRowProps can keep names or be adjusted for clarity) are updated so names and file structure follow the PascalCase ComponentName.tsx guideline.
73-97: 💤 Low value
useMediaQuerywithout SSR configuration will cause hydration mismatches on this server-rendered page.JourneyListRows is rendered on
apps/journeys-admin/pages/index.tsx, which usesgetServerSideProps. TheuseMediaQueryhook returnsfalseon the server buttrueon the client (for desktop users), causing the mobile layout to render server-side and then swap to the table on hydration. Consider either passing{ noSsr: true }(paired with a skeleton loader) or configuring{ ssrMatchMedia }at the theme level to prevent content mismatches.🤖 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/journeys-admin/src/components/JourneyList/JourneyListRow/JourneyListRow.tsx` around lines 73 - 97, The useMediaQuery call in JourneyListRow (smUp) causes SSR/client hydration mismatches; update the useMediaQuery invocation in JourneyListRow.tsx to include the noSsr option (e.g. useMediaQuery((theme: Theme) => theme.breakpoints.up('sm'), { noSsr: true })) and pair that change with a temporary skeleton/fallback UI when !smUp to avoid layout shift, or alternatively configure ssrMatchMedia on your theme provider so useMediaQuery returns the correct server value; modify the useMediaQuery call (smUp) or the theme provider configuration accordingly and ensure the mobile/table branch rendering logic (the Paper/List mobile block and the desktop table branch) uses the new behavior.
387-395: ⚡ Quick winReplace
onLoadingCompletewithonLoadin the next/image component.The
onLoadingCompleteprop is deprecated in Next.js 14+ and will be removed in a future major version. Use the standardonLoadevent instead—it fires at the same moment (after the image loads and placeholder is removed).♻️ Proposed change
<Image data-testid="JourneyListRowImage" src={journey.primaryImageBlock.src} alt={journey.primaryImageBlock.alt ?? ''} fill style={{ objectFit: 'cover' }} sizes={`${width}px`} - onLoadingComplete={() => setIsImageLoading(false)} + onLoad={() => setIsImageLoading(false)} />Note: The same pattern appears in
JourneyCard.tsxand should also be updated.🤖 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/journeys-admin/src/components/JourneyList/JourneyListRow/JourneyListRow.tsx` around lines 387 - 395, In JourneyListRow (and similarly in JourneyCard) replace the deprecated Next/Image prop onLoadingComplete with the standard onLoad handler: update the Image component usage in JourneyListRow.tsx (the Image with data-testid="JourneyListRowImage") to use onLoad and call setIsImageLoading(false) inside that handler (ensure the signature accepts the load event if needed), and make the same swap in JourneyCard.tsx so both components use onLoad instead of onLoadingComplete.apps/journeys-admin/src/components/JourneyList/JourneyListView/DisplayModes/shared.ts (1)
16-17: 💤 Low valueConsider tightening optionality.
displayandsetDisplayare typed as optional onSharedControlProps, but bothTeamModeandSharedWithMeModedestructure them and pass them straight toDisplayControl. If they're always provided byJourneyListView, making them required removes the need for downstream undefined checks insideDisplayControl. If they're truly optional for some consumer, leave as-is.🤖 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/journeys-admin/src/components/JourneyList/JourneyListView/DisplayModes/shared.ts` around lines 16 - 17, SharedControlProps currently marks display and setDisplay as optional but TeamMode and SharedWithMeMode destructure and forward them to DisplayControl as if always present; change SharedControlProps by removing the optional modifier from display and setDisplay (make them required: display: JourneyListDisplay; setDisplay: (display: JourneyListDisplay) => void) so downstream components (TeamMode, SharedWithMeMode, DisplayControl) don't need undefined checks, then ensure JourneyListView still provides these props where it constructs TeamMode/SharedWithMeMode.apps/journeys-admin/src/components/JourneyList/JourneyListContent/JourneyListContent.tsx (1)
431-468: 💤 Low value
listRowsis computed even whendisplay === 'grid'.The memo always derives priority-bucketed rows from
sortedJourneys, but the value is only consumed in thedisplay === 'list' && listRows != nullbranch (line 623). For grid users (the default), this is unused work each render. Either guard the computation ondisplay === 'list', or simply inline the mapping into the list branch.♻️ Optional fix
- const listRows = useMemo(() => { - if (sortedJourneys == null) return undefined + const listRows = useMemo(() => { + if (display !== 'list' || sortedJourneys == null) return undefined…and add
displayto the dependency array.🤖 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/journeys-admin/src/components/JourneyList/JourneyListContent/JourneyListContent.tsx` around lines 431 - 468, The memoized computation for listRows runs even when display === 'grid'; update the useMemo so it only computes when the list view is active by adding a guard at the top (if (display !== 'list') return undefined) and include display in the dependency array, or alternatively inline the priority-bucketing logic into the render branch that checks display === 'list'; locate the useMemo that defines listRows and the sortedJourneys/usePriorityList/user?.id deps and either short-circuit when display !== 'list' or move the mapping to the list rendering branch.
🤖 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/journeys-admin/src/components/JourneyList/JourneyListRow/JourneyListRow.tsx`:
- Around line 154-247: Add a new test file JourneyListRow.spec.tsx that mirrors
repo conventions and covers JourneyListRow behavior: render the component
(import JourneyListRow) with a mock journey and assert Thumbnail, ActivityCell,
OwnerCell, LastModifiedDate and JourneyCardMenu are rendered; test variant
branches by passing variant values from JourneyCardVariant to verify TagChip
labels for "New" and "Action Required"; test journey.customizable and
journey.website toggles produce Quick Start and Website chips; verify template
vs non-template rendering and published prop passed into JourneyCardMenu;
simulate navigation state (isNavigating true/false) to assert title opacity and
pointerEvents changes and opening the menu triggers setHasOpenDialog via
onMenuClose; include snapshots or assertions for accessibility attributes and
suppression of hydration warning on LastModifiedDate. Ensure mocks/stubs for
refetch and any external hooks and follow existing test patterns
(ActiveJourneyList.spec.tsx, JourneyCard.spec.tsx) for setup and cleanup.
- Around line 145-148: The language-name selection logic in JourneyListRow and
JourneyListMobileRow is incorrect because the .find predicate uses .some on the
whole array and can return the wrong item; replace the inline expression that
builds languageName with a small shared helper (e.g.,
getPreferredLanguageName(language: { name: { value: string; primary?: boolean
}[] })) that returns the primary name if present otherwise the first name's
value, then use that helper to set languageName in both JourneyListRow and
JourneyListMobileRow.
In
`@apps/journeys-admin/src/components/JourneyList/JourneyListView/JourneyListView.tsx`:
- Line 91: The display state (display / setDisplay initialized from
displayFromQuery) isn't synced when the URL changes; update the existing sync
useEffect that currently handles contentType and status to also watch the
relevant query-derived value and call setDisplay(displayFromQuery) when it
changes—i.e., add displayFromQuery (or the parsed view/query value) to the
effect's dependency array and include a branch that updates display via
setDisplay(displayFromQuery) so the component reflects URL changes
(back/forward) for the view toggle.
---
Nitpick comments:
In
`@apps/journeys-admin/src/components/JourneyList/JourneyListContent/JourneyListContent.tsx`:
- Around line 431-468: The memoized computation for listRows runs even when
display === 'grid'; update the useMemo so it only computes when the list view is
active by adding a guard at the top (if (display !== 'list') return undefined)
and include display in the dependency array, or alternatively inline the
priority-bucketing logic into the render branch that checks display === 'list';
locate the useMemo that defines listRows and the
sortedJourneys/usePriorityList/user?.id deps and either short-circuit when
display !== 'list' or move the mapping to the list rendering branch.
In
`@apps/journeys-admin/src/components/JourneyList/JourneyListRow/JourneyListRow.tsx`:
- Line 144: The state declaration const [, setHasOpenDialog] = useState(false)
in JourneyListRow is only written to (never read), causing unnecessary
re-renders; replace it with a ref or remove it: either change to a mutable ref
(useRef(false)) named hasOpenDialogRef and update hasOpenDialogRef.current from
JourneyCardMenu, or remove the state and pass a no-op setter to JourneyCardMenu
if the dialog-open signal isn't needed, updating all references to
setHasOpenDialog accordingly.
- Around line 417-435: Extract the duplicate owner-lookup into a single helper
function (e.g., getAuthenticatedOwner(journey): UserJourney | undefined) that
performs journey.userJourneys?.find(...) and the AuthenticatedUser __typename
check and returns the matched UserJourney or undefined; replace the inline
lookup in OwnerAvatar and OwnerCell with calls to getAuthenticatedOwner(journey)
and use the returned object for role/type-safe access (owner.role, owner.user)
so the find logic and type narrowing live in one place.
- Around line 248-253: The TemplateBreakdownAnalyticsDialog is being rendered
for every JourneyListRow even when not a template; update the JSX in
JourneyListRow (where TemplateBreakdownAnalyticsDialog, breakdownDialogOpen and
setBreakdownDialogOpen are used) to conditionally render the dialog only when
isTemplateCard is true (the same condition used by the template branch in
ActivityCell) so the component is only mounted for template rows and passes
journey.id; keep the existing props (open and handleClose) unchanged.
- Around line 54-66: The exported component JourneyListRows and the
file/directory named JourneyListRow are inconsistent; rename the directory and
primary file to JourneyListRows (so the component file is JourneyListRows.tsx)
and update any imports accordingly, and rename the internal helper currently
named JourneyListRow (the un-exported helper at line ~135) to a clearer name
such as JourneyListDesktopRow to match the existing JourneyListMobileRow; ensure
all references (props interfaces JourneyListRowItem / JourneyListRowProps can
keep names or be adjusted for clarity) are updated so names and file structure
follow the PascalCase ComponentName.tsx guideline.
- Around line 73-97: The useMediaQuery call in JourneyListRow (smUp) causes
SSR/client hydration mismatches; update the useMediaQuery invocation in
JourneyListRow.tsx to include the noSsr option (e.g. useMediaQuery((theme:
Theme) => theme.breakpoints.up('sm'), { noSsr: true })) and pair that change
with a temporary skeleton/fallback UI when !smUp to avoid layout shift, or
alternatively configure ssrMatchMedia on your theme provider so useMediaQuery
returns the correct server value; modify the useMediaQuery call (smUp) or the
theme provider configuration accordingly and ensure the mobile/table branch
rendering logic (the Paper/List mobile block and the desktop table branch) uses
the new behavior.
- Around line 387-395: In JourneyListRow (and similarly in JourneyCard) replace
the deprecated Next/Image prop onLoadingComplete with the standard onLoad
handler: update the Image component usage in JourneyListRow.tsx (the Image with
data-testid="JourneyListRowImage") to use onLoad and call
setIsImageLoading(false) inside that handler (ensure the signature accepts the
load event if needed), and make the same swap in JourneyCard.tsx so both
components use onLoad instead of onLoadingComplete.
In
`@apps/journeys-admin/src/components/JourneyList/JourneyListView/DisplayModes/shared.ts`:
- Around line 16-17: SharedControlProps currently marks display and setDisplay
as optional but TeamMode and SharedWithMeMode destructure and forward them to
DisplayControl as if always present; change SharedControlProps by removing the
optional modifier from display and setDisplay (make them required: display:
JourneyListDisplay; setDisplay: (display: JourneyListDisplay) => void) so
downstream components (TeamMode, SharedWithMeMode, DisplayControl) don't need
undefined checks, then ensure JourneyListView still provides these props where
it constructs TeamMode/SharedWithMeMode.
🪄 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: 4c83fb09-b151-4141-a53d-2d903d995e72
📒 Files selected for processing (10)
apps/journeys-admin/src/components/JourneyList/JourneyList.tsxapps/journeys-admin/src/components/JourneyList/JourneyListContent/JourneyListContent.tsxapps/journeys-admin/src/components/JourneyList/JourneyListRow/JourneyListRow.tsxapps/journeys-admin/src/components/JourneyList/JourneyListRow/index.tsapps/journeys-admin/src/components/JourneyList/JourneyListView/DisplayModes/Controls.tsxapps/journeys-admin/src/components/JourneyList/JourneyListView/DisplayModes/SharedWithMeMode/SharedWithMeMode.tsxapps/journeys-admin/src/components/JourneyList/JourneyListView/DisplayModes/TeamMode/TeamMode.tsxapps/journeys-admin/src/components/JourneyList/JourneyListView/DisplayModes/shared.tsapps/journeys-admin/src/components/JourneyList/JourneyListView/JourneyListView.tsxapps/journeys-admin/src/components/JourneyList/JourneyListView/index.ts
Comment on lines
+145
to
+148
| const languageName = journey.language.name.find( | ||
| ({ primary }) => | ||
| primary || journey.language.name.some(({ primary }) => !primary) | ||
| )?.value |
Contributor
There was a problem hiding this comment.
Buggy language-name selection logic.
The .some() call inside the .find() predicate doesn't reference the current item being iterated — it just checks whether the whole array has any non-primary entry. So for any mixed array, the predicate returns true for the very first item (regardless of whether it's primary), and find returns that first item. The same broken logic is duplicated at lines 265-268 in JourneyListMobileRow.
The intent appears to be: prefer the primary name; fall back to the first available name.
🐛 Proposed fix (extract a helper, since it's used in two places)
+function getLanguageName(journey: Journey): string | undefined {
+ return (
+ journey.language.name.find(({ primary }) => primary)?.value ??
+ journey.language.name[0]?.value
+ )
+}Then in both JourneyListRow and JourneyListMobileRow:
- const languageName = journey.language.name.find(
- ({ primary }) =>
- primary || journey.language.name.some(({ primary }) => !primary)
- )?.value
+ const languageName = getLanguageName(journey)🤖 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/journeys-admin/src/components/JourneyList/JourneyListRow/JourneyListRow.tsx`
around lines 145 - 148, The language-name selection logic in JourneyListRow and
JourneyListMobileRow is incorrect because the .find predicate uses .some on the
whole array and can return the wrong item; replace the inline expression that
builds languageName with a small shared helper (e.g.,
getPreferredLanguageName(language: { name: { value: string; primary?: boolean
}[] })) that returns the primary name if present otherwise the first name's
value, then use that helper to set languageName in both JourneyListRow and
JourneyListMobileRow.
Comment on lines
+154
to
+247
| <TableRow | ||
| hover | ||
| data-testid={`JourneyListRow-${journey.id}`} | ||
| sx={{ | ||
| cursor: 'pointer', | ||
| '& .MuiTableCell-root': { | ||
| py: 1.25 | ||
| } | ||
| }} | ||
| > | ||
| <TableCell> | ||
| <Thumbnail | ||
| journey={journey} | ||
| isNavigating={isNavigating} | ||
| isImageLoading={isImageLoading} | ||
| setIsImageLoading={setIsImageLoading} | ||
| width={56} | ||
| height={36} | ||
| /> | ||
| </TableCell> | ||
| <TableCell> | ||
| <Typography | ||
| component={NextLink} | ||
| prefetch={false} | ||
| href={`/journeys/${journey.id}`} | ||
| sx={{ | ||
| display: 'block', | ||
| fontWeight: 500, | ||
| color: 'text.primary', | ||
| textDecoration: 'none', | ||
| overflow: 'hidden', | ||
| textOverflow: 'ellipsis', | ||
| whiteSpace: 'nowrap', | ||
| opacity: isNavigating ? 0.5 : 1, | ||
| pointerEvents: isNavigating ? 'none' : 'auto' | ||
| }} | ||
| > | ||
| {journey.title} | ||
| </Typography> | ||
| <Stack direction="row" spacing={0.5} sx={{ mt: 0.5 }}> | ||
| {variant !== JourneyCardVariant.default && ( | ||
| <TagChip | ||
| label={ | ||
| variant === JourneyCardVariant.new | ||
| ? t('New') | ||
| : t('Action Required') | ||
| } | ||
| /> | ||
| )} | ||
| {journey.customizable === true && ( | ||
| <TagChip | ||
| label={t('Quick Start')} | ||
| icon={<Lightning2 sx={{ fontSize: 12 }} />} | ||
| /> | ||
| )} | ||
| {journey.website === true && ( | ||
| <TagChip | ||
| label={t('Website')} | ||
| icon={<Globe sx={{ fontSize: 12 }} />} | ||
| /> | ||
| )} | ||
| </Stack> | ||
| </TableCell> | ||
| <TableCell align="right"> | ||
| <ActivityCell | ||
| journey={journey} | ||
| isTemplateCard={isTemplateCard} | ||
| onOpenBreakdown={() => setBreakdownDialogOpen(true)} | ||
| /> | ||
| </TableCell> | ||
| <TableCell sx={{ color: 'text.secondary' }}>{languageName}</TableCell> | ||
| <TableCell> | ||
| <OwnerCell journey={journey} /> | ||
| </TableCell> | ||
| <TableCell sx={{ color: 'text.secondary' }} suppressHydrationWarning> | ||
| <LastModifiedDate modifiedDate={journey.updatedAt} /> | ||
| </TableCell> | ||
| <TableCell> | ||
| <JourneyCardMenu | ||
| id={journey.id} | ||
| status={journey.status} | ||
| slug={journey.slug} | ||
| published={journey.publishedAt != null} | ||
| refetch={refetch} | ||
| journey={journey} | ||
| hovered | ||
| onMenuClose={() => { | ||
| setHasOpenDialog(false) | ||
| }} | ||
| template={journey.template ?? false} | ||
| setHasOpenDialog={setHasOpenDialog} | ||
| /> | ||
| </TableCell> | ||
| </TableRow> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f . apps/journeys-admin/src/components/JourneyList/JourneyListRowRepository: JesusFilm/core
Length of output: 210
🏁 Script executed:
#!/bin/bash
# Check structure of other components in JourneyList for test patterns
fd -t f . apps/journeys-admin/src/components/JourneyList --max-depth 3 | head -30Repository: JesusFilm/core
Length of output: 2849
🏁 Script executed:
#!/bin/bash
# Look for spec files in the broader components directory
find apps/journeys-admin/src/components/JourneyList -name "*.spec.tsx" -o -name "*.test.tsx"Repository: JesusFilm/core
Length of output: 3282
🏁 Script executed:
#!/bin/bash
# Verify the naming convention by checking a few other component directories
ls -la apps/journeys-admin/src/components/ | grep -E '^d' | head -10Repository: JesusFilm/core
Length of output: 666
Create JourneyListRow.spec.tsx test file.
JourneyListRow is a substantial component with navigation states, template/non-template branching, owner avatars, and tag chips. Per repo conventions demonstrated across all other components in the JourneyList directory (ActiveJourneyList.spec.tsx, JourneyCard.spec.tsx, ArchivedJourneyList.spec.tsx, etc.), a corresponding spec file is required.
🤖 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/journeys-admin/src/components/JourneyList/JourneyListRow/JourneyListRow.tsx`
around lines 154 - 247, Add a new test file JourneyListRow.spec.tsx that mirrors
repo conventions and covers JourneyListRow behavior: render the component
(import JourneyListRow) with a mock journey and assert Thumbnail, ActivityCell,
OwnerCell, LastModifiedDate and JourneyCardMenu are rendered; test variant
branches by passing variant values from JourneyCardVariant to verify TagChip
labels for "New" and "Action Required"; test journey.customizable and
journey.website toggles produce Quick Start and Website chips; verify template
vs non-template rendering and published prop passed into JourneyCardMenu;
simulate navigation state (isNavigating true/false) to assert title opacity and
pointerEvents changes and opening the menu triggers setHasOpenDialog via
onMenuClose; include snapshots or assertions for accessibility attributes and
suppression of hydration warning on LastModifiedDate. Ensure mocks/stubs for
refetch and any external hooks and follow existing test patterns
(ActiveJourneyList.spec.tsx, JourneyCard.spec.tsx) for setup and cleanup.
| useState(contentTypeTabIndex) | ||
| const [selectedStatus, setSelectedStatus] = | ||
| useState<JourneyStatus>(statusFromQuery) | ||
| const [display, setDisplay] = useState<JourneyListDisplay>(displayFromQuery) |
Contributor
There was a problem hiding this comment.
display state is never synced with the URL after initial mount
useState on line 91 initializes display from displayFromQuery only once. The sync useEffect (lines 110–134) already re-syncs contentType and status when the URL changes externally (e.g., browser back/forward), but view is absent from both the dependency array and the effect body. As a result, clicking the back button after switching to list view will revert the URL to view=grid while the component still shows the list toggle as active.
🐛 Proposed fix — add display sync to the existing useEffect
useEffect(() => {
const contentTypeFromRouter =
(router?.query?.type as ContentType) ?? 'journeys'
const statusFromRouter =
(router?.query?.status as JourneyStatus) ?? 'active'
const contentTypeIndex =
contentTypeOptions.find(
(option) => option.queryParam === contentTypeFromRouter
)?.tabIndex ?? 0
if (contentTypeIndex !== activeContentTypeTab) {
setActiveContentTypeTab(contentTypeIndex)
}
if (statusFromRouter !== selectedStatus) {
setSelectedStatus(statusFromRouter)
}
+
+ const displayFromRouter: JourneyListDisplay =
+ router?.query?.view === 'list' ? 'list' : 'grid'
+ if (displayFromRouter !== display) {
+ setDisplay(displayFromRouter)
+ }
}, [
router?.query?.type,
router?.query?.status,
+ router?.query?.view,
contentTypeOptions,
activeContentTypeTab,
selectedStatus,
+ display,
])Also applies to: 110-134
🤖 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/journeys-admin/src/components/JourneyList/JourneyListView/JourneyListView.tsx`
at line 91, The display state (display / setDisplay initialized from
displayFromQuery) isn't synced when the URL changes; update the existing sync
useEffect that currently handles contentType and status to also watch the
relevant query-derived value and call setDisplay(displayFromQuery) when it
changes—i.e., add displayFromQuery (or the parsed view/query value) to the
effect's dependency array and include a branch that updates display via
setDisplay(displayFromQuery) so the component reflects URL changes
(back/forward) for the view toggle.
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
Notes
Validation
Summary by CodeRabbit