adding navigation log support for angular#10147
Conversation
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces automatic Angular router navigation tracking for Firebase Crashlytics by adding 'setupNavigationTracking', 'getSafeRoutePath', and 'getRawPath' functions. Feedback is provided to optimize 'setupNavigationTracking' to prevent redundant initial telemetry logs before the router completes its initial navigation, and to simplify the type annotation of 'currentRoute' in 'getSafeRoutePath'.
| export function getRawPath(url: string): string; | ||
|
|
||
| // @public | ||
| export function getSafeRoutePath(router: Router): string; |
There was a problem hiding this comment.
Why do these two functions (getRawPath and getSafeRoutePath) need to be publicly exported?
There was a problem hiding this comment.
added @internal tags for them so that they are not public
There was a problem hiding this comment.
Can they actually just be private functions in angular/index.ts? And then they don't need to be exposed in the API at all?
| const subscription = router.events.subscribe(event => { | ||
| if (event instanceof NavigationEnd) { | ||
| const currentRawPath = getRawPath(event.urlAfterRedirects); | ||
| if (currentRawPath !== lastRawPath) { |
There was a problem hiding this comment.
I'm not familiar with angular routing - if the user refreshes the page, would that trigger a NavigationEnd event?
There was a problem hiding this comment.
If the user refreshes the page, then it is the equivalent of the page loading again for the first time. My comment on the AI's comment above mentions the two cases that may occur on the page load.
Essentially, a NavigationEnd would trigger for case 2. That particular check on 177 shouldn't be relevant for the page reload. It is for if only a query parameter or hash changes in the url; in which case, those NavigationEnd events are ignored.
| * Extracts the raw path portion from a full URL by stripping query parameters and hashes. | ||
| */ | ||
| export function getRawPath(url: string): string { | ||
| return url.split('?')[0].split('#')[0]; |
There was a problem hiding this comment.
String manipulation in general is brittle and generally avoided. Specifically, in this case it won't work when sanitizing angular matrix parameters. Apparently you should use UrlTree instead.
3 participants
Adding a function to call on the client side to set up a navigation listener to create navigation logs.
Also refactored getSafeRoutePath to be a shared function between error logging and navigation tracking.