feat(redirectUri): Custom redirect URIs #408

pellicceama · 2025-03-30T18:46:25Z

Important

Add support for custom OAuth redirect URIs and refactor URL input handling across components and server logic.

Behavior:
- Added support for custom OAuth redirect URIs in client.tsx and page.tsx.
- Updated generateOAuth2Server in server.ts to use getOauthRedirectUri() for redirect URIs.
- setMetadataUrl mutation in organization.ts now handles both webhook and OAuth redirect URLs.
Components:
- Renamed WebhookInput to UrlInputForm and updated its logic to handle generic URL inputs.
- Updated SecureInput to include a className prop for styling.
Tests:
- Added tests for setMetadataUrl in organization.spec.ts to validate URL updates and error handling.
- Updated server.spec.ts to include orgId in test cases.
Misc:
- Renamed onboardingRouter to organizationRouter in index.ts and related files.

^{This description was created by}^{for cbb6d0e. It will automatically update as commits are pushed.}

vercel · 2025-03-30T18:46:29Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
v1	❌ Failed (Inspect)			Mar 31, 2025 7:27pm

ellipsis-dev

❌ Changes requested. Reviewed everything up to fd9048f in 2 minutes and 10 seconds

More details

Looked at 133 lines of code in 4 files
Skipped 0 files when reviewing.
Skipped posting 16 drafted comments based on config settings.

1. connectors/cnext/_defaults/oauth2/server.spec.ts:117

Draft comment:
Test inputs now include an 'orgId' field. Consider using a properly typed constant instead of as any.
Reason this comment was not posted:
Confidence changes required: 50% <= threshold 50%
None

2. kits/cdk/connector-meta.types.ts:116

Draft comment:
The ConnectContext now requires an 'orgId' using a template literal type. Ensure all consumers provide this format.
Reason this comment was not posted:
Confidence changes required: 30% <= threshold 50%
None

3. packages/api-v1/routers/connect.ts:90

Draft comment:
Repeated orgId validation; consider extracting this check into shared middleware.
Reason this comment was not posted:
Confidence changes required: 50% <= threshold 50%
None

4. packages/api-v1/routers/connect.ts:173

Draft comment:
PostConnect procedure also validates orgId. Consider centralizing the validation to avoid duplication.
Reason this comment was not posted:
Confidence changes required: 50% <= threshold 50%
None

5. packages/engine-backend/router/customerRouter.ts:310

Draft comment:
Validation for orgId is added here. Ensure consistent error messaging and consider centralizing this repetitive check.
Reason this comment was not posted:
Confidence changes required: 50% <= threshold 50%
None

6. packages/engine-backend/router/customerRouter.ts:403

Draft comment:
Repeating orgId validation in postConnect; consider refactoring to reduce duplication.
Reason this comment was not posted:
Confidence changes required: 50% <= threshold 50%
None

7. connectors/cnext/_defaults/oauth2/server.spec.ts:118

Draft comment:
Test objects now include an 'orgId'. Consider using a value that conforms to the expected pattern instead of casting as any.
Reason this comment was not posted:
Confidence changes required: 33% <= threshold 50%
None

8. kits/cdk/connector-meta.types.ts:117

Draft comment:
New 'orgId' property in ConnectContext lacks documentation; add a comment explaining the expected format (e.g. 'org_${string}').
Reason this comment was not posted:
Confidence changes required: 33% <= threshold 50%
None

9. packages/api-v1/routers/connect.ts:90

Draft comment:
Validate that 'ctx.viewer.orgId' exists before proceeding. Consider abstracting this repeated check to a common middleware 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 validation check is already implemented in both places where it's needed. The suggestion to move it to middleware could be valuable since it would reduce duplication, but this is more of a refactoring suggestion than a critical issue. The main validation the comment asks for is already done.
The comment might be pointing out a valid architectural improvement. Moving repeated checks to middleware is generally good practice.
However, the primary validation request is already implemented, and the middleware suggestion, while valid, is more of an optional refactoring that could be done later.
The comment should be deleted since its main concern (validation) is already addressed in the code, and the middleware suggestion could be made in a separate refactoring PR.

10. packages/engine-backend/router/customerRouter.ts:307

Draft comment:
PreConnect now enforces the presence of 'ctx.viewer.orgId'. If this pattern repeats across routes, consider a shared validation helper.
Reason this comment was not posted:
Marked as duplicate.

11. packages/engine-backend/router/customerRouter.ts:329

Draft comment:
OrgId is propagated to the preConnect call; this addition looks correct.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 50%
None

12. packages/engine-backend/router/customerRouter.ts:400

Draft comment:
Enforce 'orgId' presence in postConnect. As with other routes, consider centralizing this check.
Reason this comment was not posted:
Marked as duplicate.

13. packages/engine-backend/router/customerRouter.ts:427

Draft comment:
Passing 'orgId' to the postConnect call correctly; no changes needed.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 50%
None

14. packages/api-v1/routers/connect.ts:179

Draft comment:
Typographical error: In the postConnect mutation, the console.log statement at line 179 logs 'preConnect'. Please update it to 'postConnect' to accurately reflect the current operation.
Reason this comment was not posted:
Comment was on unchanged code.

15. packages/api-v1/routers/connect.ts:208

Draft comment:
Typographical error in inline comment: In the note starting on line 208, "its not clear to me..." should be corrected to "it's not clear to me..." to improve readability.
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.

16. packages/engine-backend/router/customerRouter.ts:24

Draft comment:
Typo in comment: "we dont' need it" should be corrected to "we don't need it".
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_niUf3J81odoXt1Qi

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.

ellipsis-dev · 2025-03-30T18:48:41Z

packages/api-v1/routers/connect.ts

@@ -162,6 +170,12 @@ export const connectRouter = router({
        })
      }

+      if (!ctx.viewer.orgId) {


The postConnect route now validates ctx.viewer.orgId. Consider consolidating such repeated orgId validations into a helper.

ellipsis-dev

❌ Changes requested. Reviewed everything up to cbb6d0e in 2 minutes and 34 seconds

More details

Looked at 728 lines of code in 13 files
Skipped 0 files when reviewing.
Skipped posting 10 drafted comments based on config settings.

1. connectors/cnext/_defaults/oauth2/utils.ts:149

Draft comment:
Inconsistent metadata field naming: fallback uses 'org.metadata?.redirect_uri' while client and router use 'oauth_redirect_url'. Ensure consistency to avoid confusion.
Reason this comment was not posted:
Marked as duplicate.

2. apps/web/app/console/(authenticated)/settings/client.tsx:64

Draft comment:
Using a single mutation hook (setMetadataUrl) for both webhook and OAuth redirect URL saves may lead to UI state conflicts if both are edited concurrently. Consider separate mutation hooks for independent state management.
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 comment raises a valid point about shared state - if a user edits both URLs quickly, the isPending state would affect both forms. However, this is a minor UX issue that may never occur in practice. The mutation itself is properly differentiated by urlType parameter. The code is functionally correct and follows common React patterns.
The shared isPending state could cause minor UI confusion, but is this really a significant enough issue to warrant refactoring? The current approach is more DRY.
While technically correct, the issue is minor and the current implementation has benefits like code reuse. The tradeoff seems reasonable.
The comment should be removed as it suggests a refactor for a minor UX edge case that may never occur in practice. The current implementation is valid and maintainable.

3. packages/api-v1/routers/organization.ts:51

Draft comment:
Await the async call to getOauthRedirectUri so that 'oauth_redirect_url' is a string instead of a Promise.
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%
Without seeing the implementation of getOauthRedirectUri, I can't be 100% certain if it's async or not. However, the function name doesn't suggest any async operations (it's just getting a URI), and the code compiles and runs without await, suggesting it's likely synchronous. If it was async and not awaited, TypeScript would likely have caught this error.
I don't have access to the implementation of getOauthRedirectUri, so I can't be completely certain it's not async. The commenter might have seen something I haven't.
Even with that uncertainty, TypeScript's type system would have caught this if getOauthRedirectUri returned a Promise, since the return type is expected to be a string based on the zod schema.
The comment should be deleted. There's no strong evidence that getOauthRedirectUri is async, and TypeScript's type system suggests it's synchronous.

4. packages/ui-v1/components/UrlInputForm.tsx:30

Draft comment:
URL validation here only checks if the URL starts with 'https://'. Consider enhancing validation to ensure the URL is well-formed.
Reason this comment was not posted:
Comment was on unchanged code.

5. connectors/cnext/_defaults/oauth2/server.ts:121

Draft comment:
Remove commented-out debug code and '@ts-expect-error' notes if they're no longer needed to improve code clarity.
Reason this comment was not posted:
Confidence changes required: 50% <= threshold 50%
None

6. packages/api-v1/routers/organization.ts:227

Draft comment:
Good validation for URL updates in setMetadataUrl, ensuring differences in URL types are handled appropriately.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 50%
None

7. connectors/cnext/_defaults/oauth2/server.ts:101

Draft comment:
In the log message at line 101, the phrase 'called with for connectionId' is redundant. Consider choosing either 'called with connectionId' or 'called for connectionId' to improve 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.

8. connectors/cnext/_defaults/oauth2/utils.ts:129

Draft comment:
Typographical suggestion: In the comment describing mapOauthParams, the wording is confusing. Consider rewording it to something like: 'Write tests for this function in different scenarios to ensure that clientKey is returned with the value initially set for client_id' to improve 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.

9. packages/api-v1/routers/connect.ts:179

Draft comment:
Typo: The console log in the postConnect mutation (line 179) displays 'preConnect', which is likely a copy-paste error. Consider updating it to 'postConnect' for clarity.
Reason this comment was not posted:
Comment was on unchanged code.

10. packages/api-v1/routers/connect.ts:208

Draft comment:
Typo: In the comment starting on line 208, 'its not clear to me ...' should be corrected to "it's not clear to me ..." for proper grammar.
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_hpSjeUt070Zgr6Xh

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/api-v1/routers/organization.ts

connectors/cnext/_defaults/oauth2/utils.ts

Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>

Due to build failures on main This reverts commit 7190340.

* adding org id to connect context * fetching redirect uri from db * refactoring webhook input form * renaming onboarding->organization router * renaming onboarding->organization router * abstracting metadata url endpoint * updating import * Update packages/api-v1/routers/organization.ts Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com> * Update connectors/cnext/_defaults/oauth2/utils.ts Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com> --------- Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>

Due to build failures on main This reverts commit 216af96.

adding org id to connect context

fd9048f

ellipsis-dev bot reviewed Mar 30, 2025

View reviewed changes

vercel bot deployed to Preview – v1 March 30, 2025 18:54 View deployment

pellicceama marked this pull request as draft March 30, 2025 20:06

merging main

53134ca

vercel bot deployed to Preview – v1 March 31, 2025 17:26 View deployment

pellicceama added 4 commits March 31, 2025 10:37

fetching redirect uri from db

de830a7

refactoring webhook input form

2e4ddd4

renaming onboarding->organization router

0513052

renaming onboarding->organization router

2a99ccc

vercel bot had a problem deploying to Preview – v1 March 31, 2025 18:14 Failure

pellicceama added 2 commits March 31, 2025 11:23

abstracting metadata url endpoint

d3c2aed

updating import

cbb6d0e

vercel bot had a problem deploying to Preview – v1 March 31, 2025 18:29 Failure

pellicceama marked this pull request as ready for review March 31, 2025 18:30

ellipsis-dev bot reviewed Mar 31, 2025

View reviewed changes

packages/api-v1/routers/organization.ts Outdated Show resolved Hide resolved

connectors/cnext/_defaults/oauth2/utils.ts Outdated Show resolved Hide resolved

pellicceama and others added 2 commits March 31, 2025 21:21

Update packages/api-v1/routers/organization.ts

43f8e5b

Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>

Update connectors/cnext/_defaults/oauth2/utils.ts

1c7ee7f

Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>

pellicceama merged commit 7190340 into main Mar 31, 2025
5 of 8 checks passed

pellicceama deleted the redirecturi branch March 31, 2025 19:22

vercel bot had a problem deploying to Preview – v1 March 31, 2025 19:27 Failure

openint-bot added a commit that referenced this pull request Mar 31, 2025

Revert "feat(redirectUri): Custom redirect URIs (#408)"

7ad99b0

Due to build failures on main This reverts commit 7190340.

pellicceama pushed a commit that referenced this pull request Jul 16, 2025

Revert "feat(redirectUri): Custom redirect URIs (#408)"

a0b328c

Due to build failures on main This reverts commit 216af96.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(redirectUri): Custom redirect URIs #408

feat(redirectUri): Custom redirect URIs #408

Uh oh!

pellicceama commented Mar 30, 2025 •

edited by ellipsis-dev bot

Loading

Uh oh!

vercel bot commented Mar 30, 2025 •

edited

Loading

Uh oh!

ellipsis-dev bot left a comment

Uh oh!

ellipsis-dev bot Mar 30, 2025

Uh oh!

ellipsis-dev bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

feat(redirectUri): Custom redirect URIs #408

feat(redirectUri): Custom redirect URIs #408

Uh oh!

Conversation

pellicceama commented Mar 30, 2025 • edited by ellipsis-dev bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vercel bot commented Mar 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

Uh oh!

ellipsis-dev bot Mar 30, 2025

Choose a reason for hiding this comment

Uh oh!

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pellicceama commented Mar 30, 2025 •

edited by ellipsis-dev bot

Loading

vercel bot commented Mar 30, 2025 •

edited

Loading