Repository navigation
Fix connect-secret rollback error copy - #86
Merged
Merged
Conversation
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new /connect/secret feature: a client-side multi-step Remix route for creating/updating secrets, server-side UI and API handlers for session-backed initialization and persistence, route registrations, and a new MCP capability returning a static guide for the feature. Changes
Sequence DiagramsequenceDiagram
participant Browser as Client (Browser)
participant UI as Server UI Handler (/connect/secret)
participant API as Server API Handler (/connect/secret.json)
participant Session as Session Manager
participant Store as Secret Storage
Browser->>UI: GET /connect/secret?name&scope&connector...
UI->>Session: validate user session
Session-->>UI: session valid / redirect
UI-->>Browser: render page (form or update-confirm)
Browser->>API: GET /connect/secret.json?scope&name&connector
API->>Session: create/verify generated UI session token
Session-->>API: token + sessionId + endpoints
API-->>Browser: return JSON (sessionToken, endpoints)
Browser->>Browser: user fills & confirms
Browser->>API: POST /connect/secret.json {name, scope, sessionToken, value?, connector?, allowedHosts?, allowedCapabilities?}
API->>Session: verify sessionToken / app user
Session-->>API: verification result
alt verified
API->>Store: resolve existing secret (by name/scope) and save value (if provided)
Store-->>API: resolved/saved
API->>Store: save connector config (normalize allowedHosts/capabilities)
Store-->>API: config saved
API-->>Browser: { ok: true }
else verification failed
API-->>Browser: 401/403 JSON error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Owner
Author
|
@cursoragent could this not just be a regular remix route? I don't mind a bit of duplication with the generated UI code. This is an official route, not generated ui. Also, do we have the right query params and is there a capability added for agents to know how to generate a URL for this route? They should use this route whenever they have a need for the user to provide a secret. It should optionally have a prefilled set of allowed hosts and allowed capabilities as well. |
Contributor
|
Taking a look!   |
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
kentcdodds
commented
Mar 28, 2026
| &scope=user | ||
| &dashboardUrl=https://linear.app/settings/api | ||
| &instructions=Go to Linear Settings → API → Personal API Keys → Create key | ||
| &allowedCapabilities=linear_issue_list,linear_issue_create |
Owner
Author
There was a problem hiding this comment.
@cursoragent make sure the agent understands that allowedCapabilities should only be capabilities which Kody actually has. I don't want the agent to make up capabilities that don't exist.
Contributor
There was a problem hiding this comment.
I updated the connect-secret guide to explicitly warn that allowedCapabilities must only include real, registered Kody capabilities (no made-up names) and pushed the change. The PR description now reflects that clarification.
No further code changes are pending.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
kentcdodds
marked this pull request as ready for review
March 28, 2026 17:52
Contributor
|
🔎 Preview deployed: https://kody-pr-86.kentcdodds.workers.dev Worker: Mocks:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/worker/client/routes/connect-secret.tsx (2)
300-309: Consider handling partial failure more gracefully.If
saveSecretValuesucceeds butupdateConnectorConfigfails, the secret is saved but the user sees an error. On retry, they may encounter the "Secret already exists" flow. This is an edge case but could cause user confusion.Consider either:
- Showing a specific message when connector config fails but secret was saved
- Checking for existing secret before retry to inform the user their secret was saved
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/client/routes/connect-secret.tsx` around lines 300 - 309, Split the combined try into two phases so partial success is detected: first call saveSecretValue(params, session, state.secretValue) and if it succeeds then call updateConnectorConfig(params, session) in a separate try/catch; on updateConnectorConfig failure setState with step: 'error' plus a specific message/flag (e.g. partialSaved or error: 'Connector config update failed; secret saved') so the UI can show that the secret exists, and optionally expose a retry path that checks for an existing secret (use an API/helper like getSecret or checkSecretExists before retrying) to avoid "Secret already exists" confusion. Ensure setState usage and error branching are updated where saveSecretValue and updateConnectorConfig are referenced.
601-608: Consider disabling the save button during the saving step to prevent double submission.When
state.step === 'saving', the "Save secret" button remains enabled (onlyconfirmedReviewis checked). Users could click multiple times, triggering duplicate save requests.♻️ Proposed fix to disable during save
<button type="button" css={primaryButtonCss} - disabled={!state.confirmedReview} + disabled={!state.confirmedReview || state.step === 'saving'} on={{ click: () => void handleSave() }} > Save secret </button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/client/routes/connect-secret.tsx` around lines 601 - 608, The Save button can be clicked multiple times because only state.confirmedReview controls disabled; update the button logic (the element using primaryButtonCss and onClick -> handleSave) to also disable when state.step === 'saving' (e.g., set disabled to !state.confirmedReview || state.step === 'saving') and ensure handleSave is a no-op or ignored when state.step === 'saving' to prevent duplicate submissions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/worker/client/routes/connect-secret.tsx`:
- Around line 186-213: The saveSecretValue function is missing the required
action field in the POST body which causes the API to reject requests; update
saveSecretValue (taking ConnectSecretParams and ConnectSecretSession and posting
to session.endpoints.secrets) to include action: 'save' in the JSON body along
with name, value, description, and scope so the handler (account-secrets) can
dispatch correctly.
In `@packages/worker/src/app/handlers/connect-secret.ts`:
- Line 9: Remove the unused import buildAccountSecretPath from the top of
connect-secret.ts to fix the TypeScript build failure; locate the import
statement "import { buildAccountSecretPath } from
'@kody-internal/shared/account-secret-route.ts'" in connect-secret.ts and delete
it (or remove buildAccountSecretPath from the named import) so no unused symbol
remains.
- Around line 53-56: The call to buildConnectSecretAppId in connect-secret.ts is
unresolved; fix it by either importing the exported helper or implementing it
locally: locate the existing utility that constructs connect-secret app IDs
(matching the signature buildConnectSecretAppId({ connector, name }) and
returning a string), add an import for buildConnectSecretAppId at the top of the
file, or add a small local function with that exact name and signature that
returns the intended appId string; ensure the symbol is exported from its module
if you add it there so the import resolves and TypeScript compiles.
---
Nitpick comments:
In `@packages/worker/client/routes/connect-secret.tsx`:
- Around line 300-309: Split the combined try into two phases so partial success
is detected: first call saveSecretValue(params, session, state.secretValue) and
if it succeeds then call updateConnectorConfig(params, session) in a separate
try/catch; on updateConnectorConfig failure setState with step: 'error' plus a
specific message/flag (e.g. partialSaved or error: 'Connector config update
failed; secret saved') so the UI can show that the secret exists, and optionally
expose a retry path that checks for an existing secret (use an API/helper like
getSecret or checkSecretExists before retrying) to avoid "Secret already exists"
confusion. Ensure setState usage and error branching are updated where
saveSecretValue and updateConnectorConfig are referenced.
- Around line 601-608: The Save button can be clicked multiple times because
only state.confirmedReview controls disabled; update the button logic (the
element using primaryButtonCss and onClick -> handleSave) to also disable when
state.step === 'saving' (e.g., set disabled to !state.confirmedReview ||
state.step === 'saving') and ensure handleSave is a no-op or ignored when
state.step === 'saving' to prevent duplicate submissions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f520e857-c9f4-41db-8152-bac014df3487
📒 Files selected for processing (7)
packages/worker/client/routes/connect-secret.tsxpackages/worker/client/routes/index.tsxpackages/worker/src/app/handlers/connect-secret.tspackages/worker/src/app/router.tspackages/worker/src/app/routes.tspackages/worker/src/mcp/capabilities/coding/domain.tspackages/worker/src/mcp/capabilities/coding/generated-ui-secret-guide.ts
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/worker/client/routes/connect-secret.tsx (1)
186-213:⚠️ Potential issue | 🔴 CriticalMissing
actionfield in POST body will cause API rejection.The
saveSecretValuefunction posts to the secrets endpoint without the requiredaction: 'save'field. Based on past review verification, theaccount-secretshandler reads theactionfield from the request body and returns{ ok: false, error: 'Invalid action.' }with status 400 when no valid action is provided.🐛 Proposed fix
body: JSON.stringify({ + action: 'save', name: params.name, value, description: params.description, scope: params.scope, }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/client/routes/connect-secret.tsx` around lines 186 - 213, The POST body sent by saveSecretValue lacks the required action field so the account-secrets API rejects it; update saveSecretValue to include action: 'save' in the JSON body (alongside name, value, description, scope) so the account-secrets handler recognizes the request, and keep the existing error handling in place if the response indicates failure.packages/worker/src/app/handlers/connect-secret.ts (1)
52-55:⚠️ Potential issue | 🔴 Critical
buildConnectSecretAppIdis undefined — build failure.The function
buildConnectSecretAppIdis called but never defined or imported. The TypeScript compiler confirms this causes a build failure.You need to either:
- Import the function from an existing module, or
- Define it locally in this file
🐛 Proposed fix (local definition)
+function buildConnectSecretAppId(input: { + connector: string | null + name: string | null +}): string | null { + if (!input.connector && !input.name) return null + return `connect-secret:${input.connector ?? 'unknown'}:${input.name ?? 'unknown'}` +} + export function createConnectSecretApiHandler(env: Env) {Note: Adjust the implementation to match the expected app ID format used elsewhere in the codebase.
#!/bin/bash # Search for similar buildAppId functions to determine the expected pattern rg -n "buildAppId|build.*AppId" --type ts -A 3 | head -40 # Check if there's an existing function that should be imported rg -n "export.*function.*AppId|export const.*AppId" --type ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/app/handlers/connect-secret.ts` around lines 52 - 55, The call to buildConnectSecretAppId in connect-secret.ts is failing because buildConnectSecretAppId is not defined or imported; fix by either importing buildConnectSecretAppId from the module that exports it (search for an existing build*AppId helper and add an import) or implement a local function named buildConnectSecretAppId that accepts ({ connector, name }: { connector: string; name: string }) and returns the app ID string in the same format used elsewhere (matching other build*AppId helpers); update the call site that uses scope, connector, and name to rely on the imported or newly defined buildConnectSecretAppId.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/worker/client/routes/connect-secret.tsx`:
- Around line 186-213: The POST body sent by saveSecretValue lacks the required
action field so the account-secrets API rejects it; update saveSecretValue to
include action: 'save' in the JSON body (alongside name, value, description,
scope) so the account-secrets handler recognizes the request, and keep the
existing error handling in place if the response indicates failure.
In `@packages/worker/src/app/handlers/connect-secret.ts`:
- Around line 52-55: The call to buildConnectSecretAppId in connect-secret.ts is
failing because buildConnectSecretAppId is not defined or imported; fix by
either importing buildConnectSecretAppId from the module that exports it (search
for an existing build*AppId helper and add an import) or implement a local
function named buildConnectSecretAppId that accepts ({ connector, name }: {
connector: string; name: string }) and returns the app ID string in the same
format used elsewhere (matching other build*AppId helpers); update the call site
that uses scope, connector, and name to rely on the imported or newly defined
buildConnectSecretAppId.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 692453bd-4c12-4f88-9f1a-00c8115d23a4
📒 Files selected for processing (2)
packages/worker/client/routes/connect-secret.tsxpackages/worker/src/app/handlers/connect-secret.ts
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Owner
Author
|
@cursoragent please check CI on this PR and fix any issues. Also address any valid feedback on this PR |
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/worker/client/routes/connect-secret.tsx`:
- Around line 216-244: The current flow saves the secret first then calls
updateConnectorConfig(), which can fail and leave a partial state; change the
client-server contract so the client sends the secret and connector config
together and the server performs a single atomic operation instead of two
separate requests (modify the POST payload sent by handleSave() to include
connector data and update the server handler to persist both or roll back on
failure). If changing the server endpoint is not possible immediately, implement
rollback on connector-config failure by calling the secret-delete endpoint from
updateConnectorConfig() (or return a distinct partial-success error) so the UI
can surface a clear partial-success/rollback state; target symbols: handleSave,
updateConnectorConfig.
- Around line 246-250: ConnectSecretRoute currently keeps state and session in
closure and initialize() only updates parts, causing secretValue/confirmation
flags to leak and stale async inits to overwrite newer ones; modify initialize()
(and any async init logic used in the blocks around lines referenced) to
immediately reset state = { ...defaultState } and set session = null when the
incoming query/search changes (compare against lastSearch), then bump a local
initVersion token (or capture a unique local version) before awaiting async work
and ignore any async results whose captured version does not match the current
token so stale completions are discarded; update lastSearch after committing the
reset so subsequent runs see the new value.
In `@packages/worker/src/app/handlers/connect-secret.ts`:
- Around line 84-85: The handler reads requestedAllowedCapabilities via
readOptionalStringArray(body, 'allowedCapabilities') and persists them to the
connector config; you must validate each requested capability against the
canonical set of registered capabilities (the app's capability registry or
constant) before saving and reject the request with a 400 if any unknown names
are present. Update the connect-secret handler (and the same logic used around
the save flow at the second occurrence) to compute unknown =
requestedAllowedCapabilities.filter(c => !registeredCapabilities.has(c)), and
when unknown.length > 0 return a 400 response listing the invalid names instead
of proceeding to persist the config.
- Around line 47-58: The GET branch wrongly references session.sessionId which
isn't declared there; update the appId computation to derive the app identifier
from the request instead of using the POST-only session variable—e.g. replace
"const appId = scope === 'app' ? session.sessionId : null" with code that reads
the app id from the URL (e.g. url.searchParams.get('appId')) or another
request-specific source, keeping the rest of the flow that calls readSecretScope
and createGeneratedUiAppSession intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0c94c430-57dc-4e1b-ab18-83ca21214e3e
📒 Files selected for processing (2)
packages/worker/client/routes/connect-secret.tsxpackages/worker/src/app/handlers/connect-secret.ts
Owner
Author
|
@cursoragent please check CI on this PR and fix any issues. Also address any valid feedback on this PR |
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Owner
Author
|
@cursoragent please check CI on this PR and fix all issues. Also address any valid feedback on this PR |
Contributor
|
Bugbot Autofix prepared fixes for both issues found in the latest run.
|
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
cursor
Bot
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/worker/client/routes/connect-secret.tsx`:
- Around line 275-277: The cached closure variable session (and related state
like lastSearch/initVersion) is reused across retries, but generated UI sessions
expire after 60 minutes; on auth failures (401 or explicit session-expired),
clear the cached session and force a re-init so a fresh generated session is
minted instead of reusing the stale token. Concretely: inside handleErrorBack
and any error paths that detect 401/session-expired (also where session is
referenced around the other noted blocks), set session = null (and optionally
reset lastSearch or bump initVersion) and then route the flow to the existing
init logic that creates a new generated UI session so the subsequent retry uses
a fresh token.
- Around line 352-379: The rollback currently deletes the secret for all
failures; change the logic so rollbackSecretValue(params, session) is only
invoked when the secret was newly created (not when updating an existing
secret). Update the call site in the catch block of updateConnectorConfig to
check a creation flag (e.g., params.isNewSecret or a new createdSecretId) before
calling rollbackSecretValue, and if the connector was an update (existing
secret), skip deletion and setState to an error that notes connector-config
failed but the original secret was retained; if needed, extend
rollbackSecretValue to accept a flag like { deleteOnlyIfNew: true } or add a
helper (e.g., shouldRollbackSecret(params)) so rollbackSecretValue will not
delete secrets that were pre-existing. Ensure references to
updateConnectorConfig, rollbackSecretValue, params.connector (or new
params.isNewSecret/createdSecretId), and formatConnectorConfigFailureMessage are
updated accordingly.
In `@packages/worker/src/app/handlers/connect-secret.ts`:
- Around line 80-81: The code currently reads scope with readScope and trusts
the body value (sessionToken via readString), which allows unknown or mismatched
scopes to slip through; change readScope handling to fail closed by validating
the parsed scope against allowed values and returning a 400/error on unknown
values, and when handling POSTs that use sessionToken verify the requested scope
is compatible with the verified session (e.g., if session.app_id is null, reject
requests asking for 'app' scope, and if session.app_id is set ensure the scope
matches that app), updating the logic around the scope variable and the POST
handling to reject incompatible or misspelled scopes instead of silently falling
back to 'user'.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 66c34459-08cc-4ef6-b5ed-fd0195830d95
📒 Files selected for processing (6)
packages/worker/client/routes/connect-secret-errors.node.test.tspackages/worker/client/routes/connect-secret-errors.tspackages/worker/client/routes/connect-secret.tsxpackages/worker/src/app/handlers/connect-secret.tspackages/worker/src/app/router.tspackages/worker/src/app/routes.ts
✅ Files skipped from review due to trivial changes (2)
- packages/worker/client/routes/connect-secret-errors.node.test.ts
- packages/worker/src/app/router.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/worker/src/app/routes.ts
Owner
Author
|
@cursoragent please take a look at the feedback on this pull request and address valid feedback. |
Contributor
Summary
Testing
|
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
This was referenced Mar 31, 2026
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
appIdfor app-scoped/connect/secretsessions and reject invalid or incompatible secret scopesappIdquery parameter in the secret guideTesting
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests