feat: add option to rename a project's folder from the Scenes view - #1357
Conversation
Test this pull request on windows-latestDownload the correct version for your architecture: |
Test this pull request on macos-latestDownload the correct version for your architecture:Click here if you don't know which version to downloadFor running this unsigned version of the app, you will need to run the xattr command on it:
|
Signed-off-by: Nicolas Earnshaw <earnshaw.nico@gmail.com>
decentraland-bot
left a comment
There was a problem hiding this comment.
Code Review: PR #1357 — feat: add option to rename a project's folder from the Scenes view
Overall Assessment
Clean, well-structured PR that follows established codebase patterns closely. The feature spans all layers (shared → preload → renderer) correctly, the validation is thorough, and test coverage is solid (happy path, collision, invalid name, no-op, and config update all tested). No P0 or P1 issues found — this looks good to merge.
Findings
[P2] Submit button enabled when input is empty
renderer/src/components/Modals/RenameFolder/component.tsx:36, 68
When the user clears the input field:
trimmedName = ""isInvalid = false(short-circuited bytrimmedName.length > 0)isUnchanged = false(empty ≠ currentName)- Button
disabled = false || false || false→ enabled
Clicking Submit sends an empty name to the preload layer, which rejects it, and the modal shows "Couldn't rename the folder. A folder with that name may already exist" — a misleading message for an empty name.
Suggested fix — add an isEmpty guard to the disabled condition:
const isEmpty = trimmedName.length === 0;
// ...
disabled={loading || isEmpty || isUnchanged || isInvalid}[P2] Case-only rename blocked on case-insensitive filesystems (macOS/Windows)
preload/src/modules/workspace.ts:402-406
Renaming "My Scene" → "my scene":
newPath !== _path(case-sensitive string comparison) → proceedsfs.exists(newPath)→true(HFS+/NTFS are case-insensitive)- Throws: "A folder named 'my scene' already exists"
The user cannot change only the case of a folder name. A two-step rename via a temp name would fix this, but it's a non-trivial edge case — fine to defer to a follow-up.
[P2] Windows trailing-dot normalization mismatch
shared/utils.ts:27-35
Names ending with . (e.g. "My Scene.") pass isValidFolderName but Windows silently strips trailing dots, creating a mismatch between the stored path and the actual filesystem path. Consider rejecting trailing dots/spaces after trimming:
if (/[. ]$/.test(trimmed)) return false;[P2] No rollback if config update fails after rename
preload/src/modules/workspace.ts:410-414
If fs.rename succeeds but config.setConfig throws (IPC failure, disk write error), the folder is moved on disk but config.workspace.paths still references the old path — the project appears "missing" on next load. This is consistent with existing patterns (duplicateProject, createProject all have the same non-transactional shape), so not blocking, but a rollback catch would make this more robust.
[P2] Parameter shadowing in config callback
preload/src/modules/workspace.ts:412
config.setConfig(config => { ... }) — the callback parameter config shadows the outer config service variable. Renaming to draft would prevent accidental misuse:
await config.setConfig(draft => {
draft.workspace.paths = draft.workspace.paths.map($ => ($ === _path ? newPath : $));
});[P2] Branch name / PR title type mismatch
Branch is fix/791-rename-project-folder but PR title uses feat:. Per ADR-6, the branch type should match the commit type. Since this adds a new feature, feat: is correct — the branch name is the mismatch. Cosmetic since squash-merge uses the PR title.
[P2] Module-level vi.mock('node:fs/promises') broadens test scope
preload/tests/modules/workspace.spec.ts
The new module-level mock affects all tests in the file, not just the renameProject suite. The comments explain the rationale (FileSystemStorage uses node:fs/promises directly), but this could mask future issues in other test suites. Consider scoping the mock to the describe('renameProject', ...) block if vitest supports it.
Security Review
No security issues found:
- Path traversal: Well-defended —
/,\,.., null bytes, and control characters all blocked byisValidFolderName. Unicode lookalikes do not resolve to path separators at the OS level. - Injection: No shell commands spawned with user input. Errors use translated strings, not raw input. React auto-escapes in JSX.
- Secrets: None found.
- TOCTOU race: Between
fs.existsandfs.rename— negligible risk in a single-user desktop app.
CI Status
All checks passing: lint ✅ typecheck ✅ unit tests ✅ E2E tests ✅ builds (macOS + Windows) ✅
Reviewed by Jarvis 🤖 · Requested by Gabriel Díaz (<@U03MGHMAJL8>) via Slack
| </Button> | ||
| <Button | ||
| onClick={handleSubmit} | ||
| disabled={loading || isUnchanged || isInvalid} |
There was a problem hiding this comment.
[P2] The submit button is enabled when the input is empty. When trimmedName is "", isInvalid is false (short-circuited by trimmedName.length > 0) and isUnchanged is false (empty ≠ currentName), so none of the disabled conditions fire.
The backend catches it gracefully, but the user sees a misleading error ("A folder with that name may already exist") instead of the button simply being disabled.
Suggested fix:
disabled={loading || !trimmedName || isUnchanged || isInvalid}There was a problem hiding this comment.
Fixed in 7f6a1f0 — added an isEmpty guard to both the disabled condition and handleSubmit.
|
|
||
| await fs.rename(_path, newPath); | ||
|
|
||
| await config.setConfig(config => { |
There was a problem hiding this comment.
[P2] Minor: config parameter shadows the outer config service variable. Consider renaming to draft for clarity:
await config.setConfig(draft => {
draft.workspace.paths = draft.workspace.paths.map($ => ($ === _path ? newPath : $));
});This matches what the parameter actually represents (an Immer draft) and prevents accidental misuse in future edits.
There was a problem hiding this comment.
Fixed in 7f6a1f0 — renamed the callback parameter to draft.
| if (trimmed === '.' || trimmed === '..') return false; | ||
| if (trimmed.length > MAX_FOLDER_NAME_LENGTH) return false; | ||
| if (ILLEGAL_FILENAME_CHARS.test(trimmed)) return false; | ||
| if (RESERVED_WINDOWS_NAMES.test(trimmed)) return false; |
There was a problem hiding this comment.
[P2] On Windows, trailing dots and spaces in folder names are silently stripped by NTFS (e.g. "My Scene." → "My Scene"). This creates a mismatch between the path stored in config and the actual directory. Consider adding:
if (/[. ]$/.test(trimmed)) return false;after the existing checks.
There was a problem hiding this comment.
Fixed in 7f6a1f0 — isValidFolderName now rejects names ending with a dot (trailing spaces are already removed by the trim()), with tests.
nicoecheza
left a comment
There was a problem hiding this comment.
The architecture is right (rename lives in preload beside duplicateProject/deleteProject, no unnecessary main IPC, preload independently re-validates, traversal genuinely blocked) and the validation/config-remap tests are solid. But three things need fixing before merge:
1. Every rename failure is invisible — the modal's inline error UI is unreachable. renameProject.pending sets state.status = 'loading' (workspace/slice.ts), and ScenesPage swaps the entire SceneList — including the open modal — for a full-page loader while any workspace thunk is loading. So the modal unmounts mid-await, and when the preload rejects (collision, invalid name, EBUSY/EPERM on Windows with the folder open) the catch's setError fires on an unmounted component: modal vanishes, spinner flashes, no error anywhere. This contradicts the PR's "surfaced as an inline error" claim. The fix has in-repo precedent one case above: duplicateProject.pending is deliberately a no-op for exactly this reason — make renameProject.pending a no-op too (the modal already has its own loading state).
2. Case-only renames are always blocked on macOS/Windows. newPath === _path is case-sensitive, so "my scene" → "My Scene" proceeds to the collision check — where fs.exists(newPath) resolves case-insensitively on APFS/NTFS and matches the project's own folder → "already exists". Since re-syncing the folder name with the display title is the feature's stated purpose, casing fixes are a primary use case. Skip the collision check when the paths differ only by case (fs.rename handles case-only renames fine on both platforms), and add a test for it.
3. es.json/zh.json were not updated — only en.json has the rename_folder keys (verified), and getKeys(locale) has no merge-with-en fallback, so Spanish/Chinese users see raw key ids in the dropdown and modal.
Smaller points, non-blocking:
- Empty/whitespace name leaves Confirm enabled (
isInvalidis only computed whentrimmedName.length > 0), contradicting the PR's "empty → submit disabled" claim; the resulting preload throw is then invisible per #1. - The single generic error message conflates collision vs
EBUSY/EPERMvs other failures; the preload already throws distinct messages — surface them (at least the collision case). isValidFolderNameallows trailing dots ("Foo."), which Win32 strips at the API level — recreating exactly the name/desync this feature exists to fix. AlsoCOM0/LPT0are over-blocked (harmless) and length is counted in UTF-16 code units rather than bytes.- If
fs.renamesucceeds but the config write or finalgetProjectthrows, the store keeps the old path while disk has the new one until the next workspace refresh — consider tolerating/refreshing on that partial failure.
- Make renameProject.pending a no-op so the rename modal survives the await and its inline error UI stays reachable (same pattern as duplicateProject) - Allow case-only renames on case-insensitive filesystems by skipping the collision check when paths differ only by case - Add missing rename_folder translation keys to es.json and zh.json - Disable Confirm when the name is empty/whitespace - Surface a distinct error message for folder-name collisions - Reject folder names ending with a dot (Windows strips them silently) - Undo the rename if the config write fails, keeping disk and config consistent - Rename shadowed config callback param to draft Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed the review feedback in 7f6a1f0: Blocking items
Smaller points
Not changed
All unit tests, typecheck, lint, and prettier pass locally. 🤖 Generated with Claude Code |
Test @dcl/inspector package
|
DafGreco
left a comment
There was a problem hiding this comment.
✔️ PR reviewed and approved by QA on both platforms following instructions playing both happy and un-happy path
Regressions for this ticket had been performed in order to verify that the normal flow is working as expected:
Summary
scene.json) is intentionally left untouched — this only fixes the folder-name side of the desync described in the issue.config.workspace.pathsentry accordingly.Plan
Full implementation plan (root cause analysis, file references, edge cases)
title: Option to rename folder
type: feat
date: 2026-07-06
Option to rename folder
When a project's display name (
scene.json'sdisplay.title) is changed after creation, thefolder that holds the project's files on disk keeps its original name. This desyncs the folder
name from the project name and confuses users. Issue #791 asks for a targeted, simple fix: add a
"Rename folder" option to the three-dot menu of each project card in the Scenes view (the grid of
projects shown when no project is open), letting the user rename the on-disk folder directly.
This does not require handling the "project is currently open" case, since the Scenes view only
ever lists projects that are not open (opening a project navigates to
/editor).Root Cause Analysis
packages/creator-hub/preload/src/modules/workspace.tshas no folder rename/move primitive.duplicateProjectcopies (fs.cp) into a freshly generated available path, anddeleteProjectunlists +
fs.rms — neither is a rename.packages/creator-hub/preload/src/services/fs.tswraps a small subset offs/promises(cp,rm,mkdir,stat, ...) but has norename.shared/types/projects.tsProject.path)and the persisted workspace config (
shared/types/config.tsConfig.workspace.paths: string[])is keyed by the on-disk path, so any rename must update both, not just move the folder.
Location, View Deployments, and Delete.
validateScenesPath/isProjectPathAvailable, which check writability and path collision but notillegal characters.
Institutional Learnings
fsaccess; main process only handles a smaller set of Electron/OS-levelIPC. No new
mainIPC channel is needed — implemented entirely inpreload/src/modules/workspace.tslike
duplicateProject/deleteProjectalready are..editor/(per-project metadata, incl.project.jsonwith the projectid) lives inside theproject folder, so a plain
fs.renameof the directory carries it along automatically — nospecial-casing needed to preserve project identity/settings across the rename.
extraReducersmatchprojects by
pathand replace the whole object onfulfilled.User Flows & Edge Cases
Happy path: three-dot menu → "Rename Folder" → modal pre-filled with current folder name →
type new name → client-side validation passes → confirm → folder renamed on disk,
config.workspace.pathsupdated, Reduxprojectslist reflects the new path.Edge cases handled:
avoided).
/ \ : * ? " < > |, control chars), reserved Windows device names(
CON,PRN,AUX,NUL,COM1-9,LPT1-9), or names over 255 chars → rejected byisValidFolderName, inline error shown.fs.exists(newPath)before renaming and throws; surfaced as an inline error.
fs.renamethrows(ENOENT); thunk's
rejectedcase setsstate.error, same as existing thunks.EXDEV) is not a concern here since the new path is always a sibling of theexisting one (same parent directory, only the last path segment changes).
limitation shared with
duplicateProject(which also changespath); out of scope for Option to rename folder #791,noted here for future follow-up.
lists non-open projects (opening navigates to
/editor) — matches the issue's own scoping.Proposed Changes
isValidFolderName(name)andgetBaseName(path)toshared/utils.ts(pure, importablefrom both preload and renderer — renderer has no Node built-ins).
rename()topreload/src/services/fs.ts, wrappingfs.promises.rename.renameProject({ path, newName })topreload/src/modules/workspace.ts: validates the newname, computes the sibling path, checks for collisions, calls
fs.rename, updatesconfig.workspace.paths(old path → new path), returns the refreshedProject.renameProjectthunk +extraReducerscases in the workspace Redux slice, and arenameProjectwrapper inuseWorkspace().RenameFoldermodal (renderer/src/components/Modals/RenameFolder), modeled onCreateProject(validated input, inline error, disabled submit) andDeleteProject(project-scoped modal props).
renderer/src/components/SceneList/Projects/component.tsx.scene_list.project_actions.rename_folderandmodal.rename_folder.*in
en.json.Testing
Ran from a clean
npm installof the monorepo:cd packages/creator-hub && npm run test:unit(main + preload + renderer + shared) — 176/176 tests passing, including new coverage forisValidFolderName,getBaseName, andrenameProject(happy path, name-collision rejection, invalid-name rejection, unchanged-name no-op, andconfig.workspace.pathsupdate).npm run typecheck(root, all three sub-projects) — no errors.npm run lint(root) — no errors. Also explicitly linted the two changed.tsxfiles, since the rootlintscript's--ext js,cjs,tsdoesn't include.tsx— no errors.npm run format(root, Prettier check) — no errors.make build/make build-creator-hub— this sandbox can't complete Electron's postinstall binary download, so a full Electron build couldn't be exercised here. Worth a CI check before merge.Closes
Closes #791
🤖 Created via Slack with Claude
Requested by Nico Earnshaw via Slack