Connect knowledge bases from Nova chat - #1390
Conversation
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
supermemory-mcp | 1b4a85d | Aug 01 2026, 03:19 PM |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
supermemory-app | 1b4a85d | Commit Preview URL Branch Preview URL |
Aug 01 2026, 03:20 PM |
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. This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Claude finished @ishaanxgupta's task in 3m 21s —— View job Code Review Complete
Summary: Reviewed ~750 lines adding knowledge base connection cards to Nova chat. No bugs, security issues, or logic errors found. The implementation handles OAuth flows, popup management, and live sync status polling with proper security measures and cleanup. Score: 9/10 — Good to ship. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview: Adds knowledge base connection cards (Notion, Google Drive, OneDrive, Granola) to Nova chat with OAuth flow integration and live sync status polling.
Issues found: None — this looks good to ship.
The implementation is well-designed:
- Security: Uses
safeExternalUrlto validate auth links, setspopup.opener = nullto prevent reverse tabnabbing, and usestextContent(not innerHTML) for popup content - UX: Pre-opens popup on message submit to avoid popup blockers, monitors popup closure with proper cleanup timers
- State management: Uses refs appropriately to prevent stale closures, handles connection completion via polling with appropriate intervals
- Error handling: Cleans up reserved windows on errors, proper timeout handling (2-min component timeout + 5-min window cleanup)
Score: 9/10
|
Note Production impact unlikely. Checked the knowledge-base connector UI against supermemory-app (~6k req/hr, 0 errors); all new code is additive client-side React calling existing Polylane analysed |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| throw new Error( | ||
| response.error.message || "Failed to start connection", | ||
| ) |
There was a problem hiding this comment.
Violation of the 'Exception handling' rule: supporting data should be passed as the cause argument instead of being inlined into the error message string. The expression response.error.message || "Failed to start connection" inlines the error detail into the message string. Instead, throw new Error("Failed to start connection", { cause: response.error }) to pass the supporting data as the cause.
| throw new Error( | |
| response.error.message || "Failed to start connection", | |
| ) | |
| throw new Error( | |
| "Failed to start connection", | |
| { cause: response.error }, | |
| ) | |
Spotted by Graphite (based on custom rule: TypeScript style guide (Google))
Is this helpful? React 👍 or 👎 to let us know.
| } catch (cause) { | ||
| stopWaiting( | ||
| cause instanceof Error ? cause.message : "Failed to connect", | ||
| ) |
There was a problem hiding this comment.
Violation of the 'Exception handling' rule: supporting data should be passed as the cause argument instead of being inlined into the error message string. In the catch block, cause instanceof Error ? cause.message : "Failed to connect" inlines the original error's message into the new error string. Instead, use new Error("Failed to connect", { cause }) to preserve the original error as the cause.
| } catch (cause) { | |
| stopWaiting( | |
| cause instanceof Error ? cause.message : "Failed to connect", | |
| ) | |
| } catch (cause) { | |
| stopWaiting( | |
| new Error("Failed to connect", { cause }), | |
| ) | |
Spotted by Graphite (based on custom rule: TypeScript style guide (Google))
Is this helpful? React 👍 or 👎 to let us know.
| body: { containerTags: [] }, | ||
| }) | ||
| if (response.error) { | ||
| throw new Error(response.error.message || "Failed to load connections") |
There was a problem hiding this comment.
Violation of the 'Exception handling' rule: supporting data should be passed as the cause argument instead of being inlined into the error message string. The expression response.error.message || "Failed to load connections" inlines the error detail into the message string. Instead, throw new Error("Failed to load connections", { cause: response.error }) to pass the supporting data as the cause.
| throw new Error(response.error.message || "Failed to load connections") | |
| throw new Error("Failed to load connections", { cause: response.error }) | |
Spotted by Graphite (based on custom rule: TypeScript style guide (Google))
Is this helpful? React 👍 or 👎 to let us know.
1 participant
Summary