fix: adjust props and function to format filename on upload input#786
fix: adjust props and function to format filename on upload input#786
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughCentralizes media-value logic by adding Changes
Sequence Diagram(s)(Skipped — changes are deletions/utility addition and do not introduce new multi-component sequential flows.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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
🤖 Fix all issues with AI agents
In `@src/utils/methods.js`:
- Around line 536-571: In getMediaInputValue, guard against undefined fileUrl
and names without extensions: first derive fileUrl safely (fallback to empty
string or use img.filename||img.file_path||img.file_url || ""), check existence
before calling includes/split, and compute basename robustly (use lastSegment =
fileUrl.includes("/") ? ... only if fileUrl; otherwise use an empty string or
img.name). When splitting the filename, detect if there is no dot and treat the
whole name as nameWithoutExtension with ext as an empty string (or a default
like "bin") so filename construction doesn't produce ".name"; ensure final
filename assembly handles empty ext (e.g., omit ".") and that the timestamp
concat is applied to a string (use String(filename) + "?t=" + Date.now()).
Update references inside getMediaInputValue (variables fileUrl, filename, parts,
ext, nameWithoutExtension) accordingly.
| import FormRepeater from "../form-repeater"; | ||
| import InventoryItemMetaFieldForm from "./inventory-item-meta-field-form"; | ||
| import { scrollToError, shallowEqual, hasErrors } from "../../utils/methods"; | ||
| import { |
There was a problem hiding this comment.
@tomrndom this is dead code please remove the file. No file in the codebase imports it. Inventory item editing is handled entirely by SponsorInventoryDialog
| @@ -22,7 +22,12 @@ import TextEditorV3 from "openstack-uicore-foundation/lib/components/inputs/edit | |||
| import Swal from "sweetalert2"; | |||
There was a problem hiding this comment.
src/utils/methods.js
Outdated
| return mediaFiles.map((img) => { | ||
| const fileUrl = img.filename ?? img.file_path ?? img.file_url; | ||
|
|
||
| let filename = fileUrl.includes("/") ? fileUrl.split("/").pop() : fileUrl; |
There was a problem hiding this comment.
@tomrndom If img.filename, img.file_path, and img.file_url are all undefined/null, then fileUrl is undefined and fileUrl.includes("/") throws TypeError.
add a guard ( something like):
const fileUrl = img.filename ?? img.file_path ?? img.file_url;
if (!fileUrl) return { ...img, filename: "" };
add an unit test for this
src/utils/methods.js
Outdated
|
|
||
| let filename = fileUrl.includes("/") ? fileUrl.split("/").pop() : fileUrl; | ||
|
|
||
| const parts = filename.split("."); |
There was a problem hiding this comment.
@tomrndom
Files without extensions break
A filename like "README" (no dot) produces parts = ["README"], then ext = "README" and nameWithoutExtension = "". The result becomes ".README".
- Handle the no-extension case
- add proper unit test
| name="image" | ||
| onUploadComplete={handleImageUploadComplete} | ||
| value={getMediaInputValue()} | ||
| value={getMediaInputValue(initialEntity)} |
There was a problem hiding this comment.
@tomrndom
Double truncation with UploadInputV2
UploadInputV2 already does its own truncation internally (https://github.com/OpenStackweb/openstack-uicore-foundation/blob/master/src/components/inputs/upload-input-v2/index.js):
const FileNameMaxLen = 20;
// ...
if (path.length > FileNameMaxLen) {
path = path.substring(0, FileNameMaxLen);
}
So filenames get truncated twice , first by getMediaInputValue (middle-ellipsis) then again by UploadInputV2 (prefix). The component should be the only one responsible for display truncation.
So for a filename like "my-very-long-image-filename-for-sponsor.png":
- Original -> my-very-long-image-filename-for-sponsor.png
- After getMediaInputValue -> my-very-...sponsor.png?t=1709400000000
- UploadInputV2 splits ext -> ext = png?t=1709400000000
- UploadInputV2 truncates path -> my-very-...sponsor (already ≤20, no-op)
- Displayed -> my-very-...sponsor.png?t=1709400000000
The cache-bust query string leaks into the visible filename displayed to the user. This was already happening with the old code (pre-PR), but the double truncation + ?t= makes the display worse
src/utils/methods.js
Outdated
| return { | ||
| ...img, | ||
| public_url: img?.public_url || fileUrl, | ||
| filename: filename.concat("?t=", Date.now()) |
There was a problem hiding this comment.
@tomrndom
t= cache-bust leaks into displayed filename
The ?t=Date.now() is appended to the filename property, but UploadInputV2 uses v.filename for:
- The visible filename shown to the user (displayed as-is after the component's own split/truncation)
- Extension extraction via filename.split('.').pop() . which yields "png?t=1709400000000" instead of "png"
The cache-bust should go on public_url (the actual image URL used for and ), not on filename. This way the browser fetches the fresh image, but the displayed filename stays
clean.
Suggested shape for the return value:
return {
...img,
public_url: (img?.public_url || fileUrl) + "?t=" + Date.now(),
filename: fileUrl.includes("/") ? fileUrl.split("/").pop() : fileUrl
};
This keeps filename clean for display and for the handleRemoveImage filter comparison (i.filename !== imageFile.name), and lets UploadInputV2 handle its own display truncation. The cache-bust on
public_url is where it belongs , on the URL the browser actually fetches.
please do add jest unit test for diplay name without ?t= and for handleRemoveImage logic
src/utils/methods.js
Outdated
| let nameWithoutExtension = parts.join("."); | ||
|
|
||
| if (nameWithoutExtension.length > maxLength) { | ||
| const startChars = Math.floor(maxLength * FILENAME_TRUNCATE_SIDE_PERCENT); |
There was a problem hiding this comment.
@tomrndom please remove all this logic
let UploadInputV2 handle it
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
tomrndom
force-pushed
the
fix/image-upload-link-filename
branch
from
ebbb962 to
3b3446a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/forms/inventory-item-form.js`:
- Line 323: getMediaInputValue(entity) is being recomputed every render (it
appends Date.now() in its output), causing UploadInputV2 value churn; memoize
the computed media value by entity.images so unrelated form edits via
handleChange don't trigger new values. Replace the direct call in the value prop
with a memoized value (useMemo or equivalent) computed from
getMediaInputValue(entity) with entity.images as the dependency, ensuring only
image add/remove operations change the memo and that UploadInputV2 continues to
receive the stable computed value.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/components/forms/form-template-form.jssrc/components/forms/inventory-item-form.jssrc/pages/sponsors-global/form-templates/sponsor-inventory-popup.jssrc/utils/constants.jssrc/utils/methods.js
🚧 Files skipped from review as they are similar to previous changes (4)
- src/pages/sponsors-global/form-templates/sponsor-inventory-popup.js
- src/components/forms/form-template-form.js
- src/utils/constants.js
- src/utils/methods.js
| id="image" | ||
| onUploadComplete={handleImageUploadComplete} | ||
| value={getMediaInputValue()} | ||
| value={getMediaInputValue(entity)} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 1) Inspect getMediaInputValue output behavior ==="
rg -n -C4 'export const getMediaInputValue|filename: filename.concat|public_url' src/utils/methods.js
echo
echo "=== 2) Inspect handleRemoveImage payload expectations ==="
rg -n -C3 'const handleRemoveImage|imageFile\.name|imageFile\.filename' src/components/forms/inventory-item-form.js
echo
echo "=== 3) Find UploadInputV2 usages and onRemove handlers ==="
rg -n -C3 'UploadInputV2|onRemove=\{|handleRemoveImage' src/componentsRepository: fntechgit/summit-admin
Length of output: 11940
🏁 Script executed:
# Check if entity state updates consistently create new images array reference
rg -n 'entity\.images.*=' src/components/forms/inventory-item-form.js | head -20
# Check if the useMemo dependency [entity?.images] is safe by verifying images immutability pattern
rg -n 'setEntity.*images' src/components/forms/inventory-item-form.js | head -10Repository: fntechgit/summit-admin
Length of output: 48
🏁 Script executed:
# Read inventory-item-form.js to understand entity state management and handleChange
wc -l src/components/forms/inventory-item-form.js
head -120 src/components/forms/inventory-item-form.js | tail -50
# Check the actual state initialization and handleChange
sed -n '1,100p' src/components/forms/inventory-item-form.js | grep -A15 'useState\|handleChange\|const entity'Repository: fntechgit/summit-admin
Length of output: 3049
Memoize media value to prevent timestamp churn on every render.
On line 323, getMediaInputValue(entity) is recomputed each render with Date.now() appended to filenames (line 569 of methods.js), making the value prop unstable during unrelated form edits. This causes unnecessary UploadInputV2 re-renders and potential list churn behavior.
The fix is to memoize by entity.images since only upload/remove operations mutate the images array—unrelated field changes via handleChange do not.
Proposed fix (memoize by images changes)
-import React, { useState, useEffect, useRef } from "react";
+import React, { useState, useEffect, useRef, useMemo } from "react";
@@
const [entity, setEntity] = useState({ ...initialEntity });
const [errors, setErrors] = useState(initialErrors);
+ const mediaInputValue = useMemo(
+ () => getMediaInputValue(entity),
+ [entity?.images]
+ );
@@
- value={getMediaInputValue(entity)}
+ value={mediaInputValue}📝 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.
| value={getMediaInputValue(entity)} | |
| import React, { useState, useEffect, useRef, useMemo } from "react"; | |
| // ... other code ... | |
| const [entity, setEntity] = useState({ ...initialEntity }); | |
| const [errors, setErrors] = useState(initialErrors); | |
| const mediaInputValue = useMemo( | |
| () => getMediaInputValue(entity), | |
| [entity?.images] | |
| ); | |
| // ... other code ... | |
| value={mediaInputValue} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/forms/inventory-item-form.js` at line 323,
getMediaInputValue(entity) is being recomputed every render (it appends
Date.now() in its output), causing UploadInputV2 value churn; memoize the
computed media value by entity.images so unrelated form edits via handleChange
don't trigger new values. Replace the direct call in the value prop with a
memoized value (useMemo or equivalent) computed from getMediaInputValue(entity)
with entity.images as the dependency, ensuring only image add/remove operations
change the memo and that UploadInputV2 continues to receive the stable computed
value.
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils/methods.js`:
- Around line 544-545: The public_url construction and filename extraction can
produce malformed URLs and leak query/hash fragments; update the public_url
logic to append the cache-busting timestamp using the correct separator (use '&'
if img?.public_url or fileUrl already contains '?', otherwise use '?') when
building public_url (`public_url: ...`) and ensure filename (`filename: ...`) is
the sanitized basename by stripping any query string or fragment (split the
basename on /[?#]/ and take the first part) after extracting the final path
segment from img?.public_url or fileUrl; reference the existing symbols img,
public_url, fileUrl, and filename to locate and modify the code.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/components/forms/inventory-item-form.jssrc/layouts/page-template-layout.jssrc/pages/sponsors-global/page-templates/edit-page-template-page.jssrc/utils/__tests__/methods.test.jssrc/utils/methods.js
💤 Files with no reviewable changes (3)
- src/pages/sponsors-global/page-templates/edit-page-template-page.js
- src/layouts/page-template-layout.js
- src/components/forms/inventory-item-form.js
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: sebastian marcet <smarcet@gmail.com>
2 participants
ref: https://app.clickup.com/t/86b883mp7
Signed-off-by: Tomás Castillo tcastilloboireau@gmail.com
Summary by CodeRabbit
New Features
Bug Fixes
Removed
Tests