VPS-96/Allowed renaming of resources in edit view#413
Conversation
leowla
left a comment
leowla
left a comment
There was a problem hiding this comment.
There should be a character limit
|
Warning Review limit reached
Next review available in: 15 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds an authenticated file-rename API endpoint and inline renaming in ChangesFile Renaming Feature
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ManageResourcesPage
participant FilesRoute
participant StoredFile
User->>ManageResourcesPage: Open inline rename editor
ManageResourcesPage->>ManageResourcesPage: Validate and trim new name
ManageResourcesPage->>FilesRoute: PATCH /api/files/:fileId
FilesRoute->>StoredFile: Update and save file metadata
StoredFile-->>FilesRoute: Return updated file
FilesRoute-->>ManageResourcesPage: Return renamed file
ManageResourcesPage->>ManageResourcesPage: Update groups and selectedFile
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
frontend/src/features/resources/ManageResourcesPage.jsx (1)
390-404: 💤 Low valuePrefer the
truncateutility and a focusable control for the file name.Two small refinements on the default file row:
- The inline
overflow/textOverflow/whiteSpacestyles duplicate Tailwind'struncate(already used for the group name on Line 329).- The clickable
<a>has nohref, so it isn't keyboard-focusable/operable. A<button>(or addingrole="button"+tabIndex={0}and key handling) makes selection accessible.♻️ Optional
- <a - className="text--1" - title={f.name} - style={{ overflow: "hidden", textOverflow: "ellipsis", whiteSpace: "nowrap" }} - onClick={() => - setSelectedFile({ - ...f, - groupId: group.id, - groupName: group.name, - }) - } - > - {f.name} - </a> + <button + type="button" + className="text--1 truncate text-left" + title={f.name} + onClick={() => + setSelectedFile({ + ...f, + groupId: group.id, + groupName: group.name, + }) + } + > + {f.name} + </button>🤖 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 `@frontend/src/features/resources/ManageResourcesPage.jsx` around lines 390 - 404, Replace the inline truncation styles on the file name link with the existing truncate utility and make the clickable element keyboard-focusable: remove overflow/textOverflow/whiteSpace inline styles and use the truncate class (e.g., className="truncate text--1") for the element that displays f.name, and change the non-href <a> to a focusable control that calls setSelectedFile (either convert to a <button type="button"> that invokes setSelectedFile({...f, groupId: group.id, groupName: group.name}) or keep the element but add role="button" tabIndex={0} and key handlers for Enter/Space to call setSelectedFile). Ensure the handler calls the same setSelectedFile signature used in the diff.
🤖 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 `@backend/src/routes/api/files.js`:
- Around line 159-161: The if-check using name.trim().length > 255 returns a
JSON error response that exceeds the project's Prettier print width; run
prettier --write on the file or reflow the call so the res.status(400).json(...)
is wrapped across lines (preserving the same message string) to satisfy
formatting. Locate the snippet containing name.trim().length and the
res.status(400).json({ error: "Name must be 255 characters or fewer" }) call and
apply Prettier formatting (or manually break the .json call into multiple lines)
so the linter/formatter CI passes.
- Around line 155-158: Validate that req.body.name is a string before calling
name.trim(): change the guard around "const { name } = req.body" to check typeof
name !== 'string' || !name.trim() and return res.status(400).json({ error: "Name
is required" }) for non-string or empty values so name.trim() cannot throw;
reference the existing "name" variable, the name.trim() call, and the
res.status(...) return to locate and update the check.
- Around line 152-172: The PATCH handler is missing an authorization check and
allows IDOR; after fetching meta via StoredFile.findById(fileId) (and similarly
in DELETE /:fileId and state-conditionals handlers), verify the caller has
access to the file's scenarioId/groupId using the existing authorization helpers
(e.g., call scenarioAuth or dashboardAuth or the same access-check logic used
elsewhere) using the authenticated principal from req (uid or user on req set by
router.use(auth)); if the check fails return 403 and do not modify/save the
StoredFile, otherwise proceed to set meta.name and save. Ensure the same pattern
is applied to delete and other mutation routes that touch StoredFile.
---
Nitpick comments:
In `@frontend/src/features/resources/ManageResourcesPage.jsx`:
- Around line 390-404: Replace the inline truncation styles on the file name
link with the existing truncate utility and make the clickable element
keyboard-focusable: remove overflow/textOverflow/whiteSpace inline styles and
use the truncate class (e.g., className="truncate text--1") for the element that
displays f.name, and change the non-href <a> to a focusable control that calls
setSelectedFile (either convert to a <button type="button"> that invokes
setSelectedFile({...f, groupId: group.id, groupName: group.name}) or keep the
element but add role="button" tabIndex={0} and key handlers for Enter/Space to
call setSelectedFile). Ensure the handler calls the same setSelectedFile
signature used in the diff.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4d4a9819-71a0-46f8-9d55-0cf623aaa679
📒 Files selected for processing (2)
backend/src/routes/api/files.jsfrontend/src/features/resources/ManageResourcesPage.jsx
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)
frontend/src/features/resources/ManageResourcesPage.jsx (1)
163-165:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix stale-state overwrite when rename resolves asynchronously.
Line 163 uses a stale
selectedFilesnapshot afterawait, so selecting another file mid-request can incorrectly overwrite that file’s name in Line 164.Suggested fix
- if (selectedFile?.id === fileId) { - setSelectedFile((prev) => ({ ...prev, name: data.name })); - } + setSelectedFile((prev) => + prev?.id === fileId ? { ...prev, name: data.name } : prev + );🤖 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 `@frontend/src/features/resources/ManageResourcesPage.jsx` around lines 163 - 165, The code reads the stale selectedFile snapshot before an async rename completes and may overwrite a newly selected file; replace the external if (selectedFile?.id === fileId) check with a functional state update that compares prev.id inside setSelectedFile so the comparison is done against the latest state, e.g. call setSelectedFile(prev => prev?.id === fileId ? { ...prev, name: data.name } : prev) so only the currently selected file with matching id is updated after the async response.
🤖 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 `@frontend/src/features/resources/ManageResourcesPage.jsx`:
- Around line 580-582: In the ManageResourcesPage component, update the anchor
element that renders the download link (the <a> with href={downloadUrl}) to
replace the deprecated Tailwind utility class "flex-shrink-0" with the v4
canonical "shrink-0" in its className string so the download button uses the
correct shrinking behavior.
---
Outside diff comments:
In `@frontend/src/features/resources/ManageResourcesPage.jsx`:
- Around line 163-165: The code reads the stale selectedFile snapshot before an
async rename completes and may overwrite a newly selected file; replace the
external if (selectedFile?.id === fileId) check with a functional state update
that compares prev.id inside setSelectedFile so the comparison is done against
the latest state, e.g. call setSelectedFile(prev => prev?.id === fileId ? {
...prev, name: data.name } : prev) so only the currently selected file with
matching id is updated after the async response.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4cee5d3e-a027-4c3a-b0cf-1b1a6d52b3b2
📒 Files selected for processing (2)
backend/src/routes/api/files.jsfrontend/src/features/resources/ManageResourcesPage.jsx
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/src/routes/api/files.js
harbassan
left a comment
harbassan
left a comment
There was a problem hiding this comment.
just this one + the coderabbit stuff which is valid.
…rce-name # Conflicts: # frontend/src/features/resources/ManageResourcesPage.jsx
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/features/resources/ManageResourcesPage.jsx (1)
154-201: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRename failures silently discard the user's edit, and empty input fails without feedback.
renameFileswallows all errors internally and never reports success/failure back to the caller.handleRenameSubmitthen unconditionally callssetRenamingFileId(null)after awaitingrenameFile, so if the PATCH fails (duplicate name, invalid characters, network error, backend validation reject), the inline editor closes anyway and the typed name is lost — the user must reopen the editor and retype from scratch. Separately, whentrimmedis empty, the function silently cancels the edit with no toast/error feedback, unlike every other mutation path in this file which surfaces failures viatoast.error.🛠️ Proposed fix: propagate rename outcome and surface empty-name feedback
async function renameFile(fileId, newName) { try { const user = getAuth().currentUser; if (!user) { toast.error("You must be logged in."); - return; + return false; } const idToken = await user.getIdToken(); const { data } = await axios.patch( `/api/files/${fileId}`, { name: newName }, { headers: { Authorization: `Bearer ${idToken}` } } ); setGroups((prev) => prev.map((g) => ({ ...g, files: (g.files || []).map((f) => f.id === fileId ? { ...f, name: data.name } : f ), })) ); if (selectedFile?.id === fileId) { setSelectedFile((prev) => ({ ...prev, name: data.name })); } toast.success("Renamed"); + return true; } catch (err) { console.error(err); toast.error(err?.response?.data?.error || "Rename failed"); + return false; } } async function handleRenameSubmit() { const trimmed = renameInput.trim(); if (!trimmed) { - setRenamingFileId(null); + toast.error("File name cannot be empty"); return; } const current = groups .flatMap((g) => g.files) .find((f) => f.id === renamingFileId); if (current && trimmed !== current.name) { - await renameFile(renamingFileId, trimmed); + const ok = await renameFile(renamingFileId, trimmed); + if (!ok) return; } setRenamingFileId(null); }🤖 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 `@frontend/src/features/resources/ManageResourcesPage.jsx` around lines 154 - 201, Update renameFile to return a success/failure outcome after handling the PATCH error, and change handleRenameSubmit to clear renamingFileId only when the rename succeeds or no change is needed; keep the editor open after failures so the typed value is preserved. In the empty trimmed-input branch, display a toast.error validation message before returning instead of silently cancelling the edit.
🤖 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.
Outside diff comments:
In `@frontend/src/features/resources/ManageResourcesPage.jsx`:
- Around line 154-201: Update renameFile to return a success/failure outcome
after handling the PATCH error, and change handleRenameSubmit to clear
renamingFileId only when the rename succeeds or no change is needed; keep the
editor open after failures so the typed value is preserved. In the empty
trimmed-input branch, display a toast.error validation message before returning
instead of silently cancelling the edit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 95950db5-54c3-4e6c-90a2-fc2d86d26bdb
📒 Files selected for processing (2)
backend/src/routes/api/files.jsfrontend/src/features/resources/ManageResourcesPage.jsx
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/src/routes/api/files.js
3 participants
Issue
After uploading a resource, there was no way to rename it. Users were stuck with the original filename even if it was unclear or incorrect.
Solution
Added the ability to rename uploaded resources from the collections sidebar in the manage resources view. A pencil icon appears next to each file; clicking it replaces the filename with an inline input field. The user can confirm with Enter or the checkmark button, or cancel with Escape or the X button.
This required a new PATCH /api/files/:fileId endpoint on the backend that validates and updates the name field on StoredFile. The frontend calls this endpoint, then updates both the sidebar list and the active preview panel (if the renamed file is currently selected) without a page reload.
Risk
Summary by CodeRabbit