-
Notifications
You must be signed in to change notification settings - Fork 2.7k
fix(memory-workspace): show all native Composio memory-sync sources (not Gmail-only) #2685
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
Changes from all commits
ec7a7f4
1d91728
f2bfaa9
38023b0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -54,11 +54,17 @@ interface MemoryWorkspaceProps { | |
| * adding chunks to the memory tree. | ||
| * | ||
| * Source of truth: providers under | ||
| * `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 | ||
| * 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([ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Refactor / suggestion (non-blocking) — static allowlist will drift from the Rust source of truth. This The core already exposes the authoritative data: 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 |
||
| 'clickup', | ||
| 'github', | ||
| 'gmail', | ||
| 'linear', | ||
| 'notion', | ||
| 'slack', | ||
| ]); | ||
|
|
||
| export function MemoryWorkspace({ onToast }: MemoryWorkspaceProps) { | ||
| const { t } = useT(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -235,24 +235,23 @@ describe('MemoryWorkspace (graph view)', () => { | |
| }); | ||
| }); | ||
|
|
||
| it('hides toolkits without a memory-tree ingest provider entirely', async () => { | ||
| it('shows sync rows for provider-backed toolkits and hides non-syncable ones', async () => { | ||
| listConnections.mockResolvedValue({ | ||
| connections: [ | ||
| { id: 'conn-gmail', toolkit: 'gmail', status: 'ACTIVE', accountEmail: 'a@x' }, | ||
| { id: 'conn-slack', toolkit: 'slack', status: 'ACTIVE', workspace: 'acme' }, | ||
| { id: 'conn-notion', toolkit: 'notion', status: 'ACTIVE' }, | ||
| { id: 'conn-discord', toolkit: 'discord', status: 'ACTIVE' }, | ||
| ], | ||
| }); | ||
| renderWithProviders(<MemoryWorkspace />); | ||
| // Gmail row exists with a working Sync button. | ||
| // Provider-backed toolkits should render actionable Sync rows | ||
| expect(await screen.findByTestId('memory-source-sync-gmail')).toBeInTheDocument(); | ||
| // Non-syncable toolkits are filtered out completely — neither | ||
| // the row nor the Sync button render. Cleaner than a "no sync | ||
| // yet" placeholder for an action the user can't take. | ||
| expect(screen.queryByTestId('memory-source-row-slack')).toBeNull(); | ||
| 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(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpick (test completeness). This test exercises 3 of the 6 newly-syncable toolkits (gmail/slack/notion) plus the hidden case (discord), but not 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 |
||
| expect(screen.getByTestId('memory-source-sync-notion')).toBeInTheDocument(); | ||
| // Non-syncable toolkits stay hidden. | ||
| expect(screen.queryByTestId('memory-source-row-discord')).toBeNull(); | ||
| expect(screen.queryByTestId('memory-source-sync-discord')).toBeNull(); | ||
| }); | ||
|
|
||
| it('toggling to Contacts mode re-fetches the graph with mode=contacts', async () => { | ||
|
|
||
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 (doc accuracy). This says the source-of-truth providers "persist items via
store_skill_syncinto the memory tree." That's true forclickup/github/linear/notion(they go throughpersist_single_item→store_skill_syncinmemory_sync/composio/providers/sync_state.rs:306), butgmailandslackingest viaingest_page_into_memory_tree(gmail/ingest.rs,slack/ingest.rs), notstore_skill_sync. Consider generalizing, e.g. "…that ingest items into the memory tree (viastore_skill_syncor a provider-specific ingest path)."