Feat: Adding file through Notion and onedrive #401

StrongMonkey · 2024-09-03T17:58:43Z

This PR adds ability to add files through notion and onedrive when using knowledge.

It mainly use https://github.com/gptscript-ai/knowledge-notion-integration and https://github.com/gptscript-ai/knowledge-onedrive-integration to sync down files from notion/onedrive to local disk and keep those up-to-sync. The sync will be run every 24 hours or can be triggered manually.

#323

components/edit/configure.tsx

actions/knowledge/filehelper.ts

components/knowledge/FileModal.tsx

components/knowledge/Notion.tsx

contexts/edit.tsx

tylerslaton · 2024-09-05T20:45:46Z

Does this implementation make it so my local machine is a mirror of whatever account I've selected for Notion?

tylerslaton

Just ran through this flow and had some thoughts about it but overall this looks good and functions really well! Most of this is just UX questions.

Could/should we add an option to settings where I can manage all of this instead of having to go to the edit page
It may make sense to add this to the "Add Knowledge" section of a chat interaction. However, that currently doesn't have parity with this either so I'll leave that to your and @cjellick's discretion.
When I open choose to add a file through notion and then login I come back and eventually have to push the "sync" button after a successful login. Can we instead make it so that it immediately performs a "first time" sync automatically?
Why did we go with syncing the files down locally? Is there anyway that we could do a "just in time" sync where Notion is the source of truth and I pull from it whenever I actually add a file?

components/knowledge/Notion.tsx

StrongMonkey · 2024-09-06T05:59:40Z

@tylerslaton

When I open choose to add a file through notion and then login I come back and eventually have to push the "sync" button after a successful login. Can we instead make it so that it immediately performs a "first time" sync automatically?

It should perform the sync at the first time. I will look at it if it doesn't perform this way.

Why did we go with syncing the files down locally? Is there anyway that we could do a "just in time" sync where Notion is the source of truth and I pull from it whenever I actually add a file?

Yeah, I kind of went back and forth on this. With onedrive I am going to change this so we only sync down files when you add it into assistant, because in onedrive you can't select the file/folders when you do oauth. For notion it is less of an issue because you can select the pages when doing oauth and that will limit the files it syncs initially. The benefit of syncing down all the files is that later when you add files through assistants, it is relatively fast but yes the initial sync will be slow.

StrongMonkey · 2024-09-06T06:00:14Z

Does this implementation make it so my local machine is a mirror of whatever account I've selected for Notion?

Yes, files that you select through notion will eventually match files in your local laptop.

StrongMonkey · 2024-09-06T08:46:22Z

I think I am going to just change the import logic to sync on demand for now. This means that we will only start syncing when the file has been used by assistants.

StrongMonkey · 2024-09-09T07:48:59Z

Could/should we add an option to settings where I can manage all of this instead of having to go to the edit page
It may make sense to add this to the "Add Knowledge" section of a chat interaction. However, that currently doesn't have parity with this either so I'll leave that to your and @cjellick's discretion.

Good point, i will fix Add knowledge to bring parity. For setting page I am not sure, I leans towards not adding it in setting page for now for simplicity of the feature but open to opinions for @cjellick.

StrongMonkey · 2024-09-09T22:30:37Z

@tylerslaton Most of your comments shoud be addressed.

Could/should we add an option to settings where I can manage all of this instead of having to go to the edit page

I think we are just going to punt on this until we have more UX feedback from craig and will

It may make sense to add this to the "Add Knowledge" section of a chat interaction. However, that currently doesn't have parity with this either so I'll leave that to your and @cjellick's discretion.

Per standup we are not going to do this

When I open choose to add a file through notion and then login I come back and eventually have to push the "sync" button after a successful login. Can we instead make it so that it immediately performs a "first time" sync automatically?

This should run the sync for first time, if it is still happening to you we can sync, doesn't look like the case for me

Why did we go with syncing the files down locally? Is there anyway that we could do a "just in time" sync where Notion is the source of truth and I pull from it whenever I actually add a file?

So I changed this to be synced on demand. The initial sync will only fetch metadata.

tylerslaton

Things are looking good testing locally. Going to do a second pass on the code in the morning and likely approve around then. Thanks for all the addressing of my feedback I really appreciate it. The feature is looking awesome, I love how it all flows.

actions/knowledge/filehelper.ts

tylerslaton

No more blockers for me, just minor nits. Lots of code here so I'd like more eyes on this.

tylerslaton · 2024-09-10T14:45:14Z

actions/knowledge/filehelper.ts

+
+  for (const file of files) {
+    // check if filepath lives in notion or onedrive integration folders
+    const baseDir = path.dirname(path.dirname(file));


Why are we calling path.dirname twice? What is the path we're retrieving? Whatever the answer, it could be a good comment.

server/app.mjs

actions/knowledge/util.ts

actions/knowledge/notion.ts

actions/knowledge/util.ts

ryanhopperlowe · 2024-09-10T15:21:25Z

contexts/edit.tsx

+  droppedFiles: Map<string, FileDetail>;
+  setDroppedFiles: React.Dispatch<


nit: one thing I'd like to start enforcing is that we don't expose any setState methods anymore. It creates a brittle API and it's difficult to track down where updates are happening.

It'd be better to expose a few pieces of larger (computed) state along with event triggers that have a dedicated purpose.

i.e.

type ContextState = { droppedFileMap: Record<string, FileDetail>; removeDroppedFile: (key: string) => void; dropFile: (key: string, file: FileDetail) => void; // ... etc }

This is a key part of a larger effort I'd like to start working on to reduce some complexity in the codebase

Still seeing this issue. Can you replace the setDroppedFiles method something like dropFiles: (files: File[]) => void and keep the data-conversion logic inside of the context provider

@ryanhopperlowe I DRYed the code bit, but it is not possible to move all the data convertions because setDroppedFiles is used in multiple places where update and delete happens with different implementation.

actions/knowledge/tool.ts

cjellick · 2024-09-10T16:25:28Z

These are my notes on the feature thus far...its raw and unprocessed and i dont have solutins, just wanted to get it out there

the plus’s functionality is not obvious
the feedback while its pulling the metadata is not good
the paths could be better. maybe by just making them relative?
once i added the files, it was all my files plus the ones in that dir. that was bad
1. this is the probably the worst thing.
2. it also should probably have multi-select via range.
3. i think once i click add, it should be a page dedicated to just those items. or maybe just sorting better would be good enough. maybe a "clear" button to get rid of everything in th emodal
check is very sluggish. dont like the animation
this probably doesnt cut it for future changes to the directory (to sync a diff of files)
error messaging and handling is terrible. outputs all the logs as errors

StrongMonkey · 2024-09-10T21:56:35Z

@tylerslaton @ryanhopperlowe should have addressed all your comments.

ryanhopperlowe · 2024-09-11T14:22:41Z

Everything looks good, except I think you missed 1 thing. Once that's fixed I'll shoot the approval :)

tylerslaton

My comments have all been addressed. Thanks!

tylerslaton · 2024-09-13T15:54:11Z

Oh and definitely be sure to fix the merge conflicts 👍🏻

ryanhopperlowe · 2024-09-13T16:05:32Z

components/knowledge/OneDrive.tsx

+  const clear = async () => {
+    setIsClearing(true);
+    try {
+      await clearOneDriveFiles();
+      syncOnedrive();
+    } catch (e) {
+      setSyncError((e as Error).toString());
+    } finally {
+      setIsClearing(false);
+    }
+  };
+


nit: you can get most of this logic out of the box with the useAsync hook that I created

Signed-off-by: Daishan Peng <[email protected]>

StrongMonkey force-pushed the notion branch from 48cb300 to 771d602 Compare September 3, 2024 19:19

ryanhopperlowe reviewed Sep 3, 2024

View reviewed changes

StrongMonkey force-pushed the notion branch from d987508 to 8fab6fe Compare September 3, 2024 22:55

ryanhopperlowe previously approved these changes Sep 4, 2024

View reviewed changes

StrongMonkey requested review from thedadams, njhale, g-linville and tylerslaton September 5, 2024 16:44

StrongMonkey force-pushed the notion branch from 1cb0a91 to c3edb5b Compare September 5, 2024 19:08

tylerslaton suggested changes Sep 5, 2024

View reviewed changes

components/knowledge/Notion.tsx Outdated Show resolved Hide resolved

components/knowledge/Notion.tsx Outdated Show resolved Hide resolved

components/knowledge/Notion.tsx Outdated Show resolved Hide resolved

StrongMonkey changed the title ~~Feat: Adding file through Notion~~ Feat: Adding file through Notion and onedrive Sep 6, 2024

StrongMonkey dismissed ryanhopperlowe’s stale review via 0fec3f8 September 6, 2024 13:31

StrongMonkey force-pushed the notion branch from 0fec3f8 to faa1750 Compare September 9, 2024 07:46

StrongMonkey mentioned this pull request Sep 9, 2024

Knowledge - ingest from external sources #323

Open

StrongMonkey requested a review from tylerslaton September 9, 2024 22:27

StrongMonkey requested a review from ryanhopperlowe September 9, 2024 22:30

StrongMonkey force-pushed the notion branch from 4227375 to 7b01175 Compare September 10, 2024 00:11

tylerslaton reviewed Sep 10, 2024

View reviewed changes

ryanhopperlowe reviewed Sep 10, 2024

View reviewed changes

actions/knowledge/filehelper.ts Outdated Show resolved Hide resolved

tylerslaton previously approved these changes Sep 10, 2024

View reviewed changes

ryanhopperlowe reviewed Sep 10, 2024

View reviewed changes

actions/knowledge/util.ts Outdated Show resolved Hide resolved

ryanhopperlowe requested changes Sep 10, 2024

View reviewed changes

StrongMonkey dismissed tylerslaton’s stale review via 15b269f September 10, 2024 21:54

StrongMonkey force-pushed the notion branch from d033c59 to 15b269f Compare September 10, 2024 21:54

StrongMonkey requested review from tylerslaton and ryanhopperlowe September 10, 2024 21:56

ryanhopperlowe previously approved these changes Sep 11, 2024

View reviewed changes

StrongMonkey dismissed ryanhopperlowe’s stale review via 3510da8 September 13, 2024 01:14

tylerslaton previously approved these changes Sep 13, 2024

View reviewed changes

ryanhopperlowe previously approved these changes Sep 13, 2024

View reviewed changes

StrongMonkey added 11 commits September 13, 2024 09:22

Feat: Adding file through Notion

6d769e8

Signed-off-by: Daishan Peng <[email protected]>

Address comments

287bcfa

Signed-off-by: Daishan Peng <[email protected]>

Use external link

7e8f366

Signed-off-by: Daishan Peng <[email protected]>

Add onedrive integration

d124712

Signed-off-by: Daishan Peng <[email protected]>

Address comments

d60a877

Signed-off-by: Daishan Peng <[email protected]>

Set sync error when resyncing

69950f1

Signed-off-by: Daishan Peng <[email protected]>

Address comments

8afa300

Signed-off-by: Daishan Peng <[email protected]>

Update nextui package

1afc05a

Signed-off-by: Daishan Peng <[email protected]>

Sort table based on name, fix select with shift

5b0f5c4

Signed-off-by: Daishan Peng <[email protected]>

Address ryan's comment

24915cf

Signed-off-by: Daishan Peng <[email protected]>

Addressing craig's UX feedback

5254c8f

Signed-off-by: Daishan Peng <[email protected]>

StrongMonkey dismissed stale reviews from ryanhopperlowe and tylerslaton via 9daf2ae September 13, 2024 16:25

StrongMonkey force-pushed the notion branch from 3510da8 to 9daf2ae Compare September 13, 2024 16:25

Rebase

813a86b

Signed-off-by: Daishan Peng <[email protected]>

StrongMonkey force-pushed the notion branch from 9daf2ae to 813a86b Compare September 13, 2024 16:29

ryanhopperlowe approved these changes Sep 13, 2024

View reviewed changes

tylerslaton approved these changes Sep 13, 2024

View reviewed changes

StrongMonkey merged commit 57f25ab into gptscript-ai:main Sep 13, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: Adding file through Notion and onedrive #401

Feat: Adding file through Notion and onedrive #401

StrongMonkey commented Sep 3, 2024 •

edited

Loading

tylerslaton commented Sep 5, 2024

tylerslaton left a comment •

edited

Loading

StrongMonkey commented Sep 6, 2024 •

edited

Loading

StrongMonkey commented Sep 6, 2024

StrongMonkey commented Sep 6, 2024

StrongMonkey commented Sep 9, 2024

StrongMonkey commented Sep 9, 2024

tylerslaton left a comment

tylerslaton left a comment •

edited

Loading

tylerslaton Sep 10, 2024

ryanhopperlowe Sep 10, 2024 •

edited

Loading

ryanhopperlowe Sep 10, 2024

ryanhopperlowe Sep 11, 2024

StrongMonkey Sep 11, 2024

cjellick commented Sep 10, 2024

StrongMonkey commented Sep 10, 2024

ryanhopperlowe commented Sep 11, 2024

tylerslaton left a comment

tylerslaton commented Sep 13, 2024

ryanhopperlowe Sep 13, 2024

		droppedFiles: Map<string, FileDetail>;
		setDroppedFiles: React.Dispatch<

Feat: Adding file through Notion and onedrive #401

Feat: Adding file through Notion and onedrive #401

Conversation

StrongMonkey commented Sep 3, 2024 • edited Loading

tylerslaton commented Sep 5, 2024

tylerslaton left a comment • edited Loading

Choose a reason for hiding this comment

StrongMonkey commented Sep 6, 2024 • edited Loading

StrongMonkey commented Sep 6, 2024

StrongMonkey commented Sep 6, 2024

StrongMonkey commented Sep 9, 2024

StrongMonkey commented Sep 9, 2024

tylerslaton left a comment

Choose a reason for hiding this comment

tylerslaton left a comment • edited Loading

Choose a reason for hiding this comment

tylerslaton Sep 10, 2024

Choose a reason for hiding this comment

ryanhopperlowe Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

ryanhopperlowe Sep 10, 2024

Choose a reason for hiding this comment

ryanhopperlowe Sep 11, 2024

Choose a reason for hiding this comment

StrongMonkey Sep 11, 2024

Choose a reason for hiding this comment

cjellick commented Sep 10, 2024

StrongMonkey commented Sep 10, 2024

ryanhopperlowe commented Sep 11, 2024

tylerslaton left a comment

Choose a reason for hiding this comment

tylerslaton commented Sep 13, 2024

ryanhopperlowe Sep 13, 2024

Choose a reason for hiding this comment

StrongMonkey commented Sep 3, 2024 •

edited

Loading

tylerslaton left a comment •

edited

Loading

StrongMonkey commented Sep 6, 2024 •

edited

Loading

tylerslaton left a comment •

edited

Loading

ryanhopperlowe Sep 10, 2024 •

edited

Loading