feat: display and edit sponsorship tiers in forms list view table#827
feat: display and edit sponsorship tiers in forms list view table#827priscila-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:
📝 WalkthroughWalkthroughAdds inline sponsorship-tier editing to the sponsor forms list, expands sponsorship_types/apply_to_all_types throughout fetch/save/reducer flows, tightens date/meta_fields normalization and snackbar HTML checks, exposes dropdown lifecycle callbacks, removes silent promise swallowing, and adds two i18n keys. Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as SponsorFormsList
participant Dropdown as DropdownCheckbox
participant Action as SponsorFormsActions
participant API as Backend
participant Reducer as SponsorFormsReducer
participant Store as ReduxState
User->>UI: Click tier cell (enter edit)
UI->>Dropdown: open with current sponsorship_types
User->>Dropdown: select/deselect tiers ("all" logic)
Dropdown->>UI: onClose / onCloseMenu with new value
UI->>Action: dispatch updateFormTemplate(id, payload)
Action->>API: PATCH / sponsor form
API-->>Action: returns updated form
Action->>Reducer: dispatch RECEIVE_SPONSOR_FORMS (updated item)
Reducer->>Store: update state.sponsorForms
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 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: 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
martinquiroga-exo
left a comment
martinquiroga-exo
left a comment
There was a problem hiding this comment.
@priscila-moneo please review comment
src/actions/sponsor-forms-actions.js
Outdated
| ); | ||
| }) | ||
| .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
priscila-moneo
force-pushed
the
feature/show-level-form-see-tiers
branch
from
0da7610 to
5c6b0e2
Compare
caseylocker
left a comment
caseylocker
left a comment
There was a problem hiding this comment.
-
normalizeTiers([]) converts empty tiers to "all tiers"
src/pages/sponsors/sponsor-forms-list-page/index.js:44-51 If a form has no tiers assigned, clicking into the row auto-selects "All tiers" and closing it saves that change. This silently overwrites the form's tier scope without the user intending to. -
Tier dropdown has no options until a popup is opened
src/pages/sponsors/sponsor-forms-list-page/index.js The inline tiers column reads from sponsorships.items, but sponsorships are only fetched when the user opens the GlobalTemplate or FormTemplate popups. On initial page load, the dropdown will be empty and tier names will render as raw IDs. Needs a useEffect to call getSponsorships() on mount. -
Editor closes before save completes
src/pages/sponsors/sponsor-forms-list-page/index.js:192 setEditingTiersId(null) runs synchronously before the PUT request. If the save fails, the user's selection is already gone and they'd have to redo it. -
Double list refresh on save
src/pages/sponsors/sponsor-forms-list-page/index.js:199-201 getSponsorForms() is called here after updateFormTemplate resolves, but updateFormTemplate already dispatches that same refresh internally. This causes two full reloads per save. -
Empty string dates pass through to moment
src/actions/sponsor-forms-actions.js:455-470 The guards check for undefined and null but not empty string. If a date field is cleared, moment.tz("", summitTZ).unix() will produce an invalid value.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/pages/sponsors/sponsor-forms-list-page/index.js (2)
187-191:⚠️ Potential issue | 🟠 MajorDon’t drop empty/no-op close paths here.
[]is now a meaningful “no tiers selected” state, but Line 191 returns before persisting it. Line 188 also exits without clearingeditingTiersId, so closing the dropdown unchanged can leave the row stuck in edit mode until the user clicks again.Suggested fix
const changed = prevAll !== nextAll || !arraysEqual(prevIds, nextIds); - if (!changed) return; + if (!changed) { + setEditingTiersId(null); + return; + } const sponsorship_types = nextIds; const apply_to_all_types = nextAll; - if (!apply_to_all_types && sponsorship_types.length === 0) return; updateFormTemplate({🤖 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 187 - 191, The current early-returns drop the meaningful empty selection and leave rows in edit mode; update the block that computes changed/next values (using prevAll, nextAll, prevIds, nextIds, arraysEqual) so that an explicit empty sponsorship_types (sponsorship_types = nextIds) when apply_to_all_types is false is allowed to persist instead of returning early, and ensure editingTiersId is cleared/unset whenever the dropdown is closed (i.e., even when there is no change) so the row doesn’t remain stuck in edit mode.
192-200:⚠️ Potential issue | 🟠 Major
updateFormTemplateis the wrong action for this inline flow.That action still refreshes the list with
getSponsorForms()defaults and resolves after its internalcatch. From this screen, a tier edit can bounce users back to the default table state, and this.then()path will also close the editor after a failed save. Please use an update path that preserves the current query and returns a real rejection on error.🤖 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 192 - 200, The code is calling updateFormTemplate(...) which refreshes using getSponsorForms() defaults and swallows errors; replace this call with the update action that preserves the current query and propagates real rejections (e.g., an action named updateSponsorFormPreserveQuery or similar in your store) so saving a tier doesn't bounce the table to defaults and errors are not swallowed; ensure the setEditingTiersId(null) remains inside the successful .then() handler only, remove the empty .catch() and let the action return a rejected promise on failure so callers can handle/display the error.
🤖 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 272-283: The clickable <span> used to open tier editing should be
keyboard-accessible; change the element rendering that currently uses the span
(the block with title={label} and onClick={() => handleTiersEdit(row)}) to a
semantic <button> or, if retaining a non-button, add role="button", tabIndex={0}
and keyDown handler that calls handleTiersEdit(row) on Enter/Space and ensure
appropriate styling and aria-label/title remain; update the element where
handleTiersEdit(row) is referenced so keyboard and screen-reader users can
activate the editor.
---
Duplicate comments:
In `@src/pages/sponsors/sponsor-forms-list-page/index.js`:
- Around line 187-191: The current early-returns drop the meaningful empty
selection and leave rows in edit mode; update the block that computes
changed/next values (using prevAll, nextAll, prevIds, nextIds, arraysEqual) so
that an explicit empty sponsorship_types (sponsorship_types = nextIds) when
apply_to_all_types is false is allowed to persist instead of returning early,
and ensure editingTiersId is cleared/unset whenever the dropdown is closed
(i.e., even when there is no change) so the row doesn’t remain stuck in edit
mode.
- Around line 192-200: The code is calling updateFormTemplate(...) which
refreshes using getSponsorForms() defaults and swallows errors; replace this
call with the update action that preserves the current query and propagates real
rejections (e.g., an action named updateSponsorFormPreserveQuery or similar in
your store) so saving a tier doesn't bounce the table to defaults and errors are
not swallowed; ensure the setEditingTiersId(null) remains inside the successful
.then() handler only, remove the empty .catch() and let the action return a
rejected promise on failure so callers can handle/display the error.
🪄 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: dae62388-958b-41c8-850d-215f0dc653c6
📒 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 (2)
- src/components/mui/SnackbarNotification/index.js
- src/components/mui/dropdown-checkbox.js
| <span | ||
| style={{ | ||
| cursor: "pointer", | ||
| textDecoration: "underline dotted", | ||
| display: "block", | ||
| whiteSpace: "normal", | ||
| wordBreak: "break-word", | ||
| verticalAlign: "middle" | ||
| }} | ||
| title={label} | ||
| onClick={() => handleTiersEdit(row)} | ||
| > |
There was a problem hiding this comment.
Use a keyboard-accessible control for tier editing.
This is a clickable <span>, so keyboard users cannot focus it or open the editor. Please render a button here, or at least add button semantics plus Enter/Space handling.
Suggested fix
- <span
+ <button
+ type="button"
style={{
cursor: "pointer",
textDecoration: "underline dotted",
display: "block",
whiteSpace: "normal",
wordBreak: "break-word",
- verticalAlign: "middle"
+ verticalAlign: "middle",
+ background: "none",
+ border: 0,
+ padding: 0,
+ textAlign: "left",
+ width: "100%"
}}
title={label}
onClick={() => handleTiersEdit(row)}
>
{label}
- </span>
+ </button>📝 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.
| <span | |
| style={{ | |
| cursor: "pointer", | |
| textDecoration: "underline dotted", | |
| display: "block", | |
| whiteSpace: "normal", | |
| wordBreak: "break-word", | |
| verticalAlign: "middle" | |
| }} | |
| title={label} | |
| onClick={() => handleTiersEdit(row)} | |
| > | |
| <button | |
| type="button" | |
| style={{ | |
| cursor: "pointer", | |
| textDecoration: "underline dotted", | |
| display: "block", | |
| whiteSpace: "normal", | |
| wordBreak: "break-word", | |
| verticalAlign: "middle", | |
| background: "none", | |
| border: 0, | |
| padding: 0, | |
| textAlign: "left", | |
| width: "100%" | |
| }} | |
| title={label} | |
| onClick={() => handleTiersEdit(row)} | |
| > | |
| {label} | |
| </button> |
🤖 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 272 - 283,
The clickable <span> used to open tier editing should be keyboard-accessible;
change the element rendering that currently uses the span (the block with
title={label} and onClick={() => handleTiersEdit(row)}) to a semantic <button>
or, if retaining a non-button, add role="button", tabIndex={0} and keyDown
handler that calls handleTiersEdit(row) on Enter/Space and ensure appropriate
styling and aria-label/title remain; update the element where
handleTiersEdit(row) is referenced so keyboard and screen-reader users can
activate the editor.
| getSponsorForms(); | ||
| }, []); | ||
| getSponsorships(); | ||
| }, [getSponsorForms, getSponsorships]); |
There was a problem hiding this comment.
getSponsorships() is called with no args, defaulting to page=1, perPage=10. Summits with >10 sponsorship types will have missing options in the dropdown and unresolvable tier names in the display. The popup code elsewhere uses MAX_PER_PAGE = 100 — this call should too.
// src/pages/sponsors/sponsor-forms-list-page/index.js
import { MAX_PER_PAGE } from "../../../utils/constants";
// in the useEffect:
getSponsorships(1, MAX_PER_PAGE);
| snackbarErrorHandler | ||
| )(params)(dispatch) | ||
| .then(() => { | ||
| dispatch(getSponsorForms()); |
There was a problem hiding this comment.
updateFormTemplate on success dispatches getSponsorForms() with no arguments. That resets term, page, sort, and archive filter all to defaults. If the user had searched/filtered/paginated, they lose that state after every inline save.
Keep the action's dispatch(getSponsorForms()) but have it read the current query state from the reducer:
// in updateFormTemplate's .then():
const { term, currentPage, perPage, order, orderDir, hideArchived } =
getState().sponsorFormsListState;
dispatch(getSponsorForms(term, currentPage, perPage, order, orderDir, hideArchived));
caseylocker
left a comment
caseylocker
left a comment
There was a problem hiding this comment.
@priscila-moneo please see comments
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/pages/sponsors/sponsor-forms-list-page/index.js (2)
180-181: MovearraysEqualoutside the component.This utility function is recreated on every render. Moving it outside the component (next to
normalizeTiers) improves performance and follows the same pattern used for the other helper.Suggested refactor
const normalizeTiers = (arr) => { if (!Array.isArray(arr)) return []; return typeof arr[0] === "object" ? arr.map((t) => t.id) : arr; }; +const arraysEqual = (a, b) => + a.length === b.length && a.every((v, i) => v === b[i]); + const SponsorFormsListPage = ({Then remove lines 180-181 from inside 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 180 - 181, Move the arraysEqual helper out of the component and co-locate it with the existing normalizeTiers function: extract the const arraysEqual = (a, b) => a.length === b.length && a.every((v, i) => v === b[i]); declaration from inside the component and place it at module scope next to normalizeTiers so it isn't recreated on every render, then remove the in-component declaration and ensure any references to arraysEqual remain unchanged.
144-167: Click-outside behavior may be confusing to users.When the dropdown is closed but tiers have been changed, clicking outside does not exit edit mode. Users must re-open and close the dropdown to trigger the save. Consider either:
- Auto-saving on outside click when changes exist, or
- Providing a clear visual indicator that unsaved changes exist
This is minor and can be addressed later if user feedback warrants it.
🤖 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 144 - 167, The current click-outside handler (handleClickOutside) only exits edit mode when tiers are unchanged, leaving users stuck if they modified tiers while the dropdown is closed; change it to detect when tiersValue differs from the original sponsorship_types for the form (compare sponsorForms.find(f => f.id === editingTiersId)?.sponsorship_types || [] to tiersValue) and on outside click either call the existing save/update routine (implement a save function like saveTiersForForm(editingTiersId, tiersValue) if none exists) to persist changes and then call setEditingTiersId(null), or if you prefer not to auto-save, set a flag or UI indicator for unsaved changes and still allow setEditingTiersId(null) so edit mode exits; update handleClickOutside and its registration/removal accordingly and keep dropdownRef, dropdownOpen, editingTiersId, tiersValue, sponsorForms, and setEditingTiersId as the key references.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/pages/sponsors/sponsor-forms-list-page/index.js`:
- Around line 180-181: Move the arraysEqual helper out of the component and
co-locate it with the existing normalizeTiers function: extract the const
arraysEqual = (a, b) => a.length === b.length && a.every((v, i) => v === b[i]);
declaration from inside the component and place it at module scope next to
normalizeTiers so it isn't recreated on every render, then remove the
in-component declaration and ensure any references to arraysEqual remain
unchanged.
- Around line 144-167: The current click-outside handler (handleClickOutside)
only exits edit mode when tiers are unchanged, leaving users stuck if they
modified tiers while the dropdown is closed; change it to detect when tiersValue
differs from the original sponsorship_types for the form (compare
sponsorForms.find(f => f.id === editingTiersId)?.sponsorship_types || [] to
tiersValue) and on outside click either call the existing save/update routine
(implement a save function like saveTiersForForm(editingTiersId, tiersValue) if
none exists) to persist changes and then call setEditingTiersId(null), or if you
prefer not to auto-save, set a flag or UI indicator for unsaved changes and
still allow setEditingTiersId(null) so edit mode exits; update
handleClickOutside and its registration/removal accordingly and keep
dropdownRef, dropdownOpen, editingTiersId, tiersValue, sponsorForms, and
setEditingTiersId as the key references.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: af2466b0-fc72-4b29-9424-eb8a38dc87ce
📒 Files selected for processing (2)
src/actions/sponsor-forms-actions.jssrc/pages/sponsors/sponsor-forms-list-page/index.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/actions/sponsor-forms-actions.js
| // when snackbarMessage changes in base-reducer, we trigger the snackbar | ||
| useEffect(() => { | ||
| if (snackbarMessage?.html) { | ||
| if (snackbarMessage?.html && snackbarMessage.html.trim()) { |
There was a problem hiding this comment.
@priscila-moneo this is repetead at https://github.com/fntechgit/summit-admin/pull/827/changes#diff-2ee27bd05f6df8bd132a2bd0836ca82969b24d5ac19ad79602506e14c9d6be15R40 create an util method !empty()
| options, | ||
| onChange | ||
| onChange, | ||
| onBlur, |
There was a problem hiding this comment.
@priscila-moneo
DropdownCheckbox pollutes client contract instead of using ...rest
- Why it matters: Explicitly adding onBlur, onClose, onOpen, onCloseMenu hardcodes a growing Select prop surface. onCloseMenu is redundant with MUI's own onClose, which is now forcibly wrapped and fires even when callers pass neither handler — a subtle behavior change for the 3 existing consumers (form-template-form, page-template-popup, ItemTableField).
- Where: src/components/mui/dropdown-checkbox.js (DropdownCheckbox props + onClose wrapper)
- Suggested fix: destructure only what the component owns (name,label,allLabel,value,options,onChange) and forward ...rest to ; drop onCloseMenu and let callers combine logic inside their own onClose.
src/actions/sponsor-forms-actions.js
Outdated
| )(params)(dispatch) | ||
| .then(() => { | ||
| dispatch(getSponsorForms()); | ||
| const { term, currentPage, perPage, order, orderDir, hideArchived } = |
There was a problem hiding this comment.
@priscila-moneo please break down this on 2 actions. u are modifying the updateFormTEmplate action that is meant to update the whole entity and you are not providing any regresion test
| code: a.code, | ||
| name: a.name, | ||
| items_qty: `${a.items.length} ${ | ||
| a.items.length === 1 ? "Item" : "Items" |
There was a problem hiding this comment.
@priscila-moneo this should be resourced at i18n file
| const sponsorship_types = nextIds; | ||
| const apply_to_all_types = nextAll; | ||
| if (!apply_to_all_types && sponsorship_types.length === 0) return; | ||
| updateFormTemplate({ |
There was a problem hiding this comment.
@priscila-moneo this should be handled by an specialized action not the same action meant to save the entire entity
| setTiersValue(newValue); | ||
| }; | ||
|
|
||
| const arraysEqual = (a, b) => |
There was a problem hiding this comment.
@priscila-moneo arraysEqual compares positionally (a[i] === b[i]),
but prevIds and nextIds have no shared canonical order:
- `prevIds` comes from the reducer which preserves the backend's order
(usually sorted by id).
- `nextIds` comes from MUI `Select multiple`, which appends newly-clicked
items to the END of the value array.
So the two arrays diverge in order as soon as the user unchecks + rechecks
any item, or selects items in a different order than the backend returns
them. Repro:
1. Row has sponsorship_types = [1, 2, 3] (from backend).
2. User opens the inline editor, unchecks 2, rechecks 2.
3. tiersValue is now [1, 3, 2] — same set, different order.
4. arraysEqual([1,2,3], [1,3,2]) → false → `changed = true`.
5. A spurious PUT fires with the exact set the backend already has.
This is especially bad combined with the partial-PUT concern on
updateFormTemplate — a routine check/uncheck cycle (the most common
interaction on a multi-select) will silently re-save the row. Every
spurious call also triggers startLoading + a success snackbar + a full
getSponsorForms refetch, so the UI churns on no-ops.
The same order-sensitive bug exists in the click-outside useEffect above,
which compares via .toString():
(row.sponsorship_types || []).toString() === tiersValue.toString()
Both should use set-equality. Suggested helper (no new imports, O(n),
intent is explicit):
const sameTierSet = (a, b) => {
if (a.length !== b.length) return false;
const setB = new Set(b);
return a.every((v) => setB.has(v));
};
Then:
const changed = prevAll !== nextAll || !sameTierSet(prevIds, nextIds);
…and replace the .toString() comparison in the click-outside effect
with sameTierSet(...) as well, otherwise only half the bug is fixed.
Please add a unit test covering the "uncheck + recheck" case so this
doesn't regress.
priscila-moneo
force-pushed
the
feature/show-level-form-see-tiers
branch
3 times, most recently
from
8e07bc0 to
b3db8ce
Compare
priscila-moneo
force-pushed
the
feature/show-level-form-see-tiers
branch
from
b3db8ce to
d7760b0
Compare
| }); | ||
| }; | ||
|
|
||
| export const normalizeFormTemplate = (entity, summitTZ) => { |
There was a problem hiding this comment.
@priscila-moneo please rollback this chante to the original normalizeFormTemplate function if u need a new one for updateFormTemplateTiers then define a new pipeline
to avoid any regression
| "moment-duration-format": "^2.3.2", | ||
| "moment-timezone": "^0.5.33", | ||
| "openstack-uicore-foundation": "5.0.6-beta.1", | ||
| "openstack-uicore-foundation": "5.0.8-beta.1", |
There was a problem hiding this comment.
@priscila-moneo please update it to 5.0.8
4 participants
ref: https://app.clickup.com/t/86b8t9qw1
Summary by CodeRabbit
New Features
Bug Fixes