fix(memory-workspace): show all native Composio memory-sync sources (not Gmail-only)#2685
Conversation
…e additional syncable toolkits
…ked toolkits and hide non-syncable ones
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR updates the Memory Sync allowlist to include multiple providers (clickup, github, gmail, linear, notion, slack), adjusts tests to expect sync rows for provider-backed toolkits and hide non-syncable toolkits, and rewords documentation comments in TypeScript and Rust to reference a parent-provided syncable allowlist. ChangesMemory Sync Multi-Provider Support
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/components/intelligence/__tests__/MemoryWorkspace.test.tsx (1)
238-254: ⚡ Quick winExpand this assertion to cover all newly allowlisted toolkits.
Line 238’s test intent is good, but it currently asserts only 3 of 6 syncable toolkits. Add
clickup,github, andlinearrows here (or parameterize over the allowlist) so future drift is caught.🤖 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 `@app/src/components/intelligence/__tests__/MemoryWorkspace.test.tsx` around lines 238 - 254, The test case "shows sync rows for provider-backed toolkits and hides non-syncable ones" in MemoryWorkspace.test.tsx currently asserts only gmail, slack, and notion; update it to also assert presence of the newly allowlisted syncable toolkits by adding checks for the Sync rows 'memory-source-sync-clickup', 'memory-source-sync-github', and 'memory-source-sync-linear' (or parameterize the expectations over the allowlist used by MemoryWorkspace) so all allowlisted provider-backed toolkits are verified to render actionable sync rows.
🤖 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.
Nitpick comments:
In `@app/src/components/intelligence/__tests__/MemoryWorkspace.test.tsx`:
- Around line 238-254: The test case "shows sync rows for provider-backed
toolkits and hides non-syncable ones" in MemoryWorkspace.test.tsx currently
asserts only gmail, slack, and notion; update it to also assert presence of the
newly allowlisted syncable toolkits by adding checks for the Sync rows
'memory-source-sync-clickup', 'memory-source-sync-github', and
'memory-source-sync-linear' (or parameterize the expectations over the allowlist
used by MemoryWorkspace) so all allowlisted provider-backed toolkits are
verified to render actionable sync rows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a053f2d1-b052-4f37-a5b5-aa43ea10b60c
📒 Files selected for processing (4)
app/src/components/intelligence/MemorySources.tsxapp/src/components/intelligence/MemoryWorkspace.tsxapp/src/components/intelligence/__tests__/MemoryWorkspace.test.tsxsrc/openhuman/memory_sync/composio/providers/mod.rs
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Walkthrough: Expands the Memory Workspace sync allowlist from gmail-only to all six native provider-backed toolkits (clickup, github, gmail, linear, notion, slack). Verified each has a corresponding provider directory under src/openhuman/memory_sync/composio/providers/. Tests correctly assert provider-backed toolkits render sync rows while non-syncable toolkits (discord) stay hidden. Stale comments cleaned up in both frontend and Rust.
| File | Change |
|---|---|
MemoryWorkspace.tsx |
Allowlist expanded from ['gmail'] → 6 toolkits; comment updated to reflect current provider path |
MemorySources.tsx |
Comment fix: "passed by the parent" instead of "today: gmail" |
MemoryWorkspace.test.tsx |
Test now asserts slack/notion visible + discord hidden (was: slack/notion hidden) |
providers/mod.rs |
Removed stale comment claiming github has "no sync logic yet" |
Clean, focused, well-tested. Nice fix — this was a real UX gap for users who connected non-Gmail providers and saw nothing in Memory Workspace.
One thought for a follow-up: the static allowlist will drift again as new providers land. A future PR could source this dynamically from capability_matrix() — but that's not blocking.
YellowSnnowmann
dismissed stale reviews from graycyrus and coderabbitai[bot]
via
38023b0
| * persist items via `store_skill_sync` into the memory tree. | ||
| */ | ||
| const SYNCABLE_TOOLKITS: ReadonlySet<string> = new Set(['gmail']); | ||
| const SYNCABLE_TOOLKITS: ReadonlySet<string> = new Set([ |
There was a problem hiding this comment.
💡 Refactor / suggestion (non-blocking) — static allowlist will drift from the Rust source of truth.
This SYNCABLE_TOOLKITS set is a hand-maintained mirror of the backend has_native_provider list in src/openhuman/memory_sync/composio/providers/mod.rs:110. The two have already drifted before — clickup (#2288), linear (#2400), and github became native providers in Rust while the UI stayed Gmail-only, which is the very bug this PR fixes. Nothing stops it recurring, and no test ties this TS set to the Rust capability matrix.
The core already exposes the authoritative data: composio.list_capabilities (openhuman.composio_list_capabilities) returns ComposioCapability[] with a per-toolkit memory_ingest flag derived from has_native_provider. Deriving the set from capabilities.filter(c => c.memory_ingest) would keep the Rust core as the single source of truth (consistent with the repo's "core is authoritative; frontend presents only" rule).
Not a blocker — the static list is correct today and the PR description flags this as a known tradeoff. If kept static, at least add a one-line comment pointing at has_native_provider as the thing to keep in sync.
| * `src/openhuman/composio/providers/<toolkit>/` that call | ||
| * `ingest_page_into_memory_tree`. Today that's gmail. Add a slug here | ||
| * when a new provider lands a memory-tree ingest path. | ||
| * `src/openhuman/memory_sync/composio/providers/<toolkit>/` that |
There was a problem hiding this comment.
Nitpick (doc accuracy). This says the source-of-truth providers "persist items via store_skill_sync into the memory tree." That's true for clickup/github/linear/notion (they go through persist_single_item → store_skill_sync in memory_sync/composio/providers/sync_state.rs:306), but gmail and slack ingest via ingest_page_into_memory_tree (gmail/ingest.rs, slack/ingest.rs), not store_skill_sync. Consider generalizing, e.g. "…that ingest items into the memory tree (via store_skill_sync or a provider-specific ingest path)."
| expect(screen.queryByTestId('memory-source-row-notion')).toBeNull(); | ||
| expect(screen.queryByTestId('memory-source-sync-slack')).toBeNull(); | ||
| expect(screen.queryByTestId('memory-source-sync-notion')).toBeNull(); | ||
| expect(screen.getByTestId('memory-source-sync-slack')).toBeInTheDocument(); |
There was a problem hiding this comment.
Nitpick (test completeness). This test exercises 3 of the 6 newly-syncable toolkits (gmail/slack/notion) plus the hidden case (discord), but not clickup/github/linear. The filter is toolkit-agnostic (syncableToolkits.has(conn.toolkit)), so risk is low — but asserting all six would lock the PR's stated intent ("show all native sources") against future allowlist edits.
for (const tk of ['clickup', 'github', 'gmail', 'linear', 'notion', 'slack']) {
expect(screen.getByTestId(`memory-source-sync-${tk}`)).toBeInTheDocument();
}(with the corresponding connections added to the listConnections mock).
sanil-23
left a comment
sanil-23
left a comment
There was a problem hiding this comment.
PR #2685 — fix(memory-workspace): show all native Composio memory-sync sources (not Gmail-only)
Walkthrough
This PR fixes a user-visible mismatch where Memory Workspace only surfaced a Gmail Sync row even though the Rust core ships native memory-sync providers for several more toolkits. It expands the frontend SYNCABLE_TOOLKITS allowlist from {gmail} to {clickup, github, gmail, linear, notion, slack}, refreshes stale doc comments in MemoryWorkspace.tsx / MemorySources.tsx and memory_sync/composio/providers/mod.rs, and updates the Vitest to assert provider-backed rows render while non-syncable toolkits stay hidden.
I verified the central correctness claim: the new allowlist is exactly the backend has_native_provider set in src/openhuman/memory_sync/composio/providers/mod.rs:110 (gmail | notion | slack | clickup | github | linear), and each provider genuinely ingests into the memory tree — clickup/github/linear/notion via persist_single_item then store_skill_sync, and gmail/slack via ingest_page_into_memory_tree. The change is correct, low-risk, and well-tested. No blockers. The only substantive note is a maintainability one (a static allowlist duplicating a Rust source of truth that can silently drift) — which the PR description already acknowledges as a deliberate tradeoff.
Changes
| File | Summary |
|---|---|
app/src/components/intelligence/MemoryWorkspace.tsx |
Expands SYNCABLE_TOOLKITS from {gmail} to 6 native-provider toolkits; updates source-of-truth comment to the new memory_sync/composio/providers/ path. |
app/src/components/intelligence/MemorySources.tsx |
Doc-comment-only: gate now described as the syncable allow-list passed by the parent instead of today: gmail. |
app/src/components/intelligence/__tests__/MemoryWorkspace.test.tsx |
Rewrites the visibility test: gmail/slack/notion render Sync rows; discord stays hidden. |
src/openhuman/memory_sync/composio/providers/mod.rs |
Doc-comment-only: drops the stale github-has-no-sync-logic-yet aside. |
Actionable comments (1)
Refactor / suggestion
1. app/src/components/intelligence/MemoryWorkspace.tsx:60-67 — Static allowlist duplicates the Rust source of truth and will silently drift.
SYNCABLE_TOOLKITS is a hand-maintained mirror of backend has_native_provider (providers/mod.rs:110). The two have already drifted — clickup (issue 2288), linear (issue 2400), and github became native in Rust while the UI stayed Gmail-only (the bug this PR fixes). Nothing prevents recurrence, and no test ties the TS set to the Rust matrix. The core already exposes the authoritative data via composio.list_capabilities (ComposioCapability[] with a per-toolkit memory_ingest flag); deriving the set from capabilities.filter(c => c.memory_ingest) would keep Rust as the single source of truth. Non-blocking — the static list is correct today and the PR flags this as a known follow-up. If kept static, add a one-line comment pointing at has_native_provider as the thing to keep in sync.
Nitpicks (2)
MemoryWorkspace.tsx:56-58— Comment says providers persist items viastore_skill_sync; that is true for clickup/github/linear/notion, but gmail/slack ingest viaingest_page_into_memory_tree. Consider: ingest items into the memory tree (viastore_skill_syncor a provider-specific ingest path).MemoryWorkspace.test.tsx:238-255— Test covers 3 of 6 new toolkits (gmail/slack/notion) + the hidden case (discord), not clickup/github/linear. Looping over all six would lock the show-all-native-sources intent against future edits.
Verified / looks good
- Allowlist matches backend
has_native_providerexactly. - All six providers genuinely write into the memory tree, so every Sync button maps to a real ingest path.
discordcorrectly excluded (native_provider = false) and asserted hidden.MemorySources.tsxandproviders/mod.rscomment updates are accurate (github is now a registered native provider).- Diff-coverage: the only executable change (the set) is exercised by the updated test.
Note: this PR was already merged (2026-05-27) before this review ran — posting as a post-hoc CodeRabbit-style review. The single actionable item is a non-blocking follow-up.
3 participants
Summary
Problem
Solution
Submission Checklist
.github/workflows/coverage.yml. Run pnpm test:coverage and pnpm test:rust locally; PRs below 80% on changed lines will not merge.docs/TEST-COVERAGE-MATRIX.mddocs/RELEASE-MANUAL-SMOKE.mdupdatesImpact
Related
Summary by CodeRabbit
New Features
Documentation
Tests