Conversation
amezin
force-pushed
the
service
branch
7 times, most recently
from
c8f6ad7 to
cecaacd
Compare
And always disconnect from the cancellable.
Gio.Cancellable.connect() will return 0 if already cancelled. Exception can be thrown at any point, leaving one or both handlers undefined.
📝 WalkthroughWalkthroughThe changes refactor the ddterm shell to extract promise/timeout utilities into a shared module, transition from executable paths to app-info objects throughout service and installation layers, and restructure subprocess log collection to support journald and streaming I/O asynchronously. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
ddterm/shell/appcontrol.jsddterm/shell/extension.jsddterm/shell/install.jsddterm/shell/service.jsddterm/shell/subprocess.jsddterm/util/meson.buildddterm/util/promise.js
🧰 Additional context used
🧬 Code graph analysis (3)
ddterm/shell/subprocess.js (1)
ddterm/util/promise.js (1)
promisify(8-21)
ddterm/util/promise.js (1)
ddterm/shell/appcontrol.js (2)
cancellable(98-98)cancellable(119-119)
ddterm/shell/service.js (1)
ddterm/shell/subprocess.js (4)
argv(64-71)proc(76-79)Subprocess(161-266)Subprocess(161-266)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: ci / test (alpine-3.21)
- GitHub Check: ci / test (ubuntu-24.04)
- GitHub Check: ci / test (debian-13)
- GitHub Check: ci / test (opensuse-tumbleweed)
- GitHub Check: ci / test (ubuntu-25.10)
- GitHub Check: ci / test (fedora-43)
- GitHub Check: ci / test (ubuntu-25.04)
- GitHub Check: ci / test (fedora-42)
- GitHub Check: ci / test (alpine-3.20)
- GitHub Check: ci / test (centos-10)
- GitHub Check: ci / test (alpine-3.23)
- GitHub Check: ci / test (alpine-3.22)
- GitHub Check: ci / archlinux-package
- GitHub Check: ci / update-translations
🔇 Additional comments (23)
ddterm/util/meson.build (1)
6-6: LGTM!The build configuration correctly includes the new
promise.jsutility module alongside the existingdisplayconfig.js.ddterm/shell/appcontrol.js (1)
13-13: LGTM!Clean refactoring to use the shared promise utilities. This eliminates code duplication and centralizes timeout and property-wait logic.
ddterm/util/promise.js (3)
8-21: LGTM!The
promisifyhelper correctly implements the GIO async/finish pattern, preservingthiscontext and properly catching errors from the finish callback.
23-49: LGTM!The
wait_timeoutimplementation correctly handles both timeout and cancellation scenarios with proper cleanup. The design where it always throws (eitherCANCELLEDorTIMED_OUT) is appropriate for use withPromise.race()patterns.
51-87: LGTM!The
wait_propertyimplementation correctly handles:
- Early return when predicate is already satisfied
- Signal-based property watching with proper cleanup
- Error propagation from predicate evaluation
- Cancellation support with proper cleanup
The captured
valueensures consistency between predicate check and return value.ddterm/shell/install.js (3)
10-22: LGTM!Good backward-compatible handling of the GioUnix namespace split introduced in GLib 2.79.2. The fallback to
Gio.DesktopAppInfoensures the code works on older GLib versions.
40-76: LGTM!The enhanced return types from
get_existing_content()andinstall()provide necessary information for callers to make conditional decisions (e.g., DBus reload only when changed, and returning the correct filename forDesktopAppInfo).
128-149: LGTM!The refactored
install()method:
- Correctly captures install results to conditionally trigger DBus reload
- Creates
app_infofrom the effective desktop entry file (whether newly installed or existing)- Returns
app_infofor downstream consumption byServiceThis is a cleaner design that avoids unnecessary DBus reload calls.
ddterm/shell/extension.js (3)
137-166: LGTM!The
install()function now correctly returnsapp_infofor use by theServiceconfiguration, enabling the transition from executable path to a properGio.AppInfoobject.
240-256: LGTM!The
Serviceis now configured withapp_infoinstead ofexecutable, aligning with the updatedServiceAPI and enabling proper app launch semantics.
289-298: LGTM!Proper error handling for log collection:
- Gracefully handles missing subprocess
- Shows error notification with logs when available
- Falls back to notification without logs if log collection fails
ddterm/shell/subprocess.js (6)
63-85: LGTM!The
collect_journald_logsfunction correctly builds and executes a journalctl query to retrieve recent logs, using proper timestamp formatting and PID filtering.
87-104: LGTM!Clean async generator implementation for streaming reads. The
finallyblock ensures proper stream cleanup regardless of how iteration ends.
106-121: LGTM!The generator correctly splits arrays while preserving delimiters, which is essential for accurate line counting in
collect_stdio_logs.
123-159: LGTM!The
collect_stdio_logsfunction implements a proper tee-style log collection:
- Streams output to stderr for real-time visibility
- Maintains a rolling buffer of the last 50 lines
- Correctly handles partial lines across chunk boundaries
203-215: LGTM!The simplified environment handling with
set_environand the unified log collection setup are clean improvements. The differentiated handling for journald (lazy function) vs stdio (eager promise) correctly reflects the different collection strategies.
233-260: LGTM!Clean promisified implementations of
wait()andwait_check(). Theget_pid()andget_logs()accessors provide proper encapsulation for the subprocess identification and log retrieval functionality.ddterm/shell/service.js (6)
5-10: LGTM!Required imports for the new functionality: GLib for
shell_parse_argvandwait_propertyfor reactive property monitoring.
28-34: LGTM!Property type change from string (
executable) toGio.AppInfo(app-info) enables richer app launch semantics and aligns with theInstaller.install()return type.
122-124: LGTM!Passing both
nameandownerto the update handler enables more informative logging.
172-206: LGTM!The
#create_subprocessmethod correctly:
- Parses the commandline from
app_info- Configures the launch context with extra environment variables
- Emits proper
launch-startedandlaunchedsignals with platform data- Creates the appropriate subprocess type for the display backend
222-235: LGTM!The enhanced logging now includes the bus name, making it easier to debug name ownership changes.
237-290: Well-designed concurrent start handling!The reworked
start()method properly handles:
- Concurrent calls: The
while (this.starting)loop withwait_propertyensures subsequent callers wait for an in-progress startup- Early exit: Returns immediately if already registered
- Subprocess lifecycle: The
Promise.racepattern correctly detects subprocess termination before D-Bus registration- Cancellation: Proper chaining and cleanup via
inner_cancellableThis addresses the PR objective of handling concurrent
.start()calls and cancellation.
| async #wait_subprocess(cancellable) { | ||
| this.#subprocess_wait_cancel = new Gio.Cancellable(); | ||
|
|
||
| this.emit('error', ex); | ||
| }).finally(() => { | ||
| try { | ||
| await this.subprocess.wait_check(cancellable); | ||
| } catch (ex) { | ||
| if (!this.starting && !ex.matches(Gio.io_error_quark(), Gio.IOErrorEnum.CANCELLED)) | ||
| this.emit('error', ex); | ||
| } finally { | ||
| this.#subprocess_running = false; | ||
| this.notify('is-running'); | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all call sites of #wait_subprocess and usage of #subprocess_wait_cancel
echo "=== Calls to #wait_subprocess ==="
rg -n '#wait_subprocess' ddterm/shell/service.js
echo ""
echo "=== Usage of #subprocess_wait_cancel ==="
rg -n '#subprocess_wait_cancel' ddterm/shell/service.jsRepository: ddterm/gnome-shell-extension-ddterm
Length of output: 496
🏁 Script executed:
#!/bin/bash
# Get more context around line 160 (unwatch) and line 212 (wait_check)
echo "=== Context around unwatch (line 160) ==="
sed -n '155,165p' ddterm/shell/service.js
echo ""
echo "=== Context around wait_check (line 208-220) ==="
sed -n '208,220p' ddterm/shell/service.jsRepository: ddterm/gnome-shell-extension-ddterm
Length of output: 880
Fix the subprocess cancellation logic: the created #subprocess_wait_cancel is never passed to wait_check.
The #wait_subprocess method creates a new Gio.Cancellable stored in #subprocess_wait_cancel (line 209) but passes an undefined cancellable parameter to wait_check (line 212). Since all call sites (lines 115 and 261) invoke #wait_subprocess() without arguments, the cancellable parameter is always undefined. Consequently, unwatch() cannot cancel the subprocess wait—it attempts to cancel #subprocess_wait_cancel, which was never passed to wait_check. Either pass #subprocess_wait_cancel to wait_check, or remove the unused cancellable parameter.
🤖 Prompt for AI Agents
In ddterm/shell/service.js around lines 208 to 220, the method creates
this.#subprocess_wait_cancel but calls this.subprocess.wait_check(cancellable)
with the unused parameter, so the cancellable passed is undefined and
unwatch()/cancel cannot stop the wait; update the method to pass the internal
cancellable to wait_check (i.e. call
this.subprocess.wait_check(this.#subprocess_wait_cancel)) and remove the unused
parameter from the method signature and all callers (or alternatively keep the
parameter and assign it to this.#subprocess_wait_cancel before calling
wait_check) so that the cancellable used to start the wait is the same one
cancelled by unwatch().
1 participant
Summary by CodeRabbit
Release Notes
Improvements
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.