Conversation
melvin-bot
bot
requested review from
dukenv0307 and
joekaufmanexpensify
and removed request for
a team
|
@dukenv0307 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
@elirangoshen Should I review this PR? |
|
No product considerations, removing my review |
|
is this enough? what about Are RHP not founds also captured? 
We also need some information about why this Not Found encoutnered.
|
src/pages/ErrorPage/NotFoundPage.tsx
Outdated
| return () => { | ||
| endSpan(CONST.TELEMETRY.SPAN_NOT_FOUND_PAGE); | ||
|
|
||
| if (typeof window !== 'undefined') { | ||
| window.removeEventListener('beforeunload', handleBeforeUnload); | ||
| } | ||
| }; |
There was a problem hiding this comment.
if 404 is from deeplink (eg. email) user most probably will close the tab - in that situation this span will not be closed and therefore it'll be infinite in Sentry.
We don't care about length of the span but rather the count. so in my opinion we could track every Not found with arbitrary duration eg. duration: 1s.
We more care about the circumstances
There was a problem hiding this comment.
Sure ill modify those
There was a problem hiding this comment.
What other factors if there is need we should add ?
@rlinoz
There was a problem hiding this comment.
Hey, sorry, I am not sure I understand the question.
There was a problem hiding this comment.
If there is more data can be useful to send to sentry except url and if its via deeplink
There was a problem hiding this comment.
Ah yeah, I think the url along with all the other default parameters we are sending should be good enough.
src/hooks/useNotFoundSpan.ts
Outdated
| url: currentUrl, | ||
| navigation_source: navigationSource, |
src/hooks/useNotFoundSpan.ts
Outdated
| const timeoutId = setTimeout(() => { | ||
| endSpanSafely(); | ||
| }, 5000); | ||
|
|
||
| // Handle page unload on web (closing tab/window, full page reload) - fallback | ||
| const handleBeforeUnload = () => { | ||
| endSpanSafely(); | ||
| }; | ||
|
|
||
| if (typeof window !== 'undefined') { | ||
| window.addEventListener('beforeunload', handleBeforeUnload); | ||
| } |
There was a problem hiding this comment.
did you explore the possibility to finish the span right away?
startSpan(...);
endSpan(...);| @@ -0,0 +1,64 @@ | |||
| import {useContext, useEffect} from 'react'; | |||
There was a problem hiding this comment.
move file to src/libs/telemetry
There was a problem hiding this comment.
move file to
src/libs/telemetry
done
| testID={NotFoundPage.displayName} | ||
| shouldShowOfflineIndicator={shouldShowOfflineIndicator} | ||
| > | ||
| <FullPageNotFoundView |
There was a problem hiding this comment.
FullPageNotFoundView already uses this hook, so I don't think we need this here (line 25)
There was a problem hiding this comment.
I think that we should move it to FullPageNotFoundView and remove from her (NotFoundPage)
There was a problem hiding this comment.
Also would this RHP not found page be handled by this?
https://callstack-hq.slack.com/archives/C07NMDKEFMH/p1765355007404189
|
@elirangoshen there is a conflict in |
yes i fixed it thanks |
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
No its ok thanks |
| import CONST from '@src/CONST'; | ||
| import {endSpan, startSpan} from './activeSpans'; | ||
|
|
||
| export default function useAbsentPageSpan() { |
There was a problem hiding this comment.
Can we rename this to useNotFoundPageSpan?
There was a problem hiding this comment.
I did it and than got es lint error because it not allow to use negative names..
There was a problem hiding this comment.
Eslint does not allow negativity in the code. I'd personally use useNotFoundPageSpan name and add eslint exception.
After some time it'll be hard to remember what is useAbsentPage as "absent" is never used in this context.
At least write a comment doc about what this is :)
|
Also, lint passed with a warning, should we address that? https://github.com/Expensify/App/actions/runs/19967116598/job/57261767414?pr=76629 |
fixed |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
| testID={NotFoundPage.displayName} | ||
| shouldShowOfflineIndicator={shouldShowOfflineIndicator} | ||
| > | ||
| <FullPageNotFoundView |
There was a problem hiding this comment.
I think that we should move it to FullPageNotFoundView and remove from her (NotFoundPage)
| testID={NotFoundPage.displayName} | ||
| shouldShowOfflineIndicator={shouldShowOfflineIndicator} | ||
| > | ||
| <FullPageNotFoundView |
There was a problem hiding this comment.
Also would this RHP not found page be handled by this?
https://callstack-hq.slack.com/archives/C07NMDKEFMH/p1765355007404189
|
How in Sentry can we filter out 404 created via invalid deep link (eg. from mail) to see only errors that are from in app? eg this: https://expensify.slack.com/archives/C07NMDKEFMH/p1765355007404189 @elirangoshen |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Im not sure if i understand as what we see in sentry when we filter this span is only from the app ? (and if we can filter also by attribute there's one to check if its via deeplink) |
|
We should be able to filter by |
yes 
|
|
🚀 Deployed to staging by https://github.com/rlinoz in version: 9.2.75-0 🚀
|
|
🚀 Deployed to staging by https://github.com/rlinoz in version: 9.2.75-0 🚀
|
|
🚀 Deployed to staging by https://github.com/rlinoz in version: 9.2.77-0 🚀
|
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.2.77-1 🚀
|
6 participants
Explanation of Change
Fixed Issues
$#74137
PROPOSAL:
This pr tracks time spend on 404 pages
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari