fix the database connection is closing issue.#748
fix the database connection is closing issue.#748Krishna2323 wants to merge 5 commits intoExpensify:mainfrom
Conversation
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
All contributors have signed the CLA ✍️ ✅ |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
@Krishna2323 let me know once it's ready for review! |
|
@fabioh8010 you can start reviewing the code. I'll add the app test recordings soon. |
|
@fabioh8010 can you please review when you get a time for it? thanks |
|
Saving to review tomorrow morning 🫡 |
|
Reviewing |
| // It seems like Safari sometimes likes to just close the connection. | ||
| // It's supposed to fire this event when that happens. Let's hope it does! | ||
| // eslint-disable-next-line no-param-reassign | ||
| db.onclose = () => { |
There was a problem hiding this comment.
Coul we improve the comment here to be more explanatory? An external link about it is also appreciated
|
|
||
| return (txMode, callback) => | ||
| executeTransaction(txMode, callback).catch((error) => { | ||
| if (error instanceof DOMException && error.name === 'InvalidStateError') { |
There was a problem hiding this comment.
We should have a comment explaining why we need this check
|
@Krishna2323 What's the latest here? |
|
Apologies for the delay 🙏🏻 — I missed this earlier. Reviewing the comments now. |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
| // If the connection was closed between getDB() resolving and db.transaction() executing, | ||
| // the transaction throws InvalidStateError. We catch it and retry once with a fresh connection. | ||
| return (txMode, callback) => | ||
| executeTransaction(txMode, callback).catch((error) => { | ||
| if (error instanceof DOMException && error.name === 'InvalidStateError') { |
There was a problem hiding this comment.
| // If the connection was closed between getDB() resolving and db.transaction() executing, | |
| // the transaction throws InvalidStateError. We catch it and retry once with a fresh connection. | |
| return (txMode, callback) => | |
| executeTransaction(txMode, callback).catch((error) => { | |
| if (error instanceof DOMException && error.name === 'InvalidStateError') { | |
| return (txMode, callback) => | |
| executeTransaction(txMode, callback).catch((error) => { | |
| // If the connection was closed between getDB() resolving and db.transaction() executing, | |
| // the transaction throws InvalidStateError. We catch it and retry once with a fresh connection. | |
| if (error.name === 'InvalidStateError') { |
- Are we sure it's always instance of
DOMException? I would remove it. - Also I think it's best to leave comment closer to IF.
|
@tgolen mind reviewing since Vit is OOO? Thanks! |
Reviewer Checklist
Automated TestsScreenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safariweb.mov |
|
Codex Review: Didn't find any major issues. Keep it up! ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
@Krishna2323 I followed 2. Cross-tab recovery on |
Details
Related Issues
GH_LINK Expensify/App#84192
Automated Tests
Manual Tests
1. Basic persistence (no regression)
InvalidStateErroror new errors should appear2. Cross-tab recovery
onversionchangehandler closes the stale connection and the next operation opens a fresh oneAuthor Checklist
### Related Issuessection aboveTestssectiontoggleReportand notonIconClick)myBool && <MyComponent />.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)Avataris modified, I verified thatAvataris working as expected in all cases)mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop