feat(openIntProxyLink): Adding Openint proxy link #39
Conversation
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 4feb3cf in 1 minute and 51 seconds
More details
- Looked at
257lines of code in5files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. packages/fetch-links/links/openIntProxyLink.ts:60
- Draft comment:
The TODO comment indicates that theopenIntProxyLinkfunction needs testing. Ensure this function is tested before merging. - Reason this comment was not posted:
Comment did not seem useful.
2. sdks/sdk-qbo/src/index.ts:27
- Draft comment:
TheaccessTokenis destructured fromoptions.authbut not used. If not needed, consider removing it to avoid confusion. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_nlUPX5czG40GYYuT
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| return expectsAuthProxy | ||
| } | ||
|
|
||
| interface OpenIntProxyHeaders { |
There was a problem hiding this comment.
The OpenIntProxyHeaders interface is defined twice in this file. Consider removing the duplicate definition to avoid confusion.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on f34d9ad in 20 seconds
More details
- Looked at
117lines of code in3files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. packages/runtime/createClient.ts:57
- Draft comment:
Consider providing a more descriptive error message to help users understand the issue better. For example, specify which authentication methods are conflicting. - Reason this comment was not posted:
Confidence changes required:50%
The error message in thecreateClientfunction should be more descriptive to help users understand the issue better.
2. packages/runtime/createClient.ts:63
- Draft comment:
Check iffetchLinkis already included indefaultLinksbefore adding it again to avoid redundancy. - Reason this comment was not posted:
Confidence changes required:50%
ThefetchLinkis added to the links array whenhasOtherAuthMethodsis true, but it might not be necessary if the default links already include it.
3. sdks/sdk-qbo/src/index.spec.ts:14
- Draft comment:
Ensure that all possible authentication methods are covered in the test cases. - Reason this comment was not posted:
Confidence changes required:50%
The test cases inindex.spec.tsare correctly updated to use the newauthstructure, but it's important to ensure that all possible authentication methods are covered.
Workflow ID: wflow_aB5JoGKMOgo4JnIH
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on e20fed1 in 1 minute and 3 seconds
More details
- Looked at
217lines of code in5files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. packages/fetch-links/links/authLink.ts:27
- Draft comment:
Consider adding a validation step to ensure thatheaders['authorization']is not empty before returning the link. This will prevent requests from being sent without proper authorization. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment suggests a potential improvement to ensure that requests are not sent without proper authorization. However, the current logic already handles different authentication methods and defaults to an empty string if none are provided. The necessity of this validation depends on the intended behavior of the function. If sending requests without authorization is acceptable in some cases, then the comment may not be necessary. Without strong evidence that this validation is required, the comment may not be useful.
I might be underestimating the importance of ensuring that requests are not sent without authorization. There could be security implications if requests are sent without proper authorization headers.
The function's current design seems to allow for requests without authorization if no credentials are provided, which might be intentional. Without clear evidence that this is a mistake, the comment may not be necessary.
The comment does not provide strong evidence that a validation step is required. The current logic seems to handle authorization as intended, allowing for empty headers if no credentials are provided. Therefore, the comment should be deleted.
Workflow ID: wflow_waWLQIWHvz8eH13Q
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
packages/runtime/createClient.ts
Outdated
| @@ -33,6 +44,8 @@ export function createClient<Paths extends {}>({ | |||
| }: ClientOptions = {}) { | |||
| const links = typeof _links === 'function' ? _links(defaultLinks) : _links | |||
|
|
|||
| links.unshift(authLink(clientOptions.auth ?? {}, clientOptions.baseUrl ?? '')) | |||
There was a problem hiding this comment.
Consider adding validation for the auth object before calling authLink to ensure it contains valid authentication data. This will prevent unnecessary processing and potential errors.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 4434f24 in 27 seconds
More details
- Looked at
22lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. sdks/sdk-qbo/src/index.spec.ts:5
- Draft comment:
Using the non-null assertion operator (!) on environment variables can lead to runtime errors if the variables are not set. Consider adding a check or default value to handle cases where the environment variable might be undefined. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_nrPiT7F9qbxZf1Zp
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 9e79f96 in 26 seconds
More details
- Looked at
26lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. packages/fetch-links/links/openIntProxyLink.ts:67
- Draft comment:
The use ofletforheadersis unnecessary since the variable is not reassigned. Consider usingconstinstead for better readability and adherence to best practices. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_wfHrhBG7zbniquXL
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 9fbcc5a in 29 seconds
More details
- Looked at
305lines of code in8files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. packages/runtime/createClient.ts:49
- Draft comment:
Ensure that_linksis handled properly when it isundefinedto avoid unexpected behavior increateClient. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_zGXjtEapj49TY05T
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 1356b24 in 31 seconds
More details
- Looked at
104lines of code in4files - Skipped
1files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. packages/fetch-links/package.json:46
- Draft comment:
The addition oftype-festas a devDependency is appropriate for the new types used inauthLink.ts. - Reason this comment was not posted:
Confidence changes required:0%
The import oftype-festinpackage.jsonis correct and necessary for the new types used inauthLink.ts. No issues here.
2. packages/runtime/createClient.spec.ts:58
- Draft comment:
The test description should be corrected to 'expect links array to have 1 element by default'. - Reason this comment was not posted:
Confidence changes required:50%
The test description increateClient.spec.tsshould be corrected to match the expected behavior of the code.
3. packages/runtime/createClient.ts:45
- Draft comment:
The integration ofauthLinkbased on the presence ofauthoptions is correctly implemented. - Reason this comment was not posted:
Confidence changes required:0%
The change increateClient.tsto handle theauthoption correctly integrates the newauthLinkfunctionality. No issues here.
Workflow ID: wflow_d7P0x0KZexj1q4cr
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 2787124 in 15 seconds
More details
- Looked at
35lines of code in2files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_uYHUROMIjMZ2QGmz
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on bc25c89 in 21 seconds
More details
- Looked at
12lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. packages/runtime/createClient.spec.ts:78
- Draft comment:
The expected number of links should be 3, which is correct, as the auth link should be added to the default link, making it 2, and then the fetchLink adds another, making it 3. - Reason this comment was not posted:
Confidence changes required:0%
The test case on line 70 checks for the number of links when auth is present and links are overridden. The expected number of links should be 3, which is correct, as the auth link should be added to the default link, making it 2, and then the fetchLink adds another, making it 3.
Workflow ID: wflow_Farcn1oBVl5t7AFN
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 01ebc72 in 10 seconds
More details
- Looked at
58lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. packages/runtime/createClient.ts:9
- Draft comment:
The import ofClientAuthOptionsis redundant as it is already imported inauthLink. Consider removing it to avoid duplication. - Reason this comment was not posted:
Confidence changes required:50%
The import statement forClientAuthOptionsincreateClient.tsis redundant since it's already imported inauthLink.
Workflow ID: wflow_2E09W9qzwG7omGm5
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Important
Add OpenInt proxy link feature with validation, client integration, and tests for new authentication methods.
openIntProxyLinkinopenIntProxyLink.tsfor handling proxy requests with OpenInt.validateOpenIntProxyLinkOptions()for correct authentication combinations.openIntProxyLinkintocreateClient()increateClient.tswhenauthoptions are provided.createClient.spec.tsfor link array length and overrides.index.spec.tsforsdk-qboto verify new authentication methods.ClientOptionsincreateClient.tsto includeauthfor proxy link options.index.tsinsdk-qboto support new authentication methods.authLink.tsto handle different authentication methods.This description was created by
for 01ebc72. It will automatically update as commits are pushed.