fix: enable renaming of servers and remote devices in project tree#581
Merged
Conversation
The rename functionality was missing for servers and remote devices. When attempting to rename these items via the context menu, users received the error "Only POU or datatype files can be renamed". Changes: - Add rename methods to serverActions and remoteDeviceActions in shared slice - Update handleRenameFile in project-tree to handle servers and remote devices - Update error messages to reflect the new supported item types - Fix saveFile error for servers/remote devices by marking workspace as unsaved instead (they are saved with project.json, not as separate files) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
WalkthroughThe changes extend rename functionality to servers and remote devices in the project tree. The component layer integrates rename actions into the tree UI, while the store layer implements validation logic, state updates, and error handling for rename workflows, alongside refactoring delete operations to mark workspaces as unsaved. Changes
Sequence DiagramsequenceDiagram
participant User
participant ProjectTree as ProjectTree<br/>(Component)
participant Store as Store<br/>(Shared Slice)
participant State as State<br/>(Projects)
User->>ProjectTree: Initiates rename<br/>(contextmenu action)
ProjectTree->>ProjectTree: Validate leaf type<br/>(server/remoteDevice)
ProjectTree->>Store: Call renameServer/<br/>renameRemoteDevice
Store->>Store: Validate non-empty name
Store->>Store: Check for name collisions
Store->>Store: Verify item exists
alt Validation Success
Store->>State: Update item name
Store->>State: Update tab/editor names
Store->>State: Update selected leaf<br/>if current item
Store->>State: Mark workspace<br/>as 'unsaved'
Store->>ProjectTree: Return success
ProjectTree->>User: Confirm rename
else Validation Failure
Store->>Store: Show error toast
Store->>ProjectTree: Return error
ProjectTree->>User: Show error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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 |
![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: 1
🤖 Fix all issues with AI agents
In `@src/renderer/store/slices/shared/index.ts`:
- Around line 933-997: The rename handler currently ignores the return value of
getState().projectActions.updateServerName(serverName, newServerName); capture
its ProjectResponse (e.g., const resp =
await/getState()?.projectActions.updateServerName(...)) and check resp.ok before
proceeding; if resp.ok is false, show a failure toast and return
Promise.resolve({ success: false, error: { title: 'Error renaming server',
description: resp.message || 'Rename failed' } }); only call
getState().tabsActions.updateTabName, getState().editorActions.updateEditorName,
workspaceActions.setSelectedProjectTreeLeaf and
workspaceActions.setEditingState('unsaved') when resp.ok is true so the UI/state
stays consistent.
🧹 Nitpick comments (1)
src/renderer/components/_molecules/project-tree/index.tsx (1)
461-485: Consider conditionally hiding the Duplicate option for servers and remote devices.The popover menu shows the Duplicate option for all leaf types, including servers and remote devices. However, clicking Duplicate on these items triggers an error toast since
handleDuplicateFileonly supports POUs and datatypes. This could be confusing for users.💡 Suggested approach to conditionally filter options
const popoverOptions = useMemo(() => { - return [ + const options = [ { name: 'Rename', onClick: () => { setIsEditing(true) }, icon: <PencilIcon className='h-4 w-4 stroke-brand dark:stroke-brand-light' />, }, - { + ] + if (isAPou || isDatatype) { + options.push({ name: 'Duplicate', onClick: () => { void handleDuplicateFile() }, icon: <DuplicateIcon className='h-4 w-4 stroke-brand dark:stroke-brand-light' />, - }, - { + }) + } + options.push({ name: 'Delete', onClick: () => { handleDeleteFile() }, icon: <CloseIcon className='h-4 w-4 stroke-brand dark:stroke-brand-light' />, - }, - ] - }, [handleDeleteFile, handleDuplicateFile, setIsEditing]) + }) + return options + }, [handleDeleteFile, handleDuplicateFile, setIsEditing, isAPou, isDatatype])
Comment on lines
+933
to
997
| rename: (serverName, newServerName) => { | ||
| if (serverName === newServerName) { | ||
| return Promise.resolve({ success: false }) | ||
| } | ||
|
|
||
| if (!(newServerName ?? '').trim()) { | ||
| toast({ title: 'Error', description: 'New name must be non-empty.', variant: 'fail' }) | ||
| return Promise.resolve({ | ||
| success: false, | ||
| error: { | ||
| title: 'Error renaming server', | ||
| description: 'New name must be non-empty.', | ||
| }, | ||
| }) | ||
| } | ||
|
|
||
| const servers = getState().project.data.servers | ||
| const isServerNameAlreadyUsed = servers?.some((s) => s.name === newServerName) | ||
| if (isServerNameAlreadyUsed) { | ||
| toast({ | ||
| title: 'Error renaming server', | ||
| description: `Server with name ${newServerName} already exists.`, | ||
| variant: 'fail', | ||
| }) | ||
| return Promise.resolve({ | ||
| success: false, | ||
| error: { | ||
| title: 'Error renaming server', | ||
| description: `Server with name ${newServerName} already exists.`, | ||
| }, | ||
| }) | ||
| } | ||
|
|
||
| const server = servers?.find((s) => s.name === serverName) | ||
| if (!server) { | ||
| toast({ | ||
| title: 'Error', | ||
| description: `Server with name ${serverName} not found.`, | ||
| variant: 'fail', | ||
| }) | ||
| return Promise.resolve({ | ||
| success: false, | ||
| error: { | ||
| title: 'Error renaming server', | ||
| description: `Server with name ${serverName} not found.`, | ||
| }, | ||
| }) | ||
| } | ||
|
|
||
| getState().projectActions.updateServerName(serverName, newServerName) | ||
| getState().tabsActions.updateTabName(serverName, newServerName) | ||
| getState().editorActions.updateEditorName(serverName, newServerName) | ||
|
|
||
| const selectedProjectTreeLeaf = getState().workspace.selectedProjectTreeLeaf | ||
| if (selectedProjectTreeLeaf.label === serverName) { | ||
| getState().workspaceActions.setSelectedProjectTreeLeaf({ | ||
| label: newServerName, | ||
| type: selectedProjectTreeLeaf.type, | ||
| }) | ||
| } | ||
|
|
||
| // Mark workspace as unsaved - servers are saved with project.json | ||
| getState().workspaceActions.setEditingState('unsaved') | ||
| return Promise.resolve({ success: true }) | ||
| }, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's find where projectActions is defined
echo "=== Searching for updateServerName definition ==="
rg -n 'updateServerName' --type=ts src/renderer/store/
echo -e "\n=== Searching for projectActions definition ==="
rg -n 'projectActions' --type=ts src/renderer/store/ | head -20Repository: Autonomy-Logic/openplc-editor
Length of output: 2702
🏁 Script executed:
#!/bin/bash
# Let's examine the actual implementation of updateServerName
echo "=== Implementation of updateServerName in slice.ts ==="
sed -n '1252,1270p' src/renderer/store/slices/project/slice.ts
echo -e "\n=== Type definition from types.ts ==="
sed -n '325p' src/renderer/store/slices/project/types.ts
echo -e "\n=== Context: checking what projectResponseSchema is ==="
rg -n 'projectResponseSchema' src/renderer/store/slices/project/types.ts | head -5Repository: Autonomy-Logic/openplc-editor
Length of output: 1555
🏁 Script executed:
#!/bin/bash
# Check the complete projectResponseSchema definition
echo "=== projectResponseSchema definition ==="
sed -n '144,155p' src/renderer/store/slices/project/types.ts
# Check if rename function handles the response from updateServerName
echo -e "\n=== Checking if updateServerName response is handled ==="
sed -n '975,997p' src/renderer/store/slices/shared/index.ts
# Check the return type structure
echo -e "\n=== Check other similar rename implementations ==="
rg -A 25 'pouActions\.rename.*=.*\(' src/renderer/store/slices/shared/index.ts | head -40Repository: Autonomy-Logic/openplc-editor
Length of output: 1450
🏁 Script executed:
#!/bin/bash
# Let's look for pouActions.rename to see how it handles updatePouName response
echo "=== pouActions.rename implementation ==="
sed -n '475,510p' src/renderer/store/slices/shared/index.ts
echo -e "\n=== datatypeActions.rename implementation ==="
sed -n '800,835p' src/renderer/store/slices/shared/index.tsRepository: Autonomy-Logic/openplc-editor
Length of output: 2663
🏁 Script executed:
#!/bin/bash
# Let's check the complete updateServerName implementation to see all error cases
echo "=== Full updateServerName implementation ==="
sed -n '1252,1280p' src/renderer/store/slices/project/slice.ts
echo -e "\n=== Are response errors checked in rename function? ==="
sed -n '980,997p' src/renderer/store/slices/shared/index.tsRepository: Autonomy-Logic/openplc-editor
Length of output: 2074
Check the return value of updateServerName and handle failures properly.
The rename function calls getState().projectActions.updateServerName(serverName, newServerName) but ignores its return value. The method returns a ProjectResponse with ok: boolean and can fail with messages like 'Server not found' or 'Name already exists'. When updateServerName fails, the function continues anyway, updating tabs, editor, and workspace state, then returns { success: true }. This creates inconsistent state and misrepresents failure as success to the caller.
Store the result and check response.ok before proceeding with subsequent state updates, similar to how pouActions.rename verifies the renamed POU exists in state after the update.
🤖 Prompt for AI Agents
In `@src/renderer/store/slices/shared/index.ts` around lines 933 - 997, The rename
handler currently ignores the return value of
getState().projectActions.updateServerName(serverName, newServerName); capture
its ProjectResponse (e.g., const resp =
await/getState()?.projectActions.updateServerName(...)) and check resp.ok before
proceeding; if resp.ok is false, show a failure toast and return
Promise.resolve({ success: false, error: { title: 'Error renaming server',
description: resp.message || 'Rename failed' } }); only call
getState().tabsActions.updateTabName, getState().editorActions.updateEditorName,
workspaceActions.setSelectedProjectTreeLeaf and
workspaceActions.setEditingState('unsaved') when resp.ok is true so the UI/state
stays consistent.
There was a problem hiding this comment.
Pull request overview
This PR enables renaming of servers and remote devices in the project tree, addressing errors that previously occurred when attempting to rename or delete these entities.
Changes:
- Added
renamemethods toserverActionsandremoteDeviceActionswith validation for duplicate names and empty inputs - Updated project tree component to support renaming servers and remote devices
- Fixed save errors by marking workspace as unsaved instead of calling
saveFile()for servers/remote devices (they're stored inproject.json)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/renderer/store/slices/shared/index.ts | Added rename methods for servers and remote devices with validation logic, and updated delete methods to mark workspace as unsaved |
| src/renderer/components/_molecules/project-tree/index.tsx | Updated rename handler to support servers and remote devices, and improved error messages |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
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
Changes
renamemethods toserverActionsandremoteDeviceActionsin the shared slicehandleRenameFilein project-tree component to handle servers and remote devicessaveFile()(servers/remote devices are stored inproject.json, not as separate files in the file slice)Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes