-
Notifications
You must be signed in to change notification settings - Fork 0
feat: Add session.created event type #2
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: dev
Are you sure you want to change the base?
Conversation
- Add new Event.Created definition in session module - Modify session creation to publish both session.created and session.updated events - Update SDK types to include EventSessionCreated - Add test for session.created event functionality - Update plugin documentation with examples and comprehensive event list
WalkthroughThe changes introduce a new "session.created" event to the Session event system. When a session is created via Changes
Sequence DiagramsequenceDiagram
participant Caller
participant Session
participant Bus
Caller->>Session: createNext()
Session->>Session: Create session
Note over Session: New: Event emission
Session->>Bus: publish(Event.Created, {info})
Session->>Bus: publish(Event.Updated, ...)
Bus-->>Caller: Session created with events
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes The changes follow established patterns with straightforward additions: a new event definition, a single event publication in an existing function, a focused test, and documentation updates. While spanning multiple files, each change is homogeneous and self-contained with minimal complexity. Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/web/src/content/docs/plugins.mdx (1)
94-113: Consider clarifying the relationship between session.created and session.updated.The event list is comprehensive and accurate. However, since both
session.createdandsession.updatedare fired when a session is created (as implemented inpackages/opencode/src/session/index.tslines 181-186), consider adding a note explaining this behavior to help plugin developers avoid duplicate handling.Example addition:
OpenCode supports many event types that you can listen to: - `session.created` - Fired when a new session is created - `session.updated` - Fired when a session is updated + +**Note:** When a session is created, both `session.created` and `session.updated` events are published. If you need to respond specifically to session creation, use `session.created`. The `session.updated` event also fires for subsequent updates.packages/opencode/test/session/event.test.ts (1)
6-33: Add cleanup and verify both events are published.The test correctly verifies that
session.createdis published, but has two gaps:
- Missing cleanup: The created session is not removed, which could cause test pollution.
- Incomplete verification: Since
createNextpublishes bothEvent.CreatedandEvent.Updated(lines 181-186 inpackages/opencode/src/session/index.ts), the test should verify both events are published to ensure the dual-publish behavior is maintained.Apply this diff to add cleanup and verify both events:
describe("Session Events", () => { test("should publish session.created event when session is created", async () => { // Spy on the Bus.publish method to verify it's called const publishSpy = spyOn(Bus, "publish").mockResolvedValue(undefined) // Create a new session const session = await Session.createNext({ directory: "/tmp/test", title: "Test Session" }) // Verify that Bus.publish was called expect(publishSpy).toHaveBeenCalled() - // Verify that the session.created event was published + // Verify that both session.created and session.updated events were published let createdEventPublished = false + let updatedEventPublished = false for (const call of publishSpy.mock.calls) { const [eventDef, payload] = call if (eventDef.type === "session.created" && payload.info.id === session.id) { createdEventPublished = true - break } + if (eventDef.type === "session.updated" && payload.info.id === session.id) { + updatedEventPublished = true + } } expect(createdEventPublished).toBe(true) + expect(updatedEventPublished).toBe(true) // Clean up the spy publishSpy.mockRestore() + + // Clean up the created session + await Session.remove(session.id) }) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/sdk/js/src/gen/types.gen.tsis excluded by!**/gen/**
📒 Files selected for processing (3)
packages/opencode/src/session/index.ts(2 hunks)packages/opencode/test/session/event.test.ts(1 hunks)packages/web/src/content/docs/plugins.mdx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/opencode/src/session/index.ts (2)
packages/sdk/js/src/gen/types.gen.ts (1)
Event(1182-1201)packages/opencode/src/session/compaction.ts (1)
Event(20-27)
packages/opencode/test/session/event.test.ts (1)
packages/sdk/js/src/gen/types.gen.ts (1)
Session(534-555)
🔇 Additional comments (3)
packages/web/src/content/docs/plugins.mdx (1)
78-81: LGTM! Clear example of session.created event handling.The new notification handler correctly demonstrates using the
session.createdevent type and accessing the session ID viaevent.properties.info.id.packages/opencode/src/session/index.ts (2)
75-80: LGTM! Event definition follows established patterns.The
Event.Createddefinition correctly usesBus.eventwith the event name"session.created"and a payload schema matching the pattern used byEvent.Updated.
181-186: LGTM! Dual event publication preserves backward compatibility.Publishing both
Event.Created(line 181-183) andEvent.Updated(line 184-186) during session creation is a sound design choice that:
- Provides a specific event for session creation scenarios
- Maintains backward compatibility for existing plugins listening to
session.updated- Follows semantic ordering (created before updated)
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting
| describe("Session Events", () => { | ||
| test("should publish session.created event when session is created", async () => { | ||
| // Spy on the Bus.publish method to verify it's called | ||
| const publishSpy = spyOn(Bus, "publish").mockResolvedValue(undefined) | ||
|
|
||
| // Create a new session | ||
| const session = await Session.createNext({ | ||
| directory: "/tmp/test", | ||
| title: "Test Session" | ||
| }) | ||
|
|
||
| // Verify that Bus.publish was called | ||
| expect(publishSpy).toHaveBeenCalled() | ||
|
|
||
| // Verify that the session.created event was published | ||
| let createdEventPublished = false | ||
| for (const call of publishSpy.mock.calls) { | ||
| const [eventDef, payload] = call | ||
| if (eventDef.type === "session.created" && payload.info.id === session.id) { | ||
| createdEventPublished = true | ||
| break | ||
| } | ||
| } | ||
|
|
||
| expect(createdEventPublished).toBe(true) | ||
|
|
||
| // Clean up the spy | ||
| publishSpy.mockRestore() | ||
| }) |
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.
Wrap Session.createNext in Instance context for test
The new test calls Session.createNext directly, but the session module relies on Instance’s async-local context (via Instance.project, Instance.directory, Storage, Bus, etc.). When the test runs under bun test without Instance.provide, Bus.publish will invoke Instance.state and throw Context.NotFound("instance") before any assertions execute. This will make the entire suite fail rather than validating the event. Wrap the test body in await Instance.provide({ directory: …, fn: async () => { … } }) (or the existing fixture helper) so the required context is established before invoking Session.createNext.
Useful? React with 👍 / 👎.
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.
@copilot do it
| | EventFileWatcherUpdated | ||
| | EventTodoUpdated | ||
| | EventSessionIdle | ||
| | EventSessionCreated | ||
| | EventSessionUpdated | ||
| | EventSessionDeleted |
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.
Go SDK omits new session.created event type
Adding EventSessionCreated to the JS SDK means the server will now publish session.created events, but the Go SDK still enumerates the union of event types without this new variant (EventListResponseUnion and EventListResponseType only list the previous session events). When a Go client consumes the SSE stream and receives session.created, JSON unmarshalling will fail because the type is unknown. The Go SDK needs the same struct/union additions as the JS SDK to avoid breaking Go consumers once the new event is emitted.
Useful? React with 👍 / 👎.
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.
@codex do it
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.
To use Codex here, create an environment for this repo.
|
Note Docstrings generation - SUCCESS |
|
@coderabbitai on creation we dont want upated and created, only created. |
Docstrings generation was requested by @kierr. * #2 (comment) The following files were modified: * `packages/opencode/src/session/index.ts`
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 12164 @kierr, you're absolutely right! On session creation, we should only emit the Here's the fix for export async function createNext(input: { id?: string; title?: string; parentID?: string; directory: string }) {
const result: Info = {
id: Identifier.descending("session", input.id),
version: Installation.VERSION,
projectID: Instance.project.id,
directory: input.directory,
parentID: input.parentID,
title: input.title ?? createDefaultTitle(!!input.parentID),
time: {
created: Date.now(),
updated: Date.now(),
},
}
log.info("created", result)
await Storage.write(["session", Instance.project.id, result.id], result)
const cfg = await Config.get()
if (!result.parentID && (Flag.OPENCODE_AUTO_SHARE || cfg.share === "auto"))
share(result.id)
.then((share) => {
update(result.id, (draft) => {
draft.share = share
})
})
.catch(() => {
// Silently ignore sharing errors during session creation
})
Bus.publish(Event.Created, {
info: result,
})
// Removed Event.Updated - only emit on actual updates
return result
}The key change is removing lines 183-185 that published |
This PR adds a new session.created event type that is fired when a new session is created. This allows plugins to respond to new session creation events.
Changes:
Summary by CodeRabbit
New Features
Documentation
Tests