fix(push): Resolve error when creating audience by adding PushAudienceFilter component (#2626) #2983
Conversation
Update readme for store dashboard settings parse-community#2555 Signed-off-by: Hien Nguyen <hiennguyen92@gmail.com>
chore(docs): update README with dashboard config options Signed-off-by: Hien Nguyen <hiennguyen92@gmail.com>
Signed-off-by: Hien Nguyen <hiennguyen92@gmail.com>
Signed-off-by: Hien Nguyen <hiennguyen92@gmail.com>
Signed-off-by: Hien Nguyen <hiennguyen92@gmail.com>
|
I will reformat the title to use the proper commit message syntax. |
parse-github-assistant
bot
changed the title
|
🚀 Thanks for opening this pull request! We appreciate your effort in improving the project. Please let us know once your pull request is ready for review. |
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) |
Uffizzi Ephemeral Environment
|
📝 WalkthroughPre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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
🧹 Nitpick comments (7)
src/components/PushAudienceDialog/PushAudienceDialog.react.js (1)
103-113: Good empty‑state guard; consider clearing stale errors and double‑check constraints.
- Nice fix preventing undefined access when no fields are available.
- Also clear any prior error on successful add to avoid stale banner.
- Optional: ensure
available[field]?.[0]exists before push.Apply:
const keys = Object.keys(available); if (keys.length === 0) { this.setState({ errorMessage: 'No condition available.', }); return; } - const field = keys[0]; + const field = keys[0]; + // Clear any previous error upon successful add + this.setState({ errorMessage: undefined });src/components/PushAudienceFilter/PushAudienceFilter.react.js (6)
9-9: Avoid shadowing global Map; use an alias and Immutable factory.Importing
Mapshadows the global and the file usesnew Map(...). Prefer an alias and the factory call.Apply:
-import { List, Map } from 'immutable'; +import { List, Map as IMap } from 'immutable';Then replace
new Map({...})withIMap({...})in this file. See further diffs below. Based on static analysis hints.
15-28: Use Immutable Map factory and tighten compare preservation.
- Switch to
IMap({...})(nonew).- Current type check uses
typeof; acceptable, but keep consistent.Apply:
-function changeField(schema, filters, index, newField) { +function changeField(schema, filters, index, newField) { const allowedConstraints = Filters.FieldConstraints[schema[newField].type]; const current = filters.get(index); const constraint = current.get('constraint'); const compare = current.get('compareTo'); const defaultCompare = Filters.DefaultComparisons[schema[newField].type]; const useExisting = allowedConstraints.includes(constraint); - const newFilter = new Map({ + const newFilter = IMap({ field: newField, constraint: useExisting ? constraint : Filters.FieldConstraints[schema[newField].type][0], compareTo: useExisting && typeof defaultCompare === typeof compare ? compare : defaultCompare, }); return filters.set(index, newFilter); }
30-42: Use Immutable Map factory in constraint change.Apply:
- const newFilter = new Map({ + const newFilter = IMap({ field: field, constraint: newConstraint, compareTo: prevCompareTo ?? Filters.DefaultComparisons[compareType], });
44-47: Remove unused parameter or use it.
typeisn’t used. Drop it to reduce noise and update the caller.Apply:
-function changeCompareTo(schema, filters, index, type, newCompare) { +function changeCompareTo(schema, filters, index, newCompare) { const newValue = newCompare; return filters.set(index, filters.get(index).set('compareTo', newValue)); }And here:
- onChangeCompareTo: newCompare => { - onChange(changeCompareTo(schema, filters, i, compareType, newCompare)); - }, + onChangeCompareTo: newCompare => { + onChange(changeCompareTo(schema, filters, i, newCompare)); + },
78-108: Sorting: always prefer locale/stringCompare for consistency.Fallback branch uses default
.sort()which is ASCII. To keep consistent ordering with preference branch usingstringCompare, use it in both.Apply:
-else { - fields.sort(); -} +else { + fields.sort(stringCompare); +}
155-163: PropTypes: fix wording and add missing optional props.
- “comparator” → “constraint”.
- Declare optional props used:
onChange,onSearch,blacklist,className,onDeleteRow.Apply:
PushAudienceFilter.propTypes = { schema: PropTypes.object.isRequired.describe( 'A class schema, mapping field names to their Type strings' ), filters: PropTypes.instanceOf(List).isRequired.describe( - 'An array of filter objects. Each filter contains "field", "comparator", and "compareTo" fields.' + 'An array of filter objects. Each filter contains "field", "constraint", and "compareTo" fields.' ), renderRow: PropTypes.func.isRequired.describe('A function for rendering a row of a filter.'), + onChange: PropTypes.func.describe('Called with updated filters when a row changes.'), + onSearch: PropTypes.func.describe('Invoked on Enter key. Optional.'), + blacklist: PropTypes.arrayOf(PropTypes.string).describe('Constraints to hide. Optional.'), + className: PropTypes.string.describe('Class name used to resolve column preferences. Optional.'), + onDeleteRow: PropTypes.func.describe('Called before a row is deleted. Optional.'), };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/components/PushAudienceDialog/InstallationCondition.react.js(0 hunks)src/components/PushAudienceDialog/PushAudienceDialog.react.js(3 hunks)src/components/PushAudienceFilter/PushAudienceFilter.react.js(1 hunks)src/components/SaveButton/SaveButton.react.js(1 hunks)src/dashboard/Data/CloudCode/CloudCode.react.js(7 hunks)src/lib/ParseApp.js(1 hunks)
💤 Files with no reviewable changes (1)
- src/components/PushAudienceDialog/InstallationCondition.react.js
🧰 Additional context used
🧬 Code graph analysis (3)
src/dashboard/Data/CloudCode/CloudCode.react.js (2)
src/lib/generatePath.js (2)
params(6-6)generatePath(3-34)src/components/SaveButton/SaveButton.react.js (1)
SaveButton(14-57)
src/components/PushAudienceDialog/PushAudienceDialog.react.js (1)
src/components/PushAudienceFilter/PushAudienceFilter.react.js (1)
PushAudienceFilter(53-151)
src/components/PushAudienceFilter/PushAudienceFilter.react.js (2)
src/lib/generatePath.js (2)
filters(8-8)filter(12-12)src/components/Filter/Filter.react.js (1)
newConstraint(24-24)
🪛 GitHub Check: Lint
src/dashboard/Data/CloudCode/CloudCode.react.js
[failure] 107-107:
'fileName' is never reassigned. Use 'const' instead
[failure] 102-102:
Trailing spaces not allowed
🪛 Biome (2.1.2)
src/components/PushAudienceFilter/PushAudienceFilter.react.js
[error] 9-9: Do not shadow the global "Map" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Docker linux/amd64
🔇 Additional comments (1)
src/components/PushAudienceDialog/PushAudienceDialog.react.js (1)
13-13: Swap to PushAudienceFilter looks good.Import path update is correct and aligns with the new component.
| <PushAudienceFilter | ||
| schema={this.props.schema} | ||
| filters={this.state.filters} | ||
| onChange={filters => { | ||
| this.setState({ filters }, this.fetchAudienceSize.bind(this)); | ||
| }} | ||
| onDeleteRow={() => { | ||
| this.setState({ errorMessage: undefined }); | ||
| }} | ||
| renderRow={props => <InstallationCondition {...props} />} | ||
| /> |
There was a problem hiding this comment.
Pressing Enter in a row may crash: onSearch isn’t provided to child.
PushAudienceFilter calls onSearch() on Enter without optional chaining. Since this dialog doesn’t pass onSearch, users pressing Enter can hit a TypeError. Pass a no‑op or wire it to refresh the audience size.
Apply:
<PushAudienceFilter
schema={this.props.schema}
filters={this.state.filters}
onChange={filters => {
- this.setState({ filters }, this.fetchAudienceSize.bind(this));
+ this.setState({ filters, errorMessage: undefined }, this.fetchAudienceSize.bind(this));
}}
+ onSearch={this.fetchAudienceSize.bind(this)}
onDeleteRow={() => {
this.setState({ errorMessage: undefined });
}}
renderRow={props => <InstallationCondition {...props} />}
/>📝 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.
| <PushAudienceFilter | |
| schema={this.props.schema} | |
| filters={this.state.filters} | |
| onChange={filters => { | |
| this.setState({ filters }, this.fetchAudienceSize.bind(this)); | |
| }} | |
| onDeleteRow={() => { | |
| this.setState({ errorMessage: undefined }); | |
| }} | |
| renderRow={props => <InstallationCondition {...props} />} | |
| /> | |
| <PushAudienceFilter | |
| schema={this.props.schema} | |
| filters={this.state.filters} | |
| onChange={filters => { | |
| this.setState({ filters, errorMessage: undefined }, this.fetchAudienceSize.bind(this)); | |
| }} | |
| onSearch={this.fetchAudienceSize.bind(this)} | |
| onDeleteRow={() => { | |
| this.setState({ errorMessage: undefined }); | |
| }} | |
| renderRow={props => <InstallationCondition {...props} />} | |
| /> |
🤖 Prompt for AI Agents
In src/components/PushAudienceDialog/PushAudienceDialog.react.js around lines
295 to 305, PushAudienceFilter invokes onSearch() on Enter but this dialog does
not pass an onSearch prop, causing a TypeError when users press Enter; fix by
passing an onSearch prop (either a no-op function or point it to refresh the
audience size, e.g., call this.fetchAudienceSize) so the child can safely call
onSearch without crashing and ensure binding/context is correct when using the
component's method.
| const constraints = Filters.FieldConstraints[schema[field].type].filter( | ||
| c => blacklist.indexOf(c) < 0 | ||
| ); | ||
| let compareType = schema[field].type; | ||
| if (Object.prototype.hasOwnProperty.call(Filters.Constraints[constraint], 'field')) { | ||
| compareType = Filters.Constraints[constraint].field; | ||
| } |
There was a problem hiding this comment.
Guard against missing schema entries to avoid crashes.
If a saved filter references a removed field, schema[field] is undefined and this will throw.
Apply minimal guard:
-const constraints = Filters.FieldConstraints[schema[field].type].filter(
+if (!schema[field]) {
+ // Field no longer exists; skip rendering this row safely.
+ return null;
+}
+const constraints = Filters.FieldConstraints[schema[field].type].filter(
c => blacklist.indexOf(c) < 0
);Optionally, surface a UI hint instead of skipping; happy to provide.
📝 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.
| const constraints = Filters.FieldConstraints[schema[field].type].filter( | |
| c => blacklist.indexOf(c) < 0 | |
| ); | |
| let compareType = schema[field].type; | |
| if (Object.prototype.hasOwnProperty.call(Filters.Constraints[constraint], 'field')) { | |
| compareType = Filters.Constraints[constraint].field; | |
| } | |
| if (!schema[field]) { | |
| // Field no longer exists; skip rendering this row safely. | |
| return null; | |
| } | |
| const constraints = Filters.FieldConstraints[schema[field].type].filter( | |
| c => blacklist.indexOf(c) < 0 | |
| ); | |
| let compareType = schema[field].type; | |
| if (Object.prototype.hasOwnProperty.call(Filters.Constraints[constraint], 'field')) { | |
| compareType = Filters.Constraints[constraint].field; | |
| } |
🤖 Prompt for AI Agents
In src/components/PushAudienceFilter/PushAudienceFilter.react.js around lines
110 to 116, the code assumes schema[field] exists and uses schema[field].type
which will throw if a saved filter references a removed field; add a guard that
checks if schema[field] is defined before accessing .type and bail out (e.g.,
skip this filter, return an empty constraints array, or set a fallback
compareType) and optionally surface a UI hint or console.warn when a field is
missing; ensure both uses of schema[field].type are protected so the component
does not crash.
| onKeyDown: ({ key }) => { | ||
| if (key === 'Enter') { | ||
| onSearch(); | ||
| } | ||
| }, |
There was a problem hiding this comment.
Enter key handler can throw when onSearch is undefined.
Use optional chaining to make it safe.
Apply:
onKeyDown: ({ key }) => {
if (key === 'Enter') {
- onSearch();
+ onSearch?.();
}
},📝 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.
| onKeyDown: ({ key }) => { | |
| if (key === 'Enter') { | |
| onSearch(); | |
| } | |
| }, | |
| onKeyDown: ({ key }) => { | |
| if (key === 'Enter') { | |
| onSearch?.(); | |
| } | |
| }, |
🤖 Prompt for AI Agents
In src/components/PushAudienceFilter/PushAudienceFilter.react.js around lines
138 to 142, the onKeyDown handler calls onSearch() directly which can throw if
onSearch is undefined; change the call to use optional chaining so it safely
invokes onSearch only when defined (e.g., call onSearch?.()) and keep the
existing Enter key check unchanged to avoid runtime errors.
| this.setState({ saveState: SaveButton.States.FAILED }); | ||
| this.setState({ saveError: e.message || e }); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Let the save button recover after a failure.
Once saveState is set to FAILED, SaveButton stops wiring onClick, so the user can’t retry the save without reloading the page. Please transition back to WAITING (after surfacing the error) so retries are possible.
You can mirror the success path timing and clear the failure state shortly after logging the error:
} catch (e) {
- this.setState({ saveState: SaveButton.States.FAILED });
- this.setState({ saveError: e.message || e });
+ this.setState({
+ saveState: SaveButton.States.FAILED,
+ saveError: e.message || e,
+ });
+ setTimeout(() => {
+ this.setState({ saveState: SaveButton.States.WAITING });
+ }, 2000);
}📝 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.
| } catch (e) { | |
| this.setState({ saveState: SaveButton.States.FAILED }); | |
| this.setState({ saveError: e.message || e }); | |
| } | |
| } | |
| } catch (e) { | |
| this.setState({ | |
| saveState: SaveButton.States.FAILED, | |
| saveError: e.message || e, | |
| }); | |
| setTimeout(() => { | |
| this.setState({ saveState: SaveButton.States.WAITING }); | |
| }, 2000); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/dashboard/Data/CloudCode/CloudCode.react.js around lines 114 to 118, the
catch block sets saveState to FAILED and saveError but never returns the button
to an actionable state; change the catch to set saveState:
SaveButton.States.FAILED and saveError as now, then schedule a follow-up
setState (use the same delay used on the success path) that resets saveState
back to SaveButton.States.WAITING and clears saveError so the SaveButton
re-enables its onClick and the user can retry.
| saveSource(fileName, data) { | ||
| return this.apiRequest('POST', `scripts/${fileName}`, { data }, { useMasterKey: true }); | ||
| } |
There was a problem hiding this comment.
Keep the Cloud Code cache in sync after save.
getSource() serves latestRelease.files[fileName].source when it’s already populated, so after a successful saveSource() the dashboard will still hand back the pre-save source until the minute-long cache expires. Please update (or invalidate) the cached entry once the POST resolves so users don’t reopen the file and see stale code despite a green “Saved!”.
Apply this diff to update the cache eagerly:
- saveSource(fileName, data) {
- return this.apiRequest('POST', `scripts/${fileName}`, { data }, { useMasterKey: true });
- }
+ saveSource(fileName, data) {
+ return this.apiRequest('POST', `scripts/${fileName}`, { data }, { useMasterKey: true }).then(
+ result => {
+ if (this.latestRelease.files && this.latestRelease.files[fileName]) {
+ this.latestRelease.files[fileName].source = data;
+ }
+ return result;
+ }
+ );
+ }📝 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.
| saveSource(fileName, data) { | |
| return this.apiRequest('POST', `scripts/${fileName}`, { data }, { useMasterKey: true }); | |
| } | |
| saveSource(fileName, data) { | |
| return this.apiRequest('POST', `scripts/${fileName}`, { data }, { useMasterKey: true }) | |
| .then(result => { | |
| if (this.latestRelease.files && this.latestRelease.files[fileName]) { | |
| this.latestRelease.files[fileName].source = data; | |
| } | |
| return result; | |
| }); | |
| } |
🤖 Prompt for AI Agents
In src/lib/ParseApp.js around lines 162-164, saveSource currently posts the new
script but does not update the in-memory Cloud Code cache, so getSource can
return stale content; change saveSource to update (or invalidate)
latestRelease.files[fileName].source after the POST resolves: call the
apiRequest, then on success check this.latestRelease and
this.latestRelease.files and either set
this.latestRelease.files[fileName].source = data (or delete the cached entry) so
the cache reflects the newly saved content; ensure you only mutate when the POST
succeeds and guard for missing objects.
|
This is still an issue that needs fixing! |
New Pull Request Checklist
Issue Description
Fixes #2626
Closes: #2626
Approach
PushAudienceFilterto handle audience filtering logic.PushAudienceFilterintoPushAudienceDialog.TODOs before merging
Summary by CodeRabbit
New Features
Bug Fixes