-
Notifications
You must be signed in to change notification settings - Fork 12
fix: improve uncaught exception for getAccessToken #224
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
blva
commented
May 8, 2025
•
edited
Loading
edited
- improves send events to make it clearer when we send auth/unauth
- adds one more test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR improves the handling of access tokens when sending telemetry events by making the failure mode more explicit and introducing a new test to cover the fallback logic.
- Updated test descriptions and behavior to simulate token errors more clearly.
- Renamed method hasValidAccessToken to validateAccessToken and updated its usage across the codebase.
- Modified the sendEvents logic to catch errors and fall back to an unauthenticated endpoint when needed.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
tests/unit/apiClient.test.ts | Updated tests to reflect the new exception-based token failure behavior. |
tests/integration/helpers.ts | Modified to use validateAccessToken instead of hasValidAccessToken. |
src/server.ts | Updated API client method call for token validation. |
src/common/atlas/apiClient.ts | Replaced token validation method and revised sendEvents to check token state. |
Comments suppressed due to low confidence (1)
src/common/atlas/apiClient.ts:145
- Throwing a generic Error here may make it harder to distinguish token-related failures from other errors. Consider using a specific error type (e.g., ApiClientError) for better error handling and consistency.
if (!accessToken) { throw new Error("No access token available"); }
const accessToken = await this.getAccessToken(); | ||
return accessToken !== undefined; | ||
public async validateAccessToken(): Promise<void> { | ||
await this.getAccessToken(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if I got it right but this can end up just not finding the access token right? so it'd effectively not validate it always?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it validates if it exists otherwise it will just return undefined, which is fine