Handle credential update token responses#82
Conversation
|
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:
📝 WalkthroughWalkthroughRemoves guest-to-user conversion APIs, adds a CredentialUpdateResponse and token persistence on change-password, parameterizes AI model selection in tests via env vars, updates integration polling/timeouts, and refreshes related docs and examples. ChangesGuest-to-User Account Conversion Removal & Password Change Token Persistence
Configurable AI Model Selection Infrastructure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
AnthonyRonning
force-pushed
the
aead-aad-auth-binding-compat
branch
from
c38f391 to
9c8d758
Compare
Deploying opensecret-sdk with
|
| Latest commit: |
cd0cce9
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://8b6b7b0d.opensecret-sdk.pages.dev |
| Branch Preview URL: | https://aead-aad-auth-binding-compat.opensecret-sdk.pages.dev |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
AnthonyRonning
force-pushed
the
aead-aad-auth-binding-compat
branch
from
47bffd1 to
ce28e55
Compare
AnthonyRonning
force-pushed
the
aead-aad-auth-binding-compat
branch
from
ce28e55 to
9626c74
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
website/docs/api/type-aliases/OpenSecretContextType.md (1)
908-916:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClarify password change/recovery limitations after guest-conversion removal.
Line 908 now states the account has "no email recovery," which aligns with the removal of guest-conversion APIs. However, line 916 says the password "cannot be changed or recovered without adding email address," which may mislead users into believing they can add an email when the conversion feature has been removed.
Consider revising line 916 to remove the implication that adding an email is possible, or clarify the recovery path if one still exists outside the removed
convertGuestToUserAccountmethod.📝 Suggested rewording
If adding email is no longer possible:
-User's chosen password, cannot be changed or recovered without adding email address. +User's chosen password. Cannot be changed or recovered without an email address.Or more explicitly:
-User's chosen password, cannot be changed or recovered without adding email address. +User's chosen password. Cannot be changed or recovered since this account has no email.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@website/docs/api/type-aliases/OpenSecretContextType.md` around lines 908 - 916, Update the documentation for the "Creates a new long-lived guest account" entry in OpenSecretContextType to remove any implication that a guest can later "add an email" to change or recover the password; specifically revise the password parameter description (the text currently saying "cannot be changed or recovered without adding email address") to a definitive statement that password change/recovery is not available for guest accounts (or state the exact alternative recovery path if one still exists) so readers aren’t misled by references to the removed convertGuestToUserAccount flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@website/docs/api/type-aliases/OpenSecretContextType.md`:
- Around line 908-916: Update the documentation for the "Creates a new
long-lived guest account" entry in OpenSecretContextType to remove any
implication that a guest can later "add an email" to change or recover the
password; specifically revise the password parameter description (the text
currently saying "cannot be changed or recovered without adding email address")
to a definitive statement that password change/recovery is not available for
guest accounts (or state the exact alternative recovery path if one still
exists) so readers aren’t misled by references to the removed
convertGuestToUserAccount flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 74bffbfd-e226-4116-b7fb-8b987d4c0e96
📒 Files selected for processing (9)
.github/workflows/test.ymlpackage.jsonrust/src/client.rsrust/tests/ai_integration.rssrc/lib/main.tsxsrc/lib/test/integration/ai.test.tswebsite/api/type-aliases/OpenSecretContextType.mdwebsite/docs/api/type-aliases/OpenSecretContextType.mdwebsite/docs/guides/guest-accounts.md
✅ Files skipped from review due to trivial changes (2)
- package.json
- .github/workflows/test.yml
🚧 Files skipped from review as they are similar to previous changes (6)
- website/api/type-aliases/OpenSecretContextType.md
- rust/tests/ai_integration.rs
- rust/src/client.rs
- website/docs/guides/guest-accounts.md
- src/lib/test/integration/ai.test.ts
- src/lib/main.tsx
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/test/integration/ai.test.ts (1)
816-821:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign polling budget with the test timeout.
At Line 816,
34 × 300msis ~10.2s of wait time before network latency, which can exceed the 10s timeout at Line 854 and fail before the terminal-status checks at Line 825.Suggested patch
let retrievedResponse = await fetchResponse(responseId); const terminalStatuses = new Set(["completed", "failed", "cancelled"]); + const POLL_INTERVAL_MS = 300; + const MAX_POLL_ATTEMPTS = 34; for ( let attempt = 0; - attempt < 34 && !terminalStatuses.has(retrievedResponse.status); + attempt < MAX_POLL_ATTEMPTS && !terminalStatuses.has(retrievedResponse.status); attempt++ ) { - await new Promise((resolve) => setTimeout(resolve, 300)); + await new Promise((resolve) => setTimeout(resolve, POLL_INTERVAL_MS)); retrievedResponse = await fetchResponse(responseId); } @@ -}, 10000); +}, 15000);Also applies to: 825-825, 854-854
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/test/integration/ai.test.ts` around lines 816 - 821, The polling loop using variables attempt, retrievedResponse, fetchResponse and terminalStatuses currently does up to 34 attempts with a 300ms sleep (~10.2s) which can exceed the test timeout; change the polling budget so (attempts × delay) is safely below the test timeout: either lower the max attempts (e.g., to 30) or reduce the delay (e.g., to 250ms), or tie the loop limit to the test timeout constant used at the test-level so the loop cannot wait longer than the test timeout before checking terminalStatuses and returning.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/lib/test/integration/ai.test.ts`:
- Around line 816-821: The polling loop using variables attempt,
retrievedResponse, fetchResponse and terminalStatuses currently does up to 34
attempts with a 300ms sleep (~10.2s) which can exceed the test timeout; change
the polling budget so (attempts × delay) is safely below the test timeout:
either lower the max attempts (e.g., to 30) or reduce the delay (e.g., to
250ms), or tie the loop limit to the test timeout constant used at the
test-level so the loop cannot wait longer than the test timeout before checking
terminalStatuses and returning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1c994f51-b232-4dba-be15-e38cd7ebed10
📒 Files selected for processing (4)
src/lib/api.tssrc/lib/main.tsxsrc/lib/test/integration/ai.test.tssrc/lib/test/integration/api.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/api.ts
- src/lib/test/integration/api.test.ts
- src/lib/main.tsx
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
AnthonyRonning
force-pushed
the
aead-aad-auth-binding-compat
branch
from
e296695 to
f6a2c48
Compare
AnthonyRonning
force-pushed
the
aead-aad-auth-binding-compat
branch
from
ed30fa7 to
3f46dce
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
1 participant
Summary
Verification
masterserver:masterserver:test_guest_change_password_keeps_authenticated_token_stateRollout Notes
Summary by CodeRabbit
Bug Fixes
Removed Features
Documentation
Tests / CI