Fix: handle undefined binding.size to prevent NaN -fixes #262 - #266
Fix: handle undefined binding.size to prevent NaN -fixes #262#266shriya-gatate wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthrough
ChangesFile Browser Size Handling and SPARQL Formatting
Estimated code review effort: 1 (Trivial) | ~2 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 1
🤖 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 `@public/js/components/file-browser/file-browser.js`:
- Line 145: The null fallback in file-browser.js only protects the totalSize
addition, but binding.size is still dereferenced earlier in the same flow.
Update the size parsing logic in the file-browser binding handling so the
null/undefined check happens before accessing binding.size.value or
binding.size.numericalValue, store the parsed size once, and reuse that parsed
value when updating ctrl.totalSize.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c2e1f91e-596d-4286-aa9b-8dd99213f55b
📒 Files selected for processing (1)
public/js/components/file-browser/file-browser.js
There was a problem hiding this comment.
♻️ Duplicate comments (1)
public/js/components/file-browser/file-browser.js (1)
135-135: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winGuard still doesn't prevent the crash on missing
binding.size.The ternary at Line 135 only decides the value assigned; the assignment target
binding.size.numericalValue = ...still unconditionally dereferencesbinding.size. Whenbinding.sizeisundefined, this throwsTypeError: Cannot set properties of undefined, which is swallowed by thecatchblock inquerySparql, aborting the loop before Line 145's fallback ever runs. This is the same root cause raised in the previous review round — the fix needs to guard the property write itself, not just the computed value.🐛 Proposed fix
for (var b in ctrl.queryResult.bindings) { var binding = ctrl.queryResult.bindings[b]; - binding.size.numericalValue = binding.size? parseInt(binding.size.value):0; + var numericalSize = binding.size ? parseInt(binding.size.value, 10) : 0; + if (binding.size) { + binding.size.numericalValue = numericalSize; + } ctrl.queryResult.uriList += binding.file.value + "\n"; if (binding.variant != undefined) { binding.variant.value = ctrl.formatVariant(binding.variant.value); } - ctrl.totalSize += binding.size?binding.size.numericalValue:0; + ctrl.totalSize += numericalSize; ctrl.numFiles++; }Also applies to: 145-145
🤖 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 `@public/js/components/file-browser/file-browser.js` at line 135, The `querySparql` loop in `file-browser.js` still crashes when `binding.size` is missing because the assignment to `binding.size.numericalValue` dereferences `binding.size` before the ternary runs. Update the size-handling logic around `binding.size` so the property write itself is guarded, and only set `numericalValue` when `binding.size` exists; otherwise leave it untouched so the fallback path later in the same loop can execute.
🤖 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.
Duplicate comments:
In `@public/js/components/file-browser/file-browser.js`:
- Line 135: The `querySparql` loop in `file-browser.js` still crashes when
`binding.size` is missing because the assignment to
`binding.size.numericalValue` dereferences `binding.size` before the ternary
runs. Update the size-handling logic around `binding.size` so the property write
itself is guarded, and only set `numericalValue` when `binding.size` exists;
otherwise leave it untouched so the fallback path later in the same loop can
execute.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 48877964-861b-4b22-a023-e85709757658
📒 Files selected for processing (1)
public/js/components/file-browser/file-browser.js
shriya-gatate
left a comment
shriya-gatate
left a comment
There was a problem hiding this comment.
Addressed CodeRabbit's feedback — moved the null check to a local variable
(sizeValue) to avoid dereferencing binding.size before the guard runs.
Line 145 now reuses sizeValue directly.
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)
public/js/components/file-browser/file-browser.js (1)
135-145: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve
binding.size.numericalValuefor the UI.This removes the assignment that populates
binding.size.numericalValue, butpublic/js/components/file-browser/file-browser.htmlstill renders and sorts by that property. Keep the local fallback for aggregation and assign the parsed value whenbinding.sizeexists.Suggested fix
- const sizeValue = binding.size? parseInt(binding.size.value):0; + const sizeValue = binding.size ? parseInt(binding.size.value, 10) : 0; + if (binding.size) { + binding.size.numericalValue = sizeValue; + }🤖 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 `@public/js/components/file-browser/file-browser.js` around lines 135 - 145, Restore assignment of the parsed size to binding.size.numericalValue in the file-browser result processing flow, while retaining sizeValue’s fallback of 0 for total aggregation. Guard the assignment so it only runs when binding.size exists, preserving the UI rendering and sorting contract.
🤖 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 `@public/js/components/file-browser/file-browser.js`:
- Around line 135-145: Restore assignment of the parsed size to
binding.size.numericalValue in the file-browser result processing flow, while
retaining sizeValue’s fallback of 0 for total aggregation. Guard the assignment
so it only runs when binding.size exists, preserving the UI rendering and
sorting contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9c3a5231-ecab-452c-960b-aa6eedaec736
📒 Files selected for processing (2)
public/js/components/file-browser/file-browser.jsserver/app/common/queries/sparql/get-versions-by-account.sparql
f1ed90d to
6cdf54a
Compare
1 participant
##Problem
When binding.size is undefined, totalSize becomes NaN, showing wrong file count and size in the UI.
##Fix
Added a null check to default to 0 if binding.size is undefined.
Fixes #262
Summary by CodeRabbit