Added sidebar for viewing automation steps#27973
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe PR implements step selection and a read-only details sidebar for the automation canvas. It introduces Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
eb95539 to
59d494d
Compare
d94ddd6 to
5e673e2
Compare
a66806f to
ed0a165
Compare
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)
apps/posts/src/views/Automations/components/automation-canvas.tsx (1)
339-343:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThe open sidebar is covering the centered step column.
The graph is still centered against the full canvas width, but the sidebar is absolutely overlaid on that same space. With the current constants, common viewport widths leave the selected 256px node partially under the 36rem panel, which hides part of the step and nearby affordances. Reserve layout width for the sidebar or recenter the viewport when it opens.
Also applies to: 552-565, 637-661
🤖 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/posts/src/views/Automations/components/automation-canvas.tsx` around lines 339 - 343, The viewport is centered using the full canvas width so the right-side sidebar (36rem) overlays the centered node; update getInitialViewport and the other centering logic (the blocks around NODE_COLUMN_CENTER_X and INITIAL_VIEWPORT_Y at the other two occurrences) to subtract the sidebar’s reserved width when the sidebar is open (or compute an effectiveCanvasWidth = canvasWidth - SIDEBAR_WIDTH and use that for centering), and also trigger a recentre function when the sidebar toggles so the selected node is shifted left by half the sidebar width; reference NODE_COLUMN_CENTER_X, INITIAL_VIEWPORT_Y, getInitialViewport and the matching centering code at the other two occurrences to apply the same change.
🤖 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/posts/src/views/Automations/components/automation-canvas.tsx`:
- Around line 552-565: The sidebar opens but has no keyboard-accessible way to
dismiss it; update the StepSidebar component to accept and call a close handler
(e.g., onClose or clearDetail) and render a focusable close control inside (with
an aria-label and data-testid) that invokes that handler; additionally add an
Escape keydown listener within StepSidebar that calls the same
onClose/clearDetail when detail is present (clean up listener on unmount), and
apply the same changes to the other sidebar instance that renders
StepSidebar/StepSidebarContent so both mouse and keyboard users can close the
panel.
---
Outside diff comments:
In `@apps/posts/src/views/Automations/components/automation-canvas.tsx`:
- Around line 339-343: The viewport is centered using the full canvas width so
the right-side sidebar (36rem) overlays the centered node; update
getInitialViewport and the other centering logic (the blocks around
NODE_COLUMN_CENTER_X and INITIAL_VIEWPORT_Y at the other two occurrences) to
subtract the sidebar’s reserved width when the sidebar is open (or compute an
effectiveCanvasWidth = canvasWidth - SIDEBAR_WIDTH and use that for centering),
and also trigger a recentre function when the sidebar toggles so the selected
node is shifted left by half the sidebar width; reference NODE_COLUMN_CENTER_X,
INITIAL_VIEWPORT_Y, getInitialViewport and the matching centering code at the
other two occurrences to apply the same change.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d545dc7d-151d-4e58-9536-6753ecb36b48
📒 Files selected for processing (2)
apps/posts/src/views/Automations/components/automation-canvas.tsxapps/posts/test/unit/views/automations/automation-editor.test.tsx
| label: 'Trigger', | ||
| value: 'Member signs up', | ||
| selected: selectedStepId === TRIGGER_CANVAS_ID, | ||
| onSelect: () => onSelectStep(TRIGGER_CANVAS_ID) |
There was a problem hiding this comment.
Do we want this to be configurable right now? I think not? (I think that'd mean removing this and TriggerStepSidebarDetail and the horse it rode on.)
There was a problem hiding this comment.
oh i meant to call this out before i added you as a reviewer. Other than it being in the prototype, i think maybe it makes sense to add and not let it be editable because:
- it provides a bit more detail than the node can
- in the near term we can improve something that's been asked for a few times since welcome emails launched, and provide a bit more granularity on who gets the email and when (for example, sending the paid one to comped members)
though tbf, it'd have to be pretty different than just these checkboxes, and checkboxes that do nothing will be kinda confusing
| type: 'send_email' | ||
| }; | ||
| default: { | ||
| const _exhaustive: never = action; |
8758742 to
b672e16
Compare
towards NY-1254