Remove the login tx delay #84
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (1)
WalkthroughMoves login trigger from PayButton onSuccess to onClose. Stores transaction data in a closure-scoped Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant PB as PayButton
participant JS as renderLoginPaybutton (closure)
participant S as Site/Auth
U->>PB: Open payment UI & pay
PB->>JS: onSuccess(tx)
note right of JS #F0F4C3: Store tx.inputAddresses in closure-scoped\ntransactionAttrs (no login yet)
PB-->>U: Auto-close (autoClose: true)
PB->>JS: onClose()
alt transactionAttrs.inputAddresses[0] exists
JS->>S: handleLogin(inputAddresses[0])
S-->>JS: Login result
else
JS-->>JS: No address → no login
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
xecdev
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
assets/js/paybutton-paywall-cashtab-login.js (2)
48-48: Initialize shared tx state explicitly (and document intent).Avoids implicit undefined and makes later cleanup straightforward.
-let transactionAttrs; +// Shared state: last successful PayButton tx captured in onSuccess, consumed in onClose. +let transactionAttrs = null;
61-66: Harden address extraction and clear state after use to prevent duplicate logins.Minor robustness: guard array type, downgrade console noise, and null out the latched tx so a second onClose (or re-render) can’t retrigger handleLogin.
- onClose: function () { - console.log('Login Payment TX:', transactionAttrs); - if (transactionAttrs && transactionAttrs.inputAddresses && transactionAttrs.inputAddresses.length > 0) { - const userAddress = transactionAttrs.inputAddresses[0]; - handleLogin(userAddress); - } - } + onClose: function () { + if (transactionAttrs) { + if (window.console) console.debug('Login Payment TX:', transactionAttrs); + if (Array.isArray(transactionAttrs.inputAddresses) && transactionAttrs.inputAddresses.length > 0) { + const userAddress = transactionAttrs.inputAddresses[0]; + handleLogin(userAddress); + // Consume the latched tx to avoid duplicate logins on repeated close events + transactionAttrs = null; + } else { + if (window.console) console.warn('PayButton tx missing inputAddresses; login not triggered.'); + } + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
assets/js/paybutton-paywall-cashtab-login.js(2 hunks)
🔇 Additional comments (2)
assets/js/paybutton-paywall-cashtab-login.js (2)
57-58: Good call switching to onClose with autoClose to replace the ad-hoc delay.This should eliminate timing flakiness from the sound/UX perspective.
58-60: Confirm tx shape and event-ordering assumptions.Code in assets/js/paybutton-paywall-cashtab-login.js (lines 58–60) assumes the success callback receives a tx object with inputAddresses and that onSuccess fires before onClose when autoClose=true — both vary by PayButton implementation/version.
- Verify the success callback’s tx contains inputAddresses (or the exact equivalent) for the PayButton JS you use.
- Verify onSuccess is guaranteed to run before onClose when autoClose=true; if not, update the logic to handle login data on whichever event occurs.
Provide the PayButton JS repo/docs or package name + version for concrete verification.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
assets/js/paybutton-paywall-cashtab-login.js (3)
48-49: Scope transactionAttrs locally to avoid global leakage.Keep the shared state inside renderLoginPaybutton; it’s only used by its callbacks.
Apply:
-// Shared state: last successful PayButton tx captured in onSuccess, consumed in onClose. -let transactionAttrs = null; function renderLoginPaybutton() { + // Shared state: last successful PayButton tx captured in onSuccess, consumed in onClose. + let transactionAttrs = null;I can push a tiny commit with this change if you like.
Also applies to: 51-51
62-66: Clear the stored tx after close to prevent stale retries.Avoid reusing an old tx if the modal is reopened or the login POST fails silently.
Apply:
- onClose: function () { - if (transactionAttrs && transactionAttrs.inputAddresses && transactionAttrs.inputAddresses.length > 0) { - const userAddress = transactionAttrs.inputAddresses[0]; - handleLogin(userAddress); - } - } + onClose: function () { + const addr = transactionAttrs?.inputAddresses?.[0]; + if (addr) { + handleLogin(addr); + } + // Prevent stale reuse on subsequent opens + transactionAttrs = null; + }
60-61: Persist only what you need.You only consume inputAddresses[0]; store the minimal slice to keep state simple.
Apply:
- transactionAttrs = tx; + // Persist only the needed field(s) to minimize shared state surface + transactionAttrs = { inputAddresses: tx?.inputAddresses ?? [] };And the onClose change above already reads from transactionAttrs.inputAddresses.
Also applies to: 63-65
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
assets/js/paybutton-paywall-cashtab-login.js(3 hunks)
🔇 Additional comments (2)
assets/js/paybutton-paywall-cashtab-login.js (2)
58-66: LGTM: switching to onClose with autoClose removes the brittle delay.This should preserve the success sound and avoid timing quirks.
58-58: Confirm autoClose → onClose ordering across browsers/versionsVerify which PayButton implementation/version this code uses and confirm that PayButton.render supports the autoClose option and guarantees onClose fires after a successful payment (especially Safari/iOS). If the library/docs/behaviour are unclear or onClose is not reliable, add a minimal fallback: invoke handleLogin from onSuccess (or call it when autoClose is unavailable).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
assets/js/paybutton-paywall-cashtab-login.js (2)
49-50: Good shift to onClose; simplify shared state to a single string (address).You only consume
inputAddresses[0], so store just that to reduce nesting and potential stale object reuse.- // Shared state: last successful PayButton tx captured in onSuccess, consumed in onClose. - let transactionAttrs = null; + // Shared state: login address captured in onSuccess, consumed in onClose. + let loginAddr = null; @@ - onSuccess: function (tx) { - transactionAttrs = { inputAddresses: tx?.inputAddresses ?? [] }; - }, + onSuccess: function (tx) { + loginAddr = tx?.inputAddresses?.[0] ?? null; + }, onClose: function () { - const addr = transactionAttrs?.inputAddresses?.[0]; - if (addr) { - handleLogin(addr); - } - // Prevent stale reuse on subsequent opens - transactionAttrs = null; + if (loginAddr) { + handleLogin(loginAddr); + } + // Prevent stale reuse on subsequent opens + loginAddr = null; }Also applies to: 60-69
57-57: Consider i18n for user-facing text.“Login Successful!” should be localizable (and maybe align punctuation with other strings). If you have a localization pathway, route this through it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
assets/js/paybutton-paywall-cashtab-login.js(2 hunks)
🔇 Additional comments (2)
assets/js/paybutton-paywall-cashtab-login.js (2)
79-79: No-op formatting/touch.Looks fine; nothing to change.
58-58: Provide PayButton.render docs/package name to verify autoClose/onClose ordering.
Need the PayButton.render docs URL or the npm/package name used in this repo to confirm whether onSuccess → (success audio) → onClose ordering is guaranteed with autoClose: true; autoClose may clip the success sound. File: assets/js/paybutton-paywall-cashtab-login.js (lines 58, 62–69).
This PR improves the Cashtab login process speed by removing the manual delay after the 'login with Cashtab' tx (for the success sound cut-off) and replacing the whole logic by using the transaction attrs from onSuccess callback in the onClose func.
Test plan:
Deployed on the test site.
Summary by CodeRabbit