-
Notifications
You must be signed in to change notification settings - Fork 6
feat: implement delete all conversations endpoint in Rust and TS SDKs #55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughAdds bulk conversation deletion across Rust and TypeScript SDKs: new delete_conversations/deleteConversations API methods, a ConversationsDeleteResponse type, TypeScript context/provider wiring, tests exercising the delete flow, and minor formatting/type adjustments (ResponsesRetrieveResponse.output broadened to Changes
Sequence DiagramsequenceDiagram
participant Consumer
participant Provider as OpenSecretProvider
participant SDK as API (TS/Rust)
participant Backend as /v1/conversations
Consumer->>Provider: call deleteConversations()
Provider->>SDK: deleteConversations()
SDK->>Backend: Encrypted DELETE /v1/conversations
Backend-->>SDK: {"object":"list.deleted","deleted":true}
SDK-->>Provider: Promise<ConversationsDeleteResponse>
Provider-->>Consumer: resolved response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Deploying opensecret-sdk with
|
| Latest commit: |
0dddce8
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c817d1a8.opensecret-sdk.pages.dev |
| Branch Preview URL: | https://feat-delete-all-conversation.opensecret-sdk.pages.dev |
Greptile OverviewGreptile SummaryAdded Key Changes
Implementation Quality
Confidence Score: 5/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant Client as SDK Client
participant API as OpenSecret API
participant DB as Database
Note over Client,DB: Delete All Conversations Flow
Client->>API: DELETE /v1/conversations
Note right of Client: Authenticated request<br/>(JWT or API Key)
API->>API: Verify authentication
API->>API: Decrypt request
API->>DB: Query user's conversations
DB-->>API: Return conversation IDs
API->>DB: Delete all conversations<br/>and associated items
DB-->>API: Deletion confirmed
API->>API: Encrypt response
API-->>Client: ConversationsDeleteResponse<br/>{object: "list.deleted", deleted: true}
Note over Client: Response decrypted<br/>in SDK
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
11 files reviewed, no comments
c35b528 to
0dddce8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
11 files reviewed, no comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 (1)
rust/tests/ai_integration.rs (1)
239-266: Address failingtest_guest_user_cannot_use_aibefore mergingCI indicates
test_guest_user_cannot_use_aiis currently failing (“Guest users should not be able to access models”), so the guest user is now apparently able to hit at leastget_models()successfully.Please decide whether:
- The intended behavior changed (guests may use AI) → update/relax this test accordingly, or
- Guests should still be blocked → adjust server-side behavior (or client-side error handling) so the assertions here hold again.
Either way, this failure needs to be resolved for the Rust CI pipeline to pass.
🧹 Nitpick comments (4)
rust/src/types.rs (1)
301-305:ConversationsDeleteResponsestruct matches the new endpoint shapeThe struct mirrors the TS
ConversationsDeleteResponseat runtime (object+deleted) and follows existing derive patterns. If you ever want stricter typing, you could narrowobject(e.g., to a small enum or a&'static str), but it’s not required for this PR.src/lib/main.tsx (1)
755-766: Conversation list/delete additions are wired correctly; consider DRYing the params typeAdding
listConversationsanddeleteConversationsto the context type, default value, and providervaluekeeps the React surface aligned with the new API functions and looks correct.One minor suggestion:
listConversationscurrently inlines itsparamsshape. To avoid drift ifapi.listConversationsevolves (e.g., addingorder), you might eventually:
- Export a
ConversationsListParamstype from./api, and/or- Type this as
typeof api.listConversationsfor consistency with most other methods.Not urgent, but it would reduce maintenance risk.
Also applies to: 911-911, 1325-1325
rust/tests/ai_integration.rs (1)
178-207:test_delete_conversationscovers the core contract; room for future tighteningThe integration test correctly exercises
delete_conversationson an authenticated client and asserts the expected"list.deleted"+deleted: trueresponse, which is enough to validate the endpoint wiring today.Once the Rust client exposes explicit conversation create/list APIs, you might:
- Create a few conversations first and then assert that subsequent list calls are empty after deletion.
- Trim some of the now-historical comments about SDK gaps to keep the test focused.
For now, this is fine as an initial coverage point.
src/lib/api.ts (1)
1565-1565: Consider more specific typing thanany[].The broadened type
string | any[]improves flexibility but loses type safety. Consider defining a more specific type for the array case based on the actual response structure (e.g.,Array<ResponseOutputItem>whereResponseOutputItemhas defined shape).// Consider defining a more specific type export type ResponseOutputItem = { type: "message"; content: Array<{ type: string; text?: string; }>; // ... other known fields }; export type ResponsesRetrieveResponse = { // ... output?: string | Array<ResponseOutputItem>; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
rust/src/client.rs(1 hunks)rust/src/types.rs(1 hunks)rust/tests/ai_integration.rs(1 hunks)src/lib/ai.ts(2 hunks)src/lib/api.ts(16 hunks)src/lib/encryptedApi.ts(1 hunks)src/lib/index.ts(1 hunks)src/lib/main.tsx(4 hunks)src/lib/test/integration/ai.test.ts(10 hunks)src/lib/test/integration/api.test.ts(1 hunks)src/lib/test/integration/apiKeys.test.ts(6 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-21T21:30:25.143Z
Learnt from: AnthonyRonning
Repo: OpenSecretCloud/OpenSecret-SDK PR: 44
File: src/lib/test/integration/apiKeys.test.ts:2-8
Timestamp: 2025-08-21T21:30:25.143Z
Learning: In the OpenSecret SDK codebase, integration tests follow an established architecture where the API URL is set globally via bunfig.toml's preload configuration that includes api-url-loader.ts, which runs before all tests. Individual test files do not call setApiUrl() as this would be redundant and inconsistent with the established pattern used across all integration tests.
Applied to files:
src/lib/test/integration/ai.test.tssrc/lib/test/integration/apiKeys.test.ts
📚 Learning: 2025-08-21T21:30:25.143Z
Learnt from: AnthonyRonning
Repo: OpenSecretCloud/OpenSecret-SDK PR: 44
File: src/lib/test/integration/apiKeys.test.ts:2-8
Timestamp: 2025-08-21T21:30:25.143Z
Learning: In the OpenSecret SDK codebase, integration tests use a global setup architecture where bunfig.toml preloads api-url-loader.ts, which calls setApiUrl() globally before all tests run. Individual integration test files (like api.test.ts and apiKeys.test.ts) do not call setApiUrl() as this would be redundant and inconsistent with the established pattern. The API URL is set once globally via environment variables through this preload mechanism.
Applied to files:
src/lib/test/integration/ai.test.tssrc/lib/test/integration/apiKeys.test.ts
📚 Learning: 2025-02-28T02:12:34.704Z
Learnt from: AnthonyRonning
Repo: OpenSecretCloud/OpenSecret-SDK PR: 24
File: src/lib/platformApi.ts:291-300
Timestamp: 2025-02-28T02:12:34.704Z
Learning: The listProjectSecrets function in platformApi.ts only returns metadata about secrets (key_name and timestamps) and not the actual secret values, following security best practices.
Applied to files:
src/lib/api.ts
🧬 Code graph analysis (6)
rust/src/client.rs (2)
src/lib/api.ts (1)
ConversationsDeleteResponse(1642-1645)src/lib/index.ts (1)
ConversationsDeleteResponse(28-28)
rust/src/types.rs (2)
src/lib/api.ts (1)
ConversationsDeleteResponse(1642-1645)src/lib/index.ts (1)
ConversationsDeleteResponse(28-28)
src/lib/test/integration/ai.test.ts (2)
src/lib/api.ts (3)
transcribeAudio(1511-1546)listConversations(2062-2090)deleteConversations(1944-1951)src/lib/ai.ts (1)
createCustomFetch(9-204)
src/lib/main.tsx (2)
src/lib/api.ts (1)
ConversationsListResponse(1628-1634)src/lib/index.ts (1)
ConversationsListResponse(26-26)
src/lib/test/integration/apiKeys.test.ts (2)
src/lib/api.ts (3)
listApiKeys(1247-1259)deleteApiKey(1282-1291)createApiKey(1218-1225)src/lib/ai.ts (1)
createCustomFetch(9-204)
src/lib/api.ts (2)
src/lib/index.ts (5)
ConversationsDeleteResponse(28-28)Conversation(21-21)ConversationCreateRequest(23-23)ConversationsListResponse(26-26)ResponsesCreateRequest(20-20)src/lib/encryptedApi.ts (1)
authenticatedApiCall(17-77)
🪛 GitHub Actions: Rust CI
rust/tests/ai_integration.rs
[error] 241-241: Guest users should not be able to access models. Test 'test_guest_user_cannot_use_ai' failed (exit code 101).
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (12)
src/lib/encryptedApi.ts (1)
91-104: Formatting-only change inopenAiAuthenticatedApiCallThe refactor to a single-line
internalEncryptedApiCalland whitespace cleanup are no-ops behaviorally; the API-key path and error handling remain correct and consistent withauthenticatedApiCall.src/lib/test/integration/api.test.ts (1)
335-337: Multi-lineupdatedInstruction.promptassertion is fineOnly readability changed; the expectation string is identical, so test behavior is preserved.
src/lib/index.ts (1)
27-28: Re-export ofConversationsDeleteResponsekeeps public TS surface in syncExporting
ConversationsDeleteResponsealongsideConversationDeleteResponsematches the new/v1/conversationsdelete-all API and allows consumers to import the type from the root.src/lib/ai.ts (1)
9-11:createCustomFetchsignature and TTS header tweaks are behavior‑preservingThe multi-line
createCustomFetchsignature and lowercased header names for TTS responses do not change runtime behavior; the custom fetch API and decryption logic remain intact.Also applies to: 167-170
src/lib/main.tsx (1)
645-671: Transcribe audio JSDoc spacing change is non-functionalOnly comment formatting changed around
transcribeAudio; the documented behavior and function signature remain the same.src/lib/test/integration/ai.test.ts (4)
2-8: LGTM: Import additions are correct.The new imports
deleteConversationsandlistConversationsproperly reference the functions added tosrc/lib/api.ts.
751-760: Good defensive programming with optional chaining.The addition of optional chaining (
retrievedResponse.output?.length) and runtime type guards properly handles the updated type signature ofResponsesRetrieveResponse.output, which now allowsstring | any[].
814-819: Consistent defensive programming pattern.The type checking pattern matches the changes at lines 751-760, ensuring robust handling of the updated
ResponsesRetrieveResponse.outputtype.
1101-1131: Well-structured integration test for bulk deletion.The test properly verifies the
deleteConversations()workflow:
- Creates test data (2 conversations)
- Verifies existence (at least 2 conversations)
- Calls bulk delete
- Verifies response structure
- Confirms deletion (exactly 0 conversations)
src/lib/api.ts (2)
1642-1645: LGTM: Type definition follows established patterns.The
ConversationsDeleteResponsetype is well-structured and consistent with the existingConversationDeleteResponse(singular). Theobject: "list.deleted"discriminator clearly distinguishes bulk deletion from single-item deletion.
1925-1951: Client-side implementation is correct; backend safeguards are outside scope of this PR.The function properly implements the client-side DELETE call to
/v1/conversationswith appropriate error handling and documentation. However, this repository contains only client libraries (TypeScript and Rust), not the backend implementation. Verification of backend safeguards (soft delete, backup, audit logging, batch handling for large datasets) must be conducted against the actual backend service repository, which is out of scope for this PR.Confirm with the backend team that the endpoint implements appropriate protections before deployment.
src/lib/test/integration/apiKeys.test.ts (1)
82-91: Good improvement: Cleanup infinallyblock.Moving the cleanup logic to a
finallyblock ensures that test API keys are deleted even if the test fails. The try-catch within the cleanup loop properly handles individual deletion failures without stopping the cleanup of remaining keys.
This PR implements the
DELETE /v1/conversationsendpoint in both the Rust and TypeScript SDKs.Changes:
delete_conversationsmethod toOpenSecretClientandConversationsDeleteResponsestruct.deleteConversationsfunction andConversationsDeleteResponsetype.Verified with:
cargo checkbun run buildSummary by CodeRabbit
New Features
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.