YPE-2776 - feat(ui): show loading overlay when changing chapters in BibleReader#259
Open
cameronapak wants to merge 2 commits into
Conversation
…ibleReader When changing chapters, BibleReader now keeps the previous chapter's text mounted, dims it to 40% opacity, and floats a spinner over it after a short delay — instead of pulsing stale text (confusingly reads as real content) or flashing a blank spinner. Fast/cached switches stay instant since the dim and spinner are gated behind a ~250ms delay. - Lift usePassage into BibleReader.Content and pass passageState down so the reader owns the loading treatment without touching BibleTextView. On refetch it passes loading:false to suppress BibleTextView's pulse; first load keeps BibleTextView's own centered spinner. - Reset scroll to top on book/chapter change (instant); version-only swaps keep scroll position. - Single role=status live region for the overlay; LoaderIcon is decorative. - Hoist useDelayedLoading out of bible-card.tsx into a shared lib util. - Add deterministic unit test for the delay gate + an integration story asserting stale text persistence, delayed dim+spinner, recovery, and scroll reset. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: ea38fa0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
- Center the loading spinner with sticky top-[50vh] (viewport-relative) instead of top-1/2, which resolved to 50% of the tall passage container and could strand the spinner off-screen when scrolled (e.g. on version changes where scroll position is preserved). - Drop redundant aria-live="polite"; role="status" already implies it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
bmanquen
approved these changes
Jun 5, 2026
Collaborator
bmanquen
left a comment
bmanquen
left a comment
There was a problem hiding this comment.
suggestion: go through the code changes and remove comments that you feel are not needed, if the code speaks for itself.
Great work!
Collaborator
There was a problem hiding this comment.
suggestion: We can probably remove a lot of the comments left in this file.
Comment on lines
+3
to
+9
| /** | ||
| * Returns `true` only after `loading` has been continuously truthy for `delay` ms. | ||
| * | ||
| * Suppresses loading UI on fast/cached fetches so quick swaps feel instant, surfacing | ||
| * a spinner (and any accompanying treatment, e.g. a dimmed stale view) only when a | ||
| * fetch is genuinely slow. Shared by `BibleCard` and `BibleReader`. | ||
| */ |
Collaborator
There was a problem hiding this comment.
Do we need all of these comments?
Collaborator
|
thought: should we also not change the heading (book and chapter) until the passage is loaded? So in this scenario it would just grey out the whole reader and not change until everything is loaded. The only thing that would change is the toolbar indicating which book and chapter you are on. |
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
2 participants
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.
result.mp4
https://lifechurch.atlassian.net/browse/YPE-2776
What & why
When changing chapters in
BibleReader, the previous chapter's text used to pulse while the next chapter loaded. A gentle pulse reads as real content settling in, so the stale text was mistaken for the new chapter for a beat — especially jarring because the heading swaps instantly. A bare spinner was rejected too (content → blank → content flash).This ships the agreed fix: stale-while-revalidate with a dimmed overlay. The previous chapter's text stays mounted, dims to 40% opacity (static, no pulse), and a spinner floats over it — but only after a ~250ms delay, so fast/cached switches stay instant and never flash any loading UI.
How
All driven from
BibleReader—BibleTextViewis untouched (too many components depend on it):usePassageintoBibleReader.Contentand pass results down via the existingpassageStateprop, so the reader owns the loading treatment.BibleTextView's internal fetch stays disabled — no double fetch.isRefetching = passageLoading && passage !== null→useDelayedLoading(…, 250)gates both the dim and the spinner. On refetch we passloading: falsetoBibleTextViewto kill its pulse; on first load we pass the realloadingso it keeps its own centered spinner.mainscroll to top on book/chapter change (instant); version-only swaps preserve scroll position. This also guarantees the spinner is in view.role="status"/aria-live="polite"live region;LoaderIconis decorative (aria-hidden). No nested status regions.useDelayedLoadinghook out ofbible-card.tsxintopackages/ui/src/lib/use-delayed-loading.ts, shared by both components.Tests
use-delayed-loading.test.ts): deterministic fake-timer coverage of the delay gate — a sub-threshold (fast) load never surfaces the spinner; a slow load surfaces it at the threshold.ChapterChangeLoadingOverlaystory, taggedintegration): delayed MSW passage asserts stale text stays mounted (no flash), dim + spinner appear after the delay, the overlay clears and the new chapter renders, and scroll resets to top.Full UI suite: 18 files / 146 tests green. Typecheck, lint (
--max-warnings 0), and production build + style verification all pass. Changeset included (@youversion/platform-react-ui: patch).Reviewer note
The overlay spinner uses
sticky top-1/2 -translate-y-1/2inside anabsolute inset-0layer to stay viewport-centered during version changes (where scroll is preserved). Chapter changes reset scroll to top, so they're trivially covered. Worth eyeballing the spinner position over a long, scrolled chapter on a version swap via theChapterChangeLoadingOverlaystory.🤖 Generated with Claude Code
Greptile Summary
This PR adds a stale-while-revalidate loading overlay to
BibleReader: when switching chapters, the previous chapter's text stays visible at 40% opacity with a centered spinner floating over it, appearing only after a 250 ms delay so cached/fast switches remain instant.usePassagefetch and passes results intoBibleTextViewvia the existingpassageStateprop (disablingBibleTextView's internal fetch to prevent double-fetching);isRefetchingis derived frompassageLoading && passage !== nullso first-load still usesBibleTextView's own centered spinner.useDelayedLoadingis extracted frombible-card.tsxinto a sharedpackages/ui/src/lib/use-delayed-loading.tsmodule consumed by both components; timer cleanup viauseEffectreturn correctly prevents multiple simultaneous timers.[book, chapter]changes (not version changes) via ascrollContainerRef, and the spinner uses viewport-relativetop-[50vh]so it stays in view regardless of content height.Confidence Score: 5/5
Safe to merge — the stale-while-revalidate overlay logic is correctly implemented, no double-fetching occurs, and the delayed spinner correctly suppresses loading UI for fast/cached transitions.
The stale-while-revalidate logic is sound: useApiData intentionally does not reset data to null on refetch, which is exactly what isRefetching = passageLoading && passage !== null relies on. BibleTextView's internal fetch is correctly disabled when passageState is provided. Timer cleanup, viewport-relative spinner positioning, and scroll-reset scope are all correct. The integration story provides deterministic end-to-end coverage of the overlay lifecycle.
No files require special attention.
Important Files Changed
Reviews (2): Last reviewed commit: "fix(ui): address Greptile review on chap..." | Re-trigger Greptile