Desktop: Isolate CEF-rendered UI into separate crate and process#4321
Merged
Conversation
Member
Author
|
!build desktop (Run ID 29061690651) |
Contributor
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
|
|
timon-schelling
force-pushed
the
desktop-ui-crate
branch
from
7666ae8 to
24c5cb2
Compare
Member
Author
|
!build desktop (Run ID 29065912212) |
Contributor
There was a problem hiding this comment.
2 issues found across 18 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="desktop/ui/src/dirs.rs">
<violation number="1" location="desktop/ui/src/dirs.rs:12">
P2: Replacing the panic with `tracing::error!` avoids crashing, but callers of `app_tmp_dir()` now have no way to know that directory creation failed. The function still returns the path as if it were valid, so downstream code may hit less-obvious IO failures later. Consider propagating the error by changing the return type to `io::Result<PathBuf>` (and updating `temp_dir_root()` accordingly), so callers can handle the failure explicitly.</violation>
</file>
<file name="desktop/ui/src/remote/spawn.rs">
<violation number="1" location="desktop/ui/src/remote/spawn.rs:256">
P2: Replacing `.expect()` with `?` here introduces a partial-startup risk: `InstanceReceivers` has already been consumed from the `HostHandle`, so if this or a subsequent thread spawn fails, earlier threads remain running while the caller gets no `shutdown_complete_receiver` and cannot call `start_instance` again (it would return `InstanceLimit`). Consider ensuring cleanup of already-spawned threads on error, or deferring `receivers.take()` until all spawns succeed.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
|
|
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 15 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Member
Author
|
!build desktop (Run ID 29287686618) |
|
|
|
TrueDoctor
approved these changes
Jul 14, 2026
Contributor
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="desktop/ui/src/ipc.rs">
<violation number="1" location="desktop/ui/src/ipc.rs:1">
P3: This TODO is contradicted by the current module graph: `context.rs` uses `crate::ipc` directly, so inlining `ipc.rs` into `internal` would either break `BrowserContext` or require an unmentioned re-export. Please update or remove the TODO so future refactors do not treat this shared module as internal-only.
(Based on your team's feedback about TODO expectations.) [FEEDBACK_USED].</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| @@ -1,3 +1,5 @@ | |||
| // TODO: Consider inlining this file into internal as it is no longer use outside | |||
Contributor
There was a problem hiding this comment.
P3: This TODO is contradicted by the current module graph: context.rs uses crate::ipc directly, so inlining ipc.rs into internal would either break BrowserContext or require an unmentioned re-export. Please update or remove the TODO so future refactors do not treat this shared module as internal-only.
(Based on your team's feedback about TODO expectations.) .
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At desktop/ui/src/ipc.rs, line 1:
<comment>This TODO is contradicted by the current module graph: `context.rs` uses `crate::ipc` directly, so inlining `ipc.rs` into `internal` would either break `BrowserContext` or require an unmentioned re-export. Please update or remove the TODO so future refactors do not treat this shared module as internal-only.
(Based on your team's feedback about TODO expectations.) .</comment>
<file context>
@@ -1,3 +1,5 @@
+// TODO: Consider inlining this file into internal as it is no longer use outside
+
use cef::{Frame, ImplBinaryValue, ImplFrame, ImplListValue, ImplProcessMessage, ImplV8Context, ProcessId, V8Context, sys::cef_process_id_t};
</file context>
Suggested change
| // TODO: Consider inlining this file into internal as it is no longer use outside | |
| // TODO: Consider reorganizing this file; it is shared by context and internal CEF handlers. |
timon-schelling
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TL;DR: I got tired of dealing with CEF so I isolated it