Add loading indicator and Sentry metrics for page load#488
Merged
Conversation
…s this is caused by users navigating to another page while the page is still loading
… log it to Sentry
Contributor
📝 WalkthroughWalkthroughAdds a navigation-progress UI that appears after 300ms of non-idle navigation, records Sentry metrics for show and duration, and adds Sentry ignore rules to suppress the "BodyStreamBuffer was aborted" error. Changes
Sequence DiagramsequenceDiagram
actor User
participant Router as Navigation Handler
participant Progress as NavigationProgress
participant Sentry as Sentry
participant UI as Page UI
User->>Router: start navigation
Router->>Progress: navigation state != idle
Progress->>Progress: start 300ms timer
alt timer expires && still navigating
Progress->>Sentry: increment navigation_progress.shown
Progress->>UI: show blurred backdrop + spinner
end
Router->>Progress: navigation becomes idle
Progress->>Progress: compute duration
Progress->>Sentry: record navigation_progress.duration_ms
Progress->>UI: hide backdrop + spinner
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Closed
coderabbitai
Bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/heavy-steaks-mix.md:
- Line 5: The release note sentence is a run-on and needs punctuation and a
slight reword for clarity; replace the current sentence with a concise version
such as: "If a page load takes longer than 300ms, log this to Sentry including
the total duration to help identify potential slow pages." Update the sentence
in the release note content to include the comma after the conditional and end
with a period.
In @.changeset/shy-times-fly.md:
- Line 5: Update the changelog sentence text to use hyphenation and clearer
phrasing: replace "In case a page load takes longer than 300ms show a loading
spinner and blur the page to prevent double clicking the navigation action" with
a polished version such as "If a page load takes longer than 300 ms, show a
loading spinner and blur the page to prevent double‑clicking navigation
actions." Ensure you add the comma after the time clause, use a non‑breaking
space or space before "ms" as your style guide prefers, and hyphenate
"double‑clicking" and pluralize "actions" for clarity.
In @.changeset/wide-animals-tan.md:
- Line 5: Update the release note entry that currently reads 'BodyStreambuffer
was aborted' to exactly match the runtime error string/casing used by the code
(inspect where the error is thrown or the error.name/message in the codebase and
copy it verbatim) so Sentry filters can correlate correctly; replace the typo in
the .changeset entry with that exact error text.
In `@fdm-app/app/components/custom/navigation-progress.tsx`:
- Around line 19-43: The effect currently sets startTimeRef inside the 300ms
timer and includes show in deps which lets the effect re-arm when show flips;
instead, on transition to non-idle set startTimeRef.current = Date.now()
immediately and only schedule the 300ms timer to call setShow(true) (store the
timer id and clear it on cleanup), and when state goes back to "idle" compute
duration from startTimeRef (if non-null), emit the Sentry metric, reset
startTimeRef to null and setShow(false); finally remove show from the effect
dependency array so the effect only reacts to state changes (update the
useEffect that references startTimeRef, setShow and timer accordingly).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.changeset/heavy-steaks-mix.md.changeset/shy-times-fly.md.changeset/wide-animals-tan.mdfdm-app/app/components/custom/navigation-progress.tsxfdm-app/app/entry.client.tsxfdm-app/app/root.tsxfdm-app/instrument.server.mjs
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
fdm-app/app/components/custom/navigation-progress.tsx (1)
19-45:⚠️ Potential issue | 🟠 MajorPrevent duplicate
navigation_progress.shownmetrics during a single navigation.On Line 45,
showin the dependency array causes the effect to re-run aftersetShow(true), which can arm another timer and emit an extranavigation_progress.shownwhile still non-idle.🛠️ Proposed fix
export function NavigationProgress() { const { state } = useNavigation() const [show, setShow] = useState(false) const startTimeRef = useRef<number | null>(null) + const shownRef = useRef(false) // Show after 300ms — emit a count metric when it appears useEffect(() => { if (state !== "idle") { if (startTimeRef.current === null) { startTimeRef.current = Date.now() } + if (shownRef.current) { + return + } const timer = setTimeout(() => { + shownRef.current = true setShow(true) if (clientConfig.analytics.sentry) { Sentry.metrics.count("navigation_progress.shown", 1) } }, 300) return () => clearTimeout(timer) } // Navigation finished — emit duration metric and hide - if (show && startTimeRef.current !== null) { + if (shownRef.current && startTimeRef.current !== null) { const duration = Date.now() - startTimeRef.current if (clientConfig.analytics.sentry) { Sentry.metrics.distribution( "navigation_progress.duration_ms", duration, ) } } setShow(false) + shownRef.current = false startTimeRef.current = null - }, [state, show]) + }, [state])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/custom/navigation-progress.tsx` around lines 19 - 45, The effect is re-running when setShow(true) updates show, causing duplicate navigation_progress.shown metrics; update the useEffect so it only depends on state (remove show from the dependency array) and add a guard before arming the timer that checks both startTimeRef.current === null and !show (so you only create a timer and call Sentry.metrics.count once per navigation), keeping the existing cleanup (clearTimeout) and the end-of-navigation logic that emits duration and resets startTimeRef and show.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@fdm-app/app/components/custom/navigation-progress.tsx`:
- Around line 19-45: The effect is re-running when setShow(true) updates show,
causing duplicate navigation_progress.shown metrics; update the useEffect so it
only depends on state (remove show from the dependency array) and add a guard
before arming the timer that checks both startTimeRef.current === null and !show
(so you only create a timer and call Sentry.metrics.count once per navigation),
keeping the existing cleanup (clearTimeout) and the end-of-navigation logic that
emits duration and resets startTimeRef and show.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.changeset/heavy-steaks-mix.md.changeset/shy-times-fly.md.changeset/wide-animals-tan.mdfdm-app/app/components/custom/navigation-progress.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- .changeset/shy-times-fly.md
- .changeset/heavy-steaks-mix.md
BoraIneviNMI
approved these changes
Mar 3, 2026
Collaborator
BoraIneviNMI
left a comment
BoraIneviNMI
left a comment
There was a problem hiding this comment.
Looks good to me. I see the spinner rather often, which might mean that there will be a lot of performance metric warnings; however, 300ms is also a load time that is reasonable to worry about.
This was referenced Mar 20, 2026
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.
Summary by CodeRabbit
New Features
Bug Fixes