VPS-96/Allowed renaming of resources in edit view#413
Conversation
📝 WalkthroughWalkthroughAdds a PATCH backend endpoint to update stored file names and implements frontend inline renaming: state, API integration, conditional rename editor UI, and minor layout/styling adjustments. ChangesFile Renaming Feature
Sequence DiagramsequenceDiagram
participant User
participant ManageResourcesPage
participant Backend
User->>ManageResourcesPage: Click rename button / open inline editor
ManageResourcesPage->>ManageResourcesPage: Set renamingFileId and show input
User->>ManageResourcesPage: Enter new name and press Enter or click Save
ManageResourcesPage->>ManageResourcesPage: handleRenameSubmit validates and trims name
ManageResourcesPage->>Backend: PATCH /api/files/:fileId with name payload
Backend->>Backend: Validate, trim, enforce 255-char limit
Backend->>Backend: meta.save() persists updated name
Backend->>ManageResourcesPage: Return updated file object
ManageResourcesPage->>ManageResourcesPage: Update groups and selectedFile and clear renamingFileId
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
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
| router.patch("/:fileId", async (req, res) => { | ||
| try { | ||
| const { fileId } = req.params; | ||
| const { name } = req.body; | ||
| if (!name || !name.trim()) { | ||
| return res.status(400).json({ error: "Name is required" }); | ||
| } | ||
| if (name.trim().length > 255) { | ||
| return res.status(400).json({ error: "Name must be 255 characters or fewer" }); | ||
| } | ||
| const meta = await StoredFile.findById(fileId); | ||
| if (!meta) return res.status(404).json({ error: "File not found" }); | ||
| meta.name = name.trim(); | ||
| await meta.save(); | ||
| const file = meta.toObject(); | ||
| delete file.gridFsId; | ||
| return res.json(file); | ||
| } catch (err) { | ||
| return res.status(500).json({ error: err.message }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for any scenario/file ownership or role checks in the files routes and middleware
rg -nP -C3 '(ownerUid|uploaderUid|scenarioId|authoriz|permission|forbidden|403)' backend/src/routes/api/files.js
fd -t f . backend/src/middleware --exec rg -nP -C3 '(role|owner|permission|scenario|403|forbidden)' {}
# Check how collections routes guard scenario access for comparison
fd -t f 'collections.js' backend/src --exec rg -nP -C3 '(owner|uid|role|permission|403)' {}Repository: UoaWDCC/VPS
Length of output: 5875
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Show the PATCH /:fileId handler and surrounding code in backend/src/routes/api/files.js
sed -n '1,260p' backend/src/routes/api/files.js | nl -ba | sed -n '120,220p'
# 2) Confirm auth middleware wiring in that file
rg -n --hidden "router\.use\(|auth|scenarioAuth|dashboardAuth|permission|role|403|forbidden|owner|uid" backend/src/routes/api/files.js
# 3) Find any authorization helper used for fileId-based routes (rename/delete) in this route file
rg -n --hidden "patch\\(\"/:fileId\"|delete\\(\"/:fileId\"|StoredFile\\.findById\\(|findById\\(fileId\\)|fileId\\)" backend/src/routes/api/files.js
# 4) Check if any middleware elsewhere enforces ownership for StoredFile routes
# (search for StoredFile access-control patterns)
fd -t f . backend/src/middleware backend/src/routes/api backend/src/db/daos backend/src/db/models --max-depth 6 2>/dev/null | head -n 2000 >/tmp/filelist.txt
rg -n --hidden "(StoredFile|gridFsId|uploaderUid|scenarioId|groupId|ownerUid|can[A-Z]|hasAccess|authorize|permission|role)" $(cat /tmp/filelist.txt) || true
# 5) Specifically locate scenario/file ownership checks in middleware and list them
rg -n --hidden "(assert.*Scenario|assert.*Group|scenarioAuth|dashboardAuth|access|ownerId)" backend/src/middleware backend/src/routes/api | head -n 200 || trueRepository: UoaWDCC/VPS
Length of output: 98
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Show backend/src/routes/api/files.js around the PATCH handler using cat -n (nl not available)
sed -n '1,260p' backend/src/routes/api/files.js | cat -n | sed -n '120,220p'
# 2) Confirm auth middleware wiring in that file
rg -n --hidden "router\.use\(|auth|scenarioAuth|dashboardAuth|permission|role|403|forbidden|owner|uid" backend/src/routes/api/files.js || true
# 3) Locate the patch/delete routes and where fileId is used
rg -n --hidden "router\.(patch|delete)\(\"/:fileId\"|StoredFile\.findById|findById\(\s*fileId|uploaderUid|scenarioId|groupId" backend/src/routes/api/files.js || true
# 4) Search for any middleware/utility used by files.js for access control
# (start with scenarioAuth/dashboardAuth and access checks; then also search for StoredFile ownership checks)
fd -t f backend/src/middleware backend/src/routes/api backend/src/db/daos backend/src/db/models --max-depth 6 2>/dev/null > /tmp/vps_files.txt
rg -n --hidden "(scenarioAuth|dashboardAuth|accessList|ownerId|uploaderUid|StoredFile|forbidden|403|permission|role|authorize|hasAccess|assert.*Scenario|assert.*Group)" $(cat /tmp/vps_files.txt) || trueRepository: UoaWDCC/VPS
Length of output: 5413
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show top of backend/src/routes/api/files.js to capture middleware wiring/order
sed -n '1,120p' backend/src/routes/api/files.js | nl -ba | head -n 120
# Inspect firebase auth middleware to see what it guarantees (e.g., req.user fields)
sed -n '1,220p' backend/src/middleware/firebaseAuth.js | cat -n
# Locate StoredFile model to see available fields (scenarioId/groupId/uploaderUid/etc.)
fd -t f 'storedfile*|StoredFile*|file*model*' backend/src/db/models backend/src/db --max-depth 6 2>/dev/null > /tmp/storedfile_models.txt || true
cat /tmp/storedfile_models.txt || true
for f in $(cat /tmp/storedfile_models.txt 2>/dev/null); do echo "---- $f"; sed -n '1,260p' "$f" | cat -n; done
# Find any ownership/permission checks around StoredFile elsewhere
rg -n --hidden "uploaderUid|ownerUid|scenarioId|groupId|access|permission|role|authorize|hasAccess|403|forbidden" backend/src/routes/api backend/src/middleware backend/src/db/daos backend/src/db/models | rg -n "StoredFile|gridFsId|files.js|/files|state-conditionals" || true
# Check if other handlers in this same files.js enforce scenario/group membership
rg -n --hidden "assertGroupInScenario|scenarioId|groupId" backend/src/routes/api/files.jsRepository: UoaWDCC/VPS
Length of output: 98
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Show full middleware wiring at top of files.js (no nl; use cat -n)
sed -n '1,120p' backend/src/routes/api/files.js | cat -n
# 2) Inspect firebase auth middleware
sed -n '1,260p' backend/src/middleware/firebaseAuth.js | cat -n
# 3) Locate and inspect StoredFile model (search by filename first)
fd -t f 'storedfile*|StoredFile*' backend/src/db/models backend/src/db | head -n 50
# If we found candidates, print them; otherwise rg for "name: " fields in StoredFile schema
for f in $(fd -t f 'storedfile*|StoredFile*' backend/src/db/models backend/src/db 2>/dev/null | head -n 20); do
echo "---- $f"
sed -n '1,260p' "$f" | cat -n
done
# 4) Find any scenario/role/ownership checks referenced from files.js (and any middleware that looks like it)
rg -n --hidden "(scenarioAuth|dashboardAuth|permission|role|ownerUid|uploaderUid|accessList|hasAccess|authorize|forbidden|403|assert.*Scenario|assert.*Group)" backend/src/routes/api/files.js backend/src/routes/api backend/src/middleware backend/src/db/daos backend/src/db/models || true
# 5) Confirm how this router is mounted (to see if additional middleware wraps it)
rg -n --hidden "routes/api/files|api/files|require\\(.*files|from '.*files|import .*files\\.js" backend/src | head -n 100 || trueRepository: UoaWDCC/VPS
Length of output: 20512
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect backend/src/index.js around where files router is mounted
sed -n '1,120p' backend/src/index.js | cat -n
rg -n "app\.use\(\"/api/files\"|filesRouter" backend/src/index.js
# Inspect remaining part of backend/src/routes/api/files.js for any ownership checks in other file mutations
rg -n "router\.(patch|delete|post|put)\\(\"/:fileId\"|state-conditionals|scenarioAuth|dashboardAuth|assertGroupInScenario|accessList|getAccessList|403|forbidden" backend/src/routes/api/files.js
sed -n '140,280p' backend/src/routes/api/files.js | cat -n
# Inspect delete/state-conditionals handlers quickly for auth/ownership checks usage
sed -n '170,280p' backend/src/routes/api/files.js | cat -nRepository: UoaWDCC/VPS
Length of output: 10731
Add authorization for file renames (and other file mutations) to prevent IDOR
backend/src/routes/api/files.js only applies router.use(auth) (Firebase token validation) and then PATCH /:fileId does StoredFile.findById(fileId) + updates meta.name without checking the caller’s access to the file’s scenarioId/groupId (same issue in DELETE /:fileId and the state-conditionals routes). Any authenticated user can rename/delete/modify arbitrary files by guessing fileId. Enforce scenario/group access (e.g., via the existing dashboardAuth/scenarioAuth/access checks) before mutating StoredFile.
🤖 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 `@backend/src/routes/api/files.js` around lines 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.
| const { name } = req.body; | ||
| if (!name || !name.trim()) { | ||
| return res.status(400).json({ error: "Name is required" }); | ||
| } |
There was a problem hiding this comment.
Guard against non-string name to avoid a 500.
A truthy non-string name (e.g. { "name": 123 }) passes the !name check, then name.trim() throws a TypeError, which is caught and returned as a generic 500 instead of a 400.
🛡️ Proposed fix
- const { name } = req.body;
- if (!name || !name.trim()) {
+ const { name } = req.body;
+ if (typeof name !== "string" || !name.trim()) {
return res.status(400).json({ error: "Name is required" });
}🤖 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 `@backend/src/routes/api/files.js` around lines 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.
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
| <a | ||
| className="btn btn-phantom btn-xs flex-shrink-0" | ||
| href={downloadUrl} |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Tailwind CSS v4.0.7, is flex-shrink-0still supported, or shouldshrink-0 be used instead?
💡 Result:
In Tailwind CSS v4.0.7, you should use shrink-0 instead of flex-shrink-0. While flex-shrink-0 was previously deprecated and initially removed in the early stages of Tailwind CSS v4 [1], it was subsequently brought back as a legacy utility to maintain backward compatibility [2]. However, shrink-0 is the modern, preferred utility class [3][4]. You are encouraged to use shrink-0 in new code, as flex-shrink-0 is considered a legacy alias [1][2].
Citations:
- 1: https://tailwindcss.com/docs/upgrade-guide
- 2: Bring back some deprecated utilities tailwindlabs/tailwindcss#15069
- 3: https://tailwindcss.com/docs/flex-shrink
- 4: https://kombai.com/tailwind/flex-shrink/
🌐 Web query:
Tailwind CSS v4 flex-shrink-0alias forshrink-0 documentation
💡 Result:
In Tailwind CSS v4, the documentation for the flex-shrink utilities uses shrink-* as the canonical class names. Specifically, v4 documents the utility as shrink-0 (not flex-shrink-0) to set flex-shrink: 0 [1]. flex-shrink-0 is a v3 legacy class name that maps to the v4 alternative shrink-0 during the v4 migration/upgrade process [2][3].
Citations:
- 1: https://tailwindcss.com/docs/flex-shrink
- 2: Migrate legacy classes to the v4 alternative tailwindlabs/tailwindcss#14643
- 3: https://tailwindcss.com/docs/upgrade-guide
Use Tailwind v4 shrink-0 instead of flex-shrink-0
In frontend/src/features/resources/ManageResourcesPage.jsx (line ~581), change flex-shrink-0 to shrink-0 since Tailwind v4 documents shrink-* as the canonical utility for flex-shrink.
🤖 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 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.
harbassan
left a comment
There was a problem hiding this comment.
just this one + the coderabbit stuff which is valid.
| if (!user) { | ||
| toast.error("You must be logged in."); | ||
| return; | ||
| } |
There was a problem hiding this comment.
this check is unecessary, given that this is on a protected route, so we can make the assumption that they are logged in.

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