feat: display and edit sponsorship tiers in forms list view table#827
Open
priscila-moneo wants to merge 1 commit intomasterfrom
Open
feat: display and edit sponsorship tiers in forms list view table#827priscila-moneo wants to merge 1 commit intomasterfrom
priscila-moneo wants to merge 1 commit intomasterfrom
Conversation
📝 WalkthroughWalkthroughAdds inline sponsorship-tier editing and wiring to updateFormTemplate; expands sponsorship_types/apply_to_all_types in fetches and reducer; strengthens date and meta_fields normalization; surfaces promise rejections; exposes dropdown lifecycle callbacks; tightens snackbar HTML checks; adds two i18n keys. Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as SponsorFormsList
participant Dropdown as DropdownCheckbox
participant Action as updateFormTemplate
participant API as Backend
participant Reducer as SponsorFormsReducer
participant Store as ReduxState
User->>UI: Click tier label (enter edit)
UI->>Dropdown: Open with current sponsorship_types
User->>Dropdown: Select/deselect tiers (handles "all" logic)
Dropdown->>UI: onClose / onCloseMenu callback with new value
UI->>Action: Call updateFormTemplate(formId, {sponsorship_types, apply_to_all_types})
Action->>API: PATCH form
API-->>Action: Return updated form
Action->>Reducer: Dispatch updated data
Reducer->>Store: Update sponsorForms with sponsorship_types
Store-->>UI: Re-render list with updated tiers
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 docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/actions/sponsor-forms-actions.js (1)
471-484:⚠️ Potential issue | 🔴 CriticalDo not synthesize
meta_fields: []for partial form updates.The new inline tier editor in
src/pages/sponsors/sponsor-forms-list-page/index.js(Lines 194-198) updates a form with only tier fields. Defaulting missingmeta_fieldsto[]turns that call into a destructive write and can wipe existing additional fields from the template.🛡️ Proposed fix
- const sponsorship_types = entity.sponsorship_types || []; - const meta_fields = Array.isArray(entity.meta_fields) - ? entity.meta_fields - : []; + const sponsorship_types = entity.sponsorship_types || []; + const hasMetaFields = Array.isArray(entity.meta_fields); normalizedEntity.apply_to_all_types = false; normalizedEntity.sponsorship_types = sponsorship_types; @@ - normalizedEntity.meta_fields = meta_fields.filter((mf) => !!mf.name); + if (hasMetaFields) { + normalizedEntity.meta_fields = entity.meta_fields.filter((mf) => !!mf.name); + } else { + delete normalizedEntity.meta_fields; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/actions/sponsor-forms-actions.js` around lines 471 - 484, The code currently synthesizes meta_fields = [] when entity.meta_fields is missing, causing partial updates to wipe existing fields; change the logic to only assign normalizedEntity.meta_fields when entity actually has a meta_fields property. Specifically, remove the unconditional defaulting "const meta_fields = Array.isArray(entity.meta_fields) ? entity.meta_fields : []" and instead check "if (Object.prototype.hasOwnProperty.call(entity, 'meta_fields')) { normalizedEntity.meta_fields = Array.isArray(entity.meta_fields) ? entity.meta_fields.filter(mf => !!mf.name) : undefined }" so that normalizedEntity.meta_fields is not written at all for partial updates that omit meta_fields (referencing normalizedEntity, entity, and meta_fields/filter in the diff).src/pages/sponsors/sponsor-forms-list-page/index.js (1)
53-68:⚠️ Potential issue | 🟠 MajorFetch sponsorship options before using the new tiers column.
This component now relies on
sponsorships.itemsfor both label rendering and inline editing, but it never populates that slice. Sincesponsorships.itemsstarts empty insrc/reducers/sponsors/sponsor-forms-list-reducer.js(Lines 48-53), the new tiers UI comes up with raw ids at best and an empty dropdown at worst.🔧 Proposed fix
import { archiveSponsorForm, getSponsorForm, getSponsorForms, + getSponsorships, unarchiveSponsorForm, deleteSponsorForm, updateFormTemplate } from "../../../actions/sponsor-forms-actions"; @@ totalCount, getSponsorForms, + getSponsorships, getSponsorForm, archiveSponsorForm, unarchiveSponsorForm, deleteSponsorForm, updateFormTemplate, sponsorships }) => { @@ useEffect(() => { getSponsorForms(); - }, []); + getSponsorships(); + }, [getSponsorForms, getSponsorships]); @@ export default connect(mapStateToProps, { getSponsorForms, + getSponsorships, getSponsorForm, archiveSponsorForm, unarchiveSponsorForm, deleteSponsorForm, updateFormTemplate })(SponsorFormsListPage);If
getSponsorshipsis paginated, make sure this load fetches the full tier set; otherwise some ids still won’t resolve.Also applies to: 216-289
🤖 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 53 - 68, The SponsorFormsListPage now renders and edits tiers using sponsorships.items but never loads them; call the action that populates that slice (e.g., getSponsorships) when the component mounts or before rendering the tiers UI so sponsorships.items contains the full set, and guard the tiers renderer/editors to wait for sponsorships.items to be populated (or show a loading state). If getSponsorships is paginated, ensure you request the full set (e.g., fetch all pages or use a non-paginated endpoint) so IDs resolve correctly; update the component’s useEffect (or lifecycle) where getSponsorForms/getSponsorForm is called to also dispatch getSponsorships and rely on sponsorships.items for label lookup and dropdown options.src/components/mui/dropdown-checkbox.js (1)
25-42:⚠️ Potential issue | 🟡 MinorNormalize
event.target.valuebefore using array methods.MUI Select with
multiplecan emit a string value via browser autofill. The current code will fail on line 35 when.filter()is called on a string, and.includes()on line 30 will incorrectly match substrings rather than array elements.♻️ Proposed fix
const handleChange = (ev) => { - const selected = ev.target.value; + const rawValue = ev.target.value; + const selected = Array.isArray(rawValue) + ? rawValue + : typeof rawValue === "string" + ? rawValue.split(",") + : [];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/mui/dropdown-checkbox.js` around lines 25 - 42, In handleChange normalize ev.target.value (and the existing value) to an array before calling .includes() or .filter(): inside handleChange (where selected is defined) coerce selected to an array (e.g. if typeof ev.target.value === 'string' convert to ev.target.value.split(',') or wrap in [ev.target.value]; likewise normalize the current value used in comparisons) so subsequent checks like selected.includes("all") and selected.filter(...) operate on arrays, then call onChange with the normalized arrays as before.
🤖 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/actions/sponsor-forms-actions.js`:
- Around line 455-470: The parsing of opens_at and expires_at uses moment(value,
summitTZ) which treats summitTZ as a format string; update both occurrences so
non-number timestamps are parsed with moment.tz(entity.opens_at,
summitTZ).unix() and moment.tz(entity.expires_at, summitTZ).unix() respectively,
keeping the existing typeof number branch and the delete behavior; modify the
lines that assign normalizedEntity.opens_at and normalizedEntity.expires_at to
call moment.tz(...) instead of moment(...), referencing normalizedEntity,
entity.opens_at, entity.expires_at and summitTZ.
In `@src/reducers/sponsors/sponsor-forms-list-reducer.js`:
- Around line 94-110: The reducer currently assigns sponsorship_types as the
expanded objects instead of scalar ids; update the mapping inside the
sponsorForms creation (the payload.response.data.map callback) so that
sponsorship_types is normalized to an array of scalar ids (e.g.,
a.sponsorship_types.map(t => t.id) when a.sponsorship_types is an array) and
keep the "all" behavior for a.apply_to_all_types; ensure the returned object
uses this normalized sponsorship_types so the tiers lookup in the UI (which
expects ids) receives scalar ids.
---
Outside diff comments:
In `@src/actions/sponsor-forms-actions.js`:
- Around line 471-484: The code currently synthesizes meta_fields = [] when
entity.meta_fields is missing, causing partial updates to wipe existing fields;
change the logic to only assign normalizedEntity.meta_fields when entity
actually has a meta_fields property. Specifically, remove the unconditional
defaulting "const meta_fields = Array.isArray(entity.meta_fields) ?
entity.meta_fields : []" and instead check "if
(Object.prototype.hasOwnProperty.call(entity, 'meta_fields')) {
normalizedEntity.meta_fields = Array.isArray(entity.meta_fields) ?
entity.meta_fields.filter(mf => !!mf.name) : undefined }" so that
normalizedEntity.meta_fields is not written at all for partial updates that omit
meta_fields (referencing normalizedEntity, entity, and meta_fields/filter in the
diff).
In `@src/components/mui/dropdown-checkbox.js`:
- Around line 25-42: In handleChange normalize ev.target.value (and the existing
value) to an array before calling .includes() or .filter(): inside handleChange
(where selected is defined) coerce selected to an array (e.g. if typeof
ev.target.value === 'string' convert to ev.target.value.split(',') or wrap in
[ev.target.value]; likewise normalize the current value used in comparisons) so
subsequent checks like selected.includes("all") and selected.filter(...) operate
on arrays, then call onChange with the normalized arrays as before.
In `@src/pages/sponsors/sponsor-forms-list-page/index.js`:
- Around line 53-68: The SponsorFormsListPage now renders and edits tiers using
sponsorships.items but never loads them; call the action that populates that
slice (e.g., getSponsorships) when the component mounts or before rendering the
tiers UI so sponsorships.items contains the full set, and guard the tiers
renderer/editors to wait for sponsorships.items to be populated (or show a
loading state). If getSponsorships is paginated, ensure you request the full set
(e.g., fetch all pages or use a non-paginated endpoint) so IDs resolve
correctly; update the component’s useEffect (or lifecycle) where
getSponsorForms/getSponsorForm is called to also dispatch getSponsorships and
rely on sponsorships.items for label lookup and dropdown options.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3a6a045d-c905-42c1-aece-9d9586ad55cd
📒 Files selected for processing (6)
src/actions/sponsor-forms-actions.jssrc/components/mui/SnackbarNotification/index.jssrc/components/mui/dropdown-checkbox.jssrc/i18n/en.jsonsrc/pages/sponsors/sponsor-forms-list-page/index.jssrc/reducers/sponsors/sponsor-forms-list-reducer.js
priscila-moneo
changed the title
priscila-moneo
force-pushed
the
feature/show-level-form-see-tiers
branch
2 times, most recently
from
f36c334 to
7089c0f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/pages/sponsors/sponsor-forms-list-page/index.js (1)
181-183: Use order-insensitive equality for tier sets.Current comparison treats
[1,2]and[2,1]as different and can trigger unnecessary updates.Proposed adjustment
- const arraysEqual = (a, b) => - a.length === b.length && a.every((v, i) => v === b[i]); + const arraysEqual = (a, b) => { + if (a.length !== b.length) return false; + const as = [...a].map(String).sort(); + const bs = [...b].map(String).sort(); + return as.every((v, i) => v === bs[i]); + };🤖 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 181 - 183, The current arraysEqual helper used by handleTiersSave is order-sensitive and will treat [1,2] and [2,1] as different; change arraysEqual to perform order-insensitive comparison (e.g., convert both inputs to Sets and compare sizes and that every value in one Set exists in the other, or sort both arrays of primitive ids before comparing) and ensure handleTiersSave uses the updated arraysEqual so tier sets are detected correctly without triggering unnecessary updates.src/actions/sponsor-forms-actions.js (1)
447-447: Redundant reject wrapper in catch.
.catch((e) => Promise.reject(e))is equivalent to rethrowing; it adds noise without behavior gain.Proposed simplification
- .catch((e) => Promise.reject(e)) + .catch((e) => { + throw e; + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/actions/sponsor-forms-actions.js` at line 447, The .catch((e) => Promise.reject(e)) wrapper is redundant—remove this catch (or replace it with a simple rethrow if you need to run side-effects) so the original Promise rejection propagates naturally; locate the .catch((e) => Promise.reject(e)) occurrence in src/actions/sponsor-forms-actions.js and delete that call (or change to .catch(e => { /* optional logging */ throw e }) if you want to log before rethrowing).
🤖 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/actions/sponsor-forms-actions.js`:
- Around line 455-470: The normalize logic for opens_at and expires_at treats
empty strings as present and converts them; update the guards in the block
handling entity.opens_at and entity.expires_at so you only normalize when the
value is not undefined/null/empty string (e.g., check entity.opens_at !==
undefined && entity.opens_at !== null && entity.opens_at !== "" and same for
entity.expires_at), otherwise delete normalizedEntity.opens_at /
normalizedEntity.expires_at; keep using typeof entity.* === "number" ? ... :
moment.tz(entity.*, summitTZ).unix() for conversion.
In `@src/pages/sponsors/sponsor-forms-list-page/index.js`:
- Around line 44-51: normalizeTiers currently coerces an empty tiers array into
["all"], which causes untouched/legacy empty rows to be treated as
apply_to_all_types=true when saved; change normalizeTiers so that when arr is an
empty array it returns [] (do not return ["all"]), and update any save-path
logic that checks for ["all"] (the code that maps tiers into apply_to_all_types)
to treat an empty array as the absence of selection rather than "all". Ensure
callers of normalizeTiers still handle both arrays of ids and arrays of objects
(preserve the arr.map((t) => t.id) branch) but remove the empty-array -> ["all"]
coercion.
---
Nitpick comments:
In `@src/actions/sponsor-forms-actions.js`:
- Line 447: The .catch((e) => Promise.reject(e)) wrapper is redundant—remove
this catch (or replace it with a simple rethrow if you need to run side-effects)
so the original Promise rejection propagates naturally; locate the .catch((e) =>
Promise.reject(e)) occurrence in src/actions/sponsor-forms-actions.js and delete
that call (or change to .catch(e => { /* optional logging */ throw e }) if you
want to log before rethrowing).
In `@src/pages/sponsors/sponsor-forms-list-page/index.js`:
- Around line 181-183: The current arraysEqual helper used by handleTiersSave is
order-sensitive and will treat [1,2] and [2,1] as different; change arraysEqual
to perform order-insensitive comparison (e.g., convert both inputs to Sets and
compare sizes and that every value in one Set exists in the other, or sort both
arrays of primitive ids before comparing) and ensure handleTiersSave uses the
updated arraysEqual so tier sets are detected correctly without triggering
unnecessary updates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 304a11dd-ae82-443a-a090-142f3a068270
📒 Files selected for processing (6)
src/actions/sponsor-forms-actions.jssrc/components/mui/SnackbarNotification/index.jssrc/components/mui/dropdown-checkbox.jssrc/i18n/en.jsonsrc/pages/sponsors/sponsor-forms-list-page/index.jssrc/reducers/sponsors/sponsor-forms-list-reducer.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/mui/SnackbarNotification/index.js
- src/reducers/sponsors/sponsor-forms-list-reducer.js
Comment on lines
+455
to
+470
| if (entity.opens_at !== undefined && entity.opens_at !== null) { | ||
| normalizedEntity.opens_at = | ||
| typeof entity.opens_at === "number" | ||
| ? entity.opens_at | ||
| : moment.tz(entity.opens_at, summitTZ).unix(); | ||
| } else { | ||
| delete normalizedEntity.opens_at; | ||
| } | ||
| if (entity.expires_at !== undefined && entity.expires_at !== null) { | ||
| normalizedEntity.expires_at = | ||
| typeof entity.expires_at === "number" | ||
| ? entity.expires_at | ||
| : moment.tz(entity.expires_at, summitTZ).unix(); | ||
| } else { | ||
| delete normalizedEntity.expires_at; | ||
| } |
There was a problem hiding this comment.
Guard empty date strings before conversion.
opens_at/expires_at currently treat "" as present (Line 455 and Line 463) and still normalize it. This can push invalid date values in update payloads when users clear date fields.
Proposed fix
- if (entity.opens_at !== undefined && entity.opens_at !== null) {
+ if (
+ entity.opens_at !== undefined &&
+ entity.opens_at !== null &&
+ entity.opens_at !== ""
+ ) {
normalizedEntity.opens_at =
typeof entity.opens_at === "number"
? entity.opens_at
: moment.tz(entity.opens_at, summitTZ).unix();
} else {
delete normalizedEntity.opens_at;
}
- if (entity.expires_at !== undefined && entity.expires_at !== null) {
+ if (
+ entity.expires_at !== undefined &&
+ entity.expires_at !== null &&
+ entity.expires_at !== ""
+ ) {
normalizedEntity.expires_at =
typeof entity.expires_at === "number"
? entity.expires_at
: moment.tz(entity.expires_at, summitTZ).unix();
} else {
delete normalizedEntity.expires_at;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/actions/sponsor-forms-actions.js` around lines 455 - 470, The normalize
logic for opens_at and expires_at treats empty strings as present and converts
them; update the guards in the block handling entity.opens_at and
entity.expires_at so you only normalize when the value is not
undefined/null/empty string (e.g., check entity.opens_at !== undefined &&
entity.opens_at !== null && entity.opens_at !== "" and same for
entity.expires_at), otherwise delete normalizedEntity.opens_at /
normalizedEntity.expires_at; keep using typeof entity.* === "number" ? ... :
moment.tz(entity.*, summitTZ).unix() for conversion.
Comment on lines
+44
to
+51
| const normalizeTiers = (arr) => | ||
| Array.isArray(arr) | ||
| ? arr.length === 0 | ||
| ? ["all"] | ||
| : typeof arr[0] === "object" | ||
| ? arr.map((t) => t.id) | ||
| : arr | ||
| : []; |
There was a problem hiding this comment.
Don’t coerce empty tiers to ["all"] in normalization.
Line 47 changes an empty tiers array into "all". In the current save path (Lines 183-193), that can convert untouched legacy/empty rows into apply_to_all_types=true just by entering and closing edit mode.
Proposed fix
const normalizeTiers = (arr) =>
Array.isArray(arr)
- ? arr.length === 0
- ? ["all"]
+ ? arr.length === 0
+ ? []
: typeof arr[0] === "object"
? arr.map((t) => t.id)
: arr
: [];Also applies to: 183-193
🤖 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 44 - 51,
normalizeTiers currently coerces an empty tiers array into ["all"], which causes
untouched/legacy empty rows to be treated as apply_to_all_types=true when saved;
change normalizeTiers so that when arr is an empty array it returns [] (do not
return ["all"]), and update any save-path logic that checks for ["all"] (the
code that maps tiers into apply_to_all_types) to treat an empty array as the
absence of selection rather than "all". Ensure callers of normalizeTiers still
handle both arrays of ids and arrays of objects (preserve the arr.map((t) =>
t.id) branch) but remove the empty-array -> ["all"] coercion.
martinquiroga-exo
requested changes
Mar 19, 2026
martinquiroga-exo
left a comment
martinquiroga-exo
left a comment
There was a problem hiding this comment.
@priscila-moneo please review comment
| ); | ||
| }) | ||
| .catch(() => {}) // need to catch promise reject | ||
| .catch((e) => Promise.reject(e)) |
There was a problem hiding this comment.
This is redundant, since it's an implicit rejection. A catch block returns a new Promise. If the handler function itself throws an error, the returned promise is automatically rejected with that error as the reason.
priscila-moneo
force-pushed
the
feature/show-level-form-see-tiers
branch
from
7089c0f to
0da7610
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/pages/sponsors/sponsor-forms-list-page/index.js (1)
145-168: Fragile array comparison may prevent edit cancellation for empty tiers.The
toString()comparison at lines 156-157 doesn't account for thenormalizeTierscoercion. Whenrow.sponsorship_typesis[], it compares"" === "all"(sincetiersValueis["all"]), which is always false. This means click-outside will never cancel editing for rows that originally had no tiers.Consider using a consistent normalized comparison:
Proposed fix
if ( - tiersValue.length === 0 || - ( - sponsorForms.find((f) => f.id === editingTiersId) - ?.sponsorship_types || [] - ).toString() === tiersValue.toString() + arraysEqual( + normalizeTiers( + sponsorForms.find((f) => f.id === editingTiersId) + ?.sponsorship_types + ), + tiersValue + ) ) {Note: This requires moving
arraysEqualandnormalizeTiersbefore this effect or extracting them outside the component.🤖 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 145 - 168, The click-outside handler uses toString() to compare sponsorship arrays which is brittle and fails when normalizeTiers coerces empty arrays (e.g., [] -> ["all"]); update handleClickOutside in the useEffect that watches editingTiersId/dropdownOpen to compare normalized arrays using the existing normalizeTiers and arraysEqual helpers (call normalizeTiers on both sponsorForms.find(...).sponsorship_types and tiersValue, then use arraysEqual) instead of toString(), and ensure normalizeTiers and arraysEqual are defined before this effect or moved outside the component so they can be used reliably; keep the existing logic to call setEditingTiersId(null) when the normalized arrays match or tiersValue.length === 0.
🤖 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/SnackbarNotification/index.js`:
- Around line 40-50: Both useEffect blocks call .trim() on msgData.html and
snackbarMessage.html without ensuring html is a string, which can throw if
non-strings are passed; update the effects that setOpen (watching msgData) and
setMsgData (watching snackbarMessage) to first guard html with a string check
(e.g., typeof html === 'string' && html.trim().length > 0) before calling
.trim(); factor the guard into a small helper like isNonEmptyString used by both
effects and by any local successMessage/errorMessage call sites to defensively
validate inputs.
In `@src/pages/sponsors/sponsor-forms-list-page/index.js`:
- Around line 194-203: The updateFormTemplate(...) promise chain redundantly
calls getSponsorForms() after updateFormTemplate resolves even though
updateFormTemplate (in updateFormTemplate action) already dispatches
getSponsorForms() in its .then() handler; remove the extra getSponsorForms()
call from the .then() block where updateFormTemplate is invoked (leave the
.catch() to handle errors or surface them) so the list is only refreshed once by
the action itself.
---
Nitpick comments:
In `@src/pages/sponsors/sponsor-forms-list-page/index.js`:
- Around line 145-168: The click-outside handler uses toString() to compare
sponsorship arrays which is brittle and fails when normalizeTiers coerces empty
arrays (e.g., [] -> ["all"]); update handleClickOutside in the useEffect that
watches editingTiersId/dropdownOpen to compare normalized arrays using the
existing normalizeTiers and arraysEqual helpers (call normalizeTiers on both
sponsorForms.find(...).sponsorship_types and tiersValue, then use arraysEqual)
instead of toString(), and ensure normalizeTiers and arraysEqual are defined
before this effect or moved outside the component so they can be used reliably;
keep the existing logic to call setEditingTiersId(null) when the normalized
arrays match or tiersValue.length === 0.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 17de2d18-d0d0-4c2b-8e43-78e5ec110966
📒 Files selected for processing (6)
src/actions/sponsor-forms-actions.jssrc/components/mui/SnackbarNotification/index.jssrc/components/mui/dropdown-checkbox.jssrc/i18n/en.jsonsrc/pages/sponsors/sponsor-forms-list-page/index.jssrc/reducers/sponsors/sponsor-forms-list-reducer.js
✅ Files skipped from review due to trivial changes (1)
- src/i18n/en.json
🚧 Files skipped from review as they are similar to previous changes (3)
- src/reducers/sponsors/sponsor-forms-list-reducer.js
- src/components/mui/dropdown-checkbox.js
- src/actions/sponsor-forms-actions.js
Comment on lines
+40
to
50
| if (msgData.html && msgData.html.trim()) { | ||
| setOpen(true); | ||
| } else { | ||
| setOpen(false); | ||
| } | ||
| }, [msgData]); | ||
|
|
||
| // when snackbarMessage changes in base-reducer, we trigger the snackbar | ||
| useEffect(() => { | ||
| if (snackbarMessage?.html) { | ||
| if (snackbarMessage?.html && snackbarMessage.html.trim()) { | ||
| setMsgData(snackbarMessage); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read-only verification: locate snackbar message producers and html payload shapes.
rg -n -C3 --type js --type jsx --type ts --type tsx \
'\bsuccessMessage\s*\(|\berrorMessage\s*\(|\bsnackbar(Success|Error)Handler\b|\bhtml\s*:'Repository: fntechgit/summit-admin
Length of output: 93
🏁 Script executed:
cat -n src/components/mui/SnackbarNotification/index.jsRepository: fntechgit/summit-admin
Length of output: 3665
🏁 Script executed:
rg -n -C3 'successMessage\s*\(|errorMessage\s*\(|html\s*:' --type js --type tsRepository: fntechgit/summit-admin
Length of output: 50378
🏁 Script executed:
# Search for snackbar message producers and Redux actions
rg -n 'snackbarMessage|clearSnackbarMessage|snackbar.*reducer|html.*:' --type js --type ts -A 2 -B 2Repository: fntechgit/summit-admin
Length of output: 50378
Guard html with a type check before calling .trim().
At lines 40 and 49, .trim() is called without verifying html is a string. While all current code paths use T.translate() (which returns a string), the local successMessage/errorMessage functions accept parameters directly without type validation, creating a potential runtime error if non-string values are passed. Use a shared string guard in both effects to be defensive.
💡 Proposed fix
const SnackbarNotification = ({
children,
snackbarMessage,
clearSnackbarMessage
}) => {
const [open, setOpen] = useState(false);
const [msgData, setMsgData] = useState({});
+ const hasNonEmptyHtml = (value) =>
+ typeof value === "string" && value.trim().length > 0;
// this two methods are for on-demand messaging
const successMessage = (msg) => setMsgData({ html: msg, type: "success" });
const errorMessage = (msg) => setMsgData({ html: msg, type: "warning" });
const messageContext = useMemo(() => ({ successMessage, errorMessage }), []);
@@
useEffect(() => {
- if (msgData.html && msgData.html.trim()) {
+ if (hasNonEmptyHtml(msgData?.html)) {
setOpen(true);
} else {
setOpen(false);
}
}, [msgData]);
@@
useEffect(() => {
- if (snackbarMessage?.html && snackbarMessage.html.trim()) {
+ if (hasNonEmptyHtml(snackbarMessage?.html)) {
setMsgData(snackbarMessage);
}
}, [snackbarMessage]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/mui/SnackbarNotification/index.js` around lines 40 - 50, Both
useEffect blocks call .trim() on msgData.html and snackbarMessage.html without
ensuring html is a string, which can throw if non-strings are passed; update the
effects that setOpen (watching msgData) and setMsgData (watching
snackbarMessage) to first guard html with a string check (e.g., typeof html ===
'string' && html.trim().length > 0) before calling .trim(); factor the guard
into a small helper like isNonEmptyString used by both effects and by any local
successMessage/errorMessage call sites to defensively validate inputs.
Comment on lines
+194
to
+203
| updateFormTemplate({ | ||
| id: row.id, | ||
| sponsorship_types, | ||
| apply_to_all_types | ||
| }) | ||
| .then(() => { | ||
| getSponsorForms(); | ||
| }) | ||
| .catch(() => {}); | ||
| }; |
There was a problem hiding this comment.
Redundant getSponsorForms() call - list is refreshed twice on success.
Looking at updateFormTemplate in src/actions/sponsor-forms-actions.js (lines 439-440), the action already dispatches getSponsorForms() in its .then() handler. The call at line 200 causes a duplicate refresh.
Proposed fix - remove redundant refresh
updateFormTemplate({
id: row.id,
sponsorship_types,
apply_to_all_types
- })
- .then(() => {
- getSponsorForms();
- })
- .catch(() => {});
+ }).catch(() => {});📝 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
| updateFormTemplate({ | |
| id: row.id, | |
| sponsorship_types, | |
| apply_to_all_types | |
| }) | |
| .then(() => { | |
| getSponsorForms(); | |
| }) | |
| .catch(() => {}); | |
| }; | |
| updateFormTemplate({ | |
| id: row.id, | |
| sponsorship_types, | |
| apply_to_all_types | |
| }).catch(() => {}); | |
| }; |
🤖 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 194 - 203,
The updateFormTemplate(...) promise chain redundantly calls getSponsorForms()
after updateFormTemplate resolves even though updateFormTemplate (in
updateFormTemplate action) already dispatches getSponsorForms() in its .then()
handler; remove the extra getSponsorForms() call from the .then() block where
updateFormTemplate is invoked (leave the .catch() to handle errors or surface
them) so the list is only refreshed once by the action itself.
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
2 participants
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.
ref: https://app.clickup.com/t/86b8t9qw1
Summary by CodeRabbit
New Features
Bug Fixes