chore: adding grid component and helpers#237
Open
santipalenque wants to merge 10 commits intomainfrom
Open
Conversation
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a new SponsorOrderGrid MUI component and InfoNote, tightens money utilities and constants (BPS, metafield class), includes Jest/RTL tests, adds i18n keys, and wires the component into exports and webpack with a package version bump. ChangesSponsorOrderGrid Component & Utilities
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@src/components/mui/SponsorOrderGrid/index.js`:
- Around line 152-154: The icon-only cancel button using IconButton with
onClick={() => onCancelForm(row)} and the DeleteIcon lacks an accessible name;
add an explicit accessible label (e.g., aria-label="Cancel" or
aria-label={`Cancel ${row.name || 'sponsor'}`}) to the IconButton or wrap it
with a Tooltip providing the same text so screen readers can announce the
action, ensuring the label uniquely identifies the row when appropriate; update
the IconButton element where onCancelForm(row) is used to include this
aria-label/Tooltip.
- Around line 43-47: The code assumes form.items and each it.meta_fields are
always arrays which can crash when payloads are partial; update the mapping that
builds items (the expression using form.items.filter(...).map(...)) to guard
both arrays by replacing form.items with a null-safe fallback (e.g., form.items
|| []) and use a null-safe fallback for meta fields inside the map (e.g.,
(it.meta_fields || []) before calling .filter) so the filter/map on items and
the filter on meta_fields (used with SPONSOR_FORMS_METAFIELD_CLASS.FORM) never
run on undefined/null values.
- Around line 250-254: The current truthy checks treat 0 as absent; update the
props in SponsorOrderGrid (the total prop and the label decision) to use nullish
checks so legitimate zero values are preserved — e.g. replace the truthy checks
around total and amountDue with null/undefined-safe checks (use the nullish
coalescing pattern for total and an explicit amountDue != null check for the
label) so 0 is accepted as a valid value.
- Around line 219-222: FeeRow instances in the fee map are hardcoded with
trailing={1}, which breaks alignment when the action column is hidden; update
the mapped FeeRow to use the component's trailingCols variable instead (replace
trailing={1} with trailing={trailingCols}) so FeeRow rows align with the table
layout dynamically; locate the FeeRow in the fees.map block and pass
trailing={trailingCols}.
In `@src/utils/money.js`:
- Around line 66-70: The current comma-only normalization always treats every
comma as a decimal separator (variable s), which turns "1,234" into "1.234"
incorrectly; change the logic in the block that handles s.includes(",") &&
!s.includes(".") to first detect whether the commas are thousand separators by
testing s against the regex /^-?\d{1,3}(?:,\d{3})+$/ (if it matches, remove all
commas), otherwise treat the last comma as the decimal separator by replacing
only the final comma with a dot (e.g., use s = s.replace(/,([^,]*)$/, '.$1'));
apply these checks where s is manipulated so cent conversion later produces
correct results.
🪄 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: a7a56cf4-f4be-40fc-93fa-5e25129ff63c
📒 Files selected for processing (6)
src/components/index.jssrc/components/mui/SponsorOrderGrid/__tests__/SponsorOrderGrid.test.jssrc/components/mui/SponsorOrderGrid/index.jssrc/utils/constants.jssrc/utils/money.jswebpack.common.js
Comment on lines
+43
to
+47
| items: form.items | ||
| .filter((it) => it.quantity) | ||
| .map((it) => { | ||
| const formMetaFields = it.meta_fields.filter( | ||
| (mf) => mf.class_field === SPONSOR_FORMS_METAFIELD_CLASS.FORM |
There was a problem hiding this comment.
Guard nullable API arrays before filter to prevent runtime crashes.
form.items and it.meta_fields are used as always-defined arrays. A partial payload will throw and break rendering.
💡 Suggested patch
- items: form.items
+ items: (form.items ?? [])
.filter((it) => it.quantity)
.map((it) => {
- const formMetaFields = it.meta_fields.filter(
+ const formMetaFields = (it.meta_fields ?? []).filter(
(mf) => mf.class_field === SPONSOR_FORMS_METAFIELD_CLASS.FORM
);📝 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.
Suggested change
| items: form.items | |
| .filter((it) => it.quantity) | |
| .map((it) => { | |
| const formMetaFields = it.meta_fields.filter( | |
| (mf) => mf.class_field === SPONSOR_FORMS_METAFIELD_CLASS.FORM | |
| items: (form.items ?? []) | |
| .filter((it) => it.quantity) | |
| .map((it) => { | |
| const formMetaFields = (it.meta_fields ?? []).filter( | |
| (mf) => mf.class_field === SPONSOR_FORMS_METAFIELD_CLASS.FORM |
🤖 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 `@src/components/mui/SponsorOrderGrid/index.js` around lines 43 - 47, The code
assumes form.items and each it.meta_fields are always arrays which can crash
when payloads are partial; update the mapping that builds items (the expression
using form.items.filter(...).map(...)) to guard both arrays by replacing
form.items with a null-safe fallback (e.g., form.items || []) and use a
null-safe fallback for meta fields inside the map (e.g., (it.meta_fields || [])
before calling .filter) so the filter/map on items and the filter on meta_fields
(used with SPONSOR_FORMS_METAFIELD_CLASS.FORM) never run on undefined/null
values.
Comment on lines
+152
to
+154
| <IconButton size="large" onClick={() => onCancelForm(row)}> | ||
| <DeleteIcon fontSize="large" /> | ||
| </IconButton> |
There was a problem hiding this comment.
Add an accessible label to the icon-only cancel button.
The delete action is icon-only and currently has no explicit accessible name.
💡 Suggested patch
- <IconButton size="large" onClick={() => onCancelForm(row)}>
+ <IconButton
+ size="large"
+ aria-label={T.translate("order_details_grid.action")}
+ onClick={() => onCancelForm(row)}
+ >
<DeleteIcon fontSize="large" />
</IconButton>📝 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.
Suggested change
| <IconButton size="large" onClick={() => onCancelForm(row)}> | |
| <DeleteIcon fontSize="large" /> | |
| </IconButton> | |
| <IconButton | |
| size="large" | |
| aria-label={T.translate("order_details_grid.action")} | |
| onClick={() => onCancelForm(row)} | |
| > | |
| <DeleteIcon fontSize="large" /> | |
| </IconButton> |
🤖 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 `@src/components/mui/SponsorOrderGrid/index.js` around lines 152 - 154, The
icon-only cancel button using IconButton with onClick={() => onCancelForm(row)}
and the DeleteIcon lacks an accessible name; add an explicit accessible label
(e.g., aria-label="Cancel" or aria-label={`Cancel ${row.name || 'sponsor'}`}) to
the IconButton or wrap it with a Tooltip providing the same text so screen
readers can announce the action, ensuring the label uniquely identifies the row
when appropriate; update the IconButton element where onCancelForm(row) is used
to include this aria-label/Tooltip.
Comment on lines
+219
to
+222
| {fees && | ||
| fees.map((fee) => ( | ||
| <FeeRow fee={fee} key={`fee-row-${fee.id}`} trailing={1} /> | ||
| ))} |
There was a problem hiding this comment.
Use trailingCols for fee rows to keep table alignment consistent.
FeeRow is hardcoded to trailing={1}, which misaligns columns when the action column is not shown.
💡 Suggested patch
- <FeeRow fee={fee} key={`fee-row-${fee.id}`} trailing={1} />
+ <FeeRow
+ fee={fee}
+ key={`fee-row-${fee.id}`}
+ trailing={trailingCols}
+ />📝 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.
Suggested change
| {fees && | |
| fees.map((fee) => ( | |
| <FeeRow fee={fee} key={`fee-row-${fee.id}`} trailing={1} /> | |
| ))} | |
| {fees && | |
| fees.map((fee) => ( | |
| <FeeRow | |
| fee={fee} | |
| key={`fee-row-${fee.id}`} | |
| trailing={trailingCols} | |
| /> | |
| ))} |
🤖 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 `@src/components/mui/SponsorOrderGrid/index.js` around lines 219 - 222, FeeRow
instances in the fee map are hardcoded with trailing={1}, which breaks alignment
when the action column is hidden; update the mapped FeeRow to use the
component's trailingCols variable instead (replace trailing={1} with
trailing={trailingCols}) so FeeRow rows align with the table layout dynamically;
locate the FeeRow in the fees.map block and pass trailing={trailingCols}.
Comment on lines
+250
to
+254
| total={total || amountDue} | ||
| label={ | ||
| amountDue | ||
| ? T.translate("order_details_grid.amount_due") | ||
| : null |
There was a problem hiding this comment.
Handle zero values correctly for total/amountDue.
Truthy checks here treat 0 as absent. That can produce wrong totals/labels when amountDue or total is legitimately zero.
💡 Suggested patch
- total={total || amountDue}
+ total={total ?? amountDue}
label={
- amountDue
+ amountDue != null
? T.translate("order_details_grid.amount_due")
: null
}📝 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.
Suggested change
| total={total || amountDue} | |
| label={ | |
| amountDue | |
| ? T.translate("order_details_grid.amount_due") | |
| : null | |
| total={total ?? amountDue} | |
| label={ | |
| amountDue != null | |
| ? T.translate("order_details_grid.amount_due") | |
| : null |
🤖 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 `@src/components/mui/SponsorOrderGrid/index.js` around lines 250 - 254, The
current truthy checks treat 0 as absent; update the props in SponsorOrderGrid
(the total prop and the label decision) to use nullish checks so legitimate zero
values are preserved — e.g. replace the truthy checks around total and amountDue
with null/undefined-safe checks (use the nullish coalescing pattern for total
and an explicit amountDue != null check for the label) so 0 is accepted as a
valid value.
Comment on lines
+66
to
+70
| if (s.includes(",") && s.includes(".")) { | ||
| s = s.replace(/,/g, ""); | ||
| } else if (s.includes(",") && !s.includes(".")) { | ||
| s = s.replace(",", "."); | ||
| } |
There was a problem hiding this comment.
Fix comma-only number normalization to avoid wrong cent conversion.
For comma-only inputs, this always treats , as decimal separator. 1,234 becomes 1.234 (123 cents) instead of 123400 cents.
💡 Suggested patch
- } else if (s.includes(",") && !s.includes(".")) {
- s = s.replace(",", ".");
+ } else if (s.includes(",") && !s.includes(".")) {
+ // thousands grouping (e.g. 1,234 or 1,234,567)
+ if (/^\d{1,3}(,\d{3})+$/.test(s)) {
+ s = s.replace(/,/g, "");
+ } else {
+ // decimal comma locale (e.g. 1234,56)
+ s = s.replace(",", ".");
+ }
}🤖 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 `@src/utils/money.js` around lines 66 - 70, The current comma-only
normalization always treats every comma as a decimal separator (variable s),
which turns "1,234" into "1.234" incorrectly; change the logic in the block that
handles s.includes(",") && !s.includes(".") to first detect whether the commas
are thousand separators by testing s against the regex /^-?\d{1,3}(?:,\d{3})+$/
(if it matches, remove all commas), otherwise treat the last comma as the
decimal separator by replacing only the final comma with a dot (e.g., use s =
s.replace(/,([^,]*)$/, '.$1')); apply these checks where s is manipulated so
cent conversion later produces correct results.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://app.clickup.com/t/86b66n5p9
Summary by CodeRabbit
New Features
Localization
Tests
Chores
Misc