reverting old callback #281
Merged
+139
−21
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![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 e7d95dc in 3 minutes and 11 seconds
More details
- Looked at
189lines of code in2files - Skipped
0files when reviewing. - Skipped posting
13drafted comments based on config settings.
1. apps/web/app/connect/(oauth)/callback/newpage.tsx:8
- Draft comment:
Typo: "ahve" should be "have" in the comment. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. apps/web/app/connect/(oauth)/callback/newpage.tsx:23
- Draft comment:
Usage of Buffer in client-side code may not be polyfilled in all environments. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is purely informative and does not provide a specific suggestion or question for the PR author. It does not align with the rules for good comments, as it does not ask for a specific change or confirmation of intention.
3. apps/web/app/connect/(oauth)/callback/page.tsx:36
- Draft comment:
Hardcoded URL may need to use environment config instead of 'https://app.venice.is/connect/callback'. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
The code itself acknowledges this is a temporary hack with a comment. The hardcoded URL is intentional as part of a transition period. Making this configurable would add complexity to something that's meant to be temporary. The comment doesn't add value beyond what the code comment already explains.
The hardcoded URL could cause issues if the domain changes. Environment config would make it more maintainable.
Since this is explicitly marked as temporary code, adding configuration would be overengineering something that's going to be removed soon anyway.
Delete the comment. The code already acknowledges this is temporary, and adding configuration would be unnecessary complexity for transitional code.
4. apps/web/app/connect/(oauth)/callback/page.tsx:109
- Draft comment:
Unresolved redirect behavior: Comment indicates redirect is expected here; consider implementing redirection. - 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 already has a CallbackEffect component that likely handles any needed redirects or window closing. The TODO comment appears to be addressed by this component. The code also already has redirect handling for the no-cookie case. The comment is just restating what's in a TODO comment without adding value.
The CallbackEffect component's behavior isn't visible here - maybe there really is a redirect issue that needs to be addressed?
Even if there might be a redirect issue, the automated comment doesn't provide any specific guidance or evidence of a problem. It's just repeating the TODO comment without adding value.
The comment should be deleted as it merely restates an existing TODO comment without providing actionable guidance or evidence of an actual problem.
5. apps/web/app/connect/(oauth)/callback/newpage.tsx:8
- Draft comment:
Typo in comment: fix "ahve" to "have". - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
6. apps/web/app/connect/(oauth)/callback/page.tsx:38
- Draft comment:
When appending search parameters, ensure the value is a string. 'value as string' may fail if the value is an array. - Reason this comment was not posted:
Marked as duplicate.
7. apps/web/app/connect/(oauth)/callback/page.tsx:60
- Draft comment:
Possible typo in event type: 'AUTHORIZATION_SUCEEDED' may need to be 'AUTHORIZATION_SUCCEEDED'. - Reason this comment was not posted:
Marked as duplicate.
8. apps/web/app/connect/(oauth)/callback/page.tsx:109
- Draft comment:
Unresolved redirect behavior: the comment 'How do we do redirect here?' suggests uncertainty; clarify intended redirection or remove this debug note. - 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%
While the comment identifies a potential issue (unclear redirect handling), it's not making a specific suggestion for improvement. The code already has redirect functionality and a CallbackEffect component. The comment is essentially just restating what's in a developer comment without adding actionable value.
The TODO comment might indicate a real architectural question that needs to be addressed. The CallbackEffect component's behavior is unknown without seeing its implementation.
Even if there is an architectural question, this automated review comment doesn't help resolve it - it just points out an existing comment. Per the rules, we should not make speculative comments or ask for clarification.
The comment should be deleted as it's not actionable and merely points out an existing developer comment without providing specific guidance.
9. apps/web/app/connect/(oauth)/callback/page.tsx:107
- Draft comment:
Using console.log in server-side code: consider using a structured logger for production environments. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
10. apps/web/app/connect/(oauth)/callback/newpage.tsx:8
- Draft comment:
Typo found in comment: 'Will ahve to check' should be corrected to 'Will have to check'. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is purely informative and does not provide any actionable feedback or suggestions related to the code itself. It simply points out a typo in a comment, which is not critical to the functionality or quality of the code.
11. apps/web/app/connect/(oauth)/callback/page.tsx:20
- Draft comment:
Typo detected: 'ahve' should be corrected to 'have' for clarity. - Reason this comment was not posted:
Comment was on unchanged code.
12. apps/web/app/connect/(oauth)/callback/page.tsx:60
- Draft comment:
Typo detected: 'AUTHORIZATION_SUCEEDED' appears to be misspelled. Consider verifying if it should be 'AUTHORIZATION_SUCCEEDED'. - Reason this comment was not posted:
Marked as duplicate.
13. apps/web/app/connect/(oauth)/callback/page.tsx:36
- Draft comment:
Typo detected: 'accomodate' should be corrected to 'accommodate' for correctness. - 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%
While the spelling correction is technically accurate, comments about spelling in comments are extremely low value. They don't affect functionality, readability is barely impacted, and it creates noise in the PR review. This falls under the "Do NOT make comments that are obvious or unimportant" rule.
The spelling error could theoretically reduce code searchability or make the codebase look unprofessional. Maybe maintaining perfect spelling everywhere is important to the team.
The impact of a spelling error in a comment is minimal, especially for a temporary hack comment. The noise and distraction of the PR comment outweighs any benefit.
Delete this comment as it's too trivial and doesn't affect code quality in any meaningful way.
Workflow ID: wflow_kjVwwu10ZaEVPAa4
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.
| // Temporary hack to redirect to the right place to accomodate for oauth url not fully changed yet | ||
| const url = new URL('https://app.venice.is/connect/callback') | ||
| for (const [key, value] of Object.entries(searchParams)) { | ||
| url.searchParams.append(key, value as string) |
There was a problem hiding this comment.
Casting search param values to string might be unsafe for array values.
Suggested change
| url.searchParams.append(key, value as string) | |
| url.searchParams.append(key, Array.isArray(value) ? value.join(',') : value) |
| } | ||
| } | ||
|
|
||
| if (res.eventType !== 'AUTHORIZATION_SUCEEDED') { |
There was a problem hiding this comment.
Possible typo in event type 'AUTHORIZATION_SUCEEDED'; should it be 'AUTHORIZATION_SUCCEEDED'?
Suggested change
| if (res.eventType !== 'AUTHORIZATION_SUCEEDED') { | |
| if (res.eventType !== 'AUTHORIZATION_SUCCEEDED') { |
| if ( | ||
| code && | ||
| state && | ||
| Buffer.from(state, 'base64').toString('utf8').startsWith('conn_') |
There was a problem hiding this comment.
Using Buffer in client code: consider explicitly importing it (e.g. from 'buffer') to ensure it’s available in production.
| return null | ||
| } | ||
|
|
||
| if (!cookie) { |
There was a problem hiding this comment.
Redundant cookie check: the cookie is already validated at line 34, so this check may be unnecessary.
![recurseml[bot]](https://avatars.githubusercontent.com/in/1054195?s=60&v=4){kind=small}
| if ( | ||
| code && | ||
| state && | ||
| Buffer.from(state, 'base64').toString('utf8').startsWith('conn_') |
There was a problem hiding this comment.
Using Buffer directly in browser-side code can cause issues as it's a Node.js API. For browser environments, should use window.atob() or a browser-compatible base64 decoding method instead
💬 Reply with
/praiseto tell me that this comment was useful.
Is this comment irrelevant to your project? Reply with
/ignoreto no longer receive comments like this in the future.
|
😱 Found 1 issue. Time to roll up your sleeves! 😱 |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 08a5fa1 in 1 minute and 27 seconds
More details
- Looked at
102lines of code in1files - Skipped
0files when reviewing. - Skipped posting
8drafted comments based on config settings.
1. apps/web/app/connect/(oauth)/callback/page.tsx:35
- Draft comment:
The warning message indicates 'redirecting to openint', but the redirect URL uses https://app.venice.is/connect/callback. Update the URL to match the intended domain. - 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 explicitly states this is a temporary hack with a comment. The discrepancy appears to be known and intentional during a transition period. The comment is technically correct about the mismatch, but it's addressing something that the developers are already aware of and have documented.
I might be too quick to dismiss this - inconsistent URLs in OAuth flows can be security-sensitive. Maybe this deserves more scrutiny?
While OAuth security is important, the existing code comment shows this is a known transitional state. The comment wouldn't add any new information to what the developers have already acknowledged.
Delete the comment as it's pointing out a known temporary state that's already documented in the code comments.
2. apps/web/app/connect/(oauth)/callback/page.tsx:66
- Draft comment:
Renaming 'resourceId' to 'connectionId' is applied consistently. Verify that all external integrations and error messages are updated accordingly. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
3. apps/web/app/connect/(oauth)/callback/page.tsx:110
- Draft comment:
Ensure that the error display correctly references 'connectionId' now that it replaced 'resourceId'. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
4. apps/web/app/connect/(oauth)/callback/page.tsx:37
- Draft comment:
Redirect URL still references venice.is while the log message indicates openint. Update the URL for consistency. - 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 inconsistency is real, but the code itself acknowledges this is a temporary hack with a comment. The PR appears to be in the middle of a transition from venice.is to openint. The comment isn't providing new information beyond what the code comment already explains. The temporary nature suggests this will be cleaned up in a future PR.
The inconsistency could cause confusion for other developers. Maybe this temporary state should be documented more clearly or fixed now rather than later.
The code already documents this as temporary with an inline comment. Adding another comment about a known temporary state doesn't add value.
Delete the comment since it's pointing out an inconsistency that's already acknowledged as a temporary state in the code comments.
5. apps/web/app/connect/(oauth)/callback/page.tsx:50
- Draft comment:
Redundant cookie check inside the async callback; cookie is already verified above. - Reason this comment was not posted:
Comment was on unchanged code.
6. apps/web/app/connect/(oauth)/callback/page.tsx:20
- Draft comment:
Typo in comment: 'Will ahve to check' should be corrected to 'Will have to check'. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =30%<= threshold50%
This comment is pointing out a typo in a comment within the code. While it is a minor issue, correcting typos can improve code readability and professionalism. However, it doesn't directly impact the functionality of the code. According to the rules, purely informative comments should be avoided, and this comment seems to fall into that category as it doesn't suggest a code change or improvement beyond correcting a typo.
7. apps/web/app/connect/(oauth)/callback/page.tsx:36
- Draft comment:
Typo in comment: 'redirecting to the right place to accomodate for oauth url' should use 'accommodate' instead of 'accomodate'. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =20%<= threshold50%
This comment is pointing out a typo in a comment within the code. While it is specific, it is not directly related to the functionality or logic of the code. It is more of an informative comment about spelling, which does not align with the rules provided.
8. apps/web/app/connect/(oauth)/callback/page.tsx:56
- Draft comment:
Typo in constant string: 'AUTHORIZATION_SUCEEDED' appears to be a misspelling. Consider using 'AUTHORIZATION_SUCCEEDED' if this is not an intentional external API value. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_v6JRP0dkeDnVPO38
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Important
Reverts OAuth callback to server-side logic in
page.tsx, handling cookies, redirects, and errors robustly.page.tsxto a server-side implementation.NangoConnect.doOauthCallbackand handles success and error cases.CallbackEffectto manage callback results and auto-close behavior.FullScreenCenter.newpage.tsxwhich contained the previous client-side implementation.This description was created by
for 08a5fa1. It will automatically update as commits are pushed.