split pinecone-client.ts with 3 sub files#86
Conversation
# Conflicts: # src/pinecone-client.ts
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR refactors ChangesPinecone Client Monolith Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #86 +/- ##
=======================================
Coverage ? 66.01%
=======================================
Files ? 33
Lines ? 1121
Branches ? 366
=======================================
Hits ? 740
Misses ? 381
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/pinecone-client.test.ts (1)
34-37: 💤 Low value
afterEachdoesn't cleanPINECONE_TOP_K.The constructor also reads
process.env['PINECONE_TOP_K'](pinecone-client.tsLine 44). Any future test that sets it will leak across tests. Cheap insurance to add it here alongside the other two.♻️ Proposed cleanup
afterEach(() => { delete process.env['PINECONE_INDEX_NAME']; delete process.env['PINECONE_RERANK_MODEL']; + delete process.env['PINECONE_TOP_K']; });🤖 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/pinecone-client.test.ts` around lines 34 - 37, The test teardown in afterEach currently deletes PINECONE_INDEX_NAME and PINECONE_RERANK_MODEL but misses PINECONE_TOP_K, which the PineconeClient constructor reads from process.env; update the afterEach in src/pinecone-client.test.ts to also delete process.env['PINECONE_TOP_K'] so tests don't leak environment state between runs (look for the afterEach block and add deletion of PINECONE_TOP_K).src/pinecone-client.ts (1)
152-163: 💤 Low valueEncapsulation concern: Consider wrapping reranking in a session method instead of exposing the raw client.
ensureClient()is confirmed as a public method onPineconeIndexSession(atsrc/pinecone/indexes.ts:45) that returns aPineconeSDK instance. The method is correctly used byrerankResultsImpl, which calls.inference.rerankon the client (as seen in `src/pinecone/rerank.ts:24).However, exposing the raw Pinecone client through a public method widens the encapsulation boundary that the session facade was intended to maintain. Consider refactoring
rerankResultsImplto accept the session and call a narrower reranking method (e.g.,indexSession.rerank(...)) instead, keeping SDK access fully encapsulated within the session class.🤖 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/pinecone-client.ts` around lines 152 - 163, The code currently passes a raw SDK client into rerankResultsImpl by calling this.indexSession.ensureClient(), which breaks the PineconeIndexSession encapsulation; add a narrow rerank method on PineconeIndexSession (e.g., PineconeIndexSession.rerank or similar) that internally calls the SDK's inference.rerank, change rerankResultsImpl to accept a PineconeIndexSession (or call the new session.rerank) instead of a Pinecone client, and update the call site in pinecone-client.ts to pass this.indexSession (and invoke the session.rerank wrapper) so the SDK client never leaves the session class and rerank logic remains encapsulated.
🤖 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.
Inline comments:
In `@src/pinecone-client.test.ts`:
- Line 1: Prettier style violations in src/pinecone-client.test.ts are causing
CI failures; run the code formatter on this file (e.g., run `prettier --write
src/pinecone-client.test.ts`) to fix formatting, then stage the updated file and
re-run tests; ensure imports at the top (describe, it, expect, beforeEach,
afterEach, vi) and the rest of the test file adhere to your project's
Prettier/ESLint rules before committing.
In `@src/pinecone-client.ts`:
- Around line 43-45: The assignment to this.defaultTopK should use nullish
coalescing and a radix-aware parse with NaN guarding: read the env var via
process.env['PINECONE_TOP_K'] ?? String(DEFAULT_TOP_K), call parseInt(..., 10),
then check Number.isFinite and fallback to DEFAULT_TOP_K if parseInt returns NaN
or a non-finite value; update the initialization in the constructor where
this.defaultTopK is set and ensure clampTopK will receive a valid finite integer
(refer to this.defaultTopK, clampTopK, PINECONE_TOP_K, and DEFAULT_TOP_K).
- Around line 73-78: The method listNamespacesFromKeywordIndex currently returns
Promise<Array<{ namespace: string; recordCount: number }>> which breaks the API
contract; change its return type to KeywordIndexNamespacesResult and update the
corresponding implementation in src/pinecone/indexes.ts so that
indexSession.listNamespacesFromKeywordIndex also returns the discriminated union
{ ok: true; namespaces: Array<{ namespace: string; recordCount: number }> } | {
ok: false; error: string }; ensure both listNamespacesFromKeywordIndex (in
pinecone-client.ts) and the implementation in indexes.ts preserve the
.ok/.namespaces or .ok/.error shape expected by scripts/test-search.ts.
In `@src/pinecone/indexes.test.ts`:
- Around line 1-164: The test file formatting fails Prettier; run the formatter
on the file (e.g., prettier --write src/pinecone/indexes.test.ts) or apply your
project's configured formatter to fix whitespace/quote/indentation issues so CI
passes; ensure you don't change test logic around the
PineconeIndexSessionTestDouble and ThrowingEnsureSession classes or the
listNamespaces... / checkIndexes test cases—only update formatting.
---
Nitpick comments:
In `@src/pinecone-client.test.ts`:
- Around line 34-37: The test teardown in afterEach currently deletes
PINECONE_INDEX_NAME and PINECONE_RERANK_MODEL but misses PINECONE_TOP_K, which
the PineconeClient constructor reads from process.env; update the afterEach in
src/pinecone-client.test.ts to also delete process.env['PINECONE_TOP_K'] so
tests don't leak environment state between runs (look for the afterEach block
and add deletion of PINECONE_TOP_K).
In `@src/pinecone-client.ts`:
- Around line 152-163: The code currently passes a raw SDK client into
rerankResultsImpl by calling this.indexSession.ensureClient(), which breaks the
PineconeIndexSession encapsulation; add a narrow rerank method on
PineconeIndexSession (e.g., PineconeIndexSession.rerank or similar) that
internally calls the SDK's inference.rerank, change rerankResultsImpl to accept
a PineconeIndexSession (or call the new session.rerank) instead of a Pinecone
client, and update the call site in pinecone-client.ts to pass this.indexSession
(and invoke the session.rerank wrapper) so the SDK client never leaves the
session class and rerank logic remains encapsulated.
🪄 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: 0cd654d9-49b3-4cfc-961d-c430f4a74d4e
📒 Files selected for processing (8)
src/pinecone-client.test.tssrc/pinecone-client.tssrc/pinecone/indexes.test.tssrc/pinecone/indexes.tssrc/pinecone/rerank.test.tssrc/pinecone/rerank.tssrc/pinecone/search.test.tssrc/pinecone/search.ts
Co-authored-by: Cursor <cursoragent@cursor.com>
….com/jonathanMLDev/pinecone-read-only-mcp-typescript into split-pinecone-client-rebased-branch
2 participants
Summary
src/pinecone-client.ts had grown to ~605 lines, mixing lazy SDK init, namespace discovery, single-index search, hybrid Promise.allSettled orchestration, dense/sparse merge, reranking, keywordSearch, and count. A change to any one of those required scrolling past hundreds of lines of unrelated code, and the two as unknown SDK-bypass casts (one undocumented, one with a six-line rationale) were buried inside the file.
Changes
Related Issue
close #82
Summary by CodeRabbit
Release Notes
Breaking Changes
listNamespacesFromKeywordIndex()return type updated toArray<{ namespace: string; recordCount: number }>Refactor
Tests