fix: fetch last 10 average when enabling showAverage (@d1rshan)#7963
fix: fetch last 10 average when enabling showAverage (@d1rshan)#7963d1rshan wants to merge 2 commits into
Conversation
| } | ||
|
|
||
| if (Config.showAverage !== "off") { | ||
| if (!Last10Average.hasData()) { |
There was a problem hiding this comment.
Could we listen to config events for showAverage in elements/last-10-average and update in case it is not set to off? if the data is already fetched it won't fetch again and calculating the average should be cheap enough.
I think the current implementation with additional hasFetchstate will break because it is never set back to false.
There was a problem hiding this comment.
yeah the hasFetch approach fails because we didn't reset it, sry didnt think about this scenario.
i tried the approach u asked for and there is a race condition #7963 (review).
there is a work around of doing this -> void update().then(() => ModesNotice.update()) but this will trigger modes notice update twice, i'm not sure if this is okay tbh.
or we can directly fix the issue by modiying the configEvent.subscribe in modes-notice.ts to trigger update for Last10Average and this wont call the ModeNotice.update() twice.
monkeytype/frontend/src/ts/elements/modes-notice.ts
Lines 43 to 45 in b570bd1
pls let me know which approach i should go with, i will update the fix accordingly. thanks!
2231f72 to
a082dee
Compare
Leonabcd123
left a comment
There was a problem hiding this comment.
Not sure how you tested this, but it's still broken for me when following the reproduction steps in the bug report. There seems to be a race condition going on between the update function in last-10-average and update in modes-notice. This:
monkeytype/frontend/src/ts/elements/modes-notice.ts
Lines 186 to 187 in b570bd1
Runs before this:
monkeytype/frontend/src/ts/elements/last-10-average.ts
Lines 17 to 18 in 2f00391
|
hey @Leonabcd123 yeah ik i thought it worked because i left a stale line in modes-notice.ts to update everytime 😭 (that is what i force pushed ) i realized about the race condition, i'm figuring out what is the best solution here. we could do this, but will trigger ModesNotice.update() twice, so not sure if this is a good idea. configEvent.subscribe(({ key, newValue }) => { |
Description
Modes notice for last 10 average was not updating when changing showAverage from "off" to a different value after page reload because
Last10Average.update()never ran (it's only called during test restart) and data stayed at default values (wpm = 0, acc = 0) and hence button was hidden.Fix
Closes #7961