Skip to content
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

Fix: fix delete knowledge source hanging in UI #762

Merged
merged 2 commits into from
Dec 4, 2024

Conversation

StrongMonkey
Copy link
Contributor

@StrongMonkey StrongMonkey commented Dec 4, 2024

Don't list knowledge source with deleted as it could be waiting to be cleaned by finalizer. Also validate in cache after deletion.

#746

@@ -107,7 +107,10 @@ export default function AgentKnowledgePanel({

const getKnowledgeSources = useSWR(
KnowledgeService.getKnowledgeSourcesForAgent.key(agentId),
({ agentId }) => KnowledgeService.getKnowledgeSourcesForAgent(agentId),
({ agentId }) =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when performing mutations on swr fetchers, please pull out the mutation into it's own useMemo.

If I were to try to use this key again with the generic fetcher method, my code would break because the data from the response would not align with the data in the cache

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

example

// wrong

const { data }  = useSWR(
    Service.method.key(),
    () => Service.method().then(() => someMutation()) // cache has mutated data
)

// in another file

const { data }  = useSWR(
    Service.method.key(),
    () => Service.method() // cache has data directly from response
)

// an error may be thrown because the data in the cache does not match the structure of the data from the response

instead do this:

const { data }  = useSWR(
    Service.method.key(),
    () => Service.method() // use data directly from response
)

const mutatedData = useMemo(() => someMutation(data), [data])

// in another file

const { data }  = useSWR(
    Service.method.key(),
    () => Service.method() // also use data directly from response
)


No error now, because the data in the cache will ALWAYS have the same structure

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to hop on a huddle and explain in more depth if necessary :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to know! changed 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's ugly, but it's important... If I ever get the time to replace SWR with react-query, this will be super trivial

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

@ryanhopperlowe ryanhopperlowe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for fixing that!

@StrongMonkey StrongMonkey merged commit 787f4cc into obot-platform:main Dec 4, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants