Guard Meals.init against overlapping calls#961
Open
munzzyy wants to merge 2 commits into
Open
Conversation
The meal picker's Meals tab had no re-entrancy guard on init(), so if it got triggered again before a previous init() finished resolving (re-entering the Diary "Add meal" flow, switching tabs quickly, etc.) the two overlapping getListFromDB()/renderList() calls raced each other. Both cleared and re-appended the DOM independently, which left duplicate <li> entries in the list, including a duplicated "Yesterday's X" entry pulled from the diary history. Picking one of the duplicates could then add whichever item the stale render left in that slot instead of the one the user tapped. init() now tracks its own in-flight promise. A second call while one is still running just awaits the first instead of starting a second fetch/render cycle, so the list only ever gets built once per visit.
Contributor
|
Thanks, this looks good. But the init functions for the foodlist and for recipes work exactly the same as for meals, so they will have the same problem. It makes no sense to only fix this problem for the meals tab and not for the others. Please apply the fix to the recipes and foodlist as well. |
The foodlist and recipes tabs had the same unguarded init() as the meals tab: if init() got triggered again before a previous call's getListFromDB()/renderList() had resolved, the two overlapping calls raced each other and left duplicate <li> entries in the list. Applies the same in-flight-promise guard used for Meals.init to both Foodlist.init and Recipes.init. Each now delegates to its own runInit(), and a second call while one is already running just returns the same promise instead of starting a second fetch/render cycle.
Author
|
Fair point, done. Applied the same initPromise guard to Foodlist.init and Recipes.init - same shape as the meals fix, init() now just delegates to a runInit() and returns the in-flight promise if one's already running. Checked the syntax on all three files and wrote a small standalone script mimicking the guard to confirm overlapping calls collapse into one runInit while a later, non-overlapping call still re-fetches normally. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #942.
The Meals tab in the meal picker (Diary -> Dinner -> Add meal -> Meals) had no guard against
app.Meals.init()running twice concurrently. If it gets triggered again before a previous call has finished - re-entering the Diary "Add meal" flow, switching tabs quickly, anything that firestab:initagain while the firstgetListFromDB()/renderList()pass is still awaiting IndexedDB - the two calls race each other. Each one clears and rebuilds the list independently, and depending on how the clears and appends interleave you end up with duplicate<li>rows in the DOM, including a duplicated "Yesterday's Dinner" row pulled from diary history. Since the checkbox selection is keyed off whatever ended up rendered in that slot, tapping one of the duplicates can add a different item than the one you actually picked.I reproduced this locally in browser mode by seeding a diary entry for "yesterday" and calling
app.Meals.init()twice without awaiting the first call - reliably got 2-3 duplicate rows in the DOM for a single logical item, matching the duplicate-entry behavior described in the issue. I didn't try to reproduce the differing-kcal detail from the screenshot specifically - that'd come from two independentgetTotalNutrition()calls landing in the two interleaved render passes, which is consistent with a race but I didn't verify that exact path. A normal single visit to the tab, tab bouncing, and the search bar all still rendered a single copy of each item.The fix:
init()now stores its own in-flight promise. If it's called again while that promise is still pending, the second call just returns the same promise instead of kicking off a second fetch-and-render cycle. Once it settles the guard clears, so the next visit re-fetches normally - sequential re-inits were never the problem here, sincerenderList(true)already clears the list before repopulating it. It's only overlapping calls that race and leave duplicates behind.I ran this against Cordova's browser platform (
cordova platform add browser,cordova run browser) since I don't have an Android device set up - couldn't run it throughcordova build androidon my end. Didn't touchFoodlist.js, which has the same unguarded pattern, since it's outside what's reported here - happy to follow up separately if that's wanted.