-
Notifications
You must be signed in to change notification settings - Fork 2
feat: add thread list and deletion API with new component #21
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
base: main
Are you sure you want to change the base?
Conversation
tylerslaton
commented
Oct 12, 2025
- switch docs, stories, and UI to the slot-based CopilotThreadList API
- surface delete controls with optimistic updates and error feedback
- extend useThreads with deleteThread/currentThreadId helpers plus tests
- prevent suggestion-generated runs from polluting thread history
- wire DELETE handler through runtime endpoints and runner backends
- harden CopilotChat reconnect flow and tool-call rendering keys
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
a0c2cf1 to
46c5821
Compare
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
ca7279b to
3c22a31
Compare
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
3c22a31 to
5733495
Compare
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
5733495 to
d797194
Compare
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
- switch docs, stories, and UI to the slot-based CopilotThreadList API - surface delete controls with optimistic updates and error feedback - extend useThreads with deleteThread/currentThreadId helpers plus tests - prevent suggestion-generated runs from polluting thread history - wire DELETE handler through runtime endpoints and runner backends - harden CopilotChat reconnect flow and tool-call rendering keys Signed-off-by: Tyler Slaton <[email protected]>
d797194 to
775b6a9
Compare
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
- Add undefined scope guards to thread handlers (list/get/delete) to return 401 when authentication fails. - Add scope validation and thread ownership check to handleStopAgent to prevent cross-user run termination.
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
mme
left a comment
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.
I stopped after 25 comments - needs more work to be merged safely
|
|
||
| ### 2. Server-Side: Validate and Enforce | ||
|
|
||
| Configure the `` in your runtime: |
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.
A word seems to be missing here
| } | ||
| ``` | ||
|
|
||
| ## Validation Helpers |
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.
Do we need to introduce four different helpers here?
If so, I think the whole concept of validation needs to be explained a little better, i.e. how to signal access is granted/denied (throwing exceptions?), what to return from resolveThreadsScope
|
|
||
| ## Security Best Practices | ||
|
|
||
| ### 1. Always Validate on the Server |
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.
As discussed, there are ways to skip server side validation and still be secure by generating unguessable tokens as resourceId.
| ```tsx | ||
| // Development - Warning in console | ||
| <CopilotKitProvider runtimeUrl="/api/copilotkit"> | ||
| {/* Console: "No resourceId set. All threads are globally accessible." */} |
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.
Without a resourceId, no list of threads should be accessible at all. Being able to list all the threads of all users must not be the default behavior.
| const user = await authenticate(request); | ||
|
|
||
| if (user.isAdmin) { | ||
| return null; // Admin bypass - sees all threads |
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.
It seems dangerous to have null mean "access to all threads" - is there an explicit way to signal that?
In general, I don't see having access to all the threads as a practical feature in almost any application.
|
|
||
| // Manually trigger subscribers since direct assignment doesn't notify them | ||
| // This ensures React components re-render with the empty messages | ||
| const subscribers = (agent as any).subscribers || []; |
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.
don't manually trigger agent subscribers
| try { | ||
| if (agent instanceof HttpAgent) { | ||
| agent.headers = { ...(this.core as unknown as CopilotKitCoreFriendsAccess).headers }; | ||
| agent.headers = this.core.getHeadersWithResourceId(); |
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.
This does not look right
| } | ||
| } | ||
|
|
||
| async disconnectAgent({ agent }: CopilotKitCoreDisconnectAgentParams): Promise<void> { |
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.
This looks like the coding agent added some weird hacks
| } | ||
| } | ||
|
|
||
| async function waitForAgentToStop(agent: AbstractAgent, timeoutMs = 2000): Promise<void> { |
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.
why do we need to do this?
| ...restProps | ||
| } = props; | ||
|
|
||
| useEffect(() => { |
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.
I have huge doubts about removing this piece (core logic)