fix(ui): Disable Redo & Undo if no actions available #679
Conversation
WalkthroughThe changes in this pull request involve updates to the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
✅ Deploy Preview for reearth-flow ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![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
🧹 Outside diff range and nitpick comments (3)
ui/src/features/Editor/components/OverlayUI/components/Toolbox/index.tsx (1)
Line range hint
138-145: Consider handling loading states during async operationsWhile the disabled states are correctly implemented, consider adding loading states to prevent rapid clicking during async undo/redo operations.
<IconButton key={action.id} className="rounded-[4px]" tooltipPosition="right" tooltipText={action.name} icon={action.icon} - disabled={action.id === "undo" ? !canUndo : !canRedo} + disabled={action.id === "undo" ? !canUndo || isUndoLoading : !canRedo || isRedoLoading} onClick={() => action.id === "redo" ? onRedo?.() : action.id === "undo" ? onUndo?.() : undefined } />ui/src/features/Editor/index.tsx (1)
Line range hint
12-16: Update Props type to include canUndo and canRedoThe Props type definition needs to be updated to include the new properties that are being used in the component.
type Props = { yWorkflows: YArray<YWorkflow>; undoManager: YUndoManager | null; undoTrackerActionWrapper: (callback: () => void) => void; + canUndo: boolean; + canRedo: boolean; };ui/src/features/Editor/hooks.ts (1)
Line range hint
166-178: Consider disabling keyboard shortcuts when undo/redo unavailableThe keyboard shortcuts for undo/redo are currently unconditional. Consider using the new
canUndo/canRedostates to conditionally enable these shortcuts.{ keyBinding: { key: "z", commandKey: true, shiftKey: true }, - callback: handleWorkflowRedo, + callback: canRedo ? handleWorkflowRedo : undefined, }, { keyBinding: { key: "z", commandKey: true }, - callback: handleWorkflowUndo, + callback: canUndo ? handleWorkflowUndo : undefined, },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
ui/src/features/Editor/components/OverlayUI/components/Toolbox/index.tsx(2 hunks)ui/src/features/Editor/components/OverlayUI/index.tsx(3 hunks)ui/src/features/Editor/hooks.ts(2 hunks)ui/src/features/Editor/index.tsx(2 hunks)ui/src/lib/yjs/useYjsStore.ts(2 hunks)
🔇 Additional comments (8)
ui/src/features/Editor/components/OverlayUI/components/Toolbox/index.tsx (1)
30-31: LGTM! Props change improves type safety
The change from optional undoDisabled to required canUndo and canRedo props makes the component's requirements more explicit and provides better granular control over button states.
ui/src/features/Editor/components/OverlayUI/index.tsx (1)
22-23: LGTM! Clean props flow implementation
The props are correctly typed and properly passed down to the Toolbox component.
Also applies to: 56-61
ui/src/features/Editor/index.tsx (2)
68-69: LGTM! Clean props flow to OverlayUI
The canUndo/canRedo props are correctly passed down to the OverlayUI component.
35-36: Verify undo/redo state management implementation
Let's verify that the undo/redo state management is consistently implemented across the codebase.
Also applies to: 68-69
✅ Verification successful
Let me gather more context about the undo/redo implementation.
Undo/redo implementation is consistently managed through Yjs
The undo/redo functionality is properly implemented using Yjs's UndoManager with consistent state management across the codebase:
- Initialized in
useYjsSetup.tswith proper tracking configuration - State (
canUndo/canRedo) is derived from undoManager's stack state inuseYjsStore.ts - Props are correctly passed down through component hierarchy (Editor → OverlayUI → Toolbox)
- Actions are properly handled through
undoManager.undo()andundoManager.redo()
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent implementation of undo/redo state management
# Check for useHooks implementation
echo "Checking useHooks implementation..."
rg -A 5 "canUndo.*canRedo" "ui/src/features/Editor/hooks.ts"
# Check for any direct undoManager stack access
echo "Checking undoManager usage..."
rg "undoManager\.(stack|undoStack|redoStack)"
Length of output: 458
Script:
#!/bin/bash
# Check the implementation of undo/redo related hooks and state management
# Check for canUndo/canRedo usage in components
echo "Checking canUndo/canRedo usage in components..."
rg -A 5 "canUndo|canRedo" "ui/src"
# Check for undoManager initialization and setup
echo "Checking undoManager initialization..."
rg -A 5 "new Y\.UndoManager|useUndoManager" "ui/src"
# Check for undo/redo action handlers
echo "Checking undo/redo action handlers..."
rg -A 5 "\.undo\(\)|\.redo\(\)" "ui/src"
Length of output: 7397
ui/src/lib/yjs/useYjsStore.ts (2)
51-59: LGTM! Clean implementation of undo/redo state tracking
The implementation correctly:
- Uses optional chaining for safe access to undoManager stacks
- Provides proper default values using nullish coalescing
- Memoizes values based on stack length changes
179-180: LGTM! Proper exposure of undo/redo capabilities
The new state values are correctly exposed in the return object, allowing consumers to react to undo/redo availability.
ui/src/features/Editor/hooks.ts (2)
48-49: LGTM! Proper integration of undo/redo state
The new capabilities are correctly destructured from useYjsStore.
201-202: LGTM! Proper exposure of undo/redo state
The state values are correctly exposed in the return object for UI components to consume.
| const canUndo = useMemo(() => { | ||
| const stackLength = undoManager?.undoStack?.length ?? 0; | ||
| return stackLength > 0; | ||
| }, [undoManager?.undoStack?.length]); | ||
|
|
||
| const canRedo = useMemo(() => { | ||
| const stackLength = undoManager?.redoStack?.length ?? 0; | ||
| return stackLength > 0; | ||
| }, [undoManager?.redoStack?.length]); |
There was a problem hiding this comment.
Please use these in handleWorkflowUndo/Redo in their respective conditional statements as well 🙏🏻
There was a problem hiding this comment.
You mean something like this?
const handleWorkflowUndo = useCallback(() => { const stackLength = undoManager?.undoStack?.length ?? 0; if (stackLength > 0) { undoManager?.undo(); } }, [undoManager]);
billcookie
deleted the
ui/improv-disable-undo-redo-buttons-if-no-actions-available
branch
Overview
Currently the buttons are forever clickable and no ui difference when there is, or isn’t, available undo or redo actions.
What we want is a UI indication that there are no actions available and have it so the user can’t click them either.
What I've done
Track the length of the undoManagement stack to check if it has any history.
What I haven't done
How I tested
Manually
Screenshot
Which point I want you to review particularly
Memo
Summary by CodeRabbit
Release Notes
New Features
canUndoandcanRedoproperties to enhance the functionality of the Toolbox and OverlayUI components, allowing for better control of undo and redo actions.Bug Fixes
Documentation