Conversation
…e and improve user route handling
…ency and enhance route structure
📝 WalkthroughWalkthroughRoute path updated from /auth/google to /oauth/google. Google OAuth callback redirect changed from /workflow to /workflows when no workflowId provided. Credential handling in user routes refactored to use full Config object instead of destructured credentialId. TypeScript build metadata updated across multiple packages. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/http-backend/src/routes/google_callback.ts (3)
133-149:⚠️ Potential issue | 🟠 MajorInconsistent redirect path in error handler.
The success redirect (line 135) was updated to
/workflows, but the error redirect (line 148) still points to/workflow. This looks like an oversight — the error case should redirect to the same base path.Proposed fix
const errorUrl = workflowId ? `http://localhost:3000/workflows/${workflowId}?google=error&msg=${encodeURIComponent(err?.message ?? "Token exchange failed")}` - : `http://localhost:3000/workflow?google=error&msg=${encodeURIComponent(err?.message ?? "Token exchange failed")}`; + : `http://localhost:3000/workflows?google=error&msg=${encodeURIComponent(err?.message ?? "Token exchange failed")}`;
153-199:⚠️ Potential issue | 🟠 MajorDebug endpoints expose sensitive configuration and credentials in production.
The
/debug/configand/debug/credentialsendpoints have no authentication (userMiddlewareis not applied) and expose OAuth client IDs, token metadata, and credential details. These should be removed or gated behind a development-only check before shipping.
72-75:⚠️ Potential issue | 🔴 CriticalRemove
userMiddlewarefrom/callbackendpoint—it blocks Google's OAuth redirect.The
/callbackroute receives an unauthenticated redirect from Google's servers. SinceuserMiddlewarerequires a valid JWT token and returns a 400 error when none is present (seeuserMiddleware.tslines 27-30), the OAuth callback will never reach the handler, breaking the entire OAuth flow.The
/initiateendpoint correctly usesuserMiddlewarebecause the user is already authenticated when initiating OAuth, but/callbackmust remain unprotected to accept Google's redirect.
🤖 Fix all issues with AI agents
In `@apps/http-backend/src/routes/userRoutes/userRoutes.ts`:
- Around line 577-583: The update path is inconsistent with the create handler:
it persists dataSafe.data.Config (including credentialId) into the JSON config
and also sets CredentialsID, causing duplication; modify the
prismaClient.node.update call to strip credentialId from dataSafe.data.Config
before saving (same approach as the create handler) so config does not contain
credentialId and only CredentialsID holds it — locate the update logic around
prismaClient.node.update, reference dataSafe.data.Config and CredentialsID,
remove/exclude credentialId from the config object prior to passing it into the
data: { config: ..., CredentialsID: ... } payload.
| // const { credentialId, ...restConfig } = dataSafe.data.Config; | ||
| const updateNode = await prismaClient.node.update({ | ||
| where: { id: dataSafe.data.NodeId }, | ||
| data: { | ||
| position: dataSafe.data.position, | ||
| config: restConfig, | ||
| CredentialsID: credentialId | ||
| config: dataSafe.data.Config , | ||
| CredentialsID: dataSafe.data.Config?.credentialId || null |
There was a problem hiding this comment.
Inconsistency between create and update: credentialId handling in config.
The create handler (line 527) destructures credentialId out of Config so it's stored only in CredentialsID, not in the config JSON column. The update handler now stores the full Config (including credentialId) in the config column and extracts it into CredentialsID. This means:
- After create:
confighas nocredentialId,CredentialsIDis set. - After update:
configcontainscredentialId,CredentialsIDis also set.
This divergence can cause bugs if downstream code reads credentialId from config (it'll be missing on freshly created nodes) or leads to stale data if only one location is updated.
Consider either stripping credentialId from Config in both handlers, or keeping it in both — but be consistent.
Option: Strip credentialId in the update handler too (consistent with create)
- // const { credentialId, ...restConfig } = dataSafe.data.Config;
+ const { credentialId, ...restConfig } = dataSafe.data.Config ?? {};
const updateNode = await prismaClient.node.update({
where: { id: dataSafe.data.NodeId },
data: {
position: dataSafe.data.position,
- config: dataSafe.data.Config ,
- CredentialsID: dataSafe.data.Config?.credentialId || null
+ config: restConfig || {},
+ CredentialsID: credentialId || null
},
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // const { credentialId, ...restConfig } = dataSafe.data.Config; | |
| const updateNode = await prismaClient.node.update({ | |
| where: { id: dataSafe.data.NodeId }, | |
| data: { | |
| position: dataSafe.data.position, | |
| config: restConfig, | |
| CredentialsID: credentialId | |
| config: dataSafe.data.Config , | |
| CredentialsID: dataSafe.data.Config?.credentialId || null | |
| const { credentialId, ...restConfig } = dataSafe.data.Config ?? {}; | |
| const updateNode = await prismaClient.node.update({ | |
| where: { id: dataSafe.data.NodeId }, | |
| data: { | |
| position: dataSafe.data.position, | |
| config: restConfig || {}, | |
| CredentialsID: credentialId || null |
🤖 Prompt for AI Agents
In `@apps/http-backend/src/routes/userRoutes/userRoutes.ts` around lines 577 -
583, The update path is inconsistent with the create handler: it persists
dataSafe.data.Config (including credentialId) into the JSON config and also sets
CredentialsID, causing duplication; modify the prismaClient.node.update call to
strip credentialId from dataSafe.data.Config before saving (same approach as the
create handler) so config does not contain credentialId and only CredentialsID
holds it — locate the update logic around prismaClient.node.update, reference
dataSafe.data.Config and CredentialsID, remove/exclude credentialId from the
config object prior to passing it into the data: { config: ..., CredentialsID:
... } payload.
There was a problem hiding this comment.
Pull request overview
This PR addresses execution-related fixes in the workflow automation system. The changes primarily focus on fixing OAuth redirect URLs and adjusting how node configuration data (specifically credentialId) is handled during updates.
Changes:
- Fixed OAuth redirect URL path from
/workflowto/workflowsfor consistency with the application routing - Changed OAuth route path from
/auth/googleto/oauth/google - Modified node update logic to keep credentialId in Config object instead of extracting it
- Added
@repo/commonworkspace dependency to enable interpolation utilities in the worker
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Adds @repo/common as devDependency to worker for interpolation utilities |
| packages/nodes/tsconfig.tsbuildinfo | TypeScript build artifact reflecting new execution registry files |
| packages/common/tsconfig.tsbuildinfo | TypeScript build artifact reflecting interpolation module addition |
| apps/worker/tsconfig.tsbuildinfo | TypeScript build artifact with reduced file list (test files removed) |
| apps/http-backend/tsconfig.tsbuildinfo | TypeScript build artifact reflecting route file changes |
| apps/http-backend/src/routes/userRoutes/userRoutes.ts | Modified node update to keep credentialId in Config instead of extracting it |
| apps/http-backend/src/routes/google_callback.ts | Fixed successful OAuth redirect URL from /workflow to /workflows |
| apps/http-backend/src/index.ts | Changed OAuth route mount point from /auth/google to /oauth/google |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
| const redirectUrl = workflowId | ||
| ? `http://localhost:3000/workflows/${workflowId}` | ||
| : "http://localhost:3000/workflow"; | ||
| : "http://localhost:3000/workflows"; |
There was a problem hiding this comment.
The error redirect URL on line 148 still uses the old path /workflow instead of /workflows. This inconsistency means that if the OAuth token exchange fails, users will be redirected to the wrong URL. Update this line to use /workflows to match the successful redirect path on line 135.
| config: dataSafe.data.Config , | ||
| CredentialsID: dataSafe.data.Config?.credentialId || null |
There was a problem hiding this comment.
There is an inconsistency between the node creation and update logic. The create endpoint (line 527) destructures credentialId from Config and stores it separately, while this update now keeps credentialId in Config. This means newly created nodes will have Config without credentialId, but updated nodes will have Config with credentialId. For consistency, either both should remove credentialId from Config, or both should keep it. Based on line 37 in executionRoutes.ts which uses config.credentialId as a fallback, keeping it in Config seems intentional. Consider also updating the create endpoint (line 527) to match this behavior.
Summary by CodeRabbit