YPE-1143: Mitigate BibleCard UI page shifting#177
Conversation
Replace static loading text with a spinning loader icon when book title is not yet available. This improves the user experience by providing a more visually clear indication that content is being fetched.
made it possible for the Bible text view to animate in height so that changes aren't jarring in the Bible card whenever it loads fully.
Add a pulse animation to the BibleTextView when content is loading. This improves the user experience by providing visual feedback during data fetching.
Refactors the `AnimatedHeight` component to use `offsetHeight` for more accurate height calculations. Also updates the Tailwind transition timing function to `ease-out` for a smoother animation.
🦋 Changeset detectedLatest commit: fa2afe8 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 |
…er loading state - Add role="status" and aria-label="Loading" to LoaderIcon for accessibility - Update bible-card and verse tests to use getByRole queries instead of CSS class selectors - Fix verse story to properly await loading assertion Amp-Thread-ID: https://ampcode.com/threads/T-019ca029-6aeb-73f2-970f-b06ef26ca45f Co-authored-by: Amp <amp@ampcode.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
@greptileai, please review this PR |
Greptile SummaryThis PR successfully mitigates UI page shifting in Key improvements:
Technical implementation:
All changes follow project conventions, respect package boundaries, maintain unified versioning, and include proper test coverage. Confidence Score: 5/5
Important Files Changed
Last reviewed commit: fa2afe8 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
packages/ui/src/components/verse.tsx
Outdated
| } | ||
|
|
||
| if (currentError) { | ||
| if (currentError || fetchedError) { |
There was a problem hiding this comment.
checking both currentError and fetchedError causes stale error to show
when passageState is provided, currentError correctly uses passageState?.error, but fetchedError from the disabled hook could be stale from a previous fetch
| if (currentError || fetchedError) { | |
| if (currentError) { |
There was a problem hiding this comment.
@greptileai, oddly enough, we want this to show versus the API-specific error that would be returned by fetchedError. So there is a generic error message that the user would get, but any developer could see in their logs the actual error. Because you have this context now, please re-review this pull request.
There was a problem hiding this comment.
I understand you want the generic VerseUnavailableMessage shown to users - that makes sense for UX. My concern is specifically about this scenario:
- Component initially uses
usePassagehook (nopassageStateprovided) - Fetch fails,
fetchedErroris set - Parent then provides
passageStatewith valid data anderror: null - Hook is now disabled (
enabled: false) but retainsfetchedErrorfrom step 2 - Your code checks
if (currentError || fetchedError) - Result: Shows error message even though
passageStateindicates success
Is this intentional? If you're always clearing the component when switching between controlled/uncontrolled states, this won't be an issue. But if you're hot-swapping the passageState prop, users might see an error when there shouldn't be one.
If this scenario can't happen in your app, the rest of the PR looks solid - the loading state optimization (&& !currentPassage) is excellent for preventing flicker.
There was a problem hiding this comment.
Yeah, I hear ya. What would you suggest as a guarded preventative measure in the case the user/dev does hot-swap the passageState prop?
There was a problem hiding this comment.
Just remove || fetchedError from the condition. You've already computed currentError correctly:
const currentError = hasProvidedPassageState ? (passageState?.error ?? null) : fetchedError;So the check should be:
| if (currentError || fetchedError) { | |
| if (currentError) { |
This mirrors your loading state logic - when passageState is provided, use its error; otherwise use the hook's error. No need to check both.
There was a problem hiding this comment.
Does that mean we should just clear fetchedError if passageState changes? Because if there's an error fetching the Bible verse, then I WANT to show the generic error message component
There was a problem hiding this comment.
I see the confusion. Let me clarify the scenario:
The issue isn't about hiding real errors - it's about stale errors from a previous state:
- Component starts uncontrolled → hook fetches → gets error →
fetchedErroris set - Parent then provides
passageStatewith valid data +error: null - Hook is now disabled, but React Query retains
fetchedErrorfrom step 1 - Your check
if (currentError || fetchedError)shows the old error even though new state says success
You have two options:
Option 1: Trust the computed currentError (recommended)
if (currentError) {This works because currentError already handles both cases correctly. When controlled, parent is responsible for passing errors via passageState.error.
Option 2: Reset the query when switching to controlled mode
If you're worried about edge cases, add a reset effect:
useEffect(() => {
if (hasProvidedPassageState && fetchedError) {
queryClient.resetQueries(['passage', reference, versionId]);
}
}, [hasProvidedPassageState]);But honestly, Option 1 is cleaner. If passageState is provided, the parent owns error handling. If there's a fetch error in controlled mode, it should come through passageState.error, not fetchedError.
Which approach do you prefer?
There was a problem hiding this comment.
Okay fine. I relent. I did option 1. Turns out I was testing things in a way that people may not do in the real world.
@greptileai, can you re-review this entire PR now?
There was a problem hiding this comment.
Nice! The refactoring looks solid. Here's my full review:
✅ What's Working Well
Loading UX - The delayed spinner + showing old content during refetch is a huge improvement. No more jarring flashes.
AnimatedHeight component - Clean implementation with proper ResizeObserver cleanup and motion-reduce support.
Test coverage - Comprehensive tests for the delayed spinner and refetch behavior. Good job testing the edge cases.
Accessibility - LoaderIcon now has proper role="status" and ARIA labels.
Error handling - You fixed the currentError logic. It now correctly trusts the computed value instead of checking both. 👍
🔍 Minor Issues
packages/ui/src/components/animated-height.tsx:5,9
Your initial height is 0 until ResizeObserver fires. This means there's a brief moment where the wrapper has height: 0 before the effect runs, causing a jump from 0 → actual height on mount. Consider setting initial height from the ref:
const [height, setHeight] = useState<number | 'auto'>('auto');
useLayoutEffect(() => {
const el = innerRef.current;
if (!el || typeof ResizeObserver === 'undefined') {
setHeight('auto'); // fallback for SSR
return;
}
// Set initial height immediately
setHeight(el.offsetHeight);
const observer = new ResizeObserver(() => {
if (el) setHeight(el.offsetHeight);
});
observer.observe(el);
return () => observer.disconnect();
}, []);packages/ui/src/components/bible-card.tsx:142
Using yv:h-9! with the important flag is a code smell. The ! suggests there's a specificity war happening. Can you achieve this without !important?
packages/ui/src/components/verse.tsx:413
You're applying animate-pulse to the wrapper. If the HTML content inside has its own animations (like fade-ins on footnote buttons), they might conflict. Test with content that has animations to make sure the pulse doesn't interfere.
Summary
The PR successfully fixes the FOUC/shifting issues. The error handling is now correct after your Option 1 fix. Ship it! 🚢
The AnimatedHeight initial height issue is the only thing I'd consider blocking, but it depends on whether you see the 0 → height jump in practice.
Adjusted the `BibleCard` component's styling to remove an unnecessary gap between the header and the main content, and added a bottom margin to the header section for better visual separation.
Only render `VerseUnavailableMessage` if `currentError` exists. Previously, it would also render if `fetchedError` existed, leading to an incorrect UI state.
|
@greptileai, based on your latest review, please go and update your initial review. ty ty ty |
1 participant
More info can be found in https://lifechurch.atlassian.net/browse/YPE-1143
Plz watch video demo first (1min)
Supporting SMOL video
CleanShot.2026-02-27.at.11.31.53.mp4