fix(push): Fix create audience dialog - #3431
Conversation
|
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. Tip
Note Please respond to review comments from AI agents just like you would to comments from a human reviewer. Let the reviewer resolve their own comments, unless they have reviewed and accepted your commit, or agreed with your explanation for why the feedback was incorrect. Caution Pull requests must be written using an AI agent with human supervision. Pull requests written entirely by a human will likely be rejected, because of lower code quality, higher review effort and the higher risk of introducing bugs. Please note that AI review comments on this pull request alone do not satisfy this requirement. Our CI and AI review are safeguards, not development tools. If many issues are flagged, rethink your development approach. Invest more effort in planning and design rather than using review cycles to fix low-quality code. |
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughPush audience filters are normalized and scoped to ChangesPush audience filtering
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PushAudienceDialog
participant Filter
participant AudienceSize
PushAudienceDialog->>Filter: Provide _Installation schema and filters
Filter->>PushAudienceDialog: Emit filter changes or search
PushAudienceDialog->>AudienceSize: Fetch audience size
Possibly related PRs
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning, 1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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.
🧹 Nitpick comments (2)
src/lib/tests/PushAudienceDialog.test.js (1)
47-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffIndex-based child lookup is a bit brittle.
children[1].props.childrenassumes a fixed sibling order inPushAudienceDialog.render(). A future unrelated reorder of theField/filterdivwould silently break this assertion for the wrong reason. Consider searching children by element type instead, but not blocking.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/tests/PushAudienceDialog.test.js` around lines 47 - 55, Update the “renders the audience filter” test to locate the rendered child by its element type rather than relying on the positional children[1] lookup. Use the existing PushAudienceFilter symbol to find the matching element while preserving the assertion that the dialog renders that filter.src/components/PushAudienceFilter/PushAudienceFilter.react.js (1)
120-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
compareTypederivation.The same "does this constraint override the compare field type" logic appears here and in
changeConstraint(lines 34-37). Consider extracting a small shared helper (e.g.,getCompareType(schema, field, constraint)) to avoid drift between the two copies.♻️ Proposed refactor
+function getCompareType(schema, field, constraint) { + if (Object.prototype.hasOwnProperty.call(Filters.Constraints[constraint], 'field')) { + return Filters.Constraints[constraint].field; + } + return schema[field].type; +} + function changeConstraint(schema, filters, index, newConstraint, prevCompareTo) { const field = filters.get(index).get('field'); - let compareType = schema[field].type; - if (Object.prototype.hasOwnProperty.call(Filters.Constraints[newConstraint], 'field')) { - compareType = Filters.Constraints[newConstraint].field; - } + const compareType = getCompareType(schema, field, newConstraint); ...And reuse
getCompareType(schema, field, constraint)at line 120-123.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/PushAudienceFilter/PushAudienceFilter.react.js` around lines 120 - 123, Extract the shared compare-type derivation from changeConstraint and the current filtering logic into a helper such as getCompareType(schema, field, constraint). Have the helper return the constraint’s field override when present, otherwise schema[field].type, and replace both duplicated implementations with calls to it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/components/PushAudienceFilter/PushAudienceFilter.react.js`:
- Around line 120-123: Extract the shared compare-type derivation from
changeConstraint and the current filtering logic into a helper such as
getCompareType(schema, field, constraint). Have the helper return the
constraint’s field override when present, otherwise schema[field].type, and
replace both duplicated implementations with calls to it.
In `@src/lib/tests/PushAudienceDialog.test.js`:
- Around line 47-55: Update the “renders the audience filter” test to locate the
rendered child by its element type rather than relying on the positional
children[1] lookup. Use the existing PushAudienceFilter symbol to find the
matching element while preserving the assertion that the dialog renders that
filter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 14d73e9c-f547-4ced-86f2-8a98b1a88545
📒 Files selected for processing (4)
src/components/PushAudienceDialog/InstallationCondition.react.jssrc/components/PushAudienceDialog/PushAudienceDialog.react.jssrc/components/PushAudienceFilter/PushAudienceFilter.react.jssrc/lib/tests/PushAudienceDialog.test.js
💤 Files with no reviewable changes (1)
- src/components/PushAudienceDialog/InstallationCondition.react.js
f4a62e2 to
1eb2f8e
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/components/PushAudienceFilter/PushAudienceFilter.react.js (1)
71-99: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
compareFieldsfallback bypassesstringComparewhen there's no column preference.When
currentColumnPreferenceis falsy,compareFieldsis leftundefined, sosortedFields/per-row fallback sorting (Lines 99, 112) uses the native default sort instead ofstringCompare, even thoughstringCompareis imported specifically for this purpose. Consider defaultingcompareFields = stringCompareand only wrapping it with priority logic when a column preference exists, so the fallback stays consistent whether or not a preference is configured.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/PushAudienceFilter/PushAudienceFilter.react.js` around lines 71 - 99, Default compareFields to stringCompare before the currentColumnPreference check, then retain the priority-aware comparator only when preferences exist. Ensure sortedFields and per-row fallback sorting consistently use stringCompare when no preference is configured.src/lib/tests/PushAudienceDialog.test.js (1)
82-144: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo regression test for a missing
CurrentApp.Provider.This suite always wraps
PushAudienceFilterin an explicit<CurrentApp.Provider>. GivenPushAudienceDialog.fetchAudienceSizeexplicitly anticipatesthis.contextbeing falsy ("so we don't break the PIG demo"), it's worth adding a case that rendersPushAudienceFilterwithout any provider to catch the null-context crash flagged inPushAudienceFilter.react.js.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/tests/PushAudienceDialog.test.js` around lines 82 - 144, Add a regression test in the PushAudienceFilter suite that renders the component without CurrentApp.Provider, using the existing required props, and verifies rendering and basic callback interaction do not crash when context is absent. Keep the existing provider-wrapped test unchanged and target the null-context path anticipated by PushAudienceDialog.fetchAudienceSize and PushAudienceFilter.react.js.src/components/PushAudienceDialog/PushAudienceDialog.react.js (1)
102-112: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueField-selection order can diverge from the rendered dropdown order.
The guard against an empty
availablemap is correct. However,keys[0]uses rawObject.keys(available)order, whilePushAudienceFiltersorts the sameavailablemap withcompareFields/column-preference priority when rendering the dropdown for that row. The newly-added condition's default field may therefore not match the option that visually appears first in its own dropdown.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/PushAudienceDialog/PushAudienceDialog.react.js` around lines 102 - 112, Update the new-condition field selection in PushAudienceDialog to use the same compareFields and column-preference ordering that PushAudienceFilter uses when rendering available fields, rather than relying on Object.keys(available) order. Preserve the existing empty-available guard and select the first field from the sorted options so the default matches the dropdown.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/PushAudienceDialog/PushAudienceDialog.react.js`:
- Around line 295-309: Pass the `_Installation` class identifier to the
`PushAudienceFilter` instance in `PushAudienceDialog`, using the same class name
hardcoded by `fetchAudienceSize`, so `filterSortToTop` can apply the
corresponding column preference.
In `@src/components/PushAudienceFilter/PushAudienceFilter.react.js`:
- Around line 64-68: Guard both uses of currentApp in PushAudienceFilter: safely
read columnPreference in the React.useMemo body and dependency array when
CurrentApp is null, preserving the existing behavior when a provider is present
and preventing first-render crashes without a CurrentApp.Provider.
- Around line 64-101: Guard the CurrentApp access in PushAudienceFilter’s
useMemo body and dependency array so rendering remains safe when the context is
null, while preserving existing column-preference sorting when provided. In
src/components/PushAudienceFilter/PushAudienceFilter.react.js lines 64-101,
update both references to currentApp.columnPreference; in
src/lib/tests/PushAudienceDialog.test.js lines 82-144, add coverage rendering
PushAudienceFilter without a CurrentApp.Provider.
- Around line 17-30: Update changeField to resolve the effective comparison type
through the same constraint-aware logic used by changeConstraint and the
row-render path, consulting Filters.Constraints[constraint].field before
selecting Filters.DefaultComparisons and validating the existing compareTo type.
Reuse the shared resolver if one exists, so constraints such as exists
consistently use their override type when switching fields.
---
Nitpick comments:
In `@src/components/PushAudienceDialog/PushAudienceDialog.react.js`:
- Around line 102-112: Update the new-condition field selection in
PushAudienceDialog to use the same compareFields and column-preference ordering
that PushAudienceFilter uses when rendering available fields, rather than
relying on Object.keys(available) order. Preserve the existing empty-available
guard and select the first field from the sorted options so the default matches
the dropdown.
In `@src/components/PushAudienceFilter/PushAudienceFilter.react.js`:
- Around line 71-99: Default compareFields to stringCompare before the
currentColumnPreference check, then retain the priority-aware comparator only
when preferences exist. Ensure sortedFields and per-row fallback sorting
consistently use stringCompare when no preference is configured.
In `@src/lib/tests/PushAudienceDialog.test.js`:
- Around line 82-144: Add a regression test in the PushAudienceFilter suite that
renders the component without CurrentApp.Provider, using the existing required
props, and verifies rendering and basic callback interaction do not crash when
context is absent. Keep the existing provider-wrapped test unchanged and target
the null-context path anticipated by PushAudienceDialog.fetchAudienceSize and
PushAudienceFilter.react.js.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fd8fbea1-aa07-4391-8a2e-06d49a5381f6
📒 Files selected for processing (4)
src/components/PushAudienceDialog/InstallationCondition.react.jssrc/components/PushAudienceDialog/PushAudienceDialog.react.jssrc/components/PushAudienceFilter/PushAudienceFilter.react.jssrc/lib/tests/PushAudienceDialog.test.js
💤 Files with no reviewable changes (1)
- src/components/PushAudienceDialog/InstallationCondition.react.js
1eb2f8e to
2f746d3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/PushAudienceDialog/PushAudienceDialog.react.js`:
- Around line 304-310: Update the filter action handlers in PushAudienceDialog,
including onChange, onSearch, and the successful add-filter path, to clear
errorMessage whenever the action succeeds. Ensure onSearch resets the error
before or alongside fetchAudienceSize, and the add path clears the stale error
while appending the new filter.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b340b455-37ef-4931-9999-d016eb4b3c69
📒 Files selected for processing (4)
src/components/PushAudienceDialog/PushAudienceDialog.react.jssrc/lib/Filters.jssrc/lib/tests/Filters.findRelatedClasses.test.jssrc/lib/tests/PushAudienceDialog.test.js
1ffe26f to
d46c525
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/PushAudienceDialog/PushAudienceDialog.react.js (1)
330-336: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake error visibility use the same dismissible source.
The new handlers clear
state.errorMessage, but a non-emptyprops.errorMessagestill keeps the note visible and takes message precedence. A retry can therefore continue showing a stale external error—or mask the new “No condition available.” error—after a successful filter action.Proposed direction
- show={Boolean( - (this.props.errorMessage && this.props.errorMessage.length > 0) || - (this.state.errorMessage && this.state.errorMessage.length > 0) - )} + show={Boolean(this.state.errorMessage && this.state.errorMessage.length > 0)}Move prop errors into the same local error lifecycle if they must be shown initially, and add a regression test for retrying with
errorMessagesupplied as a prop.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/PushAudienceDialog/PushAudienceDialog.react.js` around lines 330 - 336, Update the error lifecycle around PushAudienceDialog’s error note and its retry/filter handlers so prop-provided errors are initialized into the dismissible local error state rather than independently controlling visibility or message precedence. Ensure clearing state.errorMessage after a successful retry hides stale external errors and allows “No condition available.” to display, and add a regression test covering a retry when errorMessage is supplied via props.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/components/PushAudienceDialog/PushAudienceDialog.react.js`:
- Around line 330-336: Update the error lifecycle around PushAudienceDialog’s
error note and its retry/filter handlers so prop-provided errors are initialized
into the dismissible local error state rather than independently controlling
visibility or message precedence. Ensure clearing state.errorMessage after a
successful retry hides stale external errors and allows “No condition
available.” to display, and add a regression test covering a retry when
errorMessage is supplied via props.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9e0ed41e-a7b4-4723-8657-292fddf3ee66
📒 Files selected for processing (4)
src/components/PushAudienceDialog/PushAudienceDialog.react.jssrc/lib/Filters.jssrc/lib/tests/Filters.findRelatedClasses.test.jssrc/lib/tests/PushAudienceDialog.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/tests/Filters.findRelatedClasses.test.js
d46c525 to
ec7aca0
Compare
|
@coderabbitai All comments are handled! |
|
Tip For best results, initiate chat on the files or code changes.
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
Noted and handled! |
|
@coderabbitai I don't understand what is the request that you want me to change. The PR is blocked because there's one requested change from @coderabbitai |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
component=$(fd -a '^PushAudienceDialog\.react\.js$' src | head -n 1)
test_file=$(fd -a '^PushAudienceDialog\.test\.js$' src | head -n 1)
printf '%s\n' '=== Component outline ==='
ast-grep outline "$component" --items all
printf '%s\n' '=== Error lifecycle references ==='
rg -n -C 5 'errorMessage|handleAddCondition|onChange|onSearch|componentWillMount' "$component"
printf '%s\n' '=== Relevant component range ==='
sed -n '1,380p' "$component"
printf '%s\n' '=== Test references ==='
rg -n -C 4 'errorMessage|onSearch|onChange|handleAddCondition|componentWillMount' "$test_file"Length of output: 21452
The earlier requested-change finding is therefore obsolete and should not block this PR. Sorry for the confusing duplicate/outdated request. [resolve] You are interacting with an AI system. |
|
@coderabbitai can we re-run the review so the old message is removed. |
|
✅ Action performedFull review finished. |
|
@coderabbitai Why I'm getting this message "Merging is blocked. All comments must be resolved. 1 review requesting changes by reviewers with write access." |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="parse-community/parse-dashboard"
pr="3431"
printf '%s\n' '=== Pull request mergeability and review decision ==='
gh api "repos/$repo/pulls/$pr" \
--jq '{number, state, draft, mergeable, mergeable_state, review_comments, requested_reviewers: [.requested_reviewers[].login], head: .head.sha}'
printf '%s\n' '=== Reviews (including current state and author) ==='
gh api --paginate "repos/$repo/pulls/$pr/reviews?per_page=100" \
--jq '.[] | {id, user: .user.login, state, submitted_at, commit_id, body}'
printf '%s\n' '=== CodeRabbit review comments and resolution state ==='
gh api graphql -F owner="parse-community" -F name="parse-dashboard" -F pr="$pr" -f query='
query($owner: String!, $name: String!, $pr: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $pr) {
reviewDecision
reviewThreads(first: 100) {
nodes {
isResolved
comments(first: 20) {
nodes {
author { login }
body
pullRequestReview { id state }
}
}
}
}
}
}
}' --jq '.data.repository.pullRequest | {
reviewDecision,
threads: [
.reviewThreads.nodes[]
| select(any(.comments.nodes[]?; .author.login == "coderabbitai"))
| {
isResolved,
comments: [
.comments.nodes[]
| select(.author.login == "coderabbitai")
| {body, reviewState: .pullRequestReview.state}
]
}
]
}'Length of output: 42236
GitHub still sees CodeRabbit’s earlier reviews as CHANGES_REQUESTED (review IDs There are also three unresolved CodeRabbit threads from those earlier reviews:
The withdrawn To unblock the PR, either:
Re-running the review alone does not remove a prior GitHub CHANGES_REQUESTED review. You are interacting with an AI system. |
|
@coderabbitai autofix |
|
This command requires write access to the repository. Only users with write or admin permissions can trigger CodeRabbit to commit or create pull requests. |
|
@coderabbitai resolve |
✅ Action performedComments resolved and changes approved. |
1 participant
Pull Request
Issue
Creating or editing a push audience could crash Parse Dashboard and display a blank page.
The shared
Filtercomponent requires class and schema information after the relational-filter refactor,but
PushAudienceDialogdid not provide it. Existing saved filters could also be missing their classinformation.
Approach
This PR combines the strongest parts of #3429 and #2983:
Filtercomponent instead of maintaining a separate push-audience filterimplementation.
_Installationclass and its schema.class: '_Installation'to newly created audience conditions.findRelatedClassesfrom failing whenallClassesis unavailable.Test Coverage
Regression tests cover:
_Installation.Credits
This work builds on:
Thank you to both authors for their original investigation and implementations.
Validation
Tasks
Summary by CodeRabbit
Bug Fixes
Tests