-
Notifications
You must be signed in to change notification settings - Fork 372
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
Fix JS WrappedTokenResponse type #1997
Conversation
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.
👍 Looks good to me! Reviewed everything up to f8cc481 in 1 minute and 23 seconds
More details
- Looked at
67
lines of code in3
files - Skipped
1
files when reviewing. - Skipped posting
9
drafted comments based on config settings.
1. js/sdk/package.json:2
- Draft comment:
Version bump to 0.4.28 looks good. New fields (types/exports) are properly added. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
2. js/sdk/src/types.ts:283
- Draft comment:
The Token interface now uses 'token' and 'tokenType'. Ensure downstream clients are updated to reflect these field changes. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is asking the PR author to ensure that downstream clients are updated, which is similar to asking them to double-check or verify something. This violates the rule against asking the author to ensure behavior is intended or tested. Therefore, this comment should be removed.
3. js/sdk/src/types.ts:433
- Draft comment:
WrappedTokenResponse now uses TokenResponse. Verify client code correctly handles the nested token object. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to verify that the client code correctly handles the nested token object. This falls under the rule of not asking the author to ensure behavior is intended or to double-check things. Therefore, this comment should be removed.
4. js/sdk/src/v3/clients/collections.ts:331
- Draft comment:
Formatting improvement in retrieveByName for clarity. No functional change. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
5. js/sdk/package.json:3
- Draft comment:
Version bump to 0.4.28. Ensure this semver change is documented given the breaking changes in token types. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
6. js/sdk/src/types.ts:283
- Draft comment:
Updated Token interface now uses 'token' and 'tokenType' instead of separate access and refresh tokens. Verify all consumers are updated. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to verify that all consumers are updated, which is a form of asking them to double-check their work. This violates the rule against asking the author to ensure behavior is intended or to double-check things.
7. js/sdk/src/types.ts:433
- Draft comment:
Changed WrappedTokenResponse to use TokenResponse instead of Token. This aligns the response shape with the new token structure. Confirm server responses adhere to this format. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to confirm that server responses adhere to a new format. This falls under the rule of not asking the author to confirm their intention or to ensure behavior is intended. Therefore, this comment should be removed.
8. js/sdk/src/v3/clients/collections.ts:331
- Draft comment:
Formatting update for retrieveByName: no functional change. Ensure consistency in multi-line object formatting. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
9. js/sdk/src/types.ts:211
- Draft comment:
Typographical error detected: property names 'graphragMapSystem' and 'graphragReduceSystem' appear to contain an extra 'rag'. Consider renaming them to 'graphMapSystem' and 'graphReduceSystem' for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_9GsHw1Epsuhmf9x7
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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.
👍 Looks good to me! Incremental review on bf0dfa4 in 1 minute and 32 seconds
More details
- Looked at
19
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
5
drafted comments based on config settings.
1. js/sdk/src/r2rClient.ts:157
- Draft comment:
Good removal of redundant ts-ignore, now relying on proper typing. However, consider adding a safeguard or an assertion to ensure that refreshResponse.results.accessToken and refreshResponse.results.refreshToken exist to avoid potential runtime errors if the response schema changes. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The code is TypeScript and has explicit type definitions. The removal of @ts-ignore suggests the types are now properly aligned. The RefreshTokenResponse type shows these fields are required, not optional. The code is in a try-catch block already to handle any errors. Adding runtime checks would be redundant with the type system.
The type system could be bypassed at runtime if the API response doesn't match the type definition. The try-catch might not distinguish between network errors and malformed responses.
The existing try-catch will catch any runtime errors including undefined property access. Adding extra checks would be defensive programming that adds complexity without clear benefit.
The comment should be deleted. The code has proper type definitions and error handling in place, making additional runtime checks unnecessary.
2. js/sdk/src/r2rClient.ts:157
- Draft comment:
Good removal of ts-ignore. Now that RefreshTokenResponse is properly typed, this call no longer needs suppression. Ensure that refreshAccessToken always returns a valid token structure. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
3. js/sdk/src/r2rClient.ts:159
- Draft comment:
Removing optional chaining enforces strict access. Confirm that refreshResponse always has accessToken and refreshToken objects to avoid runtime errors. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
4. js/sdk/src/r2rClient.ts:9
- Draft comment:
Typo detected: 'IndiciesClient' on line 9. Consider renaming it to 'IndicesClient' if this is not intentional. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. js/sdk/src/r2rClient.ts:68
- Draft comment:
Typo detected: The property 'indices' is being initialized using 'new IndiciesClient(this)' on line 68; consider updating 'IndiciesClient' to 'IndicesClient' to correct the spelling. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_Q7YGDwz7RPhxpofo
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Important
Fix
WrappedTokenResponse
type in JS SDK and remove unnecessary@ts-ignore
comments inr2rClient.ts
.WrappedTokenResponse
type intypes.ts
to useTokenResponse
instead ofToken
.Token
interface to includetoken
andtokenType
fields.@ts-ignore
comments inr2rClient.ts
forrefreshAccessToken()
usage.package.json
from0.4.27
to0.4.28
.This description was created by
for bf0dfa4. It will automatically update as commits are pushed.