Conversation
There was a problem hiding this comment.
Pull request overview
This PR unifies the Quickshell ↔ daemon IPC layer by moving the old newline-delimited JSON-RPC “uisocket” protocol onto the existing protobuf IPC schema, using protojson over a newline-delimited stream and adding event push support.
Changes:
- Extend the IPC protobuf to include filesystem/browser RPCs plus daemon→UI event envelopes.
- Replace length-prefixed protobuf framing with newline-delimited
protojsonframing, adding broadcast support in the IPC server/client. - Remove the legacy
internal/uisocketserver/tests and update the QML service to speak the unified IPC envelope format.
Reviewed changes
Copilot reviewed 15 out of 20 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| proto/tilbo/ipc/v1/ipc.proto | Adds new request/response kinds for browser ops and a new Event envelope payload. |
| internal/ipc/server.go | Switches to scanner + protojson unmarshalling and adds event broadcast to all clients. |
| internal/ipc/client.go | Switches to scanner + protojson unmarshalling for responses. |
| internal/ipc/framing.go | Changes framing to newline-delimited protojson writes. |
| internal/ipc/ipc_test.go | Updates framing test to validate JSON line unmarshalling. |
| internal/ipc/extra_test.go | Updates extra framing tests for JSON framing. |
| internal/ipc/gen/tilbo/ipc/v1/ipc.pb.go | Regenerated Go bindings for updated IPC proto (incl. Event + new RPC messages). |
| cmd/tilbo-daemon/main.go | Removes uisocket wiring; routes UI events through IPC server + new handler wiring. |
| cmd/tilbo-daemon/handlers.go | Adds handlers mapping new IPC browser RPCs to daemonBrowserMethods. |
| cmd/tilbo-quickshell/services/TilboDaemon.qml | Updates UI transport from JSON-RPC frames to IPC envelope JSON (requestId/response/event). |
| internal/uisocket/server.go | Deleted legacy JSON-RPC socket server implementation. |
| internal/uisocket/server_test.go | Deleted tests for the removed uisocket server. |
| internal/uisocket/methods_test.go | Deleted tests for the removed uisocket server methods. |
| mise.toml | Adds Node toolchain + a generate-js task for protobuf→JS bindings. |
| package.json | Introduces JS deps for protobuf tooling/runtime. |
| package-lock.json | Lockfile for JS dependencies. |
| .gitignore | Ignores node_modules/. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for cw := range s.conns { | ||
| if err := cw.Close(); err != nil && !errors.Is(err, net.ErrClosed) { |
| delete(s.conns, conn) | ||
| } | ||
|
|
||
| // BroadcastEvent pushes an event to all connected clients asynchronously. |
| // BroadcastEvent pushes an event to all connected clients asynchronously. | ||
| func (s *Server) BroadcastEvent(event *ipcv1.Event) { | ||
| s.mu.Lock() |
| } | ||
|
|
||
| env := &ipcv1.Envelope{} | ||
| if err := protojson.Unmarshal(line, env); err != nil { |
| const maxTokenSize = 4 * 1024 * 1024 | ||
| scanner := bufio.NewScanner(c.conn) | ||
| scanner.Buffer(make([]byte, maxTokenSize), maxTokenSize) | ||
|
|
||
| for scanner.Scan() { |
| } | ||
|
|
||
| env := &ipcv1.Envelope{} | ||
| if err := protojson.Unmarshal(line, env); err != nil { |
| buf, err := m.Marshal(env) | ||
| if err != nil { | ||
| return fmt.Errorf("marshal envelope: %w", err) | ||
| } | ||
|
|
||
| buf = append(buf, '\n') |
|
@darkliquid I've opened a new pull request, #4, to work on those changes. Once the pull request is ready, I'll request review from you. |
* Initial plan * fix: add max frame size check in WriteEnvelope Co-authored-by: darkliquid <31256+darkliquid@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: darkliquid <31256+darkliquid@users.noreply.github.com>
|
@copilot fix all outstanding issues mentioned in the review and make the recommended changes/enhancements |
|
@darkliquid I've opened a new pull request, #5, to work on those changes. Once the pull request is ready, I'll request review from you. |
* Initial plan * fix: address all IPC review feedback - naming, forward-compat, connection-drop cleanup, and broadcast tests Co-authored-by: darkliquid <31256+darkliquid@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Update internal/ipc/extra_test.go * Update internal/ipc/extra_test.go --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: darkliquid <31256+darkliquid@users.noreply.github.com> Co-authored-by: Andrew Montgomery <darkliquid@darkliquid.co.uk> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
@copilot fix broken tests, just commit directly to this pr |
|
@darkliquid I've opened a new pull request, #6, to work on those changes. Once the pull request is ready, I'll request review from you. |
* Initial plan * fix: resolve race condition in TestBroadcastEvent_Delivery Co-authored-by: darkliquid <31256+darkliquid@users.noreply.github.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: darkliquid <31256+darkliquid@users.noreply.github.com> Co-authored-by: Andrew Montgomery <darkliquid@darkliquid.co.uk> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
No description provided.