[fix] Show the secret picker above the attach drawer and anchor the attach on the head revision - #6743
Conversation
… attach on the head revision The Attach a secret drawer sits at z-index 1000 above the Advanced dialog, and the shared Select portals its option list to body at z-50, so the list painted under the drawer and the picker looked empty. The drawer and the secret form now pass the layer above the drawer to every SelectContent they render. The drawer also refetches the vault list when it opens, because the vault query keeps a live subscriber for the whole page and nothing else ever refetched it. The bindings commit sent base_revision_id equal to the revision the panel displayed, and the server rejects any base that is not the head, so every attach from a panel on an older revision failed with a 409 and Retry resent the same stale base. The commit now reads the variant head first and builds the bindings-only revision on the head's data; on a real race it re-reads once and retries, then raises a plain message. Closes #6733 Closes #6734 Claude-Session: https://claude.ai/code/session_01AY886ajXX65KAa1Vc9JCXU
… and keep drafts off a newer head Review of the head-anchored commit found two data-loss paths. The callers send the full attachment list from the revision the panel displays, so a commit on a head whose attachments differ would silently drop the other change. The atom now compares the head's attachments with the displayed ones before every attempt and asks for a reload when they differ, which keeps the review rule from the design plan. Edits typed during the request are carried to the adopted revision only when it was built on the displayed revision, because copying them onto a newer head would revert the head's other fields locally. The 409 check now requires the revision_conflict code, so a proxy 409 stays a plain error. The vault refetch moved to its own effect keyed on open, because the refetch callback changes identity on every fetch. Claude-Session: https://claude.ai/code/session_01AY886ajXX65KAa1Vc9JCXU
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📘 Docs preview
This comment updates in place on every push. |
|
Mahmoud Mabrouk seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 4 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughThis PR improves legacy vault migration, refreshes vault secrets when the attachment drawer opens, fixes portaled selector layering, and anchors agent credential commits to the current workflow head with conflict handling. ChangesCustom secret attachment flow
Estimated code review effort: 4 (Complex) | ~45 minutes Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant AttachmentDrawer
participant CredentialCommit
participant WorkflowAPI
AttachmentDrawer->>CredentialCommit: Submit secret bindings
CredentialCommit->>WorkflowAPI: Retrieve current variant head
WorkflowAPI-->>CredentialCommit: Return head revision
CredentialCommit->>WorkflowAPI: Commit bindings against head
WorkflowAPI-->>CredentialCommit: Return committed revision or conflict
CredentialCommit->>WorkflowAPI: Retrieve moved head and retry once
Merge Risk: ⚪ Minimal · up to This change improves secret migration, picker refresh and layering, and concurrent credential attachment handling. No concrete merge-blocking risk remains in the supplied review context. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
…e legacy key migration The vault hook reported loading while the one-time localStorage migration had not finished. A legacy payload that an older build wrote once instead of twice made the migration throw, it rolled back to not-migrated, and every secret picker and the Advanced section showed Loading secrets forever. The migration now accepts both encodings, backs up and clears a payload it cannot read, and always ends migrated. The loading flag no longer includes the migration state. Claude-Session: https://claude.ai/code/session_01AY886ajXX65KAa1Vc9JCXU
![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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Advanced
Run ID: 7df89255-301c-4c83-9ceb-bf86ec884f2f
📒 Files selected for processing (3)
web/packages/agenta-entities/src/secret/state/atoms.tsweb/packages/agenta-entities/src/secret/state/useVaultSecret.tsweb/packages/agenta-entities/tests/unit/vault-migration.test.ts
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review.
Railway Preview Environment
Updated at 2026-09-10T12:07:05.958Z |
…a bad one CodeRabbit: a null entry in the legacy list threw inside the loop, which marked the migration done without the backup and cleanup. The list now goes through a zod schema via safeParseWithLogging, null entries drop out, a keyless entry is skipped, and one entry that fails to save no longer stops the others. Backup and cleanup always run. Claude-Session: https://claude.ai/code/session_01AY886ajXX65KAa1Vc9JCXU
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Advanced
Run ID: 22263dce-24e4-464a-8cbe-fdf0012de6f4
📒 Files selected for processing (2)
web/packages/agenta-entities/src/secret/state/atoms.tsweb/packages/agenta-entities/tests/unit/vault-migration.test.ts
Included review availability: Your plan provides up to 8 included reviews per hour; 4 remain after this review.
…e entries one by one Codex gate review: a legacy key whose save failed was cleared from storage with the migration marked done, so a temporary API error lost it for good. The entries that fail to save now stay in the active storage key in the canonical form, and the next page load retries them. The list is also validated entry by entry, so one malformed entry no longer rejects the valid ones beside it. Claude-Session: https://claude.ai/code/session_01AY886ajXX65KAa1Vc9JCXU
|
Review rounds, for the record. Codex (xhigh) on the first commit found that a commit on the head with the displayed attachment list could drop an attachment added elsewhere, and that in-flight draft edits copied onto a newer head reverted its fields locally; commit 4e98fc3 closes both. CodeRabbit found that a null legacy entry threw inside the migration loop; commit aa4448b validates entries and isolates failures. Codex on the full branch then found that a failed save cleared the key for good and that whole-list validation rejected valid entries beside a bad one; commit 0e246df keeps failed entries for a retry on the next load and validates entry by entry. All three reviewers' findings are addressed, and every thread is resolved. |
2 participants
Context
Three bugs in the custom-secret attach flow that shipped in v0.115.4, all found in live use.
The Attach a secret drawer looked like it had no secrets. Clicking "Select a secret" opened the list with every text secret in it, but the list was drawn under the drawer. The drawer sits at
zIndex1000 so it stacks above the Advanced dialog, and the sharedSelectContentportals its list tobodyatz-50. Nothing was visible, on both the desktop app and/m. Closes #6733.The drawer and the Advanced section could also sit on "Loading secrets" forever. The vault hook reported loading while the one-time migration of legacy provider keys from browser local storage had not finished. A legacy payload that an older build wrote once instead of twice made that migration throw, it rolled back to "not migrated", and every secret picker stayed on Loading. It depends on what a browser holds for the origin, so it hits some people and not others. Found during manual QA of this branch.
Attaching from the chat card could fail with a raw
409 revision_conflictbody, and Retry could never succeed. The commit sentbase_revision_idequal to the revision the panel displayed. The server rejects any base that is not the variant head. The reporter's panel showed revision 106 while the head had been 107 for two days, so every attach failed with the same stale base. Closes #6734.Changes
The picker draws above the drawer.
AgentSecretAttachmentDrawerpasseszIndex + 1as an inline style to itsSelectContent.SecretFormtakes a newpopupZIndexprop for the two selects in its JSON grid, and both the attach drawer (1000) andCreateSecretDrawer(1100) pass it. An inline style beats thez-50class, and it follows the drawer's ownzIndexprop instead of a second hard-coded number.The drawer refetches the vault list when it opens. The vault query keeps a live subscriber for the whole page, so nothing ever refetched it. A secret created on the Settings page in another tab stayed invisible until a reload. The drawer now refetches in an effect keyed on
open.Loading no longer waits on the legacy migration. The migration accepts both encodings of the old local storage payload, backs up and clears a payload it cannot read, and always ends in the migrated state. The hook's loading flag is the query's pending state plus "migrating", nothing else.
The bindings commit anchors on the head.
commitAgentCredentialsAtomreads the variant head first and builds the bindings-only revision on the head's data. The displayed revision names the variant, gates on unsaved edits, and supplies the attachments the user was looking at. Because both callers send the full attachment list, a head whose attachments differ from the displayed ones would be overwritten, so that case refuses with a plain message and the user reloads. That keeps the review rule from the design plan.Before:
After:
The lost-response recovery is unchanged. A 409 without the
revision_conflictcode stays a plain error. Edits typed during the request are carried to the adopted revision only when it was built on the displayed revision; copied onto a newer head they would revert the head's other fields locally. Adoption still keys on the displayed revision, because that is the revision the panel switches away from.Tests
web/packages/agenta-entities/tests/unit/agent-credentials-commit.test.ts: 14 tests, eight new. They cover the head anchor when the panel is on an older revision, the fallback when the head read fails, the single re-read and retry on a race, the refusal when the head's attachments differ (before and after a race), a 409 without the conflict code, and the draft that must not be carried onto a newer head. The happy path asserts one head read.web/packages/agenta-entities/tests/unit/vault-migration.test.ts: three tests for the migration, including an unreadable payload and a once-encoded payload.vitest,tsc --noEmitandeslintinagenta-entitiesandagenta-entity-ui: green.http://144.76.237.122:8580, EE dev, fresh database):/m: the picker lists both seeded secrets on screen, layer 1001 above the drawer, and the two secrets were created after the tab had loaded, so the refetch on open is covered too.request_secret, Configure opened the drawer withNPM_TOKENprefilled, Create new saved a placeholder secret, Attach committed revision 5, the card settled as "NPM_TOKEN attached" and the run resumed.Not touched: why the playground panel sat on an older revision in the first place. That is a separate question about how the chat picks its revision, and it no longer breaks the attach.
What to QA
Setup: a project with at least two text secrets in Settings → Secrets, and an agent with more than one revision.
request_secret, click Configure, create or pick a secret, attach. The card settles and the conversation resumes.https://claude.ai/code/session_01AY886ajXX65KAa1Vc9JCXU