feat: add journey list view toggle#9158
Closed
edmonday wants to merge 1 commit into
Closed
Conversation
Contributor
WalkthroughAdds a grid/list display mode toggle to the journeys admin UI, introduces ChangesDisplay Mode Selection & List View Implementation
Sequence DiagramsequenceDiagram
participant User as User
participant DC as DisplayControl
participant JLV as JourneyListView
participant JLC as JourneyListContent
participant JLR as JourneyListRows
participant Grid as GridView
User->>DC: click 'list' toggle
DC->>JLV: setDisplay('list')
JLV->>JLV: update display state
JLV->>JLC: renderList(..., display)
JLC->>JLC: compute listRows from sortedJourneys
alt display === 'list'
JLC->>JLR: render JourneyListRows with rows
JLR-->>User: show table-like rows with badges and analytics
else display === 'grid'
JLC->>Grid: render MUI Grid or ActivePriorityList
Grid-->>User: show card grid or priority layout
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 |
Contributor
|
View your CI Pipeline Execution ↗ for commit 184230b
☁️ Nx Cloud last updated this comment at |
This comment has been minimized.
This comment has been minimized.
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/journeys-admin/src/components/JourneyList/JourneyListContent/JourneyListContent.tsx (1)
603-625:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEmpty state is missing in list mode for the priority-list scenario.
When
display === 'list',usePriorityList === true, andsortedJourneys.length === 0(e.g., a first-time user on the active journeys tab),JourneyListRowsrenders with an emptyrows={[]}and the bottomisEmptyblock doesn't trigger becauseisEmptyrequires!usePriorityList. The grid path covers this case with the dedicated empty Card +AddJourneyButtonat lines 633-654, so list-mode users see a blank table with no call-to-action.🐛 One option: broaden the empty-state predicate so the bottom message renders in list mode too
- const isEmpty = - sortedJourneys != null && sortedJourneys.length === 0 && !usePriorityList + const isEmpty = + sortedJourneys != null && + sortedJourneys.length === 0 && + (!usePriorityList || display === 'list')If you want full parity with the grid view (including the
AddJourneyButton), consider rendering the same empty Card inside thedisplay === 'list'branch whenlistRows.length === 0 && usePriorityList.🤖 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 603 - 625, The empty-state for priority lists is skipped because isEmpty excludes usePriorityList; update the render logic so when display === 'list' and usePriorityList === true and listRows is an empty array you render the same empty Card + AddJourneyButton used in the grid path instead of rendering JourneyListRows with rows=[]. Concretely: either broaden the isEmpty predicate (remove the !usePriorityList condition) or add a conditional in the list branch (check listRows?.length === 0 && usePriorityList) and render the existing empty Card + AddJourneyButton block in that branch; reference JourneyListRows, listRows, usePriorityList, isEmpty and AddJourneyButton to locate the code to change.
🧹 Nitpick comments (4)
apps/journeys-admin/src/components/JourneyList/JourneyListRow/JourneyListRow.tsx (2)
213-213: ⚡ Quick win
onLoadingCompleteis deprecated — replace withonLoad.
onLoadingCompletewas deprecated in Next.js 14; the replacement isonLoad, which receives the event object instead. Since this project is on Next.js 15.5.10, using it triggers a console deprecation warning per the Next.js source. Because the argument is unused here, the fix is a one-liner.♻️ Proposed fix
- onLoadingComplete={() => setIsImageLoading(false)} + onLoad={() => setIsImageLoading(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` at line 213, Replace the deprecated Next.js Image prop onLoadingComplete with onLoad inside the JourneyListRow component: update the Image (or img) prop that currently calls onLoadingComplete={() => setIsImageLoading(false)} to use onLoad instead (i.e., onLoad={() => setIsImageLoading(false)}), keeping the existing setIsImageLoading state updater unchanged so the image-loading state logic stays the same.
137-141: 💤 Low value
updateHoverStateshould be prefixed withhandleper project conventions.As per coding guidelines, event-related functions should be named with a
handleprefix. Rename to e.g.handleHoverStateUpdateor split intohandleMouseEnter/handleMouseLeave.♻️ Proposed rename
- function updateHoverState(hovered: boolean): void { + function handleHoverStateUpdate(hovered: boolean): void { if (!hasOpenDialog) { setIsHovered(hovered) } } // ... - onMouseEnter={() => updateHoverState(true)} - onMouseLeave={() => updateHoverState(false)} + onMouseEnter={() => handleHoverStateUpdate(true)} + onMouseLeave={() => handleHoverStateUpdate(false)} // ... - onMenuClose={() => updateHoverState(false)} + onMenuClose={() => handleHoverStateUpdate(false)}As per coding guidelines: "Event handlers must be prefixed with 'handle'."
🤖 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 137 - 141, The function updateHoverState should be renamed to follow the event-handler convention (prefix with "handle"); rename updateHoverState to a descriptive handler such as handleHoverStateUpdate (or split into handleMouseEnter and handleMouseLeave) and update all references/call sites accordingly (e.g., where setIsHovered is used and any JSX event props in JourneyListRow that call updateHoverState); ensure the parameter and behavior remain unchanged and adjust any exports or tests that reference the original function name.apps/journeys-admin/src/components/JourneyList/JourneyListView/JourneyListView.tsx (1)
94-94: ⚡ Quick win
displaypreference is not persisted — resets to'grid'on every page load.Unlike
sortOrder(written to / read fromsessionStorage), the display mode is only in local component state. Users who prefer list view will need to re-select it after every navigation or refresh.If persistence is intentional for now, a brief comment would clarify intent. Otherwise, mirror the
sortOrderpersistence pattern:♻️ Proposed persistence (optional)
- const [display, setDisplay] = useState<JourneyListDisplay>('grid') + const [display, setDisplay] = useState<JourneyListDisplay>(() => { + const stored = sessionStorage.getItem('journeyListDisplay') + return stored === 'list' ? 'list' : 'grid' + })And in the setter passed to child modes, persist on change:
// Pass a wrapped setter instead of setDisplay directly + const handleSetDisplay = (value: JourneyListDisplay): void => { + setDisplay(value) + sessionStorage.setItem('journeyListDisplay', value) + }🤖 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 94, display state (const [display, setDisplay] = useState<JourneyListDisplay>('grid')) isn't persisted and resets on reload; update it to mirror sortOrder's sessionStorage pattern by reading the initial value from sessionStorage (falling back to 'grid') and writing the new value to sessionStorage whenever setDisplay is called or via a wrapper setter passed to children, or add a comment if persistence is intentional; reference the display, setDisplay, JourneyListDisplay and sortOrder usage in JourneyListView to implement the read-on-init and write-on-change behavior.apps/journeys-admin/src/components/JourneyList/JourneyListContent/JourneyListContent.tsx (1)
431-468: 💤 Low valueOptional: short-circuit
listRowswhendisplay !== 'list'.
listRowsis only consumed in thedisplay === 'list'branch, but the memo still walkssortedJourneysand categorizes by role for grid users on every render (sincesortedJourneysis recomputed without memoization upstream, the cached reference rarely sticks). A small early return avoids the work for the common grid path.♻️ Proposed early return
const listRows = useMemo(() => { + if (display !== 'list') return undefined if (sortedJourneys == null) return undefined if (!usePriorityList) { return sortedJourneys.map((journey) => ({ journey })) } // ... - }, [sortedJourneys, usePriorityList, user?.id]) + }, [display, sortedJourneys, usePriorityList, user?.id])Note: also worth considering memoizing
sortedJourneysitself (line 422) so this memo's identity actually stabilizes — out of scope for this PR.Also note that the action-required / new / active categorization here mirrors the logic inside
ActivePriorityList. Consider extracting a shared helper in a follow-up to prevent drift if the rules change.🤖 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, Add an early-return to the listRows useMemo so it does no work unless the component is rendering the list view: at the top of the memo check the display prop/value and if display !== 'list' return undefined (or the current empty/neutral value) immediately; this avoids iterating sortedJourneys and running the categorization when display is grid, and keeps the existing logic inside the memo (references: listRows, useMemo, display, sortedJourneys, usePriorityList, JourneyCardVariant).
🤖 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 395-399: TemplateBreakdownAnalyticsDialog is rendered for every
row which triggers its dynamic import even for non-template journeys; wrap the
component render in the same isTemplateCard conditional that renders the
template button (use the existing isTemplateCard boolean) so the dialog is only
mounted when isTemplateCard is true, keeping the same props
(journeyId={journey.id}, open={breakdownDialogOpen}, handleClose={() =>
setBreakdownDialogOpen(false)}) to avoid unnecessary bundle requests for
non-template rows.
- Around line 130-133: The current languageName derivation uses
journey.language.name.find with a predicate that incorrectly depends on
journey.language.name.some which is evaluated outside each element and can
return the first non-primary value; change the logic in the languageName
computation (the variable languageName and its use of
journey.language.name.find) to explicitly pick the element where primary ===
true if any exists, otherwise fall back to the first element (e.g., use find(n
=> n.primary) || journey.language.name[0]); then read its .value safely (e.g.,
via optional chaining) so primary languages are returned correctly.
- Around line 126-141: The hover state can remain true after a dialog closes
because updateHoverState(false) is skipped while hasOpenDialog is true; add a
cleanup effect that watches hasOpenDialog and when it becomes false calls
setIsHovered(false) to reset the row highlight; locate the state and updater
names (isHovered, hasOpenDialog, setIsHovered, updateHoverState) in
JourneyListRow and implement a useEffect that clears isHovered when
hasOpenDialog transitions to false.
---
Outside diff comments:
In
`@apps/journeys-admin/src/components/JourneyList/JourneyListContent/JourneyListContent.tsx`:
- Around line 603-625: The empty-state for priority lists is skipped because
isEmpty excludes usePriorityList; update the render logic so when display ===
'list' and usePriorityList === true and listRows is an empty array you render
the same empty Card + AddJourneyButton used in the grid path instead of
rendering JourneyListRows with rows=[]. Concretely: either broaden the isEmpty
predicate (remove the !usePriorityList condition) or add a conditional in the
list branch (check listRows?.length === 0 && usePriorityList) and render the
existing empty Card + AddJourneyButton block in that branch; reference
JourneyListRows, listRows, usePriorityList, isEmpty and AddJourneyButton to
locate the code to change.
---
Nitpick comments:
In
`@apps/journeys-admin/src/components/JourneyList/JourneyListContent/JourneyListContent.tsx`:
- Around line 431-468: Add an early-return to the listRows useMemo so it does no
work unless the component is rendering the list view: at the top of the memo
check the display prop/value and if display !== 'list' return undefined (or the
current empty/neutral value) immediately; this avoids iterating sortedJourneys
and running the categorization when display is grid, and keeps the existing
logic inside the memo (references: listRows, useMemo, display, sortedJourneys,
usePriorityList, JourneyCardVariant).
In
`@apps/journeys-admin/src/components/JourneyList/JourneyListRow/JourneyListRow.tsx`:
- Line 213: Replace the deprecated Next.js Image prop onLoadingComplete with
onLoad inside the JourneyListRow component: update the Image (or img) prop that
currently calls onLoadingComplete={() => setIsImageLoading(false)} to use onLoad
instead (i.e., onLoad={() => setIsImageLoading(false)}), keeping the existing
setIsImageLoading state updater unchanged so the image-loading state logic stays
the same.
- Around line 137-141: The function updateHoverState should be renamed to follow
the event-handler convention (prefix with "handle"); rename updateHoverState to
a descriptive handler such as handleHoverStateUpdate (or split into
handleMouseEnter and handleMouseLeave) and update all references/call sites
accordingly (e.g., where setIsHovered is used and any JSX event props in
JourneyListRow that call updateHoverState); ensure the parameter and behavior
remain unchanged and adjust any exports or tests that reference the original
function name.
In
`@apps/journeys-admin/src/components/JourneyList/JourneyListView/JourneyListView.tsx`:
- Line 94: display state (const [display, setDisplay] =
useState<JourneyListDisplay>('grid')) isn't persisted and resets on reload;
update it to mirror sortOrder's sessionStorage pattern by reading the initial
value from sessionStorage (falling back to 'grid') and writing the new value to
sessionStorage whenever setDisplay is called or via a wrapper setter passed to
children, or add a comment if persistence is intentional; reference the display,
setDisplay, JourneyListDisplay and sortOrder usage in JourneyListView to
implement the read-on-init and write-on-change behavior.
🪄 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: 4451b7e1-2ee8-47d2-b075-e1c9a6160f6b
📒 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
+126
to
+141
| const [isHovered, setIsHovered] = useState(false) | ||
| const [isImageLoading, setIsImageLoading] = useState(true) | ||
| const [breakdownDialogOpen, setBreakdownDialogOpen] = useState(false) | ||
| const [hasOpenDialog, setHasOpenDialog] = useState(false) | ||
| const languageName = journey.language.name.find( | ||
| ({ primary }) => | ||
| primary || journey.language.name.some(({ primary }) => !primary) | ||
| )?.value | ||
| const isTemplateCard = | ||
| journey.template === true && journey.team?.id !== 'jfp-team' | ||
|
|
||
| function updateHoverState(hovered: boolean): void { | ||
| if (!hasOpenDialog) { | ||
| setIsHovered(hovered) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Hover highlight stays on indefinitely if the mouse leaves while a dialog is open.
When hasOpenDialog is true, updateHoverState(false) from onMouseLeave is skipped. After the dialog closes and hasOpenDialog returns to false, isHovered is never reset, leaving the row permanently highlighted until the cursor re-enters.
🐛 Proposed fix — add a cleanup effect
const [hasOpenDialog, setHasOpenDialog] = useState(false)
+ const isMouseOverRef = useRef(false)
function updateHoverState(hovered: boolean): void {
if (!hasOpenDialog) {
setIsHovered(hovered)
}
+ isMouseOverRef.current = hovered
}
+ useEffect(() => {
+ if (!hasOpenDialog) {
+ setIsHovered(isMouseOverRef.current)
+ }
+ }, [hasOpenDialog])📝 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
| const [isHovered, setIsHovered] = useState(false) | |
| const [isImageLoading, setIsImageLoading] = useState(true) | |
| const [breakdownDialogOpen, setBreakdownDialogOpen] = useState(false) | |
| const [hasOpenDialog, setHasOpenDialog] = useState(false) | |
| const languageName = journey.language.name.find( | |
| ({ primary }) => | |
| primary || journey.language.name.some(({ primary }) => !primary) | |
| )?.value | |
| const isTemplateCard = | |
| journey.template === true && journey.team?.id !== 'jfp-team' | |
| function updateHoverState(hovered: boolean): void { | |
| if (!hasOpenDialog) { | |
| setIsHovered(hovered) | |
| } | |
| } | |
| const [isHovered, setIsHovered] = useState(false) | |
| const [isImageLoading, setIsImageLoading] = useState(true) | |
| const [breakdownDialogOpen, setBreakdownDialogOpen] = useState(false) | |
| const [hasOpenDialog, setHasOpenDialog] = useState(false) | |
| const isMouseOverRef = useRef(false) | |
| const languageName = journey.language.name.find( | |
| ({ primary }) => | |
| primary || journey.language.name.some(({ primary }) => !primary) | |
| )?.value | |
| const isTemplateCard = | |
| journey.template === true && journey.team?.id !== 'jfp-team' | |
| function updateHoverState(hovered: boolean): void { | |
| if (!hasOpenDialog) { | |
| setIsHovered(hovered) | |
| } | |
| isMouseOverRef.current = hovered | |
| } | |
| useEffect(() => { | |
| if (!hasOpenDialog) { | |
| setIsHovered(isMouseOverRef.current) | |
| } | |
| }, [hasOpenDialog]) |
🤖 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 126 - 141, The hover state can remain true after a dialog closes
because updateHoverState(false) is skipped while hasOpenDialog is true; add a
cleanup effect that watches hasOpenDialog and when it becomes false calls
setIsHovered(false) to reset the row highlight; locate the state and updater
names (isHovered, hasOpenDialog, setIsHovered, updateHoverState) in
JourneyListRow and implement a useEffect that clears isHovered when
hasOpenDialog transitions to false.
Comment on lines
+130
to
+133
| const languageName = journey.language.name.find( | ||
| ({ primary }) => | ||
| primary || journey.language.name.some(({ primary }) => !primary) | ||
| )?.value |
Contributor
There was a problem hiding this comment.
Logic bug in languageName derivation — may show wrong language.
journey.language.name.some(({ primary }) => !primary) is evaluated once and produces the same boolean for all iterations. When any non-primary name exists (common for multilingual journeys), the predicate is always true, so find returns the first array element — not necessarily the primary one.
Minimal counter-example:
name = [{value:'Spanish', primary:false}, {value:'English', primary:true}]
→ some(not primary) = true → returns 'Spanish' ✗
🐛 Proposed fix
- const languageName = journey.language.name.find(
- ({ primary }) =>
- primary || journey.language.name.some(({ primary }) => !primary)
- )?.value
+ const languageName = (
+ journey.language.name.find(({ primary }) => primary) ??
+ journey.language.name[0]
+ )?.value📝 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
| const languageName = journey.language.name.find( | |
| ({ primary }) => | |
| primary || journey.language.name.some(({ primary }) => !primary) | |
| )?.value | |
| const languageName = ( | |
| journey.language.name.find(({ primary }) => primary) ?? | |
| journey.language.name[0] | |
| )?.value |
🤖 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 130 - 133, The current languageName derivation uses
journey.language.name.find with a predicate that incorrectly depends on
journey.language.name.some which is evaluated outside each element and can
return the first non-primary value; change the logic in the languageName
computation (the variable languageName and its use of
journey.language.name.find) to explicitly pick the element where primary ===
true if any exists, otherwise fall back to the first element (e.g., use find(n
=> n.primary) || journey.language.name[0]); then read its .value safely (e.g.,
via optional chaining) so primary languages are returned correctly.
Comment on lines
+395
to
+399
| <TemplateBreakdownAnalyticsDialog | ||
| journeyId={journey.id} | ||
| open={breakdownDialogOpen} | ||
| handleClose={() => setBreakdownDialogOpen(false)} | ||
| /> |
Contributor
There was a problem hiding this comment.
TemplateBreakdownAnalyticsDialog is mounted for every row, including non-template journeys.
It lives outside the isTemplateCard check, so the dynamic import is triggered for all rows. Since it's { ssr: false } via next/dynamic, this also creates an unnecessary bundle chunk request per non-template row. Wrapping it in {isTemplateCard && ...} aligns it with the button that opens it.
🐛 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 395 - 399, TemplateBreakdownAnalyticsDialog is rendered for every
row which triggers its dynamic import even for non-template journeys; wrap the
component render in the same isTemplateCard conditional that renders the
template button (use the existing isTemplateCard boolean) so the dialog is only
mounted when isTemplateCard is true, keeping the same props
(journeyId={journey.id}, open={breakdownDialogOpen}, handleClose={() =>
setBreakdownDialogOpen(false)}) to avoid unnecessary bundle requests for
non-template rows.
edmonday
force-pushed
the
edmonday/journey-list-view
branch
from
9a1c251 to
f132b71
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apps/journeys-admin/src/components/JourneyList/JourneyListRow/JourneyListRow.tsx (1)
397-402: ⚡ Quick winReplace inline props object types with named
{ComponentName}Propsinterfaces.
ActivityCell,OwnerCell, andListBadgecurrently use inline object types. Please switch toActivityCellProps,OwnerCellProps, andListBadgePropsinterfaces for consistency and maintainability.As per coding guidelines: “Props interfaces must follow naming convention {ComponentName}Props” and “Define a type if possible.”
Also applies to: 462-465, 494-498
🤖 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 397 - 402, The components ActivityCell, OwnerCell, and ListBadge are using inline object prop types; create named interfaces ActivityCellProps, OwnerCellProps, and ListBadgeProps and replace the inline types with these interfaces for each component (e.g., change the inline parameter type in ActivityCell(...), OwnerCell(...), and ListBadge(...) to use ActivityCellProps, OwnerCellProps, and ListBadgeProps respectively). Ensure each interface lists the same properties currently declared inline (preserve names and types such as journey: Journey, variant: JourneyCardVariant, isTemplateCard: boolean, onOpenBreakdown: () => void where applicable) and export or scope them consistently with the component file so the components compile and follow the {ComponentName}Props naming convention.apps/journeys-admin/src/components/JourneyList/JourneyListView/DisplayModes/Controls.tsx (2)
66-68: 💤 Low valueExtract inline
onChangeas a namedhandleDisplayChangehandler.The coding guideline requires event handler functions to carry the
handleprefix. The current anonymous arrow inonChangebypasses that convention.♻️ Proposed refactor
export const DisplayControl = ({ display, setDisplay }: Pick<SharedControlProps, 'display' | 'setDisplay'>): ReactElement => { const { t } = useTranslation('apps-journeys-admin') + + const handleDisplayChange = ( + _event: React.MouseEvent<HTMLElement>, + value: string | null + ): void => { + if (value != null) setDisplay(value as JourneyListDisplay) + } return ( <ToggleButtonGroup ... - onChange={(_event, value) => { - if (value != null) setDisplay(value) - }} + onChange={handleDisplayChange}🤖 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/Controls.tsx` around lines 66 - 68, The inline anonymous onChange handler should be extracted to a named handler called handleDisplayChange to follow the "handle" prefix convention; create a function handleDisplayChange(event, value) (or with the same params used by the component) that checks value != null and calls setDisplay(value), then replace the inline onChange prop with onChange={handleDisplayChange} in the Controls component so references to setDisplay and the onChange logic are centralized and testable.
6-6: 💤 Low valueConsider extracting the onChange handler with a "handle" prefix.
The
onChangecallback on line 69 uses an inline arrow function. Per coding guidelines, event handlers should be named with a "handle" prefix (e.g.,handleDisplayChange). Extract it as:const handleDisplayChange = (value: any) => { if (value != null) setDisplay(value) }Then pass
onChange={handleDisplayChange}to ToggleButtonGroup.The
useTranslationimport from'next-i18next/pages'is correct and follows the established convention across the journeys-admin codebase.🤖 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/Controls.tsx` at line 6, Extract the inline onChange arrow into a named handler called handleDisplayChange and pass it to ToggleButtonGroup: implement const handleDisplayChange = (value: any) => { if (value != null) setDisplay(value) } and replace the inline onChange prop with onChange={handleDisplayChange}; ensure you reference the existing setDisplay state updater and ToggleButtonGroup component when making the change (keep the existing useTranslation import as-is).
🤖 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/journeys-admin/src/components/JourneyList/JourneyListRow/JourneyListRow.tsx`:
- Around line 397-402: The components ActivityCell, OwnerCell, and ListBadge are
using inline object prop types; create named interfaces ActivityCellProps,
OwnerCellProps, and ListBadgeProps and replace the inline types with these
interfaces for each component (e.g., change the inline parameter type in
ActivityCell(...), OwnerCell(...), and ListBadge(...) to use ActivityCellProps,
OwnerCellProps, and ListBadgeProps respectively). Ensure each interface lists
the same properties currently declared inline (preserve names and types such as
journey: Journey, variant: JourneyCardVariant, isTemplateCard: boolean,
onOpenBreakdown: () => void where applicable) and export or scope them
consistently with the component file so the components compile and follow the
{ComponentName}Props naming convention.
In
`@apps/journeys-admin/src/components/JourneyList/JourneyListView/DisplayModes/Controls.tsx`:
- Around line 66-68: The inline anonymous onChange handler should be extracted
to a named handler called handleDisplayChange to follow the "handle" prefix
convention; create a function handleDisplayChange(event, value) (or with the
same params used by the component) that checks value != null and calls
setDisplay(value), then replace the inline onChange prop with
onChange={handleDisplayChange} in the Controls component so references to
setDisplay and the onChange logic are centralized and testable.
- Line 6: Extract the inline onChange arrow into a named handler called
handleDisplayChange and pass it to ToggleButtonGroup: implement const
handleDisplayChange = (value: any) => { if (value != null) setDisplay(value) }
and replace the inline onChange prop with onChange={handleDisplayChange}; ensure
you reference the existing setDisplay state updater and ToggleButtonGroup
component when making the change (keep the existing useTranslation import
as-is).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 51b606e9-40de-40a3-b60e-6a0099d0faf1
📒 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
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/journeys-admin/src/components/JourneyList/JourneyListRow/index.ts
- apps/journeys-admin/src/components/JourneyList/JourneyListView/DisplayModes/TeamMode/TeamMode.tsx
- apps/journeys-admin/src/components/JourneyList/JourneyList.tsx
- apps/journeys-admin/src/components/JourneyList/JourneyListView/index.ts
- apps/journeys-admin/src/components/JourneyList/JourneyListView/DisplayModes/SharedWithMeMode/SharedWithMeMode.tsx
- apps/journeys-admin/src/components/JourneyList/JourneyListView/DisplayModes/shared.ts
- apps/journeys-admin/src/components/JourneyList/JourneyListView/JourneyListView.tsx
- apps/journeys-admin/src/components/JourneyList/JourneyListContent/JourneyListContent.tsx
edmonday
force-pushed
the
edmonday/journey-list-view
branch
from
f132b71 to
cf719d9
Compare
edmonday
force-pushed
the
edmonday/journey-list-view
branch
from
cf719d9 to
96b706c
Compare
edmonday
force-pushed
the
edmonday/journey-list-view
branch
from
96b706c to
184230b
Compare
edmonday
force-pushed
the
edmonday/journey-list-view
branch
from
184230b to
84fd911
Compare
edmonday
force-pushed
the
edmonday/journey-list-view
branch
from
84fd911 to
871ba6f
Compare
edmonday
changed the title
Contributor
Author
|
Superseded by #9159 using a CI-compliant branch name. |
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