Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate globalUserSettings, dictionaryIndex and more to jotai #175

Merged
merged 21 commits into from
Aug 1, 2024

Conversation

na2hiro
Copy link
Contributor

@na2hiro na2hiro commented Jul 9, 2024

This is a continuation of App breakdown & state migration to Jotai. Mostly each commit corresponds to each state that's migrated to Jotai atom.

Though I've quickly checked there's no errors on storybook, I couldn't have enough time to test using the app itself, because I've gotten busier recently (in fact, most of the changes were made in May). I thought it's rather better to share and not lose the progress I made so far or diverge too much from master.

For lessons without an overview file, the overview key is not included
in the lesson index entry.
Previously, when you clicked the "Study" button this would call
startFlashcards and updateFlashcardsRecommendation() to progress the
flashcards recommendations. The combination of that button and the
recommendation progression on first render would inadvertently take 2
steps instead of 1. Now, we don't explicitly update the recommendation
on click of "Study" and instead rely on the useEffect calling
updateFlashcardsRecommendation() when flashcardsCourseLevel changes and
on first render.

Altogether, we progress the recommendation by 1 step when:

- you click "Study" and then navigate back to Progress page
- you click "Skip"
- you select a flashcards level
@didoesdigital
Copy link
Owner

@na2hiro Woohoo! This looks great, nice work. Thanks for this.

In the previous work to migrate to Jotai, there was a change to study presets. After that change, I started seeing Sentry errors from trying to access [0], [1], [2], [3] on studyPresets. I could not reproduce the errors locally at all and I couldn't identify a reason why studyPresets wouldn't be present and a valid array of 4 items. I made a change in ab7590e to squash the errors but it probably didn't really fix the issue, just prevent the app from exploding over it.

I mention that here because I'm not sure if the changes to experiments in this PR could lead to a similar issue. Any ideas about that?

@didoesdigital
Copy link
Owner

@na2hiro I also reviewed all the code in the PR, which looks good to me. And I pushed some minor commits, I hope you don't mind.

@na2hiro
Copy link
Contributor Author

na2hiro commented Jul 15, 2024

Thanks for reviewing! I don't have an idea for the reason why studyPresets can be non-array. I'll think about it for a couple more days

@didoesdigital
Copy link
Owner

@na2hiro any further thoughts on this? If not, I might add some extra guards around experiments in this PR, merge it, and hope for the best.

@na2hiro
Copy link
Contributor Author

na2hiro commented Jul 30, 2024

Sorry for the delay. I don't have good ideas

The approach to using "Error" as a link title when recommended courses
fails to get fetched has been removed so we'll never hit this code to
change the UI in response to it.

Changed in didoesdigital@7f5b188
@didoesdigital didoesdigital merged commit 932bf5e into didoesdigital:master Aug 1, 2024
@didoesdigital
Copy link
Owner

This is now in production! Thanks @na2hiro 👏

I'll make a note if I see any Sentry errors around experiments.

@na2hiro na2hiro deleted the jotai-dictionaries-index branch August 2, 2024 02:19
@didoesdigital didoesdigital mentioned this pull request Aug 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants