-
Notifications
You must be signed in to change notification settings - Fork 1.5k
refactor sync session to use a queue for extension file save events #4211
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
Conversation
🦋 Changeset detectedLatest commit: dba3f32 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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.
Pull request overview
This PR refactors the SessionManager to use a queue-based approach for handling file save events, replacing the previous state-based implementation. The key improvement is better handling of asynchronous file save events from the extension through event queuing and reliance on taskId <-> sessionId mapping.
Key Changes:
- Replaced state-based file path tracking with an event queue that stores file update events with timestamps
- Removed the
destroy()method and associated cleanup calls, simplifying session lifecycle management - Consolidated session state management using per-task tracking (
taskGitUrls,taskGitHashes,sessionTitles)
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
SessionManager.ts |
Core refactor: replaced state-based tracking with queue-based event processing, changed setPath() to handleFileUpdate(), removed destroy() method, improved sync logic to process multiple tasks per sync cycle |
SessionClient.ts |
Moved KILO_PLATFORM environment variable override from manager to client's create() method; fixed URL construction bug using proper URL constructor |
session-manager-utils.ts |
Removed kilo_destroySessionManager() utility function as destroy() no longer exists |
ClineProvider.ts |
Updated to use handleFileUpdate() instead of setPath(); removed destroy() calls on task focus and disposal |
cli/src/state/atoms/effects.ts |
Updated message handlers to call handleFileUpdate() instead of setPath() |
cli/src/commands/new.ts |
Removed session cleanup logic using destroy() as it's no longer needed |
cli/src/cli.ts |
Removed destroy() call during CLI disposal |
| Test files | Split tests into two focused test suites: general SessionManager tests and specific syncSession tests with comprehensive race condition coverage |
cli/src/commands/__tests__/new.test.ts |
Updated to reflect removal of session destroy functionality |
.changeset/common-buckets-tickle.md |
Added changeset for patch release |
src/shared/kilocode/cli-sessions/core/__tests__/SessionManager.spec.ts
Outdated
Show resolved
Hide resolved
src/shared/kilocode/cli-sessions/core/__tests__/SessionManager.spec.ts
Outdated
Show resolved
Hide resolved
RSO
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.
It's hard to piece together all the different changes, but nothing stood out to me so ✅
GH diff view doesn't do it justice. |
pandemicsyn
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.
one question inline around whether we need to worry about losing stuff during tear down, but lgtm!
| try { | ||
| logs.info("Disposing Kilo Code CLI...", "CLI") | ||
|
|
||
| await this.sessionService?.destroy() |
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.
Not blocker, not even sure its really an issue, but do we need to delay exiting on SIGINT to give the background sync stuff a chance to trigger?
Wondering if we'd have stuff in the session manager queue that we're losing.
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 think we do, I'll add back the destroy method
this was caused by a previous race condition
useful if someone has their node_modules in the diff
Context
Improves
SessionManagerto better handle the asynchronicity of file save events from the extension.Implementation
Instead of maintaining state, preserve events in a queue and process on a timer. Rely on
taskId <-> sessionIdfor consistency.