app: switch to async D-Bus calls during startup#1620
Conversation
amezin
force-pushed
the
app-init-dbus-async
branch
4 times, most recently
from
b954639 to
2ff62c9
Compare
amezin
force-pushed
the
app-init-dbus-async
branch
from
2ff62c9 to
a432280
Compare
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughThe application's startup lifecycle and display configuration were refactored to async/await. Application methods (startup path, command_line, open_file, ensure_window*, restore_session) now use asynchronous flows with try/catch and hold/release sequencing; DisplayConfig unifies update paths, adds init_async(), and exposes an observable error state. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Hold as InitHold
participant DBus as Extension D-Bus Proxy
participant DC as DisplayConfig
participant Win as Window/Terminal
Note over App: vfunc_startup() (async)
App->>Hold: acquire
Note right of Hold: prevent early exit
rect rgb(240,248,255)
App->>DBus: construct proxy (constructor) & init_async()
DBus-->>App: ready / error
end
rect rgb(240,248,255)
App->>DC: new(dbus_connection) & init_async()
DC-->>App: initial state / error
end
App->>App: setup actions, bind handlers (await ensure_window())
alt init success
App->>Hold: release
else init failure
App->>App: log error & exit
end
Note over App,Win: User action (activate/open)
App->>App: await ensure_window()/open_file()
App->>Win: create/present terminal
Win-->>App: ready
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (1)ddterm/util/displayconfig.js (1)
⏰ 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)
🔇 Additional comments (12)
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: 6
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
ddterm/app/application.jsddterm/util/displayconfig.js
🧰 Additional context used
🧬 Code graph analysis (1)
ddterm/app/application.js (2)
ddterm/util/displayconfig.js (3)
DisplayConfig(25-216)DisplayConfig(25-216)i(340-340)ddterm/app/appwindow.js (4)
action(307-307)notebook(398-403)AppWindow(49-760)AppWindow(49-760)
⏰ 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 (ubuntu-24.04)
- GitHub Check: ci / test (ubuntu-25.10)
- GitHub Check: ci / test (debian-13)
- GitHub Check: ci / test (fedora-43)
- GitHub Check: ci / test (ubuntu-25.04)
- GitHub Check: ci / test (centos-10)
- GitHub Check: ci / test (alpine-3.21)
- GitHub Check: ci / test (fedora-42)
- GitHub Check: ci / test (alpine-3.22)
- GitHub Check: ci / test (alpine-3.23)
- GitHub Check: ci / test (opensuse-tumbleweed)
- GitHub Check: ci / test (alpine-3.20)
- GitHub Check: ci / archlinux-package
- GitHub Check: ci / update-translations
🔇 Additional comments (10)
ddterm/util/displayconfig.js (3)
51-57: LGTM! Error property registration.The new
errorproperty properly exposes the internal error state with the correct GLib.Error boxed type.
140-168: Approve the unified update() method with error state management.The consolidation of
update_sync/update_asyncinto a singleupdate()method is a good simplification. The error state is properly cleared on successful parse (lines 191-194), and the cancellation check correctly avoids spurious error logging.Also applies to: 191-194
207-215: Approve unwatch() error signaling for pending init_async consumers.Setting a CANCELLED error and notifying ensures that any pending
init_async()callers will properly reject rather than hang indefinitely.ddterm/app/application.js (7)
70-77: LGTM! Proxy construction without sync call.Switching from
Gio.DBusProxy.new_syncto constructing the proxy object directly and deferring initialization toinit_asyncis the right approach for async startup.
246-253: Exit status returned before async command handling completes.The
command-linesignal handler returnscommand_line.get_exit_status()synchronously at line 252, but the actual command processing happens asynchronously. At the time of return, the exit status may not yet be set by the asynccommand_line()method.This may be intentional if GLib's command-line handling expects the handler to return immediately and relies on
command_line.done()for completion signaling. Please verify this is the expected behavior.
263-274: Approve async D-Bus initialization before GTK startup.Starting D-Bus proxy and display config initialization before calling
super.vfunc_startup()(which initializes GTK) is a good optimization. The async operations can proceed in parallel with GTK init.
423-426:System.exit(1)in vfunc_startup may cause abrupt termination.Calling
System.exit(1)will terminate the process immediately without running cleanup handlers or the normal shutdown sequence. Consider usingthis.quit()followed by setting an exit code, or at minimum ensure this is the intended behavior for startup failures.
653-676: Approve hold/release pattern for async window creation.Using
this.hold()andthis.release()in a try/finally ensures the application stays alive while awaiting D-Bus and display config initialization. The second check forthis.windowafter awaits (line 663) correctly handles the case where another caller created the window while waiting.
696-700: Approve async ensure_window_with_terminal.The refactor to await
ensure_window()and then callensure_terminal()is correct. The method returnsthis.windowwhich will be set after the await completes.
790-825: Approve async restore_session with proper error handling.The method correctly awaits
ensure_window()and has comprehensive error handling for file read failures.
| this.connect('activate', () => { | ||
| this.ensure_window_with_terminal().present(); | ||
| this.ensure_window_with_terminal().then(win => win.present()); | ||
| }); |
There was a problem hiding this comment.
Unhandled promise rejection in 'activate' handler.
Similar to other handlers, the ensure_window_with_terminal().then(...) call lacks error handling.
🔎 Proposed fix
this.connect('activate', () => {
- this.ensure_window_with_terminal().then(win => win.present());
+ this.ensure_window_with_terminal().then(win => win.present()).catch(ex => logError(ex));
});Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In ddterm/app/application.js around lines 233 to 235, the 'activate' signal
handler calls ensure_window_with_terminal().then(win => win.present()) without
handling promise rejections; wrap the async call to handle errors (either
convert the handler to async and use try/catch around await
ensure_window_with_terminal() or append .catch(...) to the promise) and ensure
any caught error is logged or reported and does not cause an unhandled
rejection, while preserving the existing success path that calls win.present().
| this.connect('open', async (_, files) => { | ||
| for (const file of files) | ||
| this.open_file(file); | ||
| await this.open_file(file); // eslint-disable-line no-await-in-loop | ||
|
|
||
| this.activate(); | ||
| }); |
There was a problem hiding this comment.
Async 'open' handler: unhandled rejection.
The open signal callback is marked async but the signal connection does not handle potential rejections. If open_file throws, the error will become an unhandled promise rejection.
🔎 Proposed fix to add error handling
- this.connect('open', async (_, files) => {
- for (const file of files)
- await this.open_file(file); // eslint-disable-line no-await-in-loop
-
- this.activate();
- });
+ this.connect('open', (_, files) => {
+ (async () => {
+ for (const file of files)
+ await this.open_file(file); // eslint-disable-line no-await-in-loop
+
+ this.activate();
+ })().catch(ex => logError(ex));
+ });🤖 Prompt for AI Agents
In ddterm/app/application.js around lines 255 to 260, the async 'open' signal
handler can produce unhandled promise rejections if await this.open_file(file)
throws; wrap the async handler body in a try/catch so any error from opening
files is caught, log or propagate the error in the catch (e.g. console.error or
emit an 'error' event on the application) and ensure activate() is only called
on success or handle failure path explicitly to avoid unhandled promise
rejections.
| this.simple_action('toggle', () => { | ||
| this.ensure_window_with_terminal().then(win => win.toggle()); | ||
| }); | ||
| this.simple_action('show', () => { | ||
| this.ensure_window_with_terminal().then(win => win.present()); | ||
| }); |
There was a problem hiding this comment.
Unhandled promise rejections in action callbacks.
The toggle and show actions call ensure_window_with_terminal().then(...) without a .catch() handler. If the promise rejects (e.g., D-Bus initialization fails), the error will be unhandled.
🔎 Proposed fix to add error handling
this.simple_action('toggle', () => {
- this.ensure_window_with_terminal().then(win => win.toggle());
+ this.ensure_window_with_terminal().then(win => win.toggle()).catch(ex => logError(ex));
});
this.simple_action('show', () => {
- this.ensure_window_with_terminal().then(win => win.present());
+ this.ensure_window_with_terminal().then(win => win.present()).catch(ex => logError(ex));
});Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In ddterm/app/application.js around lines 330 to 335, the action callbacks call
ensure_window_with_terminal().then(...) without handling rejections; add error
handling so promise rejections are caught and surfaced. Replace the .then(...)
calls with either async functions using try/catch around await
ensure_window_with_terminal() or append .catch(err => /* handle */) to the
promise chain; in the catch handler log the error (or surface it to the UI) and
avoid throwing unhandled rejections so failures during D-Bus initialization or
window creation are reported safely.
| this.window.disconnect(this._save_session_handler); | ||
| this._save_session_handler = null; | ||
| } | ||
| this.restore_session(); |
There was a problem hiding this comment.
Fire-and-forget async call: restore_session() not awaited.
restore_session() is now async but is called without await in vfunc_startup(). This means:
- Session restoration happens in the background
- Startup proceeds immediately, potentially before session is restored
- Any errors are not caught by the surrounding try/catch
If this is intentional (non-blocking session restore), add .catch(ex => logError(ex)) to handle potential rejections. Otherwise, consider if this should block startup.
🔎 Proposed fix if fire-and-forget is intentional
- this.restore_session();
+ this.restore_session().catch(ex => logError(ex));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.restore_session(); | |
| this.restore_session().catch(ex => logError(ex)); |
| init_async() { | ||
| const handlers = []; | ||
|
|
||
| return new Promise((resolve, reject) => { | ||
| if (this.#error) | ||
| reject(this.#error); | ||
|
|
||
| if (this.#current_state) | ||
| resolve(this.#current_state); | ||
|
|
||
| handlers.push(this.connect('notify::error', () => { | ||
| if (this.#error) | ||
| reject(this.#error); | ||
| })); | ||
|
|
||
| handlers.push(this.connect('notify::current-state', () => { | ||
| if (this.#current_state) | ||
| resolve(this.#current_state); | ||
| })); | ||
| }).finally(() => { | ||
| while (handlers.length) | ||
| this.disconnect(handlers.pop()); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Missing early returns cause Promise to never settle in some cases.
When this.#error or this.#current_state is already set, you call reject() or resolve() but then continue executing to set up signal handlers. Since there's no return after these calls, the Promise executor continues and the handlers array gets populated. The finally block will run after the Promise settles, but the logic flow is confusing and wasteful.
More critically: if neither condition is true initially, but then both signals fire in quick succession, both resolve and reject could be called (though only the first takes effect).
🔎 Proposed fix to add early returns
init_async() {
const handlers = [];
return new Promise((resolve, reject) => {
- if (this.#error)
- reject(this.#error);
+ if (this.#error) {
+ reject(this.#error);
+ return;
+ }
- if (this.#current_state)
- resolve(this.#current_state);
+ if (this.#current_state) {
+ resolve(this.#current_state);
+ return;
+ }
handlers.push(this.connect('notify::error', () => {
if (this.#error)
reject(this.#error);
}));
handlers.push(this.connect('notify::current-state', () => {
if (this.#current_state)
resolve(this.#current_state);
}));
}).finally(() => {
while (handlers.length)
this.disconnect(handlers.pop());
});
}🤖 Prompt for AI Agents
In ddterm/util/displayconfig.js around lines 83–106, add early returns and a
settled guard so the Promise cannot continue registering handlers after already
resolving/rejecting and to prevent multiple settle attempts: after the initial
this.#error reject and this.#current_state resolve, immediately return;
introduce a local boolean (e.g., settled = false) and in each signal handler
check if settled before calling resolve/reject, set settled = true when calling,
and ensure handlers are cleaned up after the first settlement (the existing
finally can still disconnect remaining handlers). This prevents redundant
handler registration and multiple resolve/reject calls.
| if (error instanceof GLib.Error && | ||
| error.matches(Gio.io_error_quark(), Gio.IOErrorEnum.CANCELLED)) | ||
| return; | ||
|
|
||
| logError(error); | ||
| this.#error = error; | ||
| this.notify(error); |
There was a problem hiding this comment.
Typo: this.notify(error) should be this.notify('error').
Line 164 passes the error object itself instead of the property name string 'error'. This will not properly notify listeners of the error property change.
🔎 Proposed fix
logError(error);
this.#error = error;
- this.notify(error);
+ this.notify('error');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (error instanceof GLib.Error && | |
| error.matches(Gio.io_error_quark(), Gio.IOErrorEnum.CANCELLED)) | |
| return; | |
| logError(error); | |
| this.#error = error; | |
| this.notify(error); | |
| if (error instanceof GLib.Error && | |
| error.matches(Gio.io_error_quark(), Gio.IOErrorEnum.CANCELLED)) | |
| return; | |
| logError(error); | |
| this.#error = error; | |
| this.notify('error'); |
🤖 Prompt for AI Agents
In ddterm/util/displayconfig.js around lines 158 to 164, the code calls
this.notify(error) passing the error object instead of the property name; change
the notify call to this.notify('error') so listeners receive a property-change
notification for the "error" property, and ensure no other calls use the object
instead of the property string.
Should improve startup speed
Start async D-Bus calls before initializing Gtk.
amezin
force-pushed
the
app-init-dbus-async
branch
from
a432280 to
84d0380
Compare
1 participant
Should improve startup speed