-
Notifications
You must be signed in to change notification settings - Fork 1.4k
feat(tools): allow passing apiKey via options for browser support #599
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
base: main
Are you sure you want to change the base?
feat(tools): allow passing apiKey via options for browser support #599
Conversation
How to use the Graphite Merge QueueAdd the label Main to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
Code Review for PR #599: feat(tools): allow passing apiKey via options for browser supportSummaryThis PR adds browser support by allowing the API key to be passed via options instead of requiring ✅ Strengths
🔍 Issues & Concerns1. Missing Test Coverage
|
Code Review: Browser Support for API KeySummaryThis PR adds the ability to pass an API key via options to ✅ Code Quality & Best PracticesStrengths:
Minor suggestions:
🐛 Potential IssuesMissing parameter in middleware call (packages/tools/src/vercel/index.ts:68)
🔒 Security ConcernsAPI Key Exposure Risk /**
* @param options.apiKey - Optional Supermemory API key.
* WARNING: Only use this in trusted browser environments.
* For production browser apps, use a server-side proxy to protect your API key.
* Falls back to process.env.SUPERMEMORY_API_KEY for server environments.
*/Recommendation: Add a security note in the README explaining best practices:
⚡ Performance ConsiderationsNo performance concerns. The changes are minimal and don't introduce any overhead. 🧪 Test CoverageMissing tests for the new feature ❗ The existing test suite in it('should use provided apiKey from options', () => {
delete process.env.SUPERMEMORY_API_KEY
const mockModel = createMockLanguageModel()
const wrappedModel = withSupermemory(
mockModel,
TEST_CONFIG.containerTag,
{ apiKey: 'explicit-api-key' }
)
expect(wrappedModel).toBeDefined()
})
it('should prefer options.apiKey over environment variable', () => {
process.env.SUPERMEMORY_API_KEY = 'env-key'
// Test that options.apiKey takes precedence
// Would need to spy on Supermemory constructor to verify
})📋 Recommendations
Overall AssessmentStatus: ✅ Approved with suggestions The implementation is solid and the feature is useful. The main concerns are:
These can be addressed in a follow-up or before merging. Great work on maintaining backward compatibility! 🎉 |
Pull Request ReviewThank you for this contribution! This is a solid enhancement that enables browser usage of the ✅ Strengths
🔍 Code Quality ObservationsGood:
Minor Notes:
🧪 Test CoverageIssue: The PR does not include tests for the new The existing test file (
it('should accept apiKey via options parameter', () => {
delete process.env.SUPERMEMORY_API_KEY
const mockModel = createMockLanguageModel()
const wrappedModel = withSupermemory(mockModel, 'test-tag', {
apiKey: 'explicit-api-key'
})
expect(wrappedModel).toBeDefined()
})
it('should prioritize options.apiKey over environment variable', () => {
process.env.SUPERMEMORY_API_KEY = 'env-key'
const mockModel = createMockLanguageModel()
// Would need to verify the middleware receives 'explicit-key'
const wrappedModel = withSupermemory(mockModel, 'test-tag', {
apiKey: 'explicit-key'
})
expect(wrappedModel).toBeDefined()
})
it('should throw error when neither options.apiKey nor env var is set', () => {
delete process.env.SUPERMEMORY_API_KEY
const mockModel = createMockLanguageModel()
expect(() => {
withSupermemory(mockModel, 'test-tag', {})
}).toThrow('SUPERMEMORY_API_KEY is not set')
})🔒 Security ConsiderationsGood:
Consideration:
⚡ Performance Considerations
📝 Suggestions
> **⚠️ Security Note**: When using `apiKey` in browser environments, be aware that the API key will be visible in client-side code. Consider using environment variables with proper access controls, or implementing a backend proxy for sensitive applications.📋 SummaryThis is a well-implemented feature that solves a real problem for browser-based usage. The code quality is good and the approach is sound. Recommendation: ✅ Approve with minor improvements The main gap is test coverage. I'd suggest adding the tests mentioned above before merging to maintain the repository's testing standards. Review generated with assistance from Claude Code |
Pull Request Review: feat(tools): allow passing apiKey via options for browser supportSummaryThis PR adds support for passing the API key via function options, enabling browser usage where ✅ Strengths
🔍 Code Quality Issues1. Type Definition InconsistencyThe Current: interface WrapVercelLanguageModelOptions {
conversationId?: string;
verbose?: boolean;
mode?: "profile" | "query" | "full";
addMemory?: "always" | "never";
apiKey?: string; // ✅ Added here
}
export { wrapVercelLanguageModel as withSupermemory, type WrapVercelLanguageModelOptions as WithSupermemoryOptions }This is actually correct - good job keeping them in sync! 2. Middleware Function Signature ComplexityThe Current: export const createSupermemoryMiddleware = (
containerTag: string,
conversationId?: string,
verbose = false,
mode: "profile" | "query" | "full" = "profile",
addMemory: "always" | "never" = "never",
apiKey?: string, // ✅ Added as 6th parameter
)Suggestion: Consider refactoring to use an options object in a future PR: export const createSupermemoryMiddleware = (
containerTag: string,
options?: {
conversationId?: string;
verbose?: boolean;
mode?: "profile" | "query" | "full";
addMemory?: "always" | "never";
apiKey?: string;
}
)This would make future additions easier and improve readability. Note: This is a suggestion for future work, not a blocker for this PR. 🧪 Test Coverage
|
Pull Request Review: feat(tools): allow passing apiKey via options for browser supportOverall AssessmentThis is a well-implemented feature that adds browser support by allowing the API key to be passed as an option. The changes are minimal, focused, and maintain backward compatibility. Code Quality & Best PracticesStrengths:
Potential Bugs/IssuesMinor concern: The createSupermemoryMiddleware function signature change could theoretically be a breaking change if anyone is calling this function directly. However, this is likely an internal function and the parameter is optional. Recommendation: Consider documenting in release notes that createSupermemoryMiddleware signature changed if it is part of public API. Security ConcernsGood practices:
Browser exposure consideration:
Performance ConsiderationsNo performance impact - simple parameter passing with negligible overhead Test CoverageCurrent state:
Recommended test additions:
Additional SuggestionsThe error message is excellent and very clear about both options. Consider adding more usage examples in the README showing both browser and server patterns. VerdictLGTM with minor suggestions - This PR is production-ready. Before merging (optional but recommended):
Great work @codewarnab! This is a clean, focused contribution that adds real value. |
MaheshtheDev
left a comment
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.
tweaked few changes and updated the documentations. Now it all looks goo
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.
Pull request overview
This PR aims to enable browser support for the withSupermemory middleware by allowing the Supermemory API key to be passed via an options.apiKey parameter instead of requiring process.env.SUPERMEMORY_API_KEY. The implementation adds an optional apiKey field to the options interface and updates the middleware to accept this parameter.
Key Changes:
- Added optional
apiKeyfield toWithSupermemoryOptionsinterface - Updated
createSupermemoryMiddlewareto acceptapiKeyas a parameter - Modified error message to guide users on both ways to provide the API key
- Updated README documentation to mention the new option
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/tools/src/vercel/index.ts | Added apiKey to options interface, updated error handling to check both options.apiKey and process.env, and passes API key to middleware |
| packages/tools/src/vercel/middleware.ts | Updated function signature to accept apiKey parameter and passes it to Supermemory client constructor |
| packages/tools/README.md | Updated documentation to mention the new apiKey option and its usage patterns |
Critical Issues Found:
-
Breaking API Change: The
createSupermemoryMiddlewarefunction signature has been completely changed from accepting aSupermemoryclient instance as the first parameter to acceptingcontainerTagandapiKeystrings. This breaks all existing tests and potentially external users of this exported function. -
Incomplete Implementation: The
addSystemPromptfunction (called within the middleware) still usesprocess.env.SUPERMEMORY_API_KEYdirectly in its fetch call to the Supermemory API. This means the browser support is incomplete - the code will still fail in browser environments even when users provide an API key viaoptions.apiKey. -
Test Failures: The error message was updated, which will cause the existing test expecting
"SUPERMEMORY_API_KEY is not set"to fail with the new longer error message.
Comments suppressed due to low confidence (1)
packages/tools/src/vercel/middleware.ts:76
- The function signature for
createSupermemoryMiddlewarehas been changed in a breaking way. The old signature accepted aSupermemoryclient instance as the first parameter, but now expectscontainerTag(string) andapiKey(string) as the first two parameters.
This breaks all existing tests in test/vercel.test.ts which call this function with the old signature like createSupermemoryMiddleware(mockSupermemory, TEST_CONFIG.containerTag). The tests will fail because they're passing a Supermemory client instance where a string is now expected.
Additionally, this is exported as a public API function, so this breaking change could affect external users of the library. Consider either:
- Updating all tests to match the new signature, or
- Maintaining backward compatibility by accepting both signatures (e.g., using function overloads or checking the type of the first parameter)
export const createSupermemoryMiddleware = (
containerTag: string,
apiKey: string,
conversationId?: string,
verbose = false,
mode: "profile" | "query" | "full" = "profile",
addMemory: "always" | "never" = "never",
): LanguageModelV2Middleware => {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!providedApiKey) { | ||
| throw new Error("SUPERMEMORY_API_KEY is not set — provide it via `options.apiKey` or set `process.env.SUPERMEMORY_API_KEY`") |
Copilot
AI
Dec 3, 2025
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.
The error message has been updated to be more descriptive, but this will break the existing test in test/vercel.test.ts at line 72 which expects the error message to be exactly "SUPERMEMORY_API_KEY is not set".
The test uses .toThrow("SUPERMEMORY_API_KEY is not set") which will fail because the new error message is "SUPERMEMORY_API_KEY is not set — provide it via \options.apiKey` or set `process.env.SUPERMEMORY_API_KEY`"`.
The test should be updated to either:
- Match the new error message, or
- Use a partial match like
.toThrow(/SUPERMEMORY_API_KEY is not set/)
| const client = new Supermemory({ | ||
| apiKey: process.env.SUPERMEMORY_API_KEY, | ||
| apiKey, | ||
| }) |
Copilot
AI
Dec 3, 2025
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.
The middleware now accepts apiKey as a parameter and passes it to the Supermemory client constructor (line 80), but this is insufficient for browser support. The addSystemPrompt function (called later in this middleware) still uses process.env.SUPERMEMORY_API_KEY directly in its fetch call to the Supermemory API (in memory-prompt.ts line 23).
This means that even when users provide an API key via options.apiKey, the code will still fail in browser environments when addSystemPrompt tries to access process.env.
The apiKey parameter needs to be passed through to addSystemPrompt and supermemoryProfileSearch functions as well to fully support browser environments.
| apiKey?: string | ||
| } | ||
| ``` | ||
|
|
Copilot
AI
Dec 3, 2025
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.
The apiKey option is documented in the JSDoc comment (line 398-399), but it would be helpful to also add a descriptive bullet point below this code block (similar to conversationId, verbose, mode, and addMemory) to document the fallback behavior and usage guidance.
Suggested addition after line 401:
- apiKey: Optional Supermemory API key. Use this in browser environments where environment variables are not available (falls back to
process.env.SUPERMEMORY_API_KEYif not provided)
| - **apiKey**: Optional Supermemory API key. Use this in browser environments where environment variables are not available (falls back to `process.env.SUPERMEMORY_API_KEY` if not provided) |
| - `withSupermemory` will take advantage supermemory profile v4 endpoint personalized based on container tag | ||
| - Make sure you have `SUPERMEMORY_API_KEY` in env | ||
| - You can provide the Supermemory API key via the `apiKey` option to `withSupermemory` (recommended for browser usage), or fall back to `SUPERMEMORY_API_KEY` in the environment for server usage. |
Copilot
AI
Dec 3, 2025
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.
[nitpick] The documentation mentions that the apiKey option is "recommended for browser usage" (line 60), but there's no code example demonstrating how to use it. Consider adding an example after the basic usage example to show browser usage:
// Browser usage - pass apiKey directly
const modelWithMemory = withSupermemory(openai("gpt-5"), "user_id_life", {
apiKey: "your-supermemory-api-key"
})This would make it clearer how users should implement the feature that this PR is introducing.

Problem
withSupermemory only read the API key from process.env.SUPERMEMORY_API_KEY, which does not work in browser environments.
Solution
Usage
Browser usage - pass apiKey directly:
const model = withSupermemory(openai('gpt-4'), 'user-123', { apiKey: 'your-key-here' });
Server usage - still works with env variable:
const model = withSupermemory(openai('gpt-4'), 'user-123');"