refactor: collections handling for cleaner and more extendable resources system#448
Conversation
📝 WalkthroughWalkthroughCollection groups are removed in favor of collection-type ChangesResource hierarchy backend
Frontend resource workflows
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
harbassan
changed the title
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
backend/src/db/models/resource.js (1)
23-27: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winEnforce
fileIdrequirement conditionally.Since
fileIdis now optional to accommodate collections, consider adding a custom validator to ensure it remains strictly required for"file"type resources.♻️ Proposed refactor
fileId: { type: Schema.Types.ObjectId, ref: "UploadedFile", - required: false, + required: function () { + return this.type === "file"; + }, },🤖 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/db/models/resource.js` around lines 23 - 27, Update the resource schema’s fileId validation to require a value when the resource type is "file" while allowing it to remain absent for collection resources. Add the conditional validator alongside fileId and reference the existing resource type field rather than changing the general optional schema setting.frontend/src/features/playScenario/components/ResourcesPanel.jsx (1)
62-65: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
stateVariablesin the query key causes redundant refetches.
queryFnuses onlyuser/scenarioId, yetstateVariablesis part of the key, so every state-variable change refetches/api/resources/:scenarioIdeven though the response doesn't depend on it. Since filtering is client-side, drop it from the key.♻️ Proposed change
const { data, isLoading, isError, error } = useQuery({ - queryKey: ["resources", scenarioId, stateVariables], + queryKey: ["resources", scenarioId], queryFn: () => getResources(user, scenarioId), });🤖 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/playScenario/components/ResourcesPanel.jsx` around lines 62 - 65, Update the useQuery configuration in ResourcesPanel by removing stateVariables from the queryKey, leaving only the resource-fetch inputs used by getResources: user and scenarioId. Preserve the existing client-side filtering behavior.
🤖 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/resources.js`:
- Around line 58-62: Update the parent validation in the resource creation flow
around Resource.exists to require the parent resource’s type to be a collection,
preventing files from being used as parents. Preserve the existing not-found
response while changing its message from “group not found” to
collection-appropriate resource terminology.
- Around line 129-138: Update the resource deletion logic in the collection/else
branches to skip children without a fileId before converting the identifier,
preventing null access. Normalize all fileRefDeltas keys to strings, including
the non-collection resource.fileId path, so the Map uses a consistent key type.
In `@frontend/src/components/StateVariables/CreateStateConditional.jsx`:
- Around line 56-71: Move the useMutation call identified as
createConditionalMutation in
frontend/src/components/StateVariables/CreateStateConditional.jsx#L37-L71 above
all stateVariables guards, while preserving its callbacks and behavior. Apply
the same hoisting to the mutation in
frontend/src/components/StateVariables/EditStateConditional.jsx#L48-L100,
ensuring both components invoke their mutations unconditionally before any early
return based on asynchronously loaded stateVariables.
In `@frontend/src/components/StateVariables/EditStateConditional.jsx`:
- Around line 96-99: Update the onError handler for updateConditionalMutation in
EditStateConditional.jsx so its toast message refers to updating or editing the
state conditional, not deleting it. Leave the delete mutation’s error message
unchanged.
In `@frontend/src/features/playScenario/components/ResourcesPanel.jsx`:
- Around line 70-87: Update the filteredTree computation in ResourcesPanel so
filterTreeByConditions is always applied before the search/no-search branch,
including when search is empty. Pass each group’s files property expected by
filterTreeByConditions rather than children, and preserve the tree’s established
shape when applying search filtering so both default and searched views retain
conditionally visible groups and files.
In `@frontend/src/features/resources/ManageResourcesPage.jsx`:
- Around line 109-132: Update the temporary resource object created in
addResourceCollectionMutation.onMutate to include type: "collection", matching
the criterion used by buildResourceTree and the existing file-upload mutation
pattern. Preserve the current optimistic insertion and rollback behavior.
In `@frontend/src/features/resources/ResourcePreview.jsx`:
- Around line 12-16: Update the text loading condition in ResourcePreview to use
text.isInitialLoading, or gate text.isLoading with the same enabled predicate
used by the file-text query, so disabled queries do not render the text
skeleton.
In `@frontend/src/features/resources/util.js`:
- Line 9: Update the resource object field used by ResourceTree’s file badge
from type to contentType, so shortType renders the actual file format instead of
the generic resource kind. Preserve the rest of normaliseFile’s mapping
unchanged.
---
Nitpick comments:
In `@backend/src/db/models/resource.js`:
- Around line 23-27: Update the resource schema’s fileId validation to require a
value when the resource type is "file" while allowing it to remain absent for
collection resources. Add the conditional validator alongside fileId and
reference the existing resource type field rather than changing the general
optional schema setting.
In `@frontend/src/features/playScenario/components/ResourcesPanel.jsx`:
- Around line 62-65: Update the useQuery configuration in ResourcesPanel by
removing stateVariables from the queryKey, leaving only the resource-fetch
inputs used by getResources: user and scenarioId. Preserve the existing
client-side filtering behavior.
🪄 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: 0a6e7734-ff7b-4690-94d6-a8e58aefe1b4
📒 Files selected for processing (17)
backend/src/db/models/CollectionGroup.jsbackend/src/db/models/resource.jsbackend/src/routes/api/__tests__/collectionsApi.test.jsbackend/src/routes/api/__tests__/resourcesApi.test.jsbackend/src/routes/api/collections.jsbackend/src/routes/api/index.jsbackend/src/routes/api/resources.jsfrontend/src/components/StateVariables/CreateStateConditional.jsxfrontend/src/components/StateVariables/EditStateConditional.jsxfrontend/src/components/StateVariables/StateConditionalMenu.jsxfrontend/src/features/playScenario/components/ResourcePreview.jsxfrontend/src/features/playScenario/components/ResourceTree.jsxfrontend/src/features/playScenario/components/ResourcesPanel.jsxfrontend/src/features/resources/ManageResourcesPage.jsxfrontend/src/features/resources/ResourcePreview.jsxfrontend/src/features/resources/ResourcesSkeleton.jsxfrontend/src/features/resources/util.js
💤 Files with no reviewable changes (5)
- backend/src/db/models/CollectionGroup.js
- frontend/src/features/playScenario/components/ResourcePreview.jsx
- backend/src/routes/api/collections.js
- backend/src/routes/api/tests/collectionsApi.test.js
- backend/src/routes/api/index.js
| if (parentId) { | ||
| const parent = await Resource.exists({ _id: parentId, scenarioId }); | ||
| if (!parent) | ||
| throw new HttpError("group not found", HttpStatusCode.NotFound); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Verify that the parent resource is a collection.
The current lookup checks if the parent exists but does not verify its type. This allows clients to pass a file's ID as the parentId, creating a nested file structure that breaks deletion cascades and tree rendering logic. Additionally, the error message should be updated to reflect the new resource model instead of the old "group" terminology.
🐛 Proposed fix
if (parentId) {
- const parent = await Resource.exists({ _id: parentId, scenarioId });
+ const parent = await Resource.exists({ _id: parentId, scenarioId, type: "collection" });
if (!parent)
- throw new HttpError("group not found", HttpStatusCode.NotFound);
+ throw new HttpError("parent collection not found", HttpStatusCode.NotFound);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (parentId) { | |
| const parent = await Resource.exists({ _id: parentId, scenarioId }); | |
| if (!parent) | |
| throw new HttpError("group not found", HttpStatusCode.NotFound); | |
| } | |
| if (parentId) { | |
| const parent = await Resource.exists({ _id: parentId, scenarioId, type: "collection" }); | |
| if (!parent) | |
| throw new HttpError("parent collection not found", HttpStatusCode.NotFound); | |
| } |
🤖 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/resources.js` around lines 58 - 62, Update the parent
validation in the resource creation flow around Resource.exists to require the
parent resource’s type to be a collection, preventing files from being used as
parents. Preserve the existing not-found response while changing its message
from “group not found” to collection-appropriate resource terminology.
| if (resource.type === "collection") { | ||
| const children = await Resource.find({ parentId: resource._id }); | ||
| for (const child of children) { | ||
| const fileId = child.fileId.toString(); | ||
| fileRefDeltas.set(fileId, (fileRefDeltas.get(fileId) ?? 0) - 1); | ||
| } | ||
| await Resource.deleteMany({ parentId: resource._id }); | ||
| } else { | ||
| fileRefDeltas.set(resource.fileId, -1); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Prevent NPE on missing fileId and normalize map keys.
If a child resource does not have a fileId (which is now explicitly allowed by the schema), calling child.fileId.toString() will throw a TypeError and cause the deletion to fail with a 500 error.
Additionally, in the else branch, resource.fileId is passed as an ObjectId, mixing object references and strings as keys in the fileRefDeltas Map.
🐛 Proposed fix
if (resource.type === "collection") {
const children = await Resource.find({ parentId: resource._id });
for (const child of children) {
- const fileId = child.fileId.toString();
- fileRefDeltas.set(fileId, (fileRefDeltas.get(fileId) ?? 0) - 1);
+ if (child.fileId) {
+ const fileId = child.fileId.toString();
+ fileRefDeltas.set(fileId, (fileRefDeltas.get(fileId) ?? 0) - 1);
+ }
}
await Resource.deleteMany({ parentId: resource._id });
} else {
- fileRefDeltas.set(resource.fileId, -1);
+ if (resource.fileId) {
+ fileRefDeltas.set(resource.fileId.toString(), -1);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (resource.type === "collection") { | |
| const children = await Resource.find({ parentId: resource._id }); | |
| for (const child of children) { | |
| const fileId = child.fileId.toString(); | |
| fileRefDeltas.set(fileId, (fileRefDeltas.get(fileId) ?? 0) - 1); | |
| } | |
| await Resource.deleteMany({ parentId: resource._id }); | |
| } else { | |
| fileRefDeltas.set(resource.fileId, -1); | |
| } | |
| if (resource.type === "collection") { | |
| const children = await Resource.find({ parentId: resource._id }); | |
| for (const child of children) { | |
| if (child.fileId) { | |
| const fileId = child.fileId.toString(); | |
| fileRefDeltas.set(fileId, (fileRefDeltas.get(fileId) ?? 0) - 1); | |
| } | |
| } | |
| await Resource.deleteMany({ parentId: resource._id }); | |
| } else { | |
| if (resource.fileId) { | |
| fileRefDeltas.set(resource.fileId.toString(), -1); | |
| } | |
| } |
🤖 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/resources.js` around lines 129 - 138, Update the
resource deletion logic in the collection/else branches to skip children without
a fileId before converting the identifier, preventing null access. Normalize all
fileRefDeltas keys to strings, including the non-collection resource.fileId
path, so the Map uses a consistent key type.
| const createConditionalMutation = useMutation({ | ||
| mutationFn: (conditional) => | ||
| createStateConditional(user, scenarioId, resource._id, conditional), | ||
| onSettled: () => queryClient.invalidateQueries(["resources", scenarioId]), | ||
| onError: (e) => { | ||
| console.error(e); | ||
| toast.error("Error creating state conditional"); | ||
| }, | ||
| onSuccess: () => { | ||
| setSelectedState(null); | ||
| setComparator(null); | ||
| setValue(null); | ||
| setOpen(false); | ||
| toast.success("State conditional created!"); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -e
printf '== CreateStateConditional.jsx outline ==\n'
ast-grep outline frontend/src/components/StateVariables/CreateStateConditional.jsx --view expanded || true
printf '\n== EditStateConditional.jsx outline ==\n'
ast-grep outline frontend/src/components/StateVariables/EditStateConditional.jsx --view expanded || true
printf '\n== CreateStateConditional.jsx relevant lines ==\n'
nl -ba frontend/src/components/StateVariables/CreateStateConditional.jsx | sed -n '1,140p'
printf '\n== EditStateConditional.jsx relevant lines ==\n'
nl -ba frontend/src/components/StateVariables/EditStateConditional.jsx | sed -n '1,180p'Repository: UoaWDCC/VPS
Length of output: 895
🏁 Script executed:
set -e
printf '== Find ScenarioContext ==\n'
rg -n "ScenarioContext|stateVariables" frontend/src -g '!**/node_modules/**' | sed -n '1,220p'
printf '\n== Context/provider files ==\n'
fd -a "ScenarioContext" frontend/src || true
fd -a "Scenario.*Context" frontend/src || trueRepository: UoaWDCC/VPS
Length of output: 13929
🏁 Script executed:
set -e
for f in frontend/src/components/StateVariables/CreateStateConditional.jsx frontend/src/components/StateVariables/EditStateConditional.jsx; do
echo "== $f =="
nl -ba "$f" | sed -n '1,140p'
echo
doneRepository: UoaWDCC/VPS
Length of output: 365
🏁 Script executed:
set -e
for f in frontend/src/components/StateVariables/CreateStateConditional.jsx frontend/src/components/StateVariables/EditStateConditional.jsx frontend/src/context/ScenarioContextProvider.jsx; do
echo "== $f =="
awk '{printf "%4d %s\n", NR, $0}' "$f" | sed -n '1,180p'
echo
doneRepository: UoaWDCC/VPS
Length of output: 17390
Hoist the mutations above the stateVariables guards. ScenarioContextProvider fills stateVariables asynchronously, so these components can skip useMutation on one render and call it on the next. That changes hook order and can crash the subtree.
frontend/src/components/StateVariables/CreateStateConditional.jsx#L37-L71frontend/src/components/StateVariables/EditStateConditional.jsx#L48-L100
📍 Affects 2 files
frontend/src/components/StateVariables/CreateStateConditional.jsx#L56-L71(this comment)frontend/src/components/StateVariables/EditStateConditional.jsx#L52-L100
🤖 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/components/StateVariables/CreateStateConditional.jsx` around
lines 56 - 71, Move the useMutation call identified as createConditionalMutation
in frontend/src/components/StateVariables/CreateStateConditional.jsx#L37-L71
above all stateVariables guards, while preserving its callbacks and behavior.
Apply the same hoisting to the mutation in
frontend/src/components/StateVariables/EditStateConditional.jsx#L48-L100,
ensuring both components invoke their mutations unconditionally before any early
return based on asynchronously loaded stateVariables.
| onError: (e) => { | ||
| console.error(e); | ||
| toast.error("Error deleting state conditional"); | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Wrong toast message on the update mutation.
updateConditionalMutation's onError reports "Error deleting state conditional" — copy-paste from the delete mutation. It should reference updating/editing.
✏️ Fix
onError: (e) => {
console.error(e);
- toast.error("Error deleting state conditional");
+ toast.error("Error updating state conditional");
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onError: (e) => { | |
| console.error(e); | |
| toast.error("Error deleting state conditional"); | |
| }, | |
| onError: (e) => { | |
| console.error(e); | |
| toast.error("Error updating state conditional"); | |
| }, |
🤖 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/components/StateVariables/EditStateConditional.jsx` around lines
96 - 99, Update the onError handler for updateConditionalMutation in
EditStateConditional.jsx so its toast message refers to updating or editing the
state conditional, not deleting it. Leave the delete mutation’s error message
unchanged.
| const filteredTree = (() => { | ||
| if (!search.trim()) return resourceTree; | ||
| const q = search.trim().toLowerCase(); | ||
| return tree | ||
|
|
||
| // NOTE: the filtering by state variables should ideally be done on the server to prevent cheating, but here we filter before rendering | ||
|
|
||
| return filterTreeByConditions(resourceTree, stateVariables) | ||
| .map((g) => { | ||
| const matchingFiles = (g.files || []).filter((f) => { | ||
| const matchingFiles = (g.children || []).filter((f) => { | ||
| const inName = f.name.toLowerCase().includes(q); | ||
| const inPath = g.name.toLowerCase().includes(q); | ||
| return inName || inPath; | ||
| }); | ||
| if (matchingFiles.length === 0) return null; | ||
| return { ...g, files: matchingFiles }; | ||
| return { ...g, children: matchingFiles }; | ||
| }) | ||
| .filter(Boolean); | ||
| }, [tree, search]); | ||
| })(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C3 '\b(files|children)\b' frontend/src/utils/stateConditionalEvaluator.jsRepository: UoaWDCC/VPS
Length of output: 766
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant files with line numbers.
for f in frontend/src/features/playScenario/components/ResourcesPanel.jsx frontend/src/utils/stateConditionalEvaluator.js; do
echo "===== $f ====="
wc -l "$f"
cat -n "$f" | sed -n '1,220p'
echo
done
# Find where the resource tree is built to see whether it uses `children` or `files`.
rg -n "buildResourceTree|resourceTree|children:|files:" frontend/src -g '!**/node_modules/**'Repository: UoaWDCC/VPS
Length of output: 14654
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Narrowly inspect any builder/helper that produces the tree shape.
rg -n -C3 "buildResourceTree|resourceTree" frontend/src -g '!**/node_modules/**'Repository: UoaWDCC/VPS
Length of output: 6862
Apply conditionals before the search split. The empty-search branch returns resourceTree directly, so state conditionals are skipped in the default view. The search branch also passes children into filterTreeByConditions, which expects files, so it drops every group and makes search return nothing. Keep the tree shape consistent and filter conditionals unconditionally.
🤖 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/playScenario/components/ResourcesPanel.jsx` around
lines 70 - 87, Update the filteredTree computation in ResourcesPanel so
filterTreeByConditions is always applied before the search/no-search branch,
including when search is empty. Pass each group’s files property expected by
filterTreeByConditions rather than children, and preserve the tree’s established
shape when applying search filtering so both default and searched views retain
conditionally visible groups and files.
| const addResourceCollectionMutation = useMutation({ | ||
| mutationFn: (name) => createResourceCollection(user, scenarioId, name), | ||
| onMutate: async (name) => { | ||
| await queryClient.cancelQueries(["resources", scenarioId]); | ||
| const tempId = `temp.${uuid()}`; | ||
| const temp = { name, _id: tempId }; | ||
| queryClient.setQueryData(["resources", scenarioId], (prev) => [ | ||
| ...prev, | ||
| temp, | ||
| ]); | ||
| return { tempId }; | ||
| }, | ||
| onSuccess: () => queryClient.invalidateQueries(["resources", scenarioId]), | ||
| onError: (e, _, context) => { | ||
| const tempId = context?.tempId; | ||
| if (tempId) { | ||
| queryClient.setQueryData(["resources", scenarioId], (prev) => | ||
| prev.filter((r) => r._id !== tempId) | ||
| ); | ||
| } | ||
| const idToken = await user.getIdToken(); | ||
| await axios.delete(`/api/collections/groups/${groupId}`, { | ||
| headers: { Authorization: `Bearer ${idToken}` }, | ||
| }); | ||
|
|
||
| setGroups((prev) => prev.filter((g) => g.id !== groupId)); | ||
|
|
||
| if (selectedFile && selectedFile.groupId === groupId) | ||
| setSelectedFile(null); | ||
| if (selectedGroup?.id === groupId) setSelectedGroup(null); | ||
|
|
||
| toast.success("Group deleted"); | ||
| } catch (err) { | ||
| console.error(err); | ||
| toast.error(err?.response?.data?.error || "Failed to delete group"); | ||
| } | ||
| } | ||
| console.error(e); | ||
| toast.error("Something went wrong creating the collection"); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Optimistic collection is dropped by buildResourceTree (missing type).
The temp entry is { name, _id: tempId }, but buildResourceTree (Line 46) keeps only r.type === "collection". The optimistic collection therefore never renders — the new group only appears after onSuccess invalidation refetches. The file-upload mutation correctly sets type: "file"; mirror that here.
🐛 Proposed fix
const tempId = `temp.${uuid()}`;
- const temp = { name, _id: tempId };
+ const temp = { name, _id: tempId, type: "collection", children: [] };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const addResourceCollectionMutation = useMutation({ | |
| mutationFn: (name) => createResourceCollection(user, scenarioId, name), | |
| onMutate: async (name) => { | |
| await queryClient.cancelQueries(["resources", scenarioId]); | |
| const tempId = `temp.${uuid()}`; | |
| const temp = { name, _id: tempId }; | |
| queryClient.setQueryData(["resources", scenarioId], (prev) => [ | |
| ...prev, | |
| temp, | |
| ]); | |
| return { tempId }; | |
| }, | |
| onSuccess: () => queryClient.invalidateQueries(["resources", scenarioId]), | |
| onError: (e, _, context) => { | |
| const tempId = context?.tempId; | |
| if (tempId) { | |
| queryClient.setQueryData(["resources", scenarioId], (prev) => | |
| prev.filter((r) => r._id !== tempId) | |
| ); | |
| } | |
| const idToken = await user.getIdToken(); | |
| await axios.delete(`/api/collections/groups/${groupId}`, { | |
| headers: { Authorization: `Bearer ${idToken}` }, | |
| }); | |
| setGroups((prev) => prev.filter((g) => g.id !== groupId)); | |
| if (selectedFile && selectedFile.groupId === groupId) | |
| setSelectedFile(null); | |
| if (selectedGroup?.id === groupId) setSelectedGroup(null); | |
| toast.success("Group deleted"); | |
| } catch (err) { | |
| console.error(err); | |
| toast.error(err?.response?.data?.error || "Failed to delete group"); | |
| } | |
| } | |
| console.error(e); | |
| toast.error("Something went wrong creating the collection"); | |
| }, | |
| }); | |
| const addResourceCollectionMutation = useMutation({ | |
| mutationFn: (name) => createResourceCollection(user, scenarioId, name), | |
| onMutate: async (name) => { | |
| await queryClient.cancelQueries(["resources", scenarioId]); | |
| const tempId = `temp.${uuid()}`; | |
| const temp = { name, _id: tempId, type: "collection", children: [] }; | |
| queryClient.setQueryData(["resources", scenarioId], (prev) => [ | |
| ...prev, | |
| temp, | |
| ]); | |
| return { tempId }; | |
| }, | |
| onSuccess: () => queryClient.invalidateQueries(["resources", scenarioId]), | |
| onError: (e, _, context) => { | |
| const tempId = context?.tempId; | |
| if (tempId) { | |
| queryClient.setQueryData(["resources", scenarioId], (prev) => | |
| prev.filter((r) => r._id !== tempId) | |
| ); | |
| } | |
| console.error(e); | |
| toast.error("Something went wrong creating the collection"); | |
| }, | |
| }); |
🤖 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 109 -
132, Update the temporary resource object created in
addResourceCollectionMutation.onMutate to include type: "collection", matching
the criterion used by buildResourceTree and the existing file-upload mutation
pattern. Preserve the current optimistic insertion and rollback behavior.
| const text = useQuery({ | ||
| queryKey: ["file-text", file?.url], | ||
| queryFn: () => loadText(file.url), | ||
| enabled: !!(file?.contentType?.startsWith("text") && file?.url), | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
@tanstack/react-query v4 isLoading true for disabled query isInitialLoading
💡 Result:
In TanStack Query v4, the isLoading state is true for disabled queries that have no cached data [1][2][3]. This is an intentional change resulting from the removal of the idle state in v4 [2][4]. In previous versions, a disabled query without data would be in an idle status [2][3]. In v4, status now represents whether data exists ('loading', 'success', or 'error') [5][4], while fetchStatus represents whether the function is running ('fetching', 'paused', or 'idle') [5]. Because a disabled query has no data, it defaults to the loading status, even though it is not actively fetching [1][2][4]. To correctly identify when to show a loading spinner—specifically when the query is both in a loading state and actually fetching data—you should use the isInitialLoading flag [1][2][6]. The isInitialLoading flag is a derived property computed as: isInitialLoading = isLoading && isFetching [1][6] This ensures that the flag is only true when the query is currently in its initial fetch [1][6]. During migration from v3 to v4, replacing isLoading with isInitialLoading is the recommended way to maintain previous loading spinner behavior for disabled or lazy queries [2][7].
Citations:
- 1: https://tanstack.com/query/v4/docs/framework/react/guides/disabling-queries
- 2: https://tanstack.com/query/v4/docs/framework/react/guides/migrating-to-react-query-4
- 3: Breaking change: isLoading now true for disabled queries TanStack/query#4056
- 4: useQuery isLoading shouldn't be true unless the enabled condition is matched TanStack/query#3925
- 5: https://tanstack.com/query/v4/docs/framework/react/guides/queries
- 6: https://tanstack.com/query/v4/docs/framework/react/reference/useQuery
- 7: https://tanstack.com/query/latest/docs/framework/react/guides/migrating-to-react-query-4
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files frontend/src/features/resources/ResourcePreview.jsx
wc -l frontend/src/features/resources/ResourcePreview.jsx
nl -ba frontend/src/features/resources/ResourcePreview.jsx | sed -n '1,220p'Repository: UoaWDCC/VPS
Length of output: 296
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n frontend/src/features/resources/ResourcePreview.jsx | sed -n '1,220p'Repository: UoaWDCC/VPS
Length of output: 2972
Use isInitialLoading for the text skeleton
isText can be true while this query is disabled (for example, application/json), and text.isLoading stays true for a disabled query in React Query v4. Use text.isInitialLoading here, or gate the branch on the same enabled condition.
🤖 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/ResourcePreview.jsx` around lines 12 - 16,
Update the text loading condition in ResourcePreview to use
text.isInitialLoading, or gate text.isLoading with the same enabled predicate
used by the file-text query, so disabled queries do not render the text
skeleton.
| size: resource.fileId.size, | ||
| fileId: resource.fileId?._id, | ||
| url: resource.fileId?.url, | ||
| type: resource.type, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C2 'shortType\(' frontend/src/features/playScenario/components/ResourceTree.jsxRepository: UoaWDCC/VPS
Length of output: 547
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== util.js ==\n'
sed -n '1,120p' frontend/src/features/resources/util.js
printf '\n== ResourceTree.jsx ==\n'
sed -n '1,140p' frontend/src/features/playScenario/components/ResourceTree.jsx
printf '\n== search for resource.type/contentType/fileType assignments ==\n'
rg -n --hidden -S 'contentType|fileType|resource\.type|type:\s*resource\.type|shortType\(' frontend/src/features -g '!**/node_modules/**'Repository: UoaWDCC/VPS
Length of output: 4769
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('frontend/src/features/resources/util.js')
print(p.exists())
PYRepository: UoaWDCC/VPS
Length of output: 154
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== ResourceTree usages ==\n'
rg -n --hidden -S 'ResourceTree' frontend/src -g '!**/node_modules/**'
printf '\n== nearby tree shaping code in playScenario/resources ==\n'
rg -n --hidden -S 'normaliseFile\(|children\s*:|tree\s*=' frontend/src/features -g '!**/node_modules/**'
printf '\n== ManageResourcesPage relevant section ==\n'
sed -n '160,240p' frontend/src/features/resources/ManageResourcesPage.jsxRepository: UoaWDCC/VPS
Length of output: 5715
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for p in [
Path('frontend/src/features/resources/ManageResourcesPage.jsx'),
Path('frontend/src/features/playScenario/components/ResourceTree.jsx'),
]:
print(p, p.exists())
PYRepository: UoaWDCC/VPS
Length of output: 278
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,140p' frontend/src/features/playScenario/components/ResourcesPanel.jsxRepository: UoaWDCC/VPS
Length of output: 4404
Use contentType for the file badge
normaliseFile() sets type to the resource kind, so ResourceTree’s {shortType(f.type)} will render file for every file row. Switch this to contentType.
🤖 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/util.js` at line 9, Update the resource
object field used by ResourceTree’s file badge from type to contentType, so
shortType renders the actual file format instead of the generic resource kind.
Preserve the rest of normaliseFile’s mapping unchanged.
1 participant
Issue
A clear and concise description of what the issue is
Solution
A clear and concise description of what the solution is
Risk
Potential risks that this PR might bring
Checklist
Summary by CodeRabbit