feat: show admin figma-ux correlation fixes#831
feat: show admin figma-ux correlation fixes#831priscila-moneo wants to merge 3 commits intomasterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUpdated the Changes
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)
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 |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/mui/editable-table/mui-table-editable.module.less (1)
4-13:⚠️ Potential issue | 🟡 Minor
align-self: centerhas no effect on absolutely positioned elements.The
::beforepseudo-element isposition: absolute, soalign-selfwon't center it vertically. The border will render starting fromtop: 0with 60% height rather than being vertically centered.To center the dotted border vertically:
🔧 Proposed fix
&::before { content: ""; position: absolute; - top: 0; - bottom: 0; + top: 50%; left: 0; border-left: 1px dashed `#e0e0e0`; height: 60%; - align-self: center; + transform: translateY(-50%); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/mui/editable-table/mui-table-editable.module.less` around lines 4 - 13, The ::before pseudo-element in the &::before rule is absolutely positioned so align-self: center is ignored; update the &::before styles (the pseudo-element selector) to vertically center the 60% height dashed border by removing the conflicting top: 0 and bottom: 0 and instead use top: 50% plus transform: translateY(-50%) (keep height: 60% and border-left as-is) so the dashed line is centered vertically within the container.
🧹 Nitpick comments (2)
src/i18n/en.json (1)
2669-2669: Minor key-value semantic mismatch.The key
imagesuses plural form but the value is now singular"Image File". This works functionally but could cause confusion when maintaining the codebase. Consider renaming the key toimage_fileorimageif it refers to a single image upload section, or keep as-is if this is intentional for backward compatibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/i18n/en.json` at line 2669, The i18n key "images" is plural but its value is singular ("Image File"), causing a semantic mismatch; either rename the key to "image_file" or "image" and update all references (e.g., any i18n.t('images') or t('images') usages) to the new key, or alternatively change the value to a plural phrase ("Image Files") to match the existing key; if you must preserve backward compatibility, add the new key with the correct singular value and keep "images" as an alias (or vice versa) and mark the old key for deprecation.package.json (1)
92-92: Update dependency to stable version.The package.json pins
openstack-uicore-foundationto4.2.25-beta.1, but the latest stable version is4.2.23. Update to the stable release unless there is a specific requirement for the prerelease features.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 92, The package.json currently pins the dependency "openstack-uicore-foundation" to a prerelease version "4.2.25-beta.1"; update that dependency entry to the stable release "4.2.23" by replacing the version string for the "openstack-uicore-foundation" dependency in package.json (ensure package lock/yarn lock is updated afterwards and run install to refresh lockfile).
🤖 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/mui/editable-table/mui-table-editable.js`:
- Line 188: Replace the confirm button color value used in the ConfirmDialog
configuration: locate the confirmButtonColor property inside the options/object
in mui-table-editable.js (used by the ConfirmDialog component) and change its
value from the MUI theme name "warning" to the hex string "#DD6B55" so it
matches the established pattern used elsewhere (e.g., mui-table.js,
meta-field-values.js, additional-input-list.js).
In
`@src/pages/sponsors/sponsor-form-item-list-page/components/sponsor-form-item-form.js`:
- Around line 183-187: The onRemove callback is using file.filename which
doesn't match the actual payload (it provides name), causing the filter to never
match; update the filter in the onRemove handler to compare img.filename (or the
stored image identifier) against file.name instead of file.filename, keeping the
rest of the logic (const newImages = (formik.values.images || []).filter(...);
formik.setFieldValue("images", newImages)) intact so removals correctly remove
the uploaded image.
In `@src/pages/sponsors/sponsor-forms-tab/index.js`:
- Around line 216-220: The pluralization logic uses strict equality against
row.items_count which can be a string; coerce once to a numeric value in the
render/mapper where this snippet is built (e.g., compute const itemsCount =
Number(row.items_count) || 0) and then use itemsCount both for the displayed
number and for the comparison (itemsCount === 1) to decide singular/plural;
update references to row.items_count in this template to use the new itemsCount
variable so non-numeric/string API values are handled consistently.
---
Outside diff comments:
In `@src/components/mui/editable-table/mui-table-editable.module.less`:
- Around line 4-13: The ::before pseudo-element in the &::before rule is
absolutely positioned so align-self: center is ignored; update the &::before
styles (the pseudo-element selector) to vertically center the 60% height dashed
border by removing the conflicting top: 0 and bottom: 0 and instead use top: 50%
plus transform: translateY(-50%) (keep height: 60% and border-left as-is) so the
dashed line is centered vertically within the container.
---
Nitpick comments:
In `@package.json`:
- Line 92: The package.json currently pins the dependency
"openstack-uicore-foundation" to a prerelease version "4.2.25-beta.1"; update
that dependency entry to the stable release "4.2.23" by replacing the version
string for the "openstack-uicore-foundation" dependency in package.json (ensure
package lock/yarn lock is updated afterwards and run install to refresh
lockfile).
In `@src/i18n/en.json`:
- Line 2669: The i18n key "images" is plural but its value is singular ("Image
File"), causing a semantic mismatch; either rename the key to "image_file" or
"image" and update all references (e.g., any i18n.t('images') or t('images')
usages) to the new key, or alternatively change the value to a plural phrase
("Image Files") to match the existing key; if you must preserve backward
compatibility, add the new key with the correct singular value and keep "images"
as an alias (or vice versa) and mark the old key for deprecation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 17dfc42e-41a7-4f3a-97f0-c73009ea606e
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
package.jsonsrc/actions/sponsor-forms-actions.jssrc/components/mui/__tests__/mui-table-editable.test.jssrc/components/mui/editable-table/mui-table-editable.jssrc/components/mui/editable-table/mui-table-editable.module.lesssrc/components/mui/sortable-table/mui-table-sortable.jssrc/components/mui/table/mui-table.jssrc/i18n/en.jsonsrc/pages/sponsors/sponsor-form-item-list-page/components/sponsor-form-item-form.jssrc/pages/sponsors/sponsor-forms-list-page/index.jssrc/pages/sponsors/sponsor-forms-tab/index.jssrc/reducers/sponsors/sponsor-forms-list-reducer.js
💤 Files with no reviewable changes (1)
- src/components/mui/tests/mui-table-editable.test.js
src/pages/sponsors/sponsor-form-item-list-page/components/sponsor-form-item-form.js
Show resolved
Hide resolved
| `${row.items_count} ${ | ||
| row.items_count === 1 | ||
| ? T.translate("edit_sponsor.forms_tab.item") | ||
| : T.translate("edit_sponsor.forms_tab.items") | ||
| }` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify how `items_count` is produced/normalized before reaching the table
rg -n -C3 '\bitems_count\b'Repository: fntechgit/summit-admin
Length of output: 4919
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Examine the full reducer context to understand data transformation
echo "=== Examining reducer at lines 115-165 ==="
sed -n '110,170p' src/reducers/sponsors/sponsor-page-forms-list-reducer.jsRepository: fntechgit/summit-admin
Length of output: 1577
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if there's any API response processing or type coercion
echo "=== Search for API client setup or response handling ==="
rg -l "axios|fetch|api" --max-count 5 src/ | head -10
echo ""
echo "=== Check sponsor-forms-actions to see how API calls are made ==="
sed -n '500,520p' src/actions/sponsor-forms-actions.js
echo ""
echo "=== Look for any response interceptors or API configuration ==="
rg -n "interceptor|transform" src/ | head -20Repository: fntechgit/summit-admin
Length of output: 8401
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the component definition for prop types
echo "=== Examine the full component file to see prop types ==="
wc -l src/pages/sponsors/sponsor-forms-tab/index.js
# Get context around the render function and check for PropTypes
echo ""
echo "=== Check for component prop definitions ==="
rg -n "propTypes|typescript|interface.*Form" src/pages/sponsors/sponsor-forms-tab/index.js -A 3 -B 1
# Look for mock data or tests that show actual data structure
echo ""
echo "=== Search for test data showing items_count structure ==="
rg -n "items_count.*:" src/ --type json --type js | head -20Repository: fntechgit/summit-admin
Length of output: 219
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the component around the render function showing items_count
echo "=== Component render function context (lines 200-230) ==="
sed -n '200,230p' src/pages/sponsors/sponsor-forms-tab/index.js
# Check imports and setup to understand data source
echo ""
echo "=== Component imports and setup (first 50 lines) ==="
head -50 src/pages/sponsors/sponsor-forms-tab/index.js
# Look for where table data comes from
echo ""
echo "=== Search for data source in component ==="
rg -n "forms|managedForms|customizedForms" src/pages/sponsors/sponsor-forms-tab/index.js | head -15Repository: fntechgit/summit-admin
Length of output: 3456
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for any mock data or test files with actual items_count values
echo "=== Search for mock data with items_count ==="
rg -n '"items_count"' src/ --type json -B 1 -A 1 | head -30
# Check test files
echo ""
echo "=== Look for test files related to sponsor forms ==="
fd "test|spec" src/pages/sponsors/sponsor-forms-tab/ 2>/dev/null || echo "No test dir found"
# Look at reducer more carefully - check what type `a` is
echo ""
echo "=== Check where payload.response.data comes from ==="
sed -n '100,130p' src/reducers/sponsors/sponsor-page-forms-list-reducer.jsRepository: fntechgit/summit-admin
Length of output: 111
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Try to find API response examples or backend documentation
echo "=== Search for any .md or README files that document API ==="
find src/ -name "README*" -o -name "*.md" | xargs cat 2>/dev/null | grep -i "items_count" || echo "No docs found"
# Look at the beginning of the reducer file for context
echo ""
echo "=== Check reducer file start and action types ==="
head -30 src/reducers/sponsors/sponsor-page-forms-list-reducer.js
# Check the actual reducer action handler more carefully
echo ""
echo "=== Full first items_count reducer case ==="
sed -n '100,135p' src/reducers/sponsors/sponsor-page-forms-list-reducer.js
# Look at any comments or type hints
echo ""
echo "=== Search for comments about items_count ==="
rg -B 2 -A 2 "items_count" src/reducers/sponsors/sponsor-page-forms-list-reducer.js | head -40Repository: fntechgit/summit-admin
Length of output: 2790
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify the exact line numbers for the code in question
echo "=== Confirm exact code at lines 216-220 ==="
sed -n '216,220p' src/pages/sponsors/sponsor-forms-tab/index.js
# Check if there are other similar numeric comparisons that might have same issue
echo ""
echo "=== Search for similar strict equality comparisons with object properties ==="
rg -n '===\s*[0-9]' src/pages/sponsors/sponsor-forms-tab/index.jsRepository: fntechgit/summit-admin
Length of output: 413
Harden pluralization against non-numeric items_count values.
The strict equality check row.items_count === 1 on line 217 will fail if the API payload provides items_count as a string (e.g., "1"), resulting in incorrect pluralization. The reducer passes items_count directly from the API without type coercion, and one reducer case attempts a fallback (items_count: a.items_count || 0) that doesn't ensure numeric type. Coerce to a number once in the render function to handle this edge case.
Suggested patch
- render: (row) =>
- `${row.items_count} ${
- row.items_count === 1
- ? T.translate("edit_sponsor.forms_tab.item")
- : T.translate("edit_sponsor.forms_tab.items")
- }`
+ render: (row) => {
+ const itemsCount = Number(row.items_count) || 0;
+ return `${itemsCount} ${
+ itemsCount === 1
+ ? T.translate("edit_sponsor.forms_tab.item")
+ : T.translate("edit_sponsor.forms_tab.items")
+ }`;
+ }📝 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.
| `${row.items_count} ${ | |
| row.items_count === 1 | |
| ? T.translate("edit_sponsor.forms_tab.item") | |
| : T.translate("edit_sponsor.forms_tab.items") | |
| }` | |
| render: (row) => { | |
| const itemsCount = Number(row.items_count) || 0; | |
| return `${itemsCount} ${ | |
| itemsCount === 1 | |
| ? T.translate("edit_sponsor.forms_tab.item") | |
| : T.translate("edit_sponsor.forms_tab.items") | |
| }`; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/sponsors/sponsor-forms-tab/index.js` around lines 216 - 220, The
pluralization logic uses strict equality against row.items_count which can be a
string; coerce once to a numeric value in the render/mapper where this snippet
is built (e.g., compute const itemsCount = Number(row.items_count) || 0) and
then use itemsCount both for the displayed number and for the comparison
(itemsCount === 1) to decide singular/plural; update references to
row.items_count in this template to use the new itemsCount variable so
non-numeric/string API values are handled consistently.
9e66f31 to
e5545a9
Compare
smarcet
left a comment
There was a problem hiding this comment.
@priscila-moneo please review merge conflicts
|
|
||
| const formatDate = (timestamp) => { | ||
| if (!timestamp) return ""; | ||
| return epochToMoment(Number(timestamp)).format("YYYY/MM/DD"); |
There was a problem hiding this comment.
I think we should use american notation MM/DD/YYYY , thoughts @smarcet ?
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/pages/sponsors/sponsor-forms-list-page/index.js`:
- Around line 128-131: The formatDate helper coerces timestamp with
Number(timestamp) and calls epochToMoment(...).format(...) without validating —
guard against non-numeric or invalid dates by first checking
Number.isFinite(Number(timestamp)) (or isFinite) and verifying the result of
epochToMoment(timestamp) is truthy/valid (e.g., check moment.isValid() or use
optional chaining) before calling .format; if the checks fail, return an empty
string. Update the formatDate function to perform these validations using the
function name formatDate and the epochToMoment result to avoid "Invalid date"
showing in the UI.
🪄 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
Run ID: ec5cf25e-3d57-4595-a0d4-24f3fcb515a8
📒 Files selected for processing (1)
src/pages/sponsors/sponsor-forms-list-page/index.js
| const formatDate = (timestamp) => { | ||
| if (!timestamp) return ""; | ||
| return epochToMoment(Number(timestamp)).format("MM/DD/YYYY"); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/pages/sponsors/sponsor-forms-list-page/index.js | head -50Repository: fntechgit/summit-admin
Length of output: 2132
🏁 Script executed:
cat -n src/pages/sponsors/sponsor-forms-list-page/index.js | grep -E "import|require"Repository: fntechgit/summit-admin
Length of output: 983
🏁 Script executed:
rg "opens_at|expires_at" src/pages/sponsors/sponsor-forms-list-page/ -A 3 -B 3Repository: fntechgit/summit-admin
Length of output: 7307
🏁 Script executed:
rg "epochToMoment" -A 2 -B 2 -t js | head -100Repository: fntechgit/summit-admin
Length of output: 7386
🏁 Script executed:
rg "epochToMoment\(" -B 2 -A 3 | grep -E "(epochToMoment|format|optional|guard|\?\.)" | head -50Repository: fntechgit/summit-admin
Length of output: 2642
🏁 Script executed:
find . -name "*.md" -o -name "*.txt" | xargs grep -l "epochToMoment" 2>/dev/null || echo "No docs found"Repository: fntechgit/summit-admin
Length of output: 79
🏁 Script executed:
rg "\.format\(" -B 3 | grep -E "(isValid|isFinite|Number\.|if.*return)" | head -30Repository: fntechgit/summit-admin
Length of output: 219
Guard date parsing before formatting to avoid invalid UI values.
The formatDate function at line 130 coerces with Number(timestamp) and calls .format() immediately without validating the result. The codebase shows epochToMoment can return null/falsy (see cart-note component using optional chaining ?. format()), yet formatDate lacks this protection. If the API returns null, undefined, or a non-numeric value, the result could surface "Invalid date" in the table.
Add a finite check and date validity check:
Proposed fix
const formatDate = (timestamp) => {
if (!timestamp) return "";
- return epochToMoment(Number(timestamp)).format("MM/DD/YYYY");
+ const epoch = Number(timestamp);
+ if (!Number.isFinite(epoch)) return "";
+ const date = epochToMoment(epoch);
+ if (!date || !date.isValid()) return "";
+ return date.format("MM/DD/YYYY");
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/sponsors/sponsor-forms-list-page/index.js` around lines 128 - 131,
The formatDate helper coerces timestamp with Number(timestamp) and calls
epochToMoment(...).format(...) without validating — guard against non-numeric or
invalid dates by first checking Number.isFinite(Number(timestamp)) (or isFinite)
and verifying the result of epochToMoment(timestamp) is truthy/valid (e.g.,
check moment.isValid() or use optional chaining) before calling .format; if the
checks fail, return an empty string. Update the formatDate function to perform
these validations using the function name formatDate and the epochToMoment
result to avoid "Invalid date" showing in the UI.
4703adb to
c230a03
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/pages/sponsors-global/form-templates/form-template-from-duplicate-popup.js`:
- Line 22: The import of MuiTable currently uses default-import syntax but
MuiTable is a named export; update the import statement to use a named import
(replace the default import with import { MuiTable } from
"openstack-uicore-foundation/lib/components/mui/table") so the component
references to MuiTable resolve correctly and match the v5.0.3 API and project
convention.
In
`@src/pages/sponsors/show-pages-list-page/__tests__/show-pages-list-page.test.js`:
- Around line 194-196: The button name regex in the test (the await
screen.findByRole call using name: /general\.yes_delete|confirm/i) is too broad
and may match unrelated buttons; update both occurrences (the matcher at lines
~194 and the similar one at ~226-228) to use anchored regexes or exact text
match (e.g., ^(?:general\.yes_delete|confirm)$ or pass the exact localized
string) so the role lookup only matches the intended confirmation button; ensure
you adjust both findByRole calls that reference this matcher (the confirmation
button lookups) to the tightened matcher.
In `@src/pages/sponsors/sponsor-purchases-tab/index.js`:
- Line 26: The import uses an undocumented internal path
("openstack-uicore-foundation/lib/components/mui/table") to bring in MuiTable
which is brittle; either switch to the package's documented public API by
importing MuiTable (or the equivalent exported component) from the package's
main export (e.g., import { MuiTable } from "openstack-uicore-foundation") or,
if the main export does not expose it, add a short-term wrapper that re-exports
the internal module from a single place in our codebase and/or pin the
dependency version and add a TODO to confirm stability with the maintainers;
update the import of MuiTable accordingly and remove direct references to the
"/lib/components/mui/table" path.
🪄 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
Run ID: 0200d38b-bd29-4ccc-821b-97c7344f50de
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (34)
package.jsonsrc/components/forms/sponsor-general-form/extra-questions.jssrc/components/forms/sponsor-general-form/sponsorship.jssrc/components/mui/__tests__/mui-table-editable.test.jssrc/components/mui/editable-table/mui-table-editable.jssrc/components/mui/sortable-table/mui-table-sortable.jssrc/components/mui/sortable-table/styles.module.lesssrc/components/mui/table/mui-table.jssrc/components/mui/table/mui-table.module.lesssrc/pages/sponsors-global/form-templates/add-form-template-item-popup.jssrc/pages/sponsors-global/form-templates/form-template-from-duplicate-popup.jssrc/pages/sponsors-global/form-templates/form-template-item-list-page.jssrc/pages/sponsors-global/form-templates/form-template-list-page.jssrc/pages/sponsors-global/inventory/inventory-list-page.jssrc/pages/sponsors-global/page-templates/page-template-list-page.jssrc/pages/sponsors/show-pages-list-page/__tests__/show-pages-list-page.test.jssrc/pages/sponsors/show-pages-list-page/index.jssrc/pages/sponsors/sponsor-badge-scans/index.jssrc/pages/sponsors/sponsor-cart-tab/components/cart-view.jssrc/pages/sponsors/sponsor-form-item-list-page/components/sponsor-form-add-item-from-inventory-popup.jssrc/pages/sponsors/sponsor-form-item-list-page/index.jssrc/pages/sponsors/sponsor-forms-list-page/index.jssrc/pages/sponsors/sponsor-forms-tab/components/add-sponsor-form-template-popup/index.jssrc/pages/sponsors/sponsor-forms-tab/components/manage-items/sponsor-form-item-from-inventory.jssrc/pages/sponsors/sponsor-forms-tab/components/manage-items/sponsor-forms-manage-items.jssrc/pages/sponsors/sponsor-forms-tab/index.jssrc/pages/sponsors/sponsor-list-page.jssrc/pages/sponsors/sponsor-media-upload-tab/index.jssrc/pages/sponsors/sponsor-pages-tab/components/add-sponsor-page-template-popup/index.jssrc/pages/sponsors/sponsor-pages-tab/index.jssrc/pages/sponsors/sponsor-purchases-tab/index.jssrc/pages/sponsors/sponsor-users-list-page/components/request-table.jssrc/pages/sponsors/sponsor-users-list-page/components/users-table.jssrc/pages/sponsors/summit-sponsorship-list-page.js
💤 Files with no reviewable changes (6)
- src/components/mui/table/mui-table.module.less
- src/components/mui/sortable-table/styles.module.less
- src/components/mui/tests/mui-table-editable.test.js
- src/components/mui/sortable-table/mui-table-sortable.js
- src/components/mui/table/mui-table.js
- src/components/mui/editable-table/mui-table-editable.js
✅ Files skipped from review due to trivial changes (7)
- package.json
- src/pages/sponsors/sponsor-forms-tab/components/manage-items/sponsor-forms-manage-items.js
- src/pages/sponsors/sponsor-users-list-page/components/request-table.js
- src/pages/sponsors/sponsor-media-upload-tab/index.js
- src/pages/sponsors/sponsor-users-list-page/components/users-table.js
- src/pages/sponsors/sponsor-forms-tab/components/add-sponsor-form-template-popup/index.js
- src/components/forms/sponsor-general-form/sponsorship.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/pages/sponsors/sponsor-forms-list-page/index.js
- src/pages/sponsors/sponsor-forms-tab/index.js
| import SearchIcon from "@mui/icons-material/Search"; | ||
| import CloseIcon from "@mui/icons-material/Close"; | ||
| import MuiTable from "../../../components/mui/table/mui-table"; | ||
| import MuiTable from "openstack-uicore-foundation/lib/components/mui/table"; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For openstack-uicore-foundation version 5.0.3, is MuiTablea default export fromopenstack-uicore-foundation/lib/components/mui/table? If not, what is the correct import path and export style?
💡 Result:
I couldn’t retrieve the published openstack-uicore-foundation@5.0.3 file contents from the public CDNs (npmjs.com was not fetchable from this environment, and unpkg/jsDelivr didn’t expose a browsable file tree here), so I can’t directly confirm the exact export in lib/components/mui/table for v5.0.3. jsDelivr does indicate that 5.0.3 exists, though. [1]
That said, in this codebase pattern, MuiTable is typically not the default export of the .../mui/table folder/module; it’s usually a named export re-exported from an index.js in that folder.
Use one of these (depending on what exists in your installed package):
// most likely (named export from the folder barrel)
import { MuiTable } from "openstack-uicore-foundation/lib/components/mui/table";or (if you want a default export, import the component file directly):
import MuiTable from "openstack-uicore-foundation/lib/components/mui/table/MuiTable";How to verify locally (recommended): open node_modules/openstack-uicore-foundation/lib/components/mui/table/index.js and check whether it contains something like exports.MuiTable = ... / export { default as MuiTable } ... (named) vs export default ... (default).
Source(s):
[1] (jsdelivr.com)
Citations:
🏁 Script executed:
# Check if node_modules exists and look for MuiTable export structure
if [ -d "node_modules/openstack-uicore-foundation/lib/components/mui/table" ]; then
echo "=== Contents of table folder ==="
ls -la node_modules/openstack-uicore-foundation/lib/components/mui/table/
echo ""
echo "=== index.js content (if exists) ==="
if [ -f "node_modules/openstack-uicore-foundation/lib/components/mui/table/index.js" ]; then
head -30 node_modules/openstack-uicore-foundation/lib/components/mui/table/index.js
fi
else
echo "node_modules not available, searching codebase for other MuiTable imports..."
fi
# Search for all MuiTable imports in codebase
echo ""
echo "=== All MuiTable imports in codebase ==="
rg "MuiTable" --type js --type jsx -B 1 -A 1Repository: fntechgit/summit-admin
Length of output: 229
🏁 Script executed:
# Check package.json and package-lock for openstack-uicore-foundation version
echo "=== Package version info ==="
if [ -f "package.json" ]; then
jq '.dependencies["openstack-uicore-foundation"] // .devDependencies["openstack-uicore-foundation"]' package.json
fi
if [ -f "package-lock.json" ]; then
echo ""
echo "=== From package-lock.json ==="
jq '.packages["node_modules/openstack-uicore-foundation"] | {version, resolved}' package-lock.json 2>/dev/null || echo "Not found in lock file"
fiRepository: fntechgit/summit-admin
Length of output: 102
🏁 Script executed:
# Search for other imports from openstack-uicore-foundation to see import pattern
echo "=== All openstack-uicore-foundation imports ==="
rg "from.*openstack-uicore-foundation" --type js -B 1 -A 0 | head -50Repository: fntechgit/summit-admin
Length of output: 2643
🏁 Script executed:
# Verify the specific file being reviewed
cat -n src/pages/sponsors-global/form-templates/form-template-from-duplicate-popup.js | head -35Repository: fntechgit/summit-admin
Length of output: 1099
Change to named import syntax for MuiTable.
The import uses default syntax but MuiTable is a named export. Update to:
import { MuiTable } from "openstack-uicore-foundation/lib/components/mui/table";This matches the consistent pattern throughout the codebase for all openstack-uicore-foundation imports and aligns with the v5.0.3 API.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/pages/sponsors-global/form-templates/form-template-from-duplicate-popup.js`
at line 22, The import of MuiTable currently uses default-import syntax but
MuiTable is a named export; update the import statement to use a named import
(replace the default import with import { MuiTable } from
"openstack-uicore-foundation/lib/components/mui/table") so the component
references to MuiTable resolve correctly and match the v5.0.3 API and project
convention.
| await screen.findByRole("button", { | ||
| name: /general\.yes_delete|confirm/i | ||
| }) |
There was a problem hiding this comment.
Tighten confirmation button matchers to avoid accidental matches.
These regexes are broad and can match unrelated buttons. Anchoring them makes the tests more deterministic.
Suggested test matcher hardening
- await screen.findByRole("button", {
- name: /general\.yes_delete|confirm/i
- })
+ await screen.findByRole("button", {
+ name: /^(general\.yes_delete|confirm)$/i
+ })
- await screen.findByRole("button", {
- name: /cancel|general\.cancel/i
- })
+ await screen.findByRole("button", {
+ name: /^(general\.cancel|cancel)$/i
+ })Also applies to: 226-228
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/pages/sponsors/show-pages-list-page/__tests__/show-pages-list-page.test.js`
around lines 194 - 196, The button name regex in the test (the await
screen.findByRole call using name: /general\.yes_delete|confirm/i) is too broad
and may match unrelated buttons; update both occurrences (the matcher at lines
~194 and the similar one at ~226-228) to use anchored regexes or exact text
match (e.g., ^(?:general\.yes_delete|confirm)$ or pass the exact localized
string) so the role lookup only matches the intended confirmation button; ensure
you adjust both findByRole calls that reference this matcher (the confirmation
button lookups) to the tightened matcher.
| import { getSponsorPurchases } from "../../../../../actions/sponsor-purchases-actions"; | ||
| import SearchInput from "../../../../../components/mui/search-input"; | ||
| import MuiTable from "../../../../../components/mui/table/mui-table"; | ||
| import MuiTable from "openstack-uicore-foundation/lib/components/mui/table"; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
`In openstack-uicore-foundation version 5.0.3, are these documented/supported public import paths:
- openstack-uicore-foundation/lib/components/mui/table
- openstack-uicore-foundation/lib/components/mui/editable-table
Also confirm whether the table/editable-table components still support props used here: columns, data, options.sortCol, options.sortDir, onPageChange, onPerPageChange, onSort, onEdit, onArchive, onDelete, onCellChange.`
💡 Result:
1) Are these documented/supported public import paths in openstack-uicore-foundation@5.0.3?
- The README does not document any deep “public import paths” for MUI table components (it only shows importing CSS, etc.). [1]
package.jsonhas no"exports"map (so nothing is explicitly declared as a supported public subpath); it only declares"main": "lib/openstack-uicore-foundation.js". [2]
That said, the files do exist under lib/components/mui/ in 5.0.3: table.js and editable-table.js. [3]
So these imports will typically work (Node/bundlers resolve .../table → .../table.js):
openstack-uicore-foundation/lib/components/mui/table✅ (resolves totable.js) [3]openstack-uicore-foundation/lib/components/mui/editable-table✅ (resolves toeditable-table.js) [3]
But they are not documented as supported public API by the package itself. [1][2]
2) Do table / editable-table still support these props?
table (lib/components/mui/table.js)
Supported (present in the component signature):
columns,dataoptions.sortCol,options.sortDir(alsooptions.disableProp)onPageChange,onPerPageChange,onSortonEdit,onArchive,onDelete
Not present as a prop:onCellChange(this is not part oftable, onlyeditable-table) [4]
editable-table (lib/components/mui/editable-table.js)
Supported (present in the component signature):
columns,dataoptions.sortCol,options.sortDir(alsooptions.disableProp)onPageChange,onPerPageChange,onSortonEdit,onArchive,onDeleteonCellChange✅ [5]
So: everything you listed is supported except onCellChange is supported on editable-table, not on table. [4][5]
Sources: [1] README, [2] package.json, [3] file listing, [4] table.js, [5] editable-table.js.
These import paths are not documented as public API; recommend using supported alternatives or clarifying support with the maintainers.
Line 26 imports from openstack-uicore-foundation/lib/components/mui/table, which does exist in v5.0.3 and will resolve correctly. However, this is an undocumented internal path—the package has no exports map and only declares "main": "lib/openstack-uicore-foundation.js" in its public interface. Using undocumented paths creates a maintenance risk: the package could restructure its internals or add an exports map in future versions, breaking these imports silently.
Before merging, either confirm with the maintainers that /lib/components/mui/* is stable, or use the package's documented public API (the main export) if an alternative exists.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/sponsors/sponsor-purchases-tab/index.js` at line 26, The import
uses an undocumented internal path
("openstack-uicore-foundation/lib/components/mui/table") to bring in MuiTable
which is brittle; either switch to the package's documented public API by
importing MuiTable (or the equivalent exported component) from the package's
main export (e.g., import { MuiTable } from "openstack-uicore-foundation") or,
if the main export does not expose it, add a short-term wrapper that re-exports
the internal module from a single place in our codebase and/or pin the
dependency version and add a TODO to confirm stability with the maintainers;
update the import of MuiTable accordingly and remove direct references to the
"/lib/components/mui/table" path.
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
src/pages/sponsors/sponsor-page/tabs/sponsor-forms-tab/index.js (1)
216-220:⚠️ Potential issue | 🟡 MinorNormalize
items_countbefore singular/plural branching.Line 217 still does a strict numeric comparison on
row.items_count; if the API returns"1"(string), pluralization is incorrect.Suggested patch
- render: (row) => - `${row.items_count} ${ - row.items_count === 1 + render: (row) => { + const itemsCount = Number(row.items_count) || 0; + return `${itemsCount} ${ + itemsCount === 1 ? T.translate("edit_sponsor.forms_tab.item") : T.translate("edit_sponsor.forms_tab.items") - }` + }`; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/sponsors/sponsor-page/tabs/sponsor-forms-tab/index.js` around lines 216 - 220, Normalize row.items_count to a number before doing the singular/plural check and use that normalized value in both the displayed count and the comparison; e.g. in the code building the template string in sponsor-forms-tab (where `${row.items_count} ${row.items_count === 1 ? ... : ...}` appears) create a local const like count = Number(row.items_count) || 0 and then use `${count} ${count === 1 ? T.translate("edit_sponsor.forms_tab.item") : T.translate("edit_sponsor.forms_tab.items")}` so string-returning counts like "1" are handled correctly.
🤖 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/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/components/cart-view.js`:
- Line 30: The imported SearchInput now expects an onChange prop but the
component usage still passes onSearch; update the SearchInput usage in CartView
to pass the search handler via onChange (e.g., change onSearch={...} to
onChange={...}) or adapt the local handler name to match (rename handleSearch to
handleChange and pass as onChange) so the search input triggers correctly;
ensure the prop change is made where SearchInput is rendered in CartView and any
referenced handler names (onSearch/handleSearch) are updated consistently.
In
`@src/pages/sponsors/sponsor-page/tabs/sponsor-forms-tab/components/add-sponsor-form-template-popup/index.js`:
- Around line 25-29: The import statements for MenuButton, querySponsorAddons,
getSponsorForms, FIVE_PER_PAGE, and MuiFormikSelectGroup are using incorrect
shallow relative paths; update each import in add-sponsor-form-template-popup
(look for the symbols MenuButton, querySponsorAddons, getSponsorForms,
FIVE_PER_PAGE, MuiFormikSelectGroup) to the correct relative paths that reflect
this component's actual directory depth (traverse up the necessary number of ../
segments to reach the true locations or switch to absolute/alias imports if your
project supports them), then run the build/test to ensure all imports resolve.
In
`@src/pages/sponsors/sponsor-page/tabs/sponsor-forms-tab/components/manage-items/sponsor-forms-manage-items.js`:
- Around line 39-42: The import statements for SponsorInventoryDialog,
SearchInput, CustomAlert and the sponsor-forms actions have incorrect relative
paths after the file was moved; update the import paths for the symbols
SponsorInventoryDialog, SearchInput, CustomAlert and the actions exported from
sponsor-forms-actions to the correct relative locations used by the rest of this
file (match the other src-level imports in this module), fixing the number of
../ segments so each import resolves to the actual module that defines those
symbols.
In `@src/pages/sponsors/sponsor-page/tabs/sponsor-forms-tab/index.js`:
- Line 37: The imported SearchInput uses an onChange prop, but the component
instances pass onSearch (lines around 323-326); update those usages to pass the
handler via onChange instead of onSearch (e.g., change onSearch={...} to
onChange={...}) and ensure the passed handler (the function currently referenced
by onSearch) accepts the expected argument shape (event or value) used by
SearchInput so filtering executes correctly.
In
`@src/pages/sponsors/sponsor-page/tabs/sponsor-general-form/extra-questions.js`:
- Line 19: The import for DEFAULT_CURRENT_PAGE and MAX_PER_PAGE is pointing to
the wrong relative path; change the import in extra-questions.js to reference
the project root utils file (replace "../../../utils/constants" with the correct
path "../../../../../utils/constants" or, if your project supports absolute
imports, "src/utils/constants") so DEFAULT_CURRENT_PAGE and MAX_PER_PAGE resolve
to src/utils/constants.js.
In `@src/pages/sponsors/sponsor-page/tabs/sponsor-media-upload-tab/index.js`:
- Around line 28-32: The relative import paths in this file are too shallow;
update the imports that currently start with "../../../" (the import of
sponsor-mu-actions, CustomAlert, SPONSOR_MEDIA_UPLOAD_STATUS, UploadDialog and
showConfirmDialog) to the correct deeper relative path (e.g., add one more "../"
level) so they resolve from this directory; adjust the import strings for the
symbols sponsor-mu-actions, CustomAlert, SPONSOR_MEDIA_UPLOAD_STATUS,
UploadDialog and showConfirmDialog accordingly.
In
`@src/pages/sponsors/sponsor-page/tabs/sponsor-pages-tab/components/add-sponsor-page-template-popup/index.js`:
- Around line 39-41: The three imports for MenuButton, querySponsorAddons, and
getShowPages use incorrect deep-relative paths that resolve under the current
folder instead of the project root; update the three import statements in this
file (the lines importing MenuButton, querySponsorAddons, and getShowPages) to
use the correct module paths (either root-based/alias imports or the proper
relative depth) so the modules resolve at build time.
In `@src/pages/sponsors/sponsor-page/tabs/sponsor-pages-tab/index.js`:
- Around line 38-39: The two import statements for CustomAlert and SearchInput
use too-shallow relative paths and break module resolution; update the import
paths for the symbols CustomAlert and SearchInput to match the deeper
relative-import pattern used elsewhere in this file (i.e., increase the number
of "../" segments to the same depth as the other imports in this module) so CI
can resolve the modules.
In `@src/pages/sponsors/sponsor-page/tabs/sponsor-purchases-tab/index.js`:
- Around line 27-28: The two imports using relative paths for
getSponsorPurchases and SearchInput are using incorrect directory depth; change
the import statements for getSponsorPurchases (symbol: getSponsorPurchases) and
SearchInput (symbol: SearchInput) to use the same src-root import style used
elsewhere in this file (i.e., replace the
"../../../actions/sponsor-purchases-actions" and
"../../../components/mui/search-input" relative paths with the project's
root-based import paths so they align with the other imports).
---
Duplicate comments:
In `@src/pages/sponsors/sponsor-page/tabs/sponsor-forms-tab/index.js`:
- Around line 216-220: Normalize row.items_count to a number before doing the
singular/plural check and use that normalized value in both the displayed count
and the comparison; e.g. in the code building the template string in
sponsor-forms-tab (where `${row.items_count} ${row.items_count === 1 ? ... :
...}` appears) create a local const like count = Number(row.items_count) || 0
and then use `${count} ${count === 1 ?
T.translate("edit_sponsor.forms_tab.item") :
T.translate("edit_sponsor.forms_tab.items")}` so string-returning counts like
"1" are handled correctly.
🪄 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
Run ID: 97e7217a-79fc-451f-8c8a-103c8ebeccce
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (42)
package.jsonsrc/actions/sponsor-forms-actions.jssrc/components/mui/__tests__/mui-table-editable.test.jssrc/components/mui/editable-table/mui-table-editable.jssrc/components/mui/editable-table/styles.module.lesssrc/components/mui/sortable-table/mui-table-sortable.jssrc/components/mui/sortable-table/styles.module.lesssrc/components/mui/table/extra-rows/NotesRow.jsxsrc/components/mui/table/extra-rows/TotalRow.jsxsrc/components/mui/table/extra-rows/index.jssrc/components/mui/table/mui-table.jssrc/components/mui/table/mui-table.module.lesssrc/i18n/en.jsonsrc/pages/sponsors-global/form-templates/add-form-template-item-popup.jssrc/pages/sponsors-global/form-templates/form-template-from-duplicate-popup.jssrc/pages/sponsors-global/form-templates/form-template-item-list-page.jssrc/pages/sponsors-global/form-templates/form-template-list-page.jssrc/pages/sponsors-global/inventory/inventory-list-page.jssrc/pages/sponsors-global/page-templates/page-template-list-page.jssrc/pages/sponsors/show-pages-list-page/__tests__/show-pages-list-page.test.jssrc/pages/sponsors/show-pages-list-page/index.jssrc/pages/sponsors/sponsor-form-item-list-page/components/sponsor-form-add-item-from-inventory-popup.jssrc/pages/sponsors/sponsor-form-item-list-page/components/sponsor-form-item-form.jssrc/pages/sponsors/sponsor-form-item-list-page/index.jssrc/pages/sponsors/sponsor-forms-list-page/index.jssrc/pages/sponsors/sponsor-list-page.jssrc/pages/sponsors/sponsor-page/tabs/sponsor-badge-scans/index.jssrc/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/components/cart-view.jssrc/pages/sponsors/sponsor-page/tabs/sponsor-forms-tab/components/add-sponsor-form-template-popup/index.jssrc/pages/sponsors/sponsor-page/tabs/sponsor-forms-tab/components/manage-items/sponsor-form-item-from-inventory.jssrc/pages/sponsors/sponsor-page/tabs/sponsor-forms-tab/components/manage-items/sponsor-forms-manage-items.jssrc/pages/sponsors/sponsor-page/tabs/sponsor-forms-tab/index.jssrc/pages/sponsors/sponsor-page/tabs/sponsor-general-form/extra-questions.jssrc/pages/sponsors/sponsor-page/tabs/sponsor-general-form/sponsorship.jssrc/pages/sponsors/sponsor-page/tabs/sponsor-media-upload-tab/index.jssrc/pages/sponsors/sponsor-page/tabs/sponsor-pages-tab/components/add-sponsor-page-template-popup/index.jssrc/pages/sponsors/sponsor-page/tabs/sponsor-pages-tab/index.jssrc/pages/sponsors/sponsor-page/tabs/sponsor-purchases-tab/index.jssrc/pages/sponsors/sponsor-users-list-page/components/request-table.jssrc/pages/sponsors/sponsor-users-list-page/components/users-table.jssrc/pages/sponsors/summit-sponsorship-list-page.jssrc/reducers/sponsors/sponsor-forms-list-reducer.js
💤 Files with no reviewable changes (10)
- src/components/mui/table/mui-table.module.less
- src/components/mui/editable-table/styles.module.less
- src/components/mui/table/extra-rows/index.js
- src/components/mui/sortable-table/styles.module.less
- src/components/mui/table/extra-rows/NotesRow.jsx
- src/components/mui/table/extra-rows/TotalRow.jsx
- src/components/mui/tests/mui-table-editable.test.js
- src/components/mui/editable-table/mui-table-editable.js
- src/components/mui/sortable-table/mui-table-sortable.js
- src/components/mui/table/mui-table.js
✅ Files skipped from review due to trivial changes (15)
- src/pages/sponsors-global/form-templates/form-template-list-page.js
- src/pages/sponsors/sponsor-list-page.js
- src/pages/sponsors/sponsor-form-item-list-page/index.js
- src/pages/sponsors/show-pages-list-page/index.js
- src/pages/sponsors/sponsor-users-list-page/components/request-table.js
- package.json
- src/pages/sponsors-global/form-templates/add-form-template-item-popup.js
- src/pages/sponsors/summit-sponsorship-list-page.js
- src/pages/sponsors-global/page-templates/page-template-list-page.js
- src/pages/sponsors/sponsor-form-item-list-page/components/sponsor-form-add-item-from-inventory-popup.js
- src/pages/sponsors/sponsor-users-list-page/components/users-table.js
- src/pages/sponsors-global/form-templates/form-template-from-duplicate-popup.js
- src/pages/sponsors/sponsor-page/tabs/sponsor-forms-tab/components/manage-items/sponsor-form-item-from-inventory.js
- src/pages/sponsors-global/form-templates/form-template-item-list-page.js
- src/pages/sponsors/sponsor-forms-list-page/index.js
🚧 Files skipped from review as they are similar to previous changes (5)
- src/pages/sponsors-global/inventory/inventory-list-page.js
- src/reducers/sponsors/sponsor-forms-list-reducer.js
- src/pages/sponsors/show-pages-list-page/tests/show-pages-list-page.test.js
- src/i18n/en.json
- src/pages/sponsors/sponsor-form-item-list-page/components/sponsor-form-item-form.js
| import SearchInput from "../../../../../../components/mui/search-input"; | ||
| import MuiTable from "openstack-uicore-foundation/lib/components/mui/table"; | ||
| import { TotalRow } from "../../../../components/mui/table/extra-rows"; | ||
| import SearchInput from "../../../../components/mui/search-input"; |
There was a problem hiding this comment.
Fix SearchInput prop contract after the import swap.
Line 166 passes onSearch, but the imported SearchInput component expects onChange; this prevents search from triggering after the new import on Line 30.
Proposed patch
<Grid2 size={2} offset={6}>
<SearchInput
- term={term}
- onSearch={handleSearch}
+ onChange={handleSearch}
placeholder={T.translate("edit_sponsor.placeholders.search")}
/>
</Grid2>Also applies to: 164-167
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/pages/sponsors/sponsor-page/tabs/sponsor-cart-tab/components/cart-view.js`
at line 30, The imported SearchInput now expects an onChange prop but the
component usage still passes onSearch; update the SearchInput usage in CartView
to pass the search handler via onChange (e.g., change onSearch={...} to
onChange={...}) or adapt the local handler name to match (rename handleSearch to
handleChange and pass as onChange) so the search input triggers correctly;
ensure the prop change is made where SearchInput is rendered in CartView and any
referenced handler names (onSearch/handleSearch) are updated consistently.
...sors/sponsor-page/tabs/sponsor-forms-tab/components/add-sponsor-form-template-popup/index.js
Outdated
Show resolved
Hide resolved
...rs/sponsor-page/tabs/sponsor-forms-tab/components/manage-items/sponsor-forms-manage-items.js
Outdated
Show resolved
Hide resolved
c230a03 to
251eb34
Compare
ref: https://app.clickup.com/t/86b8tt4df
Summary by CodeRabbit
New Features
opens_atandexpires_atdate fields to sponsor forms with sortable table columns displaying formatted dates.Refactoring
Style
Chores