Open
Conversation
Contributor
WalkthroughThese changes introduce edge-aware positioning logic to the journey flow step arrangement system. The Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (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 f415f86
☁️ Nx Cloud last updated this comment at |
github-actions
bot
temporarily deployed
to
Preview - journeys-admin
![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: 2
🤖 Fix all issues with AI agents
In
`@apps/journeys-admin/src/components/Editor/Slider/JourneyFlow/JourneyFlow.tsx`:
- Around line 203-209: The initial arrangement uses the fallback because
edgesRef.current is empty when arrangeSteps(steps, edgesRef.current) runs;
ensure edges are computed/populated before triggering the edge-aware layout by
moving or adding the edge population step ahead of the call that invokes
allBlockPositionUpdate(true)/arrangeSteps — specifically, populate edgesRef (or
call setEdges / the edge-computation helper) before calling
allBlockPositionUpdate in the useEffect, or defer running arrangeSteps until you
have a computed edges value and pass that into arrangeSteps(steps, edges) so the
edge-aware path runs even on first load.
In
`@apps/journeys-admin/src/components/Editor/Slider/JourneyFlow/libs/arrangeSteps/arrangeSteps.ts`:
- Around line 105-116: The forEach arrow currently uses an expression body that
implicitly returns the Set from placedIds.add(s.id), which triggers the lint
warning; in processLevel change the callback to a block body (e.g., .forEach((s)
=> { placedIds.add(s.id); })) to suppress the implicit-return, and make the
identical change for the other forEach occurrence referenced around line 150 so
both callbacks use block bodies instead of expression bodies; locate these by
the function name processLevel and by occurrences of placedIds.add(...) in the
file.
🧹 Nitpick comments (2)
apps/journeys-admin/src/components/Editor/Slider/JourneyFlow/libs/arrangeSteps/arrangeSteps.ts (1)
210-234:prevActionBlockis unused — likely a leftover from prior logic.The
reducecallback receivesprevActionBlockon line 210 but never uses it inside the body. The return value on line 233 is computed but only feeds back asprevActionBlockfor the next iteration, where it's again unused.apps/journeys-admin/src/components/Editor/Slider/JourneyFlow/libs/arrangeSteps/arrangeSteps.spec.ts (1)
182-191: Cycle test assertions are weak — consider asserting actual position values.The test only checks that positions are defined, not what the actual
x/yvalues are. This doesn't verify the layout is correct for cycles. Asserting specific coordinates (or at least structural properties like "all steps in cycle A share the same x") would catch regressions more effectively.
Comment on lines
+203
to
+209
|
|
||
| const stepBlockInputs = Object.entries( | ||
| arrangeSteps(steps, edgesRef.current) | ||
| ).map(([id, position]) => ({ | ||
| id, | ||
| ...position | ||
| })) |
Contributor
There was a problem hiding this comment.
On first load with invalid positions, edgesRef.current will be [] — falling back to structure traversal.
When the useEffect at line 260 detects invalid step positions and calls allBlockPositionUpdate(true), it returns early before setEdges populates the edge state. So this initial arrangement uses the structure-based fallback. On subsequent "reset view" clicks (the actual fix target), edges are populated and the edge-aware path runs correctly.
This is likely fine for the initial load scenario, but worth being aware of — if the initial layout also needs edge-aware positioning, you'd need to compute edges before calling allBlockPositionUpdate.
🤖 Prompt for AI Agents
In `@apps/journeys-admin/src/components/Editor/Slider/JourneyFlow/JourneyFlow.tsx`
around lines 203 - 209, The initial arrangement uses the fallback because
edgesRef.current is empty when arrangeSteps(steps, edgesRef.current) runs;
ensure edges are computed/populated before triggering the edge-aware layout by
moving or adding the edge population step ahead of the call that invokes
allBlockPositionUpdate(true)/arrangeSteps — specifically, populate edgesRef (or
call setEdges / the edge-computation helper) before calling
allBlockPositionUpdate in the useEffect, or defer running arrangeSteps until you
have a computed edges value and pass that into arrangeSteps(steps, edges) so the
edge-aware path runs even on first load.
Comment on lines
+105
to
+116
| function processLevel(levelSteps: TreeStepBlock[]): void { | ||
| blocks.push(levelSteps) | ||
| levelSteps.forEach((s) => placedIds.add(s.id)) | ||
| const nextIds = levelSteps.flatMap((s) => sourceToTargets.get(s.id) ?? []) | ||
| const uniqueNextIds = [...new Set(nextIds)].filter( | ||
| (id) => !placedIds.has(id) | ||
| ) | ||
| const nextSteps = uniqueNextIds | ||
| .map((id) => steps.find((s) => s.id === id)) | ||
| .filter((s): s is TreeStepBlock => s != null) | ||
| if (nextSteps.length > 0) processLevel(nextSteps) | ||
| } |
Contributor
There was a problem hiding this comment.
Static analysis: forEach callback implicitly returns a value (lines 107, 150).
Set.add() returns the Set itself, so the arrow in .forEach((s) => placedIds.add(s.id)) implicitly returns a value. Biome flags this as suspicious. Use a block body to suppress the lint error.
🔧 Proposed fix
- levelSteps.forEach((s) => placedIds.add(s.id))
+ levelSteps.forEach((s) => { placedIds.add(s.id) })Apply the same fix at line 150:
- levelSteps.forEach((s) => loopPlacedIds.add(s.id))
+ levelSteps.forEach((s) => { loopPlacedIds.add(s.id) })📝 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
| function processLevel(levelSteps: TreeStepBlock[]): void { | |
| blocks.push(levelSteps) | |
| levelSteps.forEach((s) => placedIds.add(s.id)) | |
| const nextIds = levelSteps.flatMap((s) => sourceToTargets.get(s.id) ?? []) | |
| const uniqueNextIds = [...new Set(nextIds)].filter( | |
| (id) => !placedIds.has(id) | |
| ) | |
| const nextSteps = uniqueNextIds | |
| .map((id) => steps.find((s) => s.id === id)) | |
| .filter((s): s is TreeStepBlock => s != null) | |
| if (nextSteps.length > 0) processLevel(nextSteps) | |
| } | |
| function processLevel(levelSteps: TreeStepBlock[]): void { | |
| blocks.push(levelSteps) | |
| levelSteps.forEach((s) => { placedIds.add(s.id) }) | |
| const nextIds = levelSteps.flatMap((s) => sourceToTargets.get(s.id) ?? []) | |
| const uniqueNextIds = [...new Set(nextIds)].filter( | |
| (id) => !placedIds.has(id) | |
| ) | |
| const nextSteps = uniqueNextIds | |
| .map((id) => steps.find((s) => s.id === id)) | |
| .filter((s): s is TreeStepBlock => s != null) | |
| if (nextSteps.length > 0) processLevel(nextSteps) | |
| } |
🧰 Tools
🪛 Biome (2.3.14)
[error] 107-107: This callback passed to forEach() iterable method should not return a value.
Either remove this return or remove the returned value.
(lint/suspicious/useIterableCallbackReturn)
🤖 Prompt for AI Agents
In
`@apps/journeys-admin/src/components/Editor/Slider/JourneyFlow/libs/arrangeSteps/arrangeSteps.ts`
around lines 105 - 116, The forEach arrow currently uses an expression body that
implicitly returns the Set from placedIds.add(s.id), which triggers the lint
warning; in processLevel change the callback to a block body (e.g., .forEach((s)
=> { placedIds.add(s.id); })) to suppress the implicit-return, and make the
identical change for the other forEach occurrence referenced around line 150 so
both callbacks use block bodies instead of expression bodies; locate these by
the function name processLevel and by occurrences of placedIds.add(...) in the
file.
Contributor
|
The latest updates on your projects.
|
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
2 participants
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 by CodeRabbit