You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
We reviewed changes in 1e8df4e...9692d2d on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.
Some issues found as part of this review are outside of the diff in this pull request and aren't shown in the inline review comments due to GitHub's API limitations. You can see those issues on the DeepSource dashboard.
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
Your organization has run out of usage credits. Purchase more credits in the billing tab to continue.
⌛ How to resolve this issue?
After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.
We recommend that you space out your commits to avoid hitting the rate limit.
🚦 How do rate limits work?
CodeRabbit enforces hourly rate limits for each developer per organization.
Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.
Reviewing files that changed from the base of the PR and between e6e77b8 and 9692d2d.
📒 Files selected for processing (1)
paypal/js/frontend.js
📝 Walkthrough
Walkthrough
Apple Pay SDK eligibility discovery is reworked with bounded async readiness checks that wait for SDK availability and cache the instance/config, using an upgraded waitFor() utility supporting Promise-returning predicates. Button rendering now guards against missing ApplePaySession with an inline error. Card-field iframe height adjustment is narrowed to a reasonable pixel range and optimized to avoid repeated observer disconnect/reconnect cycles.
Changes
Apple Pay SDK Eligibility & Rendering
Layer / File(s)
Summary
Apple Pay SDK Readiness & Utility Upgrade paypal/js/frontend.js
The waitFor() utility is upgraded to support async predicates returning Promises. The new resolveApplePayEligibility() function performs bounded stepwise checks (ApplePaySession, canMakePayments(), PayPal SDK, paypal.Applepay function, and successful config() call) and caches instance/config on success. The legacy checkApplePayEligibility() function is removed.
Apple Pay Button Rendering Safety Check paypal/js/frontend.js
Button rendering adds a runtime guard: when ApplePaySession is unavailable, an inline error message is written to the container and the function returns early without rendering the button component.
Card Field Height Handling
Layer / File(s)
Summary
MutationObserver Height Adjustment Range paypal/js/frontend.js
The iframe height observer callback is optimized to avoid repeated disconnect/reconnect cycles. The 1px height adjustment now applies only when the measured offsetHeight falls within a tighter "reasonable" range (>40 and <100 pixels) instead of for any positive height.
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Strategy11/formidable-forms#3130: Modifies paypal/js/frontend.js Apple Pay/GPay SDK readiness discovery using bounded waitFor polling and adjusts Apple Pay option registration and rendering.
Strategy11/formidable-forms#3102: Overlaps at Apple Pay session-setup by introducing/using cached Apple Pay SDK instance state in the SDK readiness and click handler flow.
Poem
🐰 A wild async awaits the Apple Pay day,
When SDK readiness's checked—no errors in the way!
Heights adjust just right, observers stay quite still,
The button renders safely now, by guard and guarded will. ✨
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
✅ Passed
The title 'Update paypal apple pay eligibility checks' accurately reflects the main change in the changeset, which focuses on refactoring Apple Pay eligibility discovery and rendering logic.
Docstring Coverage
✅ Passed
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check
✅ Passed
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
✅ Passed
Check skipped because no linked issues were found for this pull request.
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches🧪 Generate unit tests (beta)
Create PR with unit tests
Commit unit tests in branch update_apple_pay_eligiblity_checks
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.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
paypal/js/frontend.js (2)
421-432: ⚡ Quick win
Async callback in setInterval can cause overlapping executions.
The setInterval callback is async, but setInterval doesn't await its callback. If checkPredicate() takes longer than the interval (50ms), multiple invocations can overlap. While the current predicates are likely fast enough, this could cause unexpected behavior with slower async predicates.
Consider using a recursive setTimeout pattern instead:
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@paypal/js/frontend.js` around lines 421 - 432, The current Promise uses an
async callback inside setInterval which can overlap when checkPredicate() is
slow; replace the setInterval pattern with a recursive setTimeout poll loop:
create an inner async function (e.g., poll) that awaits checkPredicate(),
resolves the Promise when it's true, checks elapsed time against timeout to
resolve false, and otherwise schedules the next invocation with setTimeout(poll,
interval); update the Promise at the start (where start, timeout, interval and
checkPredicate are used) to call poll() instead of using setInterval so
overlapping executions cannot occur.
379-396: ⚡ Quick win
Redundant config() call and instance creation.
The async predicate (lines 379-387) successfully creates an instance and calls config() to verify eligibility, but these are discarded. Lines 394-396 then create a new instance and call config() again. This doubles the API calls unnecessarily.
Consider caching directly in the predicate to avoid the redundant call:
♻️ Suggested refactor to avoid double config() call
// Wait for the config call to actually succeed, not just the function to exist.
// This handles the race condition where the function exists but SDK isn't fully initialized.
+ let tempInstance = null;+ let tempConfig = null;
const configReady = await waitFor( async () => {
try {
const instance = paypal.Applepay();
const config = await instance.config();
- return config?.isEligible;+ if ( config?.isEligible ) {+ tempInstance = instance;+ tempConfig = config;+ return true;+ }+ return false;
} catch ( e ) {
return false;
}
} );
if ( ! configReady ) {
return false;
}
// Config succeeded, cache it and return true.
- applePayInstance = paypal.Applepay();- applePayConfig = await applePayInstance.config();+ applePayInstance = tempInstance;+ applePayConfig = tempConfig;
return true;
🤖 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 `@paypal/js/frontend.js` around lines 379 - 396, The predicate passed to
waitFor currently creates an instance and calls paypal.Applepay().config(), but
those results are discarded and later code recreates them, causing duplicate API
calls; modify the predicate used in waitFor so that when it successfully obtains
an instance and a config (i.e., paypal.Applepay() and instance.config()), it
assigns them to the outer-scope variables applePayInstance and applePayConfig
and then returns true, and leave the fallback path to return false on
error—remove the later paypal.Applepay() and config() calls so the cached
applePayInstance/applePayConfig are reused.
🤖 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 `@paypal/js/frontend.js`:
- Around line 797-809: The observerCallback currently mutates
mutation.target.style.height which retriggers the mutation and causes a runaway
loop; modify observerCallback to skip elements already adjusted by either (A)
checking/setting a marker like mutation.target.dataset.paypalHeightAdjusted (set
it when you increase the height so subsequent mutations ignore that element) or
(B) temporarily suspending the observer around the style change (call the
MutationObserver instance's disconnect(), change mutation.target.style.height,
then re-observe the target) and only perform the +1px adjustment when the marker
is not present or while disconnected; update/remove the marker as needed if you
expect future legitimate height changes.
---
Nitpick comments:
In `@paypal/js/frontend.js`:
- Around line 421-432: The current Promise uses an async callback inside
setInterval which can overlap when checkPredicate() is slow; replace the
setInterval pattern with a recursive setTimeout poll loop: create an inner async
function (e.g., poll) that awaits checkPredicate(), resolves the Promise when
it's true, checks elapsed time against timeout to resolve false, and otherwise
schedules the next invocation with setTimeout(poll, interval); update the
Promise at the start (where start, timeout, interval and checkPredicate are
used) to call poll() instead of using setInterval so overlapping executions
cannot occur.
- Around line 379-396: The predicate passed to waitFor currently creates an
instance and calls paypal.Applepay().config(), but those results are discarded
and later code recreates them, causing duplicate API calls; modify the predicate
used in waitFor so that when it successfully obtains an instance and a config
(i.e., paypal.Applepay() and instance.config()), it assigns them to the
outer-scope variables applePayInstance and applePayConfig and then returns true,
and leave the fallback path to return false on error—remove the later
paypal.Applepay() and config() calls so the cached
applePayInstance/applePayConfig are reused.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d8854200-ec10-4d86-b17b-ee67994dbc69
📥 Commits
Reviewing files that changed from the base of the PR and between 1e8df4e and e6e77b8.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Potential runaway loop: height adjustment triggers new mutations.
Setting mutation.target.style.height triggers another mutation, causing the callback to fire again. Since there's no check to prevent re-adjustment of an already-adjusted height, this will keep incrementing until currentHeight >= 100:
PayPal sets height to 50px → callback adjusts to 51px
Mutation fires → callback adjusts to 52px
Repeats until height reaches 100px
The comment mentions "not already adjusted" but there's no such guard in the code.
🐛 Proposed fix to prevent re-adjustment
const observerCallback = ( mutationsList ) => {
for ( const mutation of mutationsList ) {
if ( mutation.type !== 'attributes' || mutation.attributeName !== 'style' ) {
continue;
}
const currentHeight = mutation.target.offsetHeight;
+ const currentStyle = mutation.target.style.height;+ // Skip if we already adjusted (style ends with our +1 adjustment)+ if ( currentStyle && currentStyle === `${ currentHeight }px` ) {+ continue;+ }
// Only adjust if height is reasonable and not already adjusted.
if ( currentHeight > 40 && currentHeight < 100 ) {
mutation.target.style.height = `${ currentHeight + 1 }px`;
}
}
};
Alternatively, track adjusted elements or disconnect during adjustment:
+ const adjustedHeights = new WeakMap();
const observerCallback = ( mutationsList ) => {
for ( const mutation of mutationsList ) {
if ( mutation.type !== 'attributes' || mutation.attributeName !== 'style' ) {
continue;
}
const currentHeight = mutation.target.offsetHeight;
+ if ( adjustedHeights.get( mutation.target ) === currentHeight + 1 ) {+ continue;+ }
// Only adjust if height is reasonable and not already adjusted.
if ( currentHeight > 40 && currentHeight < 100 ) {
+ adjustedHeights.set( mutation.target, currentHeight + 1 );
mutation.target.style.height = `${ currentHeight + 1 }px`;
}
}
};
🤖 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 `@paypal/js/frontend.js` around lines 797 - 809, The observerCallback currently
mutates mutation.target.style.height which retriggers the mutation and causes a
runaway loop; modify observerCallback to skip elements already adjusted by
either (A) checking/setting a marker like
mutation.target.dataset.paypalHeightAdjusted (set it when you increase the
height so subsequent mutations ignore that element) or (B) temporarily
suspending the observer around the style change (call the MutationObserver
instance's disconnect(), change mutation.target.style.height, then re-observe
the target) and only perform the +1px adjustment when the marker is not present
or while disconnected; update/remove the marker as needed if you expect future
legitimate height changes.
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
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.
Crabcyborg
changed the title
![deepsource-io[bot]](https://avatars.githubusercontent.com/in/16372?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Still seeing some issues with Apple Pay after merging https://github.com/Strategy11/formidable-forms/pull/3130/changes
Summary by CodeRabbit