Conversation
Merged
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds an mdBook doc generator and CLI, enforces regenerated docs in CI, refactors Rhai scripting registration/runtime wiring, introduces versioned workspace persistence with an Interrupted buffer state and atomic save/load, updates protocol/core serde derives, many generated docs/site assets, and adjusts tests and Cargo manifests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant Runtime
participant Workspace
Client->>Server: connect / normal RPCs
Server->>Runtime: start (load_workspace(workspace_path))
Runtime->>Workspace: read & deserialize persisted workspace
Workspace-->>Runtime: PersistedWorkspace / migrate
Runtime-->>Server: runtime created with restored ServerState
Client->>Server: normal operations (sessions, buffers, actions)
Note right of Runtime: Running/Created buffers may become Interrupted on persist
Server->>Runtime: shutdown requested
Runtime->>Workspace: save_workspace atomically (temp file → rename)
Workspace-->>Runtime: write success / error
Runtime-->>Server: shutdown complete (persist result)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 43-44: The CI step named "Verify checked-in config API docs"
currently runs the tracked-only check `git diff --exit-code -- docs/config-api`;
update this step so it also fails when untracked/generated files exist. Replace
the command invocation (the one currently calling `git diff --exit-code --
docs/config-api`) with a check that detects untracked files as well (for example
using `git status --porcelain` or `git ls-files --others --exclude-standard` for
the path) and exit non‑zero when any untracked or uncommitted changes are found;
keep the step name "Verify checked-in config API docs" so it’s easy to locate.
In `@crates/embers-client/src/scripting/engine.rs`:
- Around line 361-381: Add brief doc comments for the thread-local registration
helpers to explain the invariant and intended usage: document that
ACTIVE_REGISTRATION is a thread-local holding the current SharedRegistration,
that with_active_registration temporarily sets it for the duration of callback
and restores the previous value, and that clone_active_registration panics (via
expect) if no registration is active; include guidance that callers must use
with_active_registration (or otherwise ensure an active registration) before
calling clone_active_registration. Reference the symbols ACTIVE_REGISTRATION,
with_active_registration, and clone_active_registration in the comments.
- Around line 788-808: The _registration parameter in register_api is unused;
remove it from the function signature (change fn register_api(engine: &mut
Engine) { ... }) and update all call sites such as
register_documented_registration_api to call register_api(engine) (remove the
Arc::new(Mutex::new(RegistrationState::default())) argument). Also drop any
now-unused type/import (e.g., SharedRegistration) if it becomes unused after
this change.
In `@crates/embers-client/src/scripting/mod.rs`:
- Line 2: Make the documentation module private (remove the pub on the mod
declaration for documentation) and instead re-export only the specific
function(s) needed by the rest of the crate from that module (e.g., add pub use
documentation::your_needed_function_name(s) in this file). This narrows the
public API surface while keeping the implementation in the private documentation
module; target the "documentation" module and the specific function symbol(s)
that callers currently rely on for the re-export.
In `@crates/embers-client/src/scripting/runtime.rs`:
- Around line 64-74: The #[allow(dead_code)] on register_documented_runtime_api
hides why the function appears unused; either remove the attribute so the
compiler will surface real unused warnings, or keep it but add a brief comment
above register_documented_runtime_api stating that it is referenced by
documentation.rs for doc generation (and note any conditional compilation that
prevents the compiler from seeing the usage); update the comment to mention the
documented_* module registrations to make the purpose clear.
In `@crates/embers-client/tests/config_api_docs.rs`:
- Around line 50-65: The test helper visit currently unwraps multiple I/O
fallible calls which obscures which file/dir failed; change visit to return
Result<(), std::io::Error> (or anyhow::Result) and replace unwraps with ?
(fs::read_dir(path)?, entry?.path(), path.strip_prefix(root)? and
fs::read_to_string(&path)?), or alternatively use expect with contextual
messages (e.g., entry.expect("reading dir entry for ...")), and update its
callers to propagate or handle the Result so tests surface clear error
locations; reference the visit function and the BTreeMap entries insertion when
updating signatures and call sites.
In `@docs/config-api/buffer-ref.md`:
- Line 14: Generated docs contain trailing spaces and incorrect fenced-block
spacing (MD009 and MD031); update the docs generator template that renders the
tab button markup (the code that produces class="tablinks active" / the tab
button HTML) to trim trailing whitespace and ensure there is exactly one blank
line before and after fenced code blocks. Remove any stray spaces at line ends
when building strings, normalize newline insertion so fenced blocks are
preceded/followed by a single newline, and re-run generation to verify the
offending lines (those producing the tablinks/button HTML and fenced code
sections) no longer emit trailing spaces or improper fence spacing.
In `@docs/config-api/context.md`:
- Around line 29-35: Add a blank line after the Rhai code block so the fenced
code (the snippet starting with ```rhai and ending with ```) is followed by an
empty line before the closing </div>; update the documentation generator
template that emits the examples (the template that outputs the fenced code
block and the closing </div>) to always insert a newline after the code fence to
satisfy MD031.
In `@docs/config-api/registration-globals.md`:
- Line 14: The generated Markdown contains trailing spaces; update the
documentation generator template or rendering code that emits the
"registration-globals" content (the template/partial used to produce
registration-globals.md, e.g., the render/registrationGlobals template or
function that builds those Markdown lines) to trim end-of-line whitespace before
writing output: remove any hardcoded trailing spaces in the template strings,
apply a trimEnd() or rtrim operation on each generated line, and ensure the file
writer writes lines with '\n' only; then regenerate the docs and run the linter
to confirm lines previously flagged (e.g., the template producing lines 14, 36,
58, 80, 102) no longer contain trailing spaces.
- Line 39: Replace all occurrences of the non-standard closing break tag "</br>"
emitted by the documentation generator with the correct self-closing or
standalone break tag ("<br/>" or "<br>"); locate the generator routine that
serializes markdown/HTML line breaks (the template or function that renders line
breaks/newlines — e.g., the doc-generation template or the method that converts
newlines to HTML such as the renderer that currently outputs "</br>") and update
its output to emit "<br/>" (or "<br>") consistently across generated docs.
In `@docs/config-api/runtime-theme.md`:
- Line 14: Remove the trailing whitespace in the button tag markup (the snippet
containing <button group="color" id="link-color-Description" class="tablinks
active">) to satisfy MD009, and replace the nonstandard closing break tag
(</br>) in the generated docs with a self-closing <br /> to produce
standards-compliant HTML output; update the generated output generator/template
that emits these fragments so the button line has no trailing spaces and the
break uses <br />.
In `@docs/config-api/system-runtime.md`:
- Line 14: Several generated doc button rows (e.g., the HTML snippet containing
<button group="env" id="link-env-Description" class="tablinks active"> and its
siblings) include trailing spaces causing MD009 failures; update the docs
generator's button-row rendering code (the function/method that emits tab button
markup / renderTabButton or generateButtonRow in your generator) to trim
trailing whitespace before writing output (e.g., rstrip/trim each emitted line
or trim the assembled button-row string) so emitted markup contains no trailing
spaces for those tablinks entries.
In `@docs/config-api/tab-info.md`:
- Line 14: The generated tab header HTML contains trailing spaces (e.g., the
button element with group="has_activity" id="link-has_activity-Description"
class="tablinks active") which triggers MD009; update the generator/template
that emits these tab headers to trim trailing whitespace (or rstrip the
generated string) so attributes end with no extra space, and regenerate the
output for the button elements (those producing
id="link-has_activity-Description", id for other tabs at lines noted) to remove
the trailing space.
In `@docs/config-api/theme.md`:
- Line 14: Remove the trailing whitespace on the generated button tag (the
element starting with button group="set_palette"
id="link-set_palette-Description" class="tablinks active") and replace the
non-standard line-break tag `</br>` used elsewhere in the generated output with
a standard `<br>` (or `<br/>`) in the docs generator template so all emitted
pages are lint-clean (fix the template that emits the button and the line-breaks
rather than individual generated files).
In `@docs/config-api/tree.md`:
- Line 14: The generated markdown templates are emitting trailing spaces and
improperly spaced fenced code blocks; update the documentation generator
template that renders the button/group snippet (look for the HTML element with
group="buffer_attach" and id="link-buffer_attach-Description") to trim trailing
whitespace on each emitted line and to emit fenced code blocks with a language
tag and a blank line before and after the triple backticks (e.g., produce
"```rhai", then the code, then a blank line and "```"). Ensure the template
strips trailing spaces globally (no trailing-space characters) and enforces
fence-spacing rules to eliminate MD009 and MD031 violations across all API
pages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e68ef369-9c62-4727-8871-4f8f69b37b65
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (30)
.github/workflows/ci.ymlCargo.tomlcrates/embers-client/Cargo.tomlcrates/embers-client/src/bin/gen-config-docs.rscrates/embers-client/src/scripting/documentation.rscrates/embers-client/src/scripting/engine.rscrates/embers-client/src/scripting/mod.rscrates/embers-client/src/scripting/runtime.rscrates/embers-client/tests/config_api_docs.rsdocs/config-api/action.mddocs/config-api/buffer-ref.mddocs/config-api/context.mddocs/config-api/defs/registration.rhaidocs/config-api/defs/runtime.rhaidocs/config-api/event-info.mddocs/config-api/example.mddocs/config-api/floating-ref.mddocs/config-api/index.mddocs/config-api/mouse.mddocs/config-api/node-ref.mddocs/config-api/registration-globals.mddocs/config-api/runtime-theme.mddocs/config-api/session-ref.mddocs/config-api/system-runtime.mddocs/config-api/tab-bar-context.mddocs/config-api/tab-info.mddocs/config-api/tabbar.mddocs/config-api/theme.mddocs/config-api/tree.mddocs/config-api/ui.md
|
|
||
| <div> | ||
| <div class="tab"> | ||
| <button group="buffer_attach" id="link-buffer_attach-Description" class="tablinks active" |
There was a problem hiding this comment.
Generated markdown has repeated lint violations; fix at template source.
There are multiple trailing-space violations (MD009) and a fence-spacing violation (MD031 around Line 75). Please address these in the documentation generator so all API pages are emitted consistently lint-clean.
Representative output cleanup
-<button group="buffer_attach" id="link-buffer_attach-Description" class="tablinks active"
+<button group="buffer_attach" id="link-buffer_attach-Description" class="tablinks active"
@@
-```rhai
-tree.buffer_spawn(["/bin/zsh"], #{ title: "shell" })
-```
+```rhai
+tree.buffer_spawn(["/bin/zsh"], #{ title: "shell" })
+```
+Also applies to: 36-36, 58-58, 62-62, 75-75, 90-90, 112-112, 134-134, 156-156, 178-178
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 14-14: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/config-api/tree.md` at line 14, The generated markdown templates are
emitting trailing spaces and improperly spaced fenced code blocks; update the
documentation generator template that renders the button/group snippet (look for
the HTML element with group="buffer_attach" and
id="link-buffer_attach-Description") to trim trailing whitespace on each emitted
line and to emit fenced code blocks with a language tag and a blank line before
and after the triple backticks (e.g., produce "```rhai", then the code, then a
blank line and "```"). Ensure the template strips trailing spaces globally (no
trailing-space characters) and enforces fence-spacing rules to eliminate MD009
and MD031 violations across all API pages.
Pajn
force-pushed
the
api-docs
branch
2 times, most recently
from
d32fbff to
6f71d2b
Compare
This was referenced Mar 22, 2026
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
.github/workflows/ci.yml (1)
43-45: 🧹 Nitpick | 🔵 TrivialConsider adding diagnostic output on failure.
The verification correctly detects both tracked and untracked changes. However, when it fails, developers won't see what's out of date. Adding diagnostic output would improve the debugging experience.
♻️ Proposed improvement for better failure diagnostics
- name: Verify checked-in config API docs run: | - test -z "$(git status --porcelain -- docs/config-api)" + if [[ -n "$(git status --porcelain -- docs/config-api)" ]]; then + echo "docs/config-api is out of date. Run: cargo run -p embers-client --bin gen-config-docs" + git status --short -- docs/config-api + exit 1 + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 43 - 45, The CI step named "Verify checked-in config API docs" should emit diagnostic git output when the check fails: instead of silently running test -z "$(git status --porcelain -- docs/config-api)", change the step's run logic to detect non-empty git status and on failure print a clear message plus the git status (or git --no-pager status --porcelain -- docs/config-api) so contributors can see what files are changed, then exit non‑zero; update the run block around that check to print diagnostics and return failure when appropriate.docs/config-api/tree.md (1)
79-84:⚠️ Potential issue | 🟡 MinorFence-spacing lint issue persists in generated output (MD031).
The fenced Rhai block closes on Line 83 and is immediately followed by
</div>on Line 84 without a separating blank line.Minimal formatting fix (apply in generator template)
```rhai tree.buffer_spawn(["/bin/zsh"], #{ title: "shell" })```
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/config-api/tree.md` around lines 79 - 84, The MD031 lint error is caused by the fenced Rhai code block closing immediately before a </div> with no blank line; update the generator/template that emits the Rhai fence so it inserts a single blank line after the closing ``` fence (i.e., ensure the output contains a newline between the closing fence and the subsequent HTML tag), and re-generate the snippet that contains the tree.buffer_spawn(["/bin/zsh"], #{ title: "shell" }) block so the produced markdown satisfies the blank-line requirement.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/embers-client/src/scripting/documentation.rs`:
- Around line 187-193: The registration docs engine
(build_registration_docs_engine) only calls
register_documented_registration_api, so registration.rhai lacks the builder
methods that are registered in top-level config evaluation; update
build_registration_docs_engine to also register the same runtime/builder API
used during config evaluation—call register_runtime_api(&mut engine) (or the
equivalent function that registers the builder methods) and ensure the same
scope exposure for system, action, tree, and ui so the generated
registration.rhai includes those methods for building Action and TreeSpec
values.
- Around line 208-216: The loop over pages currently drops any PageSpec that
matches no exported items by using `continue`; instead fail fast: when
`items.is_empty()` for a given `page`, return or propagate an error (e.g., Err
with a descriptive message) that includes the `page`/PageSpec identity so the
caller knows which `PageSpec` matched nothing. Locate the loop that iterates
`for page in pages { ... let items = docs.items.iter().filter_map(|item|
filter_item_for_page(item, page)).collect::<Vec<_>>(); if items.is_empty() {
continue; } }` and replace the `continue` with an error return/propagation (or
panic if the surrounding API prefers) so missing-page matches are reported
rather than silently dropped.
In `@crates/embers-client/src/scripting/engine.rs`:
- Around line 368-377: The with_active_registration function currently replaces
ACTIVE_REGISTRATION but restores it only after callback() returns, so a panic in
callback() can leave the thread-local stuck; change this to use a RAII/Drop
guard: capture previous = active.replace(Some(registration.clone())), then
create a small guard struct (or closure-based guard) whose Drop implementation
restores active.replace(previous) so restoration runs on normal return and on
unwind; keep the callback invocation and return value unchanged but ensure the
guard is created before calling callback() and dropped afterward to guarantee
restoration even if callback panics.
In `@docs/config-api/node-ref.md`:
- Line 10: Update the NodeRef docs to replace the placeholder return types with
the concrete Rhai-visible types: change fn buffer(node: NodeRef) -> ? to -> int
| () ; fn geometry(node: NodeRef) -> ? to -> map | () ; fn id(node: NodeRef) ->
? to -> int ; fn parent(node: NodeRef) -> ? to -> int | () ; and fn
split_direction(node: NodeRef) -> ? to -> string | (); ensure each function
signature in the NodeRef documentation explicitly shows these types so the docs
match the actual implementations.
In `@docs/config-api/registration-globals.md`:
- Around line 10-13: The documentation for the bind function currently refers
only to a "named action" but the signatures show three overloads (fn bind(mode:
String, notation: String, action: Action), fn bind(mode: String, notation:
String, action_name: String), fn bind(mode: String, notation: String, actions:
Array)); update the descriptive text wherever bind is documented (including the
other occurrence at lines 28-30) to state that bind accepts either an Action
object, a string action name, or an array of actions, and give a concise example
or note for each overload so the description matches all three signatures.
In `@docs/config-api/tree.md`:
- Around line 58-59: The docs entry for buffer_spawn currently lists options:
Map but doesn't document accepted keys, which can hide typos because
parse_buffer_spawn only reads title, cwd, and env; update the docs for the
buffer_spawn signature (fn buffer_spawn(_: TreeApi, command: Array, options:
Map) -> TreeSpec and the other occurrence at 74-76) to explicitly list and
describe the supported options (title: string, cwd: string, env:
map/string->string) and note that unknown keys are ignored; align the wording
with the implementation by referencing parse_buffer_spawn in
crates/embers-client/src/scripting/runtime.rs as the source of truth.
---
Duplicate comments:
In @.github/workflows/ci.yml:
- Around line 43-45: The CI step named "Verify checked-in config API docs"
should emit diagnostic git output when the check fails: instead of silently
running test -z "$(git status --porcelain -- docs/config-api)", change the
step's run logic to detect non-empty git status and on failure print a clear
message plus the git status (or git --no-pager status --porcelain --
docs/config-api) so contributors can see what files are changed, then exit
non‑zero; update the run block around that check to print diagnostics and return
failure when appropriate.
In `@docs/config-api/tree.md`:
- Around line 79-84: The MD031 lint error is caused by the fenced Rhai code
block closing immediately before a </div> with no blank line; update the
generator/template that emits the Rhai fence so it inserts a single blank line
after the closing ``` fence (i.e., ensure the output contains a newline between
the closing fence and the subsequent HTML tag), and re-generate the snippet that
contains the tree.buffer_spawn(["/bin/zsh"], #{ title: "shell" }) block so the
produced markdown satisfies the blank-line requirement.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4a505b4c-cd6f-43db-9717-82737ee769b5
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (30)
.github/workflows/ci.ymlCargo.tomlcrates/embers-client/Cargo.tomlcrates/embers-client/src/bin/gen-config-docs.rscrates/embers-client/src/scripting/documentation.rscrates/embers-client/src/scripting/engine.rscrates/embers-client/src/scripting/mod.rscrates/embers-client/src/scripting/runtime.rscrates/embers-client/tests/config_api_docs.rsdocs/config-api/action.mddocs/config-api/buffer-ref.mddocs/config-api/context.mddocs/config-api/defs/registration.rhaidocs/config-api/defs/runtime.rhaidocs/config-api/event-info.mddocs/config-api/example.mddocs/config-api/floating-ref.mddocs/config-api/index.mddocs/config-api/mouse.mddocs/config-api/node-ref.mddocs/config-api/registration-globals.mddocs/config-api/runtime-theme.mddocs/config-api/session-ref.mddocs/config-api/system-runtime.mddocs/config-api/tab-bar-context.mddocs/config-api/tab-info.mddocs/config-api/tabbar.mddocs/config-api/theme.mddocs/config-api/tree.mddocs/config-api/ui.md
Pajn
force-pushed
the
api-docs
branch
2 times, most recently
from
4f3d8e5 to
16dad51
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/embers-client/src/scripting/documentation.rs`:
- Around line 187-193: The docs engine currently calls register_runtime_api(&mut
engine), which injects manual runtime closures and causes malformed entries in
defs/registration.rhai (e.g., "fn bar(...) -> EvalAltResult>>;"); replace that
call so build_registration_docs_engine() registers the exhaustive documented
runtime surface instead of the hand-registered closures — i.e., remove
register_runtime_api(&mut engine) and invoke the documented runtime-binding
registration function (the one that emits the complete, documented runtime API
used for docs generation) alongside register_documented_registration_api(&mut
engine) so defs/registration.rhai is generated from the documented surface only.
In `@crates/embers-client/src/scripting/runtime.rs`:
- Around line 64-75: register_documented_runtime_api currently registers a
reduced doc-only surface and omits many live APIs; update it to include the
missing exported modules/functions by mirroring what register_runtime_api
exposes — specifically ensure the exported symbols/modules for current_floating,
find_node, find_floating, focus_up, focus_down, scroll_page_down, buffer_empty,
tabs, split, viewport_width, buffer_count and the entire MuxApi surface are
registered with the Engine (e.g., add engine.register_global_module(...) calls
for the corresponding documented_* modules or include the same runtime_api
module used by register_runtime_api so those functions become available in the
documented runtime).
In `@crates/embers-client/tests/config_api_docs.rs`:
- Around line 118-126: The function first_difference has a rustfmt layout error:
reflow the for loop and the format! call so the code conforms to rustfmt (ensure
the for (...) line breaks and the return Some(format!(...)) expression is
wrapped across lines according to rustfmt style); locate the
first_difference(generated: &str, checked_in: &str) function and adjust the
iterator unpacking and the format! invocation (keeping the same content: "first
differing line {line}: generated=`{}` checked_in=`{}`", generated_line,
checked_in_line) so that cargo fmt --check passes.
- Around line 17-29: The current test only checks for presence of tokens and can
miss malformed Rhai fragments; update the assertions that use registration_defs
and runtime_defs to include lightweight sanity checks that fail on common
invalid outputs (e.g., assert!(!registration_defs.contains(">>")),
assert!(!runtime_defs.contains(">>")), and
assert!(!registration_defs.contains("-> EvalAltResult")) /
assert!(!runtime_defs.contains("-> EvalAltResult")) or use a small regex to
ensure no consecutive angle-brackets or malformed return types); place these
checks alongside the existing contains(...) assertions for registration_defs and
runtime_defs so malformed generated .rhai files fail the test early.
In `@docs/config-api/session-ref.md`:
- Around line 34-35: Replace the ambiguous return types marked as ? with
concrete Rhai-visible types: change fn id(session: SessionRef) -> ? to fn
id(session: SessionRef) -> STRING and change fn root_node(session: SessionRef)
-> ? to fn root_node(session: SessionRef) -> STRING so the signatures match the
documented descriptions; ensure both uses refer to the Rhai string type and
update any surrounding docs/examples to use string values where these functions
are called.
In `@docs/config-api/tree.md`:
- Around line 77-79: The docs line currently points users to an internal source
file ("parse_buffer_spawn in runtime.rs"); remove that file reference and
rephrase the sentence to describe the supported options without implementation
details—keep the list of supported `options` keys (`title`, `cwd`, `env`) and
mention that unknown keys are ignored, e.g., say "parse_buffer_spawn is the
source of truth" -> "the runtime validates these keys" or similar, updating the
sentence that references `parse_buffer_spawn` to a user-facing statement without
`runtime.rs`.
In `@docs/config-api/ui.md`:
- Around line 34-35: Update the documentation for the overloaded function
signatures of segment(_: UiApi, text: String) -> BarSegment and segment(_:
UiApi, text: String, options: Map) -> BarSegment so it describes both variants:
explain the default behavior/styling applied when only UiApi and text are
provided and then describe how the optional options: Map parameter can override
or extend styling/behavior (list supported keys and their effects). Reference
the function name segment, the UiApi parameter, the return type BarSegment, and
the options Map in the prose so readers can identify which overload each
sentence applies to, and mirror the same expanded wording for the second
occurrence noted in the comment (lines 47–49).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: cfacfc53-8c37-4656-bf44-bc90dd61ddb6
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (30)
.github/workflows/ci.ymlCargo.tomlcrates/embers-client/Cargo.tomlcrates/embers-client/src/bin/gen-config-docs.rscrates/embers-client/src/scripting/documentation.rscrates/embers-client/src/scripting/engine.rscrates/embers-client/src/scripting/mod.rscrates/embers-client/src/scripting/runtime.rscrates/embers-client/tests/config_api_docs.rsdocs/config-api/action.mddocs/config-api/buffer-ref.mddocs/config-api/context.mddocs/config-api/defs/registration.rhaidocs/config-api/defs/runtime.rhaidocs/config-api/event-info.mddocs/config-api/example.mddocs/config-api/floating-ref.mddocs/config-api/index.mddocs/config-api/mouse.mddocs/config-api/node-ref.mddocs/config-api/registration-globals.mddocs/config-api/runtime-theme.mddocs/config-api/session-ref.mddocs/config-api/system-runtime.mddocs/config-api/tab-bar-context.mddocs/config-api/tab-info.mddocs/config-api/tabbar.mddocs/config-api/theme.mddocs/config-api/tree.mddocs/config-api/ui.md
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
docs/config-api/tree.md (1)
105-112:⚠️ Potential issue | 🟡 MinorFenced code block missing blank line before closing div.
The static analysis (MD031) flags line 111 where the closing ``` fence is immediately followed by
</div>without an intervening blank line. This should be addressed in the markdown generator (`documentation.rs`) rather than manually editing this generated file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/config-api/tree.md` around lines 105 - 112, The generated markdown places the closing triple-backtick immediately before a subsequent </div>, triggering MD031; fix the generator in documentation.rs by ensuring the code-emission path for fenced blocks (e.g., the function handling code tabs such as render_code_block or write_fenced_code_block) writes a trailing blank line before emitting the closing fence or before emitting the enclosing </div>. Update that function to insert a single newline (blank line) between the closing ``` fence and the subsequent HTML tag so the generated markdown passes MD031.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Cargo.toml`:
- Around line 30-31: The workspace Cargo.toml currently enables rhai features
"internals" and "metadata" globally for the workspace; remove those feature
flags from the workspace-level dependency declaration for "rhai" and add a
crate-scoped dependency in crates/embers-client/Cargo.toml as rhai = { workspace
= true, features = ["internals", "metadata"] } so only the embers-client crate
(which uses rhai-autodocs) opts into those features; update the embers-client
Cargo.toml dependency and remove the features from the workspace Cargo.toml's
rhai entry and ensure rhai-autodocs remains as-is.
In `@crates/embers-client/src/scripting/documentation.rs`:
- Around line 261-278: The fence normalizer loop (iterating normalized_lines,
using is_fence, in_fence, and output) needs to also detect when a closing fence
is immediately followed by an HTML closing tag like </div> and insert a blank
line before that tag; update the logic in the block where you toggle in_fence
(the branch where is_fence sets in_fence = !in_fence and currently may push a
trailing blank) to look ahead at the next normalized_lines entry (or examine the
next input token) and, if the next line starts_with("</") or matches an HTML
closing tag pattern, push an extra "" into output so there is a blank line
between the closing ``` and the HTML tag.
In `@crates/embers-client/src/scripting/runtime.rs`:
- Line 22: The duplicate type alias ScriptResult<T> (currently defined in
runtime.rs and engine.rs) should be moved to a shared module to avoid
divergence; create a common types module (e.g., add types.rs or export from
scripting/mod.rs), declare the ScriptResult<T> alias there, and update both
runtime.rs and engine.rs to import/re-export that single definition (replace the
local aliases with use of the shared ScriptResult<T>) so the symbol is unique
and maintained in one place.
In `@docs/config-api/event-info.md`:
- Around line 8-11: The fenced code blocks showing pseudo-signatures for Rhai
APIs use Rust fences with "rust,ignore" and contain "-> ?" which is invalid
Rust; update those fences to use "rhai" so the pseudo-signatures render
correctly—specifically change the fences surrounding the fn buffer_id(event:
EventInfo) -> ? and fn session_id(event: EventInfo) -> ? blocks from
```rust,ignore``` to ```rhai``` while leaving the signature text unchanged.
In `@docs/config-api/floating-ref.md`:
- Line 10: Update the function signature for geometry to use the lowercase type
name used in the generated Rhai defs: change the return type in the signature
"geometry(floating: FloatingRef) -> Map" to "map" so it reads
"geometry(floating: FloatingRef) -> map"; keep the parameter type FloatingRef
unchanged to match the rest of the docs.
In `@docs/config-api/tab-info.md`:
- Line 10: The docs show fn buffer_count(tab: TabInfo) -> ?; change the
documented return type to a concrete integer type (e.g., -> int/usize) so Rhai
consumers see a numeric return; update the generated docs for buffer_count and
ensure the generator customization that emits buffer_count (so tab_buffer_count
usage via dynamic_usize in crates/embers-client/src/scripting/runtime.rs and the
dynamic_usize conversion at runtime) is updated too to produce the same integer
return type to avoid regen drift.
---
Duplicate comments:
In `@docs/config-api/tree.md`:
- Around line 105-112: The generated markdown places the closing triple-backtick
immediately before a subsequent </div>, triggering MD031; fix the generator in
documentation.rs by ensuring the code-emission path for fenced blocks (e.g., the
function handling code tabs such as render_code_block or
write_fenced_code_block) writes a trailing blank line before emitting the
closing fence or before emitting the enclosing </div>. Update that function to
insert a single newline (blank line) between the closing ``` fence and the
subsequent HTML tag so the generated markdown passes MD031.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9f685838-d750-4fb7-952f-af58106ea83d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (30)
.github/workflows/ci.ymlCargo.tomlcrates/embers-client/Cargo.tomlcrates/embers-client/src/bin/gen-config-docs.rscrates/embers-client/src/scripting/documentation.rscrates/embers-client/src/scripting/engine.rscrates/embers-client/src/scripting/mod.rscrates/embers-client/src/scripting/runtime.rscrates/embers-client/tests/config_api_docs.rsdocs/config-api/action.mddocs/config-api/buffer-ref.mddocs/config-api/context.mddocs/config-api/defs/registration.rhaidocs/config-api/defs/runtime.rhaidocs/config-api/event-info.mddocs/config-api/example.mddocs/config-api/floating-ref.mddocs/config-api/index.mddocs/config-api/mouse.mddocs/config-api/node-ref.mddocs/config-api/registration-globals.mddocs/config-api/runtime-theme.mddocs/config-api/session-ref.mddocs/config-api/system-runtime.mddocs/config-api/tab-bar-context.mddocs/config-api/tab-info.mddocs/config-api/tabbar.mddocs/config-api/theme.mddocs/config-api/tree.mddocs/config-api/ui.md
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
crates/embers-client/src/scripting/engine.rs (1)
57-59:⚠️ Potential issue | 🟠 MajorMake the registration receivers immutable as well.
These predeclared
tabbar,theme, andmousebindings are still mutable. Reassigning any of them during config load breaks subsequent registration calls against the same scope, so both the load-time scope and the documentation scope should usepush_constant.🔒 Minimal fix
- scope.push("tabbar", TabbarApi::new()); - scope.push("theme", ThemeApi::new()); - scope.push("mouse", MouseApi::new()); + scope.push_constant("tabbar", TabbarApi::new()); + scope.push_constant("theme", ThemeApi::new()); + scope.push_constant("mouse", MouseApi::new()); ... - scope.push("tabbar", TabbarApi::new()); - scope.push("theme", ThemeApi::new()); - scope.push("mouse", MouseApi::new()); + scope.push_constant("tabbar", TabbarApi::new()); + scope.push_constant("theme", ThemeApi::new()); + scope.push_constant("mouse", MouseApi::new());Also applies to: 835-837
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-client/src/scripting/engine.rs` around lines 57 - 59, The bindings for tabbar, theme, and mouse are currently registered as mutable via scope.push which allows reassignment and breaks later registrations; change these to constant registrations by using scope.push_constant for the predeclared APIs (TabbarApi::new, ThemeApi::new, MouseApi::new) in both the load-time scope and the documentation scope (the other similar occurrences around the same file) so the bindings are immutable and cannot be overwritten during config load.crates/embers-client/src/scripting/runtime.rs (1)
112-118:⚠️ Potential issue | 🟠 MajorRegister the built-in runtime receivers as constants.
Using
Scope::pushhere leavessystem,action,tree,ui,theme, andmuxrebindable from user scripts. One accidental assignment can clobber the receiver and break later API calls in the same evaluation. Usepush_constantin both scopes instead.🔒 Minimal fix
pub fn runtime_scope(context: Option<Context>, theme: ThemeSpec) -> Scope<'static> { let mut scope = Scope::new(); - scope.push("system", SystemApi); - scope.push("action", ActionApi); - scope.push("tree", TreeApi); - scope.push("ui", UiApi); - scope.push("theme", ThemeRuntimeApi { theme }); + scope.push_constant("system", SystemApi); + scope.push_constant("action", ActionApi); + scope.push_constant("tree", TreeApi); + scope.push_constant("ui", UiApi); + scope.push_constant("theme", ThemeRuntimeApi { theme }); if let Some(context) = context { - scope.push("mux", MuxApi::new(context)); + scope.push_constant("mux", MuxApi::new(context)); } scope } pub fn registration_scope() -> Scope<'static> { let mut scope = Scope::new(); - scope.push("system", SystemApi); - scope.push("action", ActionApi); - scope.push("tree", TreeApi); - scope.push("ui", UiApi); + scope.push_constant("system", SystemApi); + scope.push_constant("action", ActionApi); + scope.push_constant("tree", TreeApi); + scope.push_constant("ui", UiApi); scope }Also applies to: 125-128
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-client/src/scripting/runtime.rs` around lines 112 - 118, Change the registrations of the built-in runtime receivers from mutable bindings to constants by replacing scope.push calls with scope.push_constant for each registration of SystemApi, ActionApi, TreeApi, UiApi, ThemeRuntimeApi { theme }, and the conditional MuxApi::new(context); make the same push -> push_constant change in the other scope block referenced in the diff (the duplicate at the later lines) so these receiver bindings cannot be rebound by user scripts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/embers-client/src/scripting/documentation.rs`:
- Around line 192-197: The generate_config_api_docs function currently only
creates directories and writes files, leaving stale files in output_dir when
pages/assets are removed; modify generate_config_api_docs to purge the previous
contents of output_dir before regenerating (e.g., remove or empty the directory
at the start), then recreate output_dir (and defs_dir/theme_dir) so rewritten
docs reflect deletions; reference the function name generate_config_api_docs and
the output_dir/defs_dir/theme_dir variables when making this change.
In `@crates/embers-client/src/scripting/runtime.rs`:
- Around line 51-75: Both register_runtime_api and
register_documented_runtime_api are identical; collapse the duplicate path by
extracting a single helper that performs the shared work (call
register_runtime_types and register_global_module for each documented_* module)
and have both register_runtime_api and register_documented_runtime_api delegate
to that helper (or remove one and rename the other). Update references to the
helper from the existing functions so the registration of
documented_context_api, documented_ref_api, documented_action_api,
documented_tree_api, documented_mux_api, documented_system_api,
documented_ui_api, and documented_theme_runtime_api is centralized.
In `@crates/embers-protocol/schema/embers.fbs`:
- Around line 111-115: BufferStateWire's numeric assignments were shifted when
inserting Interrupted, breaking wire compatibility (Exited moved from 2 to 3);
restore explicit numeric stability by keeping Exited = 2 and give Interrupted a
new, unique value (e.g., 4) or append Interrupted at the end with an explicit
value so existing payloads decode correctly; update the enum declaration for
BufferStateWire to use explicit numeric assignments for Interrupted and Exited
(referencing BufferStateWire, Interrupted, Exited).
In `@crates/embers-server/src/persist.rs`:
- Around line 157-163: The save_workspace function currently uses fs::write
which can truncate the live file and use default (possibly world-readable)
permissions; instead, write atomically to a sibling temporary file with
owner-only permissions, sync it, then rename it into place: create a temp path
(e.g., path.with_extension("tmp") or ".partial"), open it with OpenOptions and
set mode 0o600 using std::os::unix::fs::OpenOptionsExt on Unix, write the
serde_json bytes (from state.to_persisted()), call file.sync_all(), rename temp
into the original path with fs::rename, and finally sync the parent directory
(open parent dir and call sync_all()) to ensure the rename is durable; keep
error mapping to MuxError as before and ensure this logic lives inside
save_workspace to replace the current fs::write usage.
- Around line 442-443: Change the timestamp_from_millis function to return
Result<Timestamp, MuxError> (not plain Timestamp); inside timestamp_from_millis
use UNIX_EPOCH.checked_add(Duration::from_millis(millis)) and map None to an
appropriate MuxError load variant (e.g., MuxError::LoadError or similar) so
overflow is propagated instead of causing a panic; update call sites of
timestamp_from_millis to handle the Result and propagate or convert the error
into the existing load error flow.
In `@crates/embers-server/src/state.rs`:
- Around line 84-87: The deserialized seeds (workspace.next_session_id,
next_buffer_id, next_node_id, next_floating_id) are used directly to construct
IdAllocator via IdAllocator::new and can be <= the restored max ID, risking ID
reuse; update the load logic to validate or clamp each seed: compute the safe
minimum with the existing helper (next_id_after_max(max_id) or equivalent) for
each corresponding map (sessions, buffers, nodes, floatings) and if the
serialized seed is below that value either set the seed to
next_id_after_max(...) or return an error to fail loading; apply this check for
session_ids, buffer_ids, node_ids, and floating_ids before calling
IdAllocator::new so no allocator is initialized with a seed inside the restored
ID space.
In `@crates/embers-server/tests/persistence.rs`:
- Around line 72-112: The test currently never proves the "attached" buffer
reached Running before shutdown; update the test (around the
BufferRequest::Create for the attached buffer and before the shutdown/restore
steps) to assert that the attached buffer transitions to Running—e.g., call the
existing helper that fetches buffer state (or use request_buffer to query
status) and assert state == Running for attached_id—or alternatively add a
focused unit test that exercises interrupt_unrecoverable_buffers() directly to
verify a Running->Interrupted transition; reference request_buffer,
BufferRequest::Create, attached_id, and interrupt_unrecoverable_buffers() to
locate where to add the check.
---
Duplicate comments:
In `@crates/embers-client/src/scripting/engine.rs`:
- Around line 57-59: The bindings for tabbar, theme, and mouse are currently
registered as mutable via scope.push which allows reassignment and breaks later
registrations; change these to constant registrations by using
scope.push_constant for the predeclared APIs (TabbarApi::new, ThemeApi::new,
MouseApi::new) in both the load-time scope and the documentation scope (the
other similar occurrences around the same file) so the bindings are immutable
and cannot be overwritten during config load.
In `@crates/embers-client/src/scripting/runtime.rs`:
- Around line 112-118: Change the registrations of the built-in runtime
receivers from mutable bindings to constants by replacing scope.push calls with
scope.push_constant for each registration of SystemApi, ActionApi, TreeApi,
UiApi, ThemeRuntimeApi { theme }, and the conditional MuxApi::new(context); make
the same push -> push_constant change in the other scope block referenced in the
diff (the duplicate at the later lines) so these receiver bindings cannot be
rebound by user scripts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0002e11e-847a-495a-999f-400cfb76cc53
⛔ Files ignored due to path filters (17)
Cargo.lockis excluded by!**/*.lockdocs/config-api-book/clipboard-1626706a.min.jsis excluded by!**/*.min.jsdocs/config-api-book/elasticlunr-ef4e11c1.min.jsis excluded by!**/*.min.jsdocs/config-api-book/favicon-8114d1fc.pngis excluded by!**/*.pngdocs/config-api-book/favicon-de23e50b.svgis excluded by!**/*.svgdocs/config-api-book/fonts/open-sans-v17-all-charsets-300-7736aa35.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-300italic-2c7b95c0.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-600-486c6759.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-600italic-1a3e8659.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-700-c22fe8c7.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-700italic-238ae959.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-800-3d2c812a.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-800italic-ba1521ec.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-italic-6c9463f7.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-regular-2e3b1d34.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/source-code-pro-v11-all-charsets-500-2bdd9410.woff2is excluded by!**/*.woff2docs/config-api-book/mark-09e88c2c.min.jsis excluded by!**/*.min.js
📒 Files selected for processing (109)
.cargo/config.toml.github/workflows/ci.ymlCargo.tomlcrates/embers-cli/tests/interactive.rscrates/embers-client/Cargo.tomlcrates/embers-client/src/bin/gen-docs.rscrates/embers-client/src/configured_client.rscrates/embers-client/src/scripting/documentation.rscrates/embers-client/src/scripting/engine.rscrates/embers-client/src/scripting/mod.rscrates/embers-client/src/scripting/model.rscrates/embers-client/src/scripting/runtime.rscrates/embers-client/tests/config_api_docs.rscrates/embers-client/tests/fixtures/repository_config.rhaicrates/embers-client/tests/script_actions.rscrates/embers-core/Cargo.tomlcrates/embers-core/src/geometry.rscrates/embers-protocol/schema/embers.fbscrates/embers-protocol/src/codec.rscrates/embers-protocol/src/types.rscrates/embers-server/Cargo.tomlcrates/embers-server/src/config.rscrates/embers-server/src/lib.rscrates/embers-server/src/model.rscrates/embers-server/src/persist.rscrates/embers-server/src/protocol.rscrates/embers-server/src/server.rscrates/embers-server/src/state.rscrates/embers-server/tests/persistence.rsdocs/config-api-book/.nojekylldocs/config-api-book/404.htmldocs/config-api-book/action.htmldocs/config-api-book/ayu-highlight-3fdfc3ac.cssdocs/config-api-book/book-a0b12cfe.jsdocs/config-api-book/book.tomldocs/config-api-book/buffer-ref.htmldocs/config-api-book/context.htmldocs/config-api-book/css/chrome-ae938929.cssdocs/config-api-book/css/general-2459343d.cssdocs/config-api-book/css/print-9e4910d8.cssdocs/config-api-book/css/variables-8adf115d.cssdocs/config-api-book/defs/registration.rhaidocs/config-api-book/defs/runtime.rhaidocs/config-api-book/event-info.htmldocs/config-api-book/example.htmldocs/config-api-book/floating-ref.htmldocs/config-api-book/fonts/OPEN-SANS-LICENSE.txtdocs/config-api-book/fonts/SOURCE-CODE-PRO-LICENSE.txtdocs/config-api-book/fonts/fonts-9644e21d.cssdocs/config-api-book/highlight-493f70e1.cssdocs/config-api-book/highlight-abc7f01d.jsdocs/config-api-book/index.htmldocs/config-api-book/mouse.htmldocs/config-api-book/mux.htmldocs/config-api-book/node-ref.htmldocs/config-api-book/print.htmldocs/config-api-book/registration-action.htmldocs/config-api-book/registration-globals.htmldocs/config-api-book/registration-system.htmldocs/config-api-book/registration-tree.htmldocs/config-api-book/registration-ui.htmldocs/config-api-book/runtime-theme.htmldocs/config-api-book/searcher-c2a407aa.jsdocs/config-api-book/searchindex-329edd6d.jsdocs/config-api-book/session-ref.htmldocs/config-api-book/system-runtime.htmldocs/config-api-book/tab-bar-context.htmldocs/config-api-book/tab-info.htmldocs/config-api-book/tabbar.htmldocs/config-api-book/theme.htmldocs/config-api-book/theme/rhai-autodocs-tabs-dbd60081.jsdocs/config-api-book/theme/rhai-autodocs-tabs.jsdocs/config-api-book/theme/rhai-highlight-8baa11ea.jsdocs/config-api-book/theme/rhai-highlight.jsdocs/config-api-book/toc-c8603eba.jsdocs/config-api-book/toc.htmldocs/config-api-book/tomorrow-night-4c0ae647.cssdocs/config-api-book/tree.htmldocs/config-api-book/ui.htmldocs/config-api/SUMMARY.mddocs/config-api/action.mddocs/config-api/book.tomldocs/config-api/buffer-ref.mddocs/config-api/context.mddocs/config-api/defs/registration.rhaidocs/config-api/defs/runtime.rhaidocs/config-api/event-info.mddocs/config-api/example.mddocs/config-api/floating-ref.mddocs/config-api/index.mddocs/config-api/mouse.mddocs/config-api/mux.mddocs/config-api/node-ref.mddocs/config-api/registration-action.mddocs/config-api/registration-globals.mddocs/config-api/registration-system.mddocs/config-api/registration-tree.mddocs/config-api/registration-ui.mddocs/config-api/runtime-theme.mddocs/config-api/session-ref.mddocs/config-api/system-runtime.mddocs/config-api/tab-bar-context.mddocs/config-api/tab-info.mddocs/config-api/tabbar.mddocs/config-api/theme.mddocs/config-api/theme/rhai-autodocs-tabs.jsdocs/config-api/theme/rhai-highlight.jsdocs/config-api/tree.mddocs/config-api/ui.md
💤 Files with no reviewable changes (4)
- crates/embers-client/tests/fixtures/repository_config.rhai
- crates/embers-client/src/configured_client.rs
- crates/embers-client/tests/script_actions.rs
- crates/embers-client/src/scripting/model.rs
Owner
Author
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/embers-client/src/scripting/runtime.rs (2)
1731-1746:⚠️ Potential issue | 🟠 MajorReject unknown option keys here instead of silently falling back.
Both parsers remove the supported keys but never check whether anything is left. Typos like
#{ focsu: false }or#{ targte: ... }currently succeed and then use defaults, which makes config mistakes very hard to spot.🛠️ Minimal guard
fn parse_floating_options(mut options: Map) -> ScriptResult<ParsedFloatingOptions> { - Ok(ParsedFloatingOptions { + let parsed = ParsedFloatingOptions { geometry: FloatingGeometrySpec { width: parse_floating_size(options.remove("width"))? .unwrap_or(FloatingSize::Percent(50)), height: parse_floating_size(options.remove("height"))? .unwrap_or(FloatingSize::Percent(50)), anchor: parse_floating_anchor(options.remove("anchor"))? .unwrap_or(FloatingAnchor::Center), offset_x: parse_i16_field(options.remove("x"), "x")?.unwrap_or(0), offset_y: parse_i16_field(options.remove("y"), "y")?.unwrap_or(0), }, title: parse_optional_string(options.remove("title"))?, focus: parse_bool_field(options.remove("focus"))?.unwrap_or(true), close_on_empty: parse_bool_field(options.remove("close_on_empty"))?.unwrap_or(true), - }) + }; + if !options.is_empty() { + let unknown = options + .keys() + .map(|key| key.to_string()) + .collect::<Vec<_>>() + .join(", "); + return Err(runtime_error(format!("unknown floating option(s): {unknown}"))); + } + Ok(parsed) } fn parse_segment_options(mut options: Map) -> ScriptResult<(StyleSpec, Option<BarTarget>)> { - Ok(( + let parsed = ( StyleSpec { fg: parse_optional_color(options.remove("fg"))?, bg: parse_optional_color(options.remove("bg"))?, bold: parse_bool_field(options.remove("bold"))?.unwrap_or(false), italic: parse_bool_field(options.remove("italic"))?.unwrap_or(false), underline: parse_bool_field(options.remove("underline"))?.unwrap_or(false), dim: parse_bool_field(options.remove("dim"))?.unwrap_or(false), }, parse_bar_target(options.remove("target"))?, - )) + ); + if !options.is_empty() { + let unknown = options + .keys() + .map(|key| key.to_string()) + .collect::<Vec<_>>() + .join(", "); + return Err(runtime_error(format!("unknown ui.segment option(s): {unknown}"))); + } + Ok(parsed) }Also applies to: 1749-1760
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-client/src/scripting/runtime.rs` around lines 1731 - 1746, The parsers (e.g., parse_floating_options) currently strip known keys but never validate there are no unknown keys left, so typos silently fall back to defaults; update parse_floating_options (and the similar parser around lines 1749-1760) to check the remaining entries in the options Map after calling parse_floating_size, parse_floating_anchor, parse_i16_field, parse_optional_string, and parse_bool_field and return a ScriptResult::Err listing the unexpected key names if the Map is non-empty; include the unexpected keys in the error message for easier debugging and use the same pattern for the other options parser mentioned.
2113-2115:⚠️ Potential issue | 🟡 MinorThread call position through validation helpers in runtime.rs.
All validation failures in this module funnel through
runtime_error, which hard-codesPosition::NONE. This means validation errors insend_keys, ID parsing, floating options, and other runtime functions report without the offending call site. The registration API inengine.rsalready demonstrates the pattern:runtime_error(message, ctx.call_position()). Modify thereturn_rawfunctions inruntime.rsto acceptNativeCallContextand passcall_position()through toruntime_errorand its validation helpers, similar to howset_leader,define_mode,bind, and other registration functions handle it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-client/src/scripting/runtime.rs` around lines 2113 - 2115, The runtime's validation errors always use Position::NONE because runtime_error currently takes only a message; update runtime_error to accept a call position (rhai::Position) or overload it, then change each return_raw function in runtime.rs to accept a &NativeCallContext (or NativeCallContext) and call ctx.call_position(), passing that position into runtime_error and all validation helper calls; follow the pattern used in engine.rs registration helpers (e.g. set_leader, define_mode, bind) so send_keys, ID parsing, floating options, and other runtime validations report the actual call site via call_position().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/embers-server/src/config.rs`:
- Around line 21-24: The workspace_path variable is a stable on-disk path for
session metadata; update the persistence writer that writes to workspace_path so
it creates/updates the file with restrictive user-only permissions (e.g., 0o600)
and performs an atomic write (write to a temp file in the same directory, set
permissions on the temp file, fsync the file and directory, then rename into
place). Ensure the writer uses workspace_path (the value from
with_extension("workspace.json")) and handles errors from permission-setting and
fsync properly.
In `@crates/embers-server/src/persist.rs`:
- Around line 166-180: The directory-sync call after fs::rename is not portable
and fails on Windows; remove or guard the code that opens the parent directory
and calls sync_all() (the block that uses
OpenOptions::new().read(true).open(parent)?.sync_all()?) so the atomic write
works cross-platform. Replace that line with either nothing or wrap the entire
parent-directory open+sync in a Unix-only cfg (#[cfg(unix)]) so temp_path,
fs::rename(...), and the atomic-write logic remain unchanged on Windows while
preserving the directory fsync on Unix.
In `@crates/embers-server/src/server.rs`:
- Around line 108-112: Quiesce the buffer runtimes before persisting and
propagate any save error: call runtime.shutdown_runtimes().await before
runtime.persist_workspace().await (so callbacks from spawn_buffer_runtime()
cannot mutate ServerState during the save), and instead of just logging the
persist error in shutdown code, return it (use ? or map_err to propagate) from
ServerHandle::shutdown(); keep logging contextual info but do not swallow the
error.
- Around line 52-60: Move the call to load_workspace(...) to occur before
creating the UnixListener::bind(...) so that any fallible restore happens before
the socket file is created; specifically, call
load_workspace(&self.config.workspace_path) and handle its error/unwrap before
invoking UnixListener::bind(&self.config.socket_path) and
set_socket_permissions(...). This ensures we don't leave a stale socket pathname
behind when load_workspace fails and avoids relying on SocketCleanup (which is
only armed inside the spawned task) to remove the socket.
In `@crates/embers-server/src/state.rs`:
- Around line 2066-2068: The helper next_id_after_max should not use
saturating_add because that masks allocator exhaustion; change it to use
checked_add and fail loudly when the max is u64::MAX (e.g. replace
ids.max().unwrap_or(0).saturating_add(1) with ids.max().and_then(|m|
m.checked_add(1)).unwrap_or_else(|| panic!("ID allocator exhausted: restored max
id == u64::MAX, cannot allocate a new id")) or alternatively return a
Result/Option up the call chain; ensure the failure message mentions
next_id_after_max and allocator exhaustion so it's obvious why allocation cannot
proceed.
- Around line 62-89: When rebuilding the persisted collections (sessions,
buffers, nodes, floating) you must reject duplicate IDs instead of allowing
BTreeMap to silently overwrite; in the blocks that process persisted_sessions,
persisted_buffers, persisted_nodes, and persisted_floating (wrapping
restored_session/restored_buffer/restored_node/restored_floating), check whether
the map already contains the key before inserting and return an Err with a clear
message if a duplicate ID is found so the load fails fast; implement this same
uniqueness check for buffers, nodes, and floating just like sessions so
duplicates are detected and propagated through the existing Result error flow.
---
Outside diff comments:
In `@crates/embers-client/src/scripting/runtime.rs`:
- Around line 1731-1746: The parsers (e.g., parse_floating_options) currently
strip known keys but never validate there are no unknown keys left, so typos
silently fall back to defaults; update parse_floating_options (and the similar
parser around lines 1749-1760) to check the remaining entries in the options Map
after calling parse_floating_size, parse_floating_anchor, parse_i16_field,
parse_optional_string, and parse_bool_field and return a ScriptResult::Err
listing the unexpected key names if the Map is non-empty; include the unexpected
keys in the error message for easier debugging and use the same pattern for the
other options parser mentioned.
- Around line 2113-2115: The runtime's validation errors always use
Position::NONE because runtime_error currently takes only a message; update
runtime_error to accept a call position (rhai::Position) or overload it, then
change each return_raw function in runtime.rs to accept a &NativeCallContext (or
NativeCallContext) and call ctx.call_position(), passing that position into
runtime_error and all validation helper calls; follow the pattern used in
engine.rs registration helpers (e.g. set_leader, define_mode, bind) so
send_keys, ID parsing, floating options, and other runtime validations report
the actual call site via call_position().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5b76b232-997b-4001-a7ef-b58cb4d8b31d
⛔ Files ignored due to path filters (17)
Cargo.lockis excluded by!**/*.lockdocs/config-api-book/clipboard-1626706a.min.jsis excluded by!**/*.min.jsdocs/config-api-book/elasticlunr-ef4e11c1.min.jsis excluded by!**/*.min.jsdocs/config-api-book/favicon-8114d1fc.pngis excluded by!**/*.pngdocs/config-api-book/favicon-de23e50b.svgis excluded by!**/*.svgdocs/config-api-book/fonts/open-sans-v17-all-charsets-300-7736aa35.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-300italic-2c7b95c0.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-600-486c6759.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-600italic-1a3e8659.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-700-c22fe8c7.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-700italic-238ae959.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-800-3d2c812a.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-800italic-ba1521ec.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-italic-6c9463f7.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-regular-2e3b1d34.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/source-code-pro-v11-all-charsets-500-2bdd9410.woff2is excluded by!**/*.woff2docs/config-api-book/mark-09e88c2c.min.jsis excluded by!**/*.min.js
📒 Files selected for processing (109)
.cargo/config.toml.github/workflows/ci.ymlCargo.tomlcrates/embers-cli/tests/interactive.rscrates/embers-client/Cargo.tomlcrates/embers-client/src/bin/gen-docs.rscrates/embers-client/src/configured_client.rscrates/embers-client/src/scripting/documentation.rscrates/embers-client/src/scripting/engine.rscrates/embers-client/src/scripting/mod.rscrates/embers-client/src/scripting/model.rscrates/embers-client/src/scripting/runtime.rscrates/embers-client/tests/config_api_docs.rscrates/embers-client/tests/fixtures/repository_config.rhaicrates/embers-client/tests/script_actions.rscrates/embers-core/Cargo.tomlcrates/embers-core/src/geometry.rscrates/embers-protocol/schema/embers.fbscrates/embers-protocol/src/codec.rscrates/embers-protocol/src/types.rscrates/embers-server/Cargo.tomlcrates/embers-server/src/config.rscrates/embers-server/src/lib.rscrates/embers-server/src/model.rscrates/embers-server/src/persist.rscrates/embers-server/src/protocol.rscrates/embers-server/src/server.rscrates/embers-server/src/state.rscrates/embers-server/tests/persistence.rsdocs/config-api-book/.nojekylldocs/config-api-book/404.htmldocs/config-api-book/action.htmldocs/config-api-book/ayu-highlight-3fdfc3ac.cssdocs/config-api-book/book-a0b12cfe.jsdocs/config-api-book/book.tomldocs/config-api-book/buffer-ref.htmldocs/config-api-book/context.htmldocs/config-api-book/css/chrome-ae938929.cssdocs/config-api-book/css/general-2459343d.cssdocs/config-api-book/css/print-9e4910d8.cssdocs/config-api-book/css/variables-8adf115d.cssdocs/config-api-book/defs/registration.rhaidocs/config-api-book/defs/runtime.rhaidocs/config-api-book/event-info.htmldocs/config-api-book/example.htmldocs/config-api-book/floating-ref.htmldocs/config-api-book/fonts/OPEN-SANS-LICENSE.txtdocs/config-api-book/fonts/SOURCE-CODE-PRO-LICENSE.txtdocs/config-api-book/fonts/fonts-9644e21d.cssdocs/config-api-book/highlight-493f70e1.cssdocs/config-api-book/highlight-abc7f01d.jsdocs/config-api-book/index.htmldocs/config-api-book/mouse.htmldocs/config-api-book/mux.htmldocs/config-api-book/node-ref.htmldocs/config-api-book/print.htmldocs/config-api-book/registration-action.htmldocs/config-api-book/registration-globals.htmldocs/config-api-book/registration-system.htmldocs/config-api-book/registration-tree.htmldocs/config-api-book/registration-ui.htmldocs/config-api-book/runtime-theme.htmldocs/config-api-book/searcher-c2a407aa.jsdocs/config-api-book/searchindex-329edd6d.jsdocs/config-api-book/session-ref.htmldocs/config-api-book/system-runtime.htmldocs/config-api-book/tab-bar-context.htmldocs/config-api-book/tab-info.htmldocs/config-api-book/tabbar.htmldocs/config-api-book/theme.htmldocs/config-api-book/theme/rhai-autodocs-tabs-dbd60081.jsdocs/config-api-book/theme/rhai-autodocs-tabs.jsdocs/config-api-book/theme/rhai-highlight-8baa11ea.jsdocs/config-api-book/theme/rhai-highlight.jsdocs/config-api-book/toc-c8603eba.jsdocs/config-api-book/toc.htmldocs/config-api-book/tomorrow-night-4c0ae647.cssdocs/config-api-book/tree.htmldocs/config-api-book/ui.htmldocs/config-api/SUMMARY.mddocs/config-api/action.mddocs/config-api/book.tomldocs/config-api/buffer-ref.mddocs/config-api/context.mddocs/config-api/defs/registration.rhaidocs/config-api/defs/runtime.rhaidocs/config-api/event-info.mddocs/config-api/example.mddocs/config-api/floating-ref.mddocs/config-api/index.mddocs/config-api/mouse.mddocs/config-api/mux.mddocs/config-api/node-ref.mddocs/config-api/registration-action.mddocs/config-api/registration-globals.mddocs/config-api/registration-system.mddocs/config-api/registration-tree.mddocs/config-api/registration-ui.mddocs/config-api/runtime-theme.mddocs/config-api/session-ref.mddocs/config-api/system-runtime.mddocs/config-api/tab-bar-context.mddocs/config-api/tab-info.mddocs/config-api/tabbar.mddocs/config-api/theme.mddocs/config-api/theme/rhai-autodocs-tabs.jsdocs/config-api/theme/rhai-highlight.jsdocs/config-api/tree.mddocs/config-api/ui.md
💤 Files with no reviewable changes (4)
- crates/embers-client/tests/fixtures/repository_config.rhai
- crates/embers-client/src/configured_client.rs
- crates/embers-client/tests/script_actions.rs
- crates/embers-client/src/scripting/model.rs
Comment on lines
+21
to
+24
| let workspace_path = socket_path.with_extension("workspace.json"); | ||
| Self { | ||
| socket_path, | ||
| workspace_path, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Harden persisted workspace file access at write time.
Given Line 21 introduces a stable on-disk workspace path, ensure the persistence writer creates/updates this file with restrictive permissions (for example, user-only read/write) to avoid leaking session metadata.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/embers-server/src/config.rs` around lines 21 - 24, The workspace_path
variable is a stable on-disk path for session metadata; update the persistence
writer that writes to workspace_path so it creates/updates the file with
restrictive user-only permissions (e.g., 0o600) and performs an atomic write
(write to a temp file in the same directory, set permissions on the temp file,
fsync the file and directory, then rename into place). Ensure the writer uses
workspace_path (the value from with_extension("workspace.json")) and handles
errors from permission-setting and fsync properly.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
crates/embers-server/src/server.rs (1)
108-112:⚠️ Potential issue | 🟠 MajorQuiesce all state-mutating tasks before persisting.
shutdown_runtimes()only stops the PTYs. The spawnedhandle_connection()tasks are still live, andspawn_buffer_runtime()also detachesrecord_buffer_output/record_buffer_exitwithHandle::spawn, so latelast_snapshot_seq/ title / buffer-state updates can still land after this save starts and even afterServerHandle::shutdown()resolves. Track those tasks and wait for them before persisting and returning.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-server/src/server.rs` around lines 108 - 112, shutdown_runtimes() currently only stops PTYs but doesn't await spawned connection/buffer tasks (e.g., tasks from handle_connection, spawn_buffer_runtime which detaches record_buffer_output/record_buffer_exit), so persist_workspace() can run while state-mutating tasks are still running; fix by tracking and awaiting those JoinHandles before calling runtime.persist_workspace(): update the runtime/ServerHandle to store handles for handle_connection and spawn_buffer_runtime (or convert Handle::spawn detach to spawn returning JoinHandle), expose a method like quiesce_state_tasks() or extend shutdown_runtimes() to join/await all recorded handles (ensuring last_snapshot_seq/title/buffer updates are flushed), then call persist_workspace() and only return from ServerHandle::shutdown() after those awaits complete.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/embers-client/src/scripting/engine.rs`:
- Around line 56-59: The same scope population (push of "tabbar", "theme",
"mouse") is duplicated between registration_scope() and
documentation_registration_scope(); extract that shared setup into a single
initializer function (e.g., populate_common_registration_scope or
init_shared_registration_items) that takes a &mut Scope (or the concrete scope
type) and calls scope.push("tabbar", TabbarApi::new()), scope.push("theme",
ThemeApi::new()), and scope.push("mouse", MouseApi::new()), then call this
initializer from both registration_scope() and
documentation_registration_scope() to avoid drift.
In `@crates/embers-client/src/scripting/runtime.rs`:
- Around line 1628-1630: parse_buffer_spawn currently ignores unknown option
keys (unlike parse_floating_options and parse_segment_options) which lets typos
silently pass; update parse_buffer_spawn to validate and reject leftover keys by
detecting any unconsumed keys after parsing and returning an error (or
ScriptError) with a clear message listing the unknown keys so callers fail fast,
mirroring the behavior implemented in parse_floating_options and
parse_segment_options; apply the same change to the other occurrence around
lines 1942-1948 that parses buffer spawn options.
- Around line 2371-2376: The helper with_call_position currently rebuilds errors
from build() via error.to_string() which loses the original EvalAltResult
variant; instead, preserve and attach the call position to the existing error
returned by build() — in with_call_position, map_err should take the original
error value (the EvalAltResult/boxed error) and set or attach the position from
ctx.call_position() (e.g., call a method to set position on the EvalAltResult or
wrap it preserving type) rather than calling runtime_error_at(error.to_string(),
position); this keeps the original error variant (used by send_keys* paths) and
avoids double-wrapping messages.
In `@crates/embers-server/src/persist.rs`:
- Around line 166-176: The temp-file open sequence using
OpenOptions::create(true).truncate(true).write(true) with #[cfg(unix)]
options.mode(0o600) can preserve stale permissions if a temp file already
exists; change the logic around temp_path so you either use create_new(true) on
OpenOptions (and handle EEXIST by retrying with a new temp name) or explicitly
remove any existing temp_path before opening to guarantee mode(0o600) is
applied; also ensure you validate that temp_path is not a symlink before
performing the final atomic rename to avoid clobbering unexpected targets (refer
to temp_path, OpenOptions, mode(0o600), file.write_all and file.sync_all in this
block).
In `@crates/embers-server/src/state.rs`:
- Around line 108-118: The local binding for state is immutable but you call the
mutable method interrupt_unrecoverable_buffers() on it; change the binding to be
mutable (e.g., let mut state = Self { ... }) so that
state.interrupt_unrecoverable_buffers() can take &mut self; ensure the rest of
the initialization (sessions, buffers, nodes, floating, session_ids, buffer_ids,
node_ids, floating_ids / IdAllocator usage) remains unchanged.
In `@crates/embers-server/tests/persistence.rs`:
- Around line 86-97: The test currently doesn't prove the Created->Interrupted
restore path because the detached buffer (created via request_buffer with
BufferRequest::Create and RequestId(3) assigned to variable detached) might have
been spawned; update the test to explicitly assert the detached buffer's state
is BufferState::Created before simulating shutdown (or add a focused unit test
for interrupt_unrecoverable_buffers()) so the code path in
interrupt_unrecoverable_buffers() that handles BufferState::Created is
exercised; locate the buffer by the RequestId(3)/detached variable and check its
persisted/runtime state is Created prior to restart to guarantee the
Created->Interrupted transition is tested.
---
Duplicate comments:
In `@crates/embers-server/src/server.rs`:
- Around line 108-112: shutdown_runtimes() currently only stops PTYs but doesn't
await spawned connection/buffer tasks (e.g., tasks from handle_connection,
spawn_buffer_runtime which detaches record_buffer_output/record_buffer_exit), so
persist_workspace() can run while state-mutating tasks are still running; fix by
tracking and awaiting those JoinHandles before calling
runtime.persist_workspace(): update the runtime/ServerHandle to store handles
for handle_connection and spawn_buffer_runtime (or convert Handle::spawn detach
to spawn returning JoinHandle), expose a method like quiesce_state_tasks() or
extend shutdown_runtimes() to join/await all recorded handles (ensuring
last_snapshot_seq/title/buffer updates are flushed), then call
persist_workspace() and only return from ServerHandle::shutdown() after those
awaits complete.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b2659560-0c00-404b-8fb1-61bfb5f54c47
⛔ Files ignored due to path filters (17)
Cargo.lockis excluded by!**/*.lockdocs/config-api-book/clipboard-1626706a.min.jsis excluded by!**/*.min.jsdocs/config-api-book/elasticlunr-ef4e11c1.min.jsis excluded by!**/*.min.jsdocs/config-api-book/favicon-8114d1fc.pngis excluded by!**/*.pngdocs/config-api-book/favicon-de23e50b.svgis excluded by!**/*.svgdocs/config-api-book/fonts/open-sans-v17-all-charsets-300-7736aa35.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-300italic-2c7b95c0.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-600-486c6759.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-600italic-1a3e8659.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-700-c22fe8c7.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-700italic-238ae959.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-800-3d2c812a.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-800italic-ba1521ec.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-italic-6c9463f7.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-regular-2e3b1d34.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/source-code-pro-v11-all-charsets-500-2bdd9410.woff2is excluded by!**/*.woff2docs/config-api-book/mark-09e88c2c.min.jsis excluded by!**/*.min.js
📒 Files selected for processing (109)
.cargo/config.toml.github/workflows/ci.ymlCargo.tomlcrates/embers-cli/tests/interactive.rscrates/embers-client/Cargo.tomlcrates/embers-client/src/bin/gen-docs.rscrates/embers-client/src/configured_client.rscrates/embers-client/src/scripting/documentation.rscrates/embers-client/src/scripting/engine.rscrates/embers-client/src/scripting/mod.rscrates/embers-client/src/scripting/model.rscrates/embers-client/src/scripting/runtime.rscrates/embers-client/tests/config_api_docs.rscrates/embers-client/tests/fixtures/repository_config.rhaicrates/embers-client/tests/script_actions.rscrates/embers-core/Cargo.tomlcrates/embers-core/src/geometry.rscrates/embers-protocol/schema/embers.fbscrates/embers-protocol/src/codec.rscrates/embers-protocol/src/types.rscrates/embers-server/Cargo.tomlcrates/embers-server/src/config.rscrates/embers-server/src/lib.rscrates/embers-server/src/model.rscrates/embers-server/src/persist.rscrates/embers-server/src/protocol.rscrates/embers-server/src/server.rscrates/embers-server/src/state.rscrates/embers-server/tests/persistence.rsdocs/config-api-book/.nojekylldocs/config-api-book/404.htmldocs/config-api-book/action.htmldocs/config-api-book/ayu-highlight-3fdfc3ac.cssdocs/config-api-book/book-a0b12cfe.jsdocs/config-api-book/book.tomldocs/config-api-book/buffer-ref.htmldocs/config-api-book/context.htmldocs/config-api-book/css/chrome-ae938929.cssdocs/config-api-book/css/general-2459343d.cssdocs/config-api-book/css/print-9e4910d8.cssdocs/config-api-book/css/variables-8adf115d.cssdocs/config-api-book/defs/registration.rhaidocs/config-api-book/defs/runtime.rhaidocs/config-api-book/event-info.htmldocs/config-api-book/example.htmldocs/config-api-book/floating-ref.htmldocs/config-api-book/fonts/OPEN-SANS-LICENSE.txtdocs/config-api-book/fonts/SOURCE-CODE-PRO-LICENSE.txtdocs/config-api-book/fonts/fonts-9644e21d.cssdocs/config-api-book/highlight-493f70e1.cssdocs/config-api-book/highlight-abc7f01d.jsdocs/config-api-book/index.htmldocs/config-api-book/mouse.htmldocs/config-api-book/mux.htmldocs/config-api-book/node-ref.htmldocs/config-api-book/print.htmldocs/config-api-book/registration-action.htmldocs/config-api-book/registration-globals.htmldocs/config-api-book/registration-system.htmldocs/config-api-book/registration-tree.htmldocs/config-api-book/registration-ui.htmldocs/config-api-book/runtime-theme.htmldocs/config-api-book/searcher-c2a407aa.jsdocs/config-api-book/searchindex-329edd6d.jsdocs/config-api-book/session-ref.htmldocs/config-api-book/system-runtime.htmldocs/config-api-book/tab-bar-context.htmldocs/config-api-book/tab-info.htmldocs/config-api-book/tabbar.htmldocs/config-api-book/theme.htmldocs/config-api-book/theme/rhai-autodocs-tabs-dbd60081.jsdocs/config-api-book/theme/rhai-autodocs-tabs.jsdocs/config-api-book/theme/rhai-highlight-8baa11ea.jsdocs/config-api-book/theme/rhai-highlight.jsdocs/config-api-book/toc-c8603eba.jsdocs/config-api-book/toc.htmldocs/config-api-book/tomorrow-night-4c0ae647.cssdocs/config-api-book/tree.htmldocs/config-api-book/ui.htmldocs/config-api/SUMMARY.mddocs/config-api/action.mddocs/config-api/book.tomldocs/config-api/buffer-ref.mddocs/config-api/context.mddocs/config-api/defs/registration.rhaidocs/config-api/defs/runtime.rhaidocs/config-api/event-info.mddocs/config-api/example.mddocs/config-api/floating-ref.mddocs/config-api/index.mddocs/config-api/mouse.mddocs/config-api/mux.mddocs/config-api/node-ref.mddocs/config-api/registration-action.mddocs/config-api/registration-globals.mddocs/config-api/registration-system.mddocs/config-api/registration-tree.mddocs/config-api/registration-ui.mddocs/config-api/runtime-theme.mddocs/config-api/session-ref.mddocs/config-api/system-runtime.mddocs/config-api/tab-bar-context.mddocs/config-api/tab-info.mddocs/config-api/tabbar.mddocs/config-api/theme.mddocs/config-api/theme/rhai-autodocs-tabs.jsdocs/config-api/theme/rhai-highlight.jsdocs/config-api/tree.mddocs/config-api/ui.md
💤 Files with no reviewable changes (4)
- crates/embers-client/src/configured_client.rs
- crates/embers-client/tests/script_actions.rs
- crates/embers-client/tests/fixtures/repository_config.rhai
- crates/embers-client/src/scripting/model.rs
Comment on lines
+86
to
+97
| let detached = request_buffer( | ||
| &mut client, | ||
| BufferRequest::Create { | ||
| request_id: RequestId(3), | ||
| title: Some("detached".to_owned()), | ||
| command: vec!["/bin/sh".to_owned()], | ||
| cwd: None, | ||
| env: Default::default(), | ||
| }, | ||
| ) | ||
| .await | ||
| .buffer; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
The Created -> Interrupted restore path is still unproven here.
Only the attached buffer is asserted to reach Running before shutdown. The detached buffer may also have spawned, so this test can still pass without ever exercising the BufferState::Created branch in interrupt_unrecoverable_buffers(). Either assert the detached buffer stays Created pre-restart or add a focused unit test around the helper.
Also applies to: 115-136
🧰 Tools
🪛 GitHub Actions: CI
[error] Build failed when compiling embers-server (lib and lib test) due to 1 previous error.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/embers-server/tests/persistence.rs` around lines 86 - 97, The test
currently doesn't prove the Created->Interrupted restore path because the
detached buffer (created via request_buffer with BufferRequest::Create and
RequestId(3) assigned to variable detached) might have been spawned; update the
test to explicitly assert the detached buffer's state is BufferState::Created
before simulating shutdown (or add a focused unit test for
interrupt_unrecoverable_buffers()) so the code path in
interrupt_unrecoverable_buffers() that handles BufferState::Created is
exercised; locate the buffer by the RequestId(3)/detached variable and check its
persisted/runtime state is Created prior to restart to guarantee the
Created->Interrupted transition is tested.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/embers-client/src/scripting/runtime.rs (1)
2027-2050:⚠️ Potential issue | 🟡 MinorReject unexpected fields inside
targetmaps too.This parser still drops leftover keys after consuming
kindand the variant-specific fields. That preserves the same typo footgun you just fixed forbuffer_spawn,floating, andsegmentoptions.🛠️ Suggested fix
fn parse_bar_target(value: Option<Dynamic>) -> ScriptResult<Option<BarTarget>> { let Some(value) = value else { return Ok(None); }; @@ let Some(mut target) = value.try_cast::<Map>() else { return Err(runtime_error("bar target must be a map")); }; let kind = parse_required_string(&mut target, "kind")?; - match kind.as_str() { - "tab" => Ok(Some(BarTarget::Tab { + let parsed = match kind.as_str() { + "tab" => BarTarget::Tab { tabs_node_id: parse_node_id(parse_required_i64(&mut target, "tabs_node_id")?)?, index: parse_index(parse_required_i64(&mut target, "index")?, "target index")?, - })), - "floating" => Ok(Some(BarTarget::Floating { + }, + "floating" => BarTarget::Floating { floating_id: parse_floating_id(parse_required_i64(&mut target, "floating_id")?)?, - })), - "buffer" => Ok(Some(BarTarget::Buffer { + }, + "buffer" => BarTarget::Buffer { buffer_id: parse_buffer_id(parse_required_i64(&mut target, "buffer_id")?)?, - })), - _ => Err(runtime_error(format!("unknown bar target kind '{kind}'"))), + }, + _ => return Err(runtime_error(format!("unknown bar target kind '{kind}'"))), + }; + if !target.is_empty() { + return Err(runtime_error(format!( + "unknown bar target option(s): {}", + unexpected_option_keys(&target) + ))); } + Ok(Some(parsed)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-client/src/scripting/runtime.rs` around lines 2027 - 2050, The parse_bar_target function currently ignores leftover keys in the parsed Map, allowing typos/unexpected fields to pass silently; update parse_bar_target (and its handling of the Map named target after calling parse_required_string and the parse_required_* helpers) to detect any remaining keys in target after extracting the required "kind" and the variant-specific fields and return Err(runtime_error(...)) when unexpected keys remain (include the unexpected key names in the error message) so that unknown/typo fields are rejected rather than dropped.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/embers-server/src/persist.rs`:
- Around line 22-32: Add an explicit on-disk schema/version field to the
persisted types (start with PersistedWorkspace) so future incompatible changes
can be migrated: add a format_version (e.g. u32) field to PersistedWorkspace and
the other persisted structs referenced in the file (the other block around lines
149-166), mark it optional/defaulted for backward compatibility during
deserialization (use serde default/Option handling) and ensure
constructors/loads set a current version constant; update save/load paths to
read the version and branch into migration code when version !=
CURRENT_FORMAT_VERSION.
- Around line 438-452: The match in restored_buffer_state currently maps
PersistedBufferState::Running to BufferState::Running; instead map
PersistedBufferState::Running { pid } to BufferState::Interrupted by
constructing InterruptedBuffer with last_known_pid set to pid so restored
workspaces are marked detached/unattached; update the
PersistedBufferState::Running arm to return
BufferState::Interrupted(InterruptedBuffer { last_known_pid: pid }) and keep
other arms unchanged.
In `@crates/embers-server/src/server.rs`:
- Around line 1588-1598: Replace the separate pre-check of runtime.shutting_down
plus awaiting runtime.shutdown_notify with a single latched shutdown future so
the shutdown can’t be missed between the check and the select; remove the
initial if runtime.shutting_down.load(...) block and instead select between a
latched shutdown future (e.g., a tokio::sync::watch::Receiver or an async helper
that returns immediately when runtime.shutting_down is set, exposed as
runtime.shutdown_rx or a helper like runtime.latched_shutdown()) and
read_frame(&mut reader), and on shutdown branch call
runtime.cleanup_connection(connection_id).await and return; update uses of
runtime.shutdown_notify to this latched receiver (and ensure
ServerHandle::shutdown() notifies the latched signal) so outbound_tx can be
dropped and shutdown drains correctly.
- Around line 90-92: The connection counter is incremented after scheduling the
wrapper task, leaving a race where shutdown (quiesce_state_tasks()) can observe
zero active connections while the new read_handle may mutate ServerState; call
connection_tasks.enter() before calling tokio::spawn and move the returned guard
into the spawned wrapper so the counter is held for the lifetime of that task.
Specifically, obtain the guard from runtime.connection_tasks.enter() prior to
tokio::spawn, ensure the guard is moved into the async block so it is dropped
only when the wrapper completes, and keep references to connection_tasks,
read_handle, and ServerState unchanged.
---
Outside diff comments:
In `@crates/embers-client/src/scripting/runtime.rs`:
- Around line 2027-2050: The parse_bar_target function currently ignores
leftover keys in the parsed Map, allowing typos/unexpected fields to pass
silently; update parse_bar_target (and its handling of the Map named target
after calling parse_required_string and the parse_required_* helpers) to detect
any remaining keys in target after extracting the required "kind" and the
variant-specific fields and return Err(runtime_error(...)) when unexpected keys
remain (include the unexpected key names in the error message) so that
unknown/typo fields are rejected rather than dropped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0f7ccf05-02b5-4d7d-8525-d44178d741ac
⛔ Files ignored due to path filters (17)
Cargo.lockis excluded by!**/*.lockdocs/config-api-book/clipboard-1626706a.min.jsis excluded by!**/*.min.jsdocs/config-api-book/elasticlunr-ef4e11c1.min.jsis excluded by!**/*.min.jsdocs/config-api-book/favicon-8114d1fc.pngis excluded by!**/*.pngdocs/config-api-book/favicon-de23e50b.svgis excluded by!**/*.svgdocs/config-api-book/fonts/open-sans-v17-all-charsets-300-7736aa35.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-300italic-2c7b95c0.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-600-486c6759.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-600italic-1a3e8659.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-700-c22fe8c7.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-700italic-238ae959.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-800-3d2c812a.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-800italic-ba1521ec.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-italic-6c9463f7.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-regular-2e3b1d34.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/source-code-pro-v11-all-charsets-500-2bdd9410.woff2is excluded by!**/*.woff2docs/config-api-book/mark-09e88c2c.min.jsis excluded by!**/*.min.js
📒 Files selected for processing (128)
.cargo/config.toml.github/workflows/ci.ymlCargo.tomlcrates/embers-cli/Cargo.tomlcrates/embers-cli/tests/integration.rscrates/embers-cli/tests/interactive.rscrates/embers-cli/tests/panes.rscrates/embers-cli/tests/popups.rscrates/embers-cli/tests/sessions.rscrates/embers-cli/tests/windows.rscrates/embers-client/Cargo.tomlcrates/embers-client/src/bin/gen-docs.rscrates/embers-client/src/configured_client.rscrates/embers-client/src/scripting/documentation.rscrates/embers-client/src/scripting/engine.rscrates/embers-client/src/scripting/mod.rscrates/embers-client/src/scripting/model.rscrates/embers-client/src/scripting/runtime.rscrates/embers-client/tests/config_api_docs.rscrates/embers-client/tests/configured_client.rscrates/embers-client/tests/context.rscrates/embers-client/tests/controller.rscrates/embers-client/tests/fixtures/repository_config.rhaicrates/embers-client/tests/integration.rscrates/embers-client/tests/presentation.rscrates/embers-client/tests/renderer.rscrates/embers-client/tests/repo_config.rscrates/embers-client/tests/script_actions.rscrates/embers-client/tests/script_engine.rscrates/embers-core/Cargo.tomlcrates/embers-core/src/geometry.rscrates/embers-protocol/Cargo.tomlcrates/embers-protocol/schema/embers.fbscrates/embers-protocol/src/codec.rscrates/embers-protocol/src/types.rscrates/embers-protocol/tests/integration.rscrates/embers-server/Cargo.tomlcrates/embers-server/src/config.rscrates/embers-server/src/lib.rscrates/embers-server/src/model.rscrates/embers-server/src/persist.rscrates/embers-server/src/protocol.rscrates/embers-server/src/server.rscrates/embers-server/src/state.rscrates/embers-server/tests/integration.rscrates/embers-server/tests/persistence.rscrates/embers-test-support/Cargo.tomlcrates/embers-test-support/tests/integration.rsdocs/config-api-book/.nojekylldocs/config-api-book/404.htmldocs/config-api-book/action.htmldocs/config-api-book/ayu-highlight-3fdfc3ac.cssdocs/config-api-book/book-a0b12cfe.jsdocs/config-api-book/book.tomldocs/config-api-book/buffer-ref.htmldocs/config-api-book/context.htmldocs/config-api-book/css/chrome-ae938929.cssdocs/config-api-book/css/general-2459343d.cssdocs/config-api-book/css/print-9e4910d8.cssdocs/config-api-book/css/variables-8adf115d.cssdocs/config-api-book/defs/registration.rhaidocs/config-api-book/defs/runtime.rhaidocs/config-api-book/event-info.htmldocs/config-api-book/example.htmldocs/config-api-book/floating-ref.htmldocs/config-api-book/fonts/OPEN-SANS-LICENSE.txtdocs/config-api-book/fonts/SOURCE-CODE-PRO-LICENSE.txtdocs/config-api-book/fonts/fonts-9644e21d.cssdocs/config-api-book/highlight-493f70e1.cssdocs/config-api-book/highlight-abc7f01d.jsdocs/config-api-book/index.htmldocs/config-api-book/mouse.htmldocs/config-api-book/mux.htmldocs/config-api-book/node-ref.htmldocs/config-api-book/print.htmldocs/config-api-book/registration-action.htmldocs/config-api-book/registration-globals.htmldocs/config-api-book/registration-system.htmldocs/config-api-book/registration-tree.htmldocs/config-api-book/registration-ui.htmldocs/config-api-book/runtime-theme.htmldocs/config-api-book/searcher-c2a407aa.jsdocs/config-api-book/searchindex-9f697510.jsdocs/config-api-book/session-ref.htmldocs/config-api-book/system-runtime.htmldocs/config-api-book/tab-bar-context.htmldocs/config-api-book/tab-info.htmldocs/config-api-book/tabbar.htmldocs/config-api-book/theme.htmldocs/config-api-book/theme/rhai-autodocs-tabs-dbd60081.jsdocs/config-api-book/theme/rhai-autodocs-tabs.jsdocs/config-api-book/theme/rhai-highlight-8baa11ea.jsdocs/config-api-book/theme/rhai-highlight.jsdocs/config-api-book/toc-c8603eba.jsdocs/config-api-book/toc.htmldocs/config-api-book/tomorrow-night-4c0ae647.cssdocs/config-api-book/tree.htmldocs/config-api-book/ui.htmldocs/config-api/SUMMARY.mddocs/config-api/action.mddocs/config-api/book.tomldocs/config-api/buffer-ref.mddocs/config-api/context.mddocs/config-api/defs/registration.rhaidocs/config-api/defs/runtime.rhaidocs/config-api/event-info.mddocs/config-api/example.mddocs/config-api/floating-ref.mddocs/config-api/index.mddocs/config-api/mouse.mddocs/config-api/mux.mddocs/config-api/node-ref.mddocs/config-api/registration-action.mddocs/config-api/registration-globals.mddocs/config-api/registration-system.mddocs/config-api/registration-tree.mddocs/config-api/registration-ui.mddocs/config-api/runtime-theme.mddocs/config-api/session-ref.mddocs/config-api/system-runtime.mddocs/config-api/tab-bar-context.mddocs/config-api/tab-info.mddocs/config-api/tabbar.mddocs/config-api/theme.mddocs/config-api/theme/rhai-autodocs-tabs.jsdocs/config-api/theme/rhai-highlight.jsdocs/config-api/tree.mddocs/config-api/ui.md
💤 Files with no reviewable changes (3)
- crates/embers-client/src/configured_client.rs
- crates/embers-client/tests/fixtures/repository_config.rhai
- crates/embers-client/src/scripting/model.rs
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/embers-server/src/server.rs (1)
91-105:⚠️ Potential issue | 🟠 MajorDon't let
connection_taskswait on a blocked writer.This ticket lives until
write_handle.awaitfinishes, but shutdown waitsconnection_tasksbefore persisting. If a client stops reading andwrite_loop()is stuck inwrite_frame_no_flush(), shutdown can hang forever even thoughhandle_connection()has already stopped mutatingServerState. Track the read/request task with this counter instead, or explicitly abort the writer on shutdown.Minimal refactor
- let read_handle = tokio::spawn(async move { + let connection_task = runtime.connection_tasks.enter(); + let read_handle = tokio::spawn(async move { + let _connection_task = connection_task; if let Err(error) = handle_connection(read_runtime, connection_id, reader, outbound_tx) .await { error!(%error, connection_id, "connection failed"); } }); - let connection_task = runtime.connection_tasks.enter(); tokio::spawn(async move { - let _connection_task = connection_task; match write_handle.await {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-server/src/server.rs` around lines 91 - 105, The current connection_tasks token (runtime.connection_tasks.enter()) is held by the spawned task and only released after write_handle.await, which can block if write_loop() is stuck in write_frame_no_flush(), causing shutdown to hang; fix by not tying connection_tasks to the writer: move the connection_tasks.enter() to track the read/request task instead (acquire the token for the read/request lifecycle around the read_handle or handle_connection work) or ensure the writer is explicitly aborted on shutdown before awaiting write_handle (call write_handle.abort() or similar during shutdown path) and only then await/join the writer; update the code paths around runtime.connection_tasks.enter(), the spawned task that awaits write_handle, read_handle.abort(), and write_runtime.cleanup_connection to reflect this change so connection_tasks no longer waits on write_loop.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/embers-client/src/scripting/runtime.rs`:
- Around line 1803-1808: The doc comment for the rhai_fn "segment" is missing
the "dim" option; update the documentation on the segment(_: UiApi, text:
String, options: Map) -> BarSegment (the doc block above the
#[rhai_fn(return_raw, name = "segment")] attribute) to include "dim" as a
supported key (e.g., explain that "dim" is a boolean to reduce text
intensity/brightness similar to a muted style), so the generated API docs list
fg, bg, bold, italic, underline, dim, and target consistently with the
implementation that accepts "dim".
- Around line 1295-1311: The move_buffer_to_floating call currently parses
close_on_empty via parse_floating_options but drops it because
Action::MoveBufferToFloating has no field to hold it; update the flow by adding
a close_on_empty (bool) field to the Action::MoveBufferToFloating variant and
propagate spec.close_on_empty when constructing the action inside
move_buffer_to_floating (keeping parse_buffer_id and existing
geometry/title/focus usage); also update any consumers/handlers of
Action::MoveBufferToFloating and the floating spec type so the new field is
respected end-to-end.
In `@crates/embers-server/src/server.rs`:
- Around line 1548-1552: The current check uses the global
self.shutdown.is_active() to decide whether to call
state.mark_buffer_interrupted or state.mark_buffer_exited, which incorrectly
downgrades buffers that exited normally while a global shutdown later became
active; instead, query the per-buffer shutdown intent and use that to choose the
call: replace the self.shutdown.is_active() condition with a per-buffer check
(e.g., a method on state like buffer_has_shutdown_intent(buffer_id) or an
existing per-buffer flag) so you call state.mark_buffer_interrupted(buffer_id)
only when that specific buffer was intended to be shutdown, otherwise call
state.mark_buffer_exited(buffer_id, exit_code).
In `@crates/embers-server/src/state.rs`:
- Around line 95-121: Restored floating windows can be unreferenced by their
owning Session because validate() only walks Session.floating while other
lookups (e.g., floating_id_by_root()) consult self.floating; after constructing
the floating BTreeMap with restored_floating(...) and before instantiating Self
and returning, iterate the floating map and for each floating window ensure its
owner session id is present and that the session's floating set references that
window (or otherwise that floating_id_by_root(...) would find it); if any
restored floating is not referenced by its owning Session return
Err(MuxError::internal(...)) to reject inconsistent persisted state.
In `@docs/config-api-book/context.html`:
- Around line 301-313: The docs list Context functions (detached_buffers,
visible_buffers) that are not exported by documented_context_api causing
misleading docs; either remove these entries from the generated Context docs or
export them from the runtime API. Fix by updating the doc generator or source:
inspect the documented_context_api export list and either add exports/wrappers
for detached_buffers and visible_buffers in the documented_context_api module
(or documented_mux_api if appropriate) or change the generator filter that
builds Context docs to only include functions present in documented_context_api;
ensure the Context symbol in the generator matches the actual exported functions
before regenerating the docs.
In `@docs/config-api-book/defs/runtime.rhai`:
- Around line 605-607: The docs declare detached_buffers(context: Context) and
visible_buffers(context: Context) on Context but at runtime those methods are
registered on MuxApi, creating a mismatch; fix by either (A) moving the runtime
registration so these functions are bound to Context (update the registration
code that currently attaches them to MuxApi to register them on the Context API
type instead), or (B) if they should remain on MuxApi, update the docs to
declare detached_buffers(mux: MuxApi) and visible_buffers(mux: MuxApi) (and
adjust the other declaration for visible_buffers accordingly) so documentation
matches runtime behavior; ensure both doc sites for these functions are updated
consistently.
---
Outside diff comments:
In `@crates/embers-server/src/server.rs`:
- Around line 91-105: The current connection_tasks token
(runtime.connection_tasks.enter()) is held by the spawned task and only released
after write_handle.await, which can block if write_loop() is stuck in
write_frame_no_flush(), causing shutdown to hang; fix by not tying
connection_tasks to the writer: move the connection_tasks.enter() to track the
read/request task instead (acquire the token for the read/request lifecycle
around the read_handle or handle_connection work) or ensure the writer is
explicitly aborted on shutdown before awaiting write_handle (call
write_handle.abort() or similar during shutdown path) and only then await/join
the writer; update the code paths around runtime.connection_tasks.enter(), the
spawned task that awaits write_handle, read_handle.abort(), and
write_runtime.cleanup_connection to reflect this change so connection_tasks no
longer waits on write_loop.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a00d5775-c733-4af1-a727-1a011d815d29
📒 Files selected for processing (39)
crates/embers-client/src/scripting/engine.rscrates/embers-client/src/scripting/runtime.rscrates/embers-server/src/persist.rscrates/embers-server/src/server.rscrates/embers-server/src/state.rsdocs/config-api-book/404.htmldocs/config-api-book/action.htmldocs/config-api-book/buffer-ref.htmldocs/config-api-book/context.htmldocs/config-api-book/defs/registration.rhaidocs/config-api-book/defs/runtime.rhaidocs/config-api-book/event-info.htmldocs/config-api-book/example.htmldocs/config-api-book/floating-ref.htmldocs/config-api-book/index.htmldocs/config-api-book/mouse.htmldocs/config-api-book/mux.htmldocs/config-api-book/node-ref.htmldocs/config-api-book/print.htmldocs/config-api-book/registration-action.htmldocs/config-api-book/registration-globals.htmldocs/config-api-book/registration-system.htmldocs/config-api-book/registration-tree.htmldocs/config-api-book/registration-ui.htmldocs/config-api-book/runtime-theme.htmldocs/config-api-book/searcher-c2a407aa.jsdocs/config-api-book/searchindex-9f697510.jsdocs/config-api-book/session-ref.htmldocs/config-api-book/system-runtime.htmldocs/config-api-book/tab-bar-context.htmldocs/config-api-book/tab-info.htmldocs/config-api-book/tabbar.htmldocs/config-api-book/theme.htmldocs/config-api-book/tree.htmldocs/config-api-book/ui.htmldocs/config-api/defs/registration.rhaidocs/config-api/defs/runtime.rhaidocs/config-api/registration-tree.mddocs/config-api/tree.md
Comment on lines
+301
to
+313
| <h2 class="func-name"> <code>fn</code> detached_buffers </h2> | ||
|
|
||
| <pre><code class="language-rust ignore">fn detached_buffers(context: Context) -> Array</code></pre> | ||
| <div> | ||
| <div class="tab"> | ||
| <button group="detached_buffers" id="link-detached_buffers-Description" class="tablinks active" onclick="openTab(event, 'detached_buffers', 'Description')"> | ||
| Description | ||
| </button> | ||
| </div> | ||
|
|
||
| <div group="detached_buffers" id="detached_buffers-Description" class="tabcontent" style="display: block;"> | ||
| Return detached buffers in the current model snapshot. | ||
| </div> |
There was a problem hiding this comment.
Context docs list functions that are not exported by documented_context_api.
detached_buffers and visible_buffers are documented here, but the runtime export list for documented_context_api does not include them (and documented_mux_api is a separate module). This will mislead users into calling unavailable APIs at runtime. Please align the generator/source docs with the actual exported Context functions.
Also applies to: 431-443
🧰 Tools
🪛 HTMLHint (1.9.2)
[warning] 306-306: The type attribute must be present on elements.
(button-type-require)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/config-api-book/context.html` around lines 301 - 313, The docs list
Context functions (detached_buffers, visible_buffers) that are not exported by
documented_context_api causing misleading docs; either remove these entries from
the generated Context docs or export them from the runtime API. Fix by updating
the doc generator or source: inspect the documented_context_api export list and
either add exports/wrappers for detached_buffers and visible_buffers in the
documented_context_api module (or documented_mux_api if appropriate) or change
the generator filter that builds Context docs to only include functions present
in documented_context_api; ensure the Context symbol in the generator matches
the actual exported functions before regenerating the docs.
Owner
Author
|
@coderabbitai review |
1 similar comment
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/embers-server/src/server.rs (1)
1587-1598:⚠️ Potential issue | 🟠 MajorOnly mark buffers as interrupted when shutdown actually killed them.
Line 1596 records the shutdown intent before we know whether that runtime is still alive. A buffer that already exited normally but still has a queued
record_buffer_exit()task is still present inbuffer_runtimes, so this path can make the delayed callback overwrite a realExitedstate withInterruptedand loseexit_code/exited_at. Keep the intent only for runtimes that shutdown genuinely terminated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-server/src/server.rs` around lines 1587 - 1598, The code currently inserts buffer_id into buffer_shutdown_intents while building the runtimes list, which marks buffers as "Interrupted" even if they already exited normally; instead, only record a shutdown intent when the shutdown actually caused termination. Remove the shutdown_intents.insert(buffer_id) from the mapping over buffer_runtimes; then, where the runtime shutdown is awaited/handled (the code path that inspects the JoinHandle result or the abort outcome), call buffer_shutdown_intents.insert(buffer_id) only when that join/abort indicates the runtime was terminated by our shutdown logic. Ensure the delayed record_buffer_exit() callback uses the existing Exited vs Interrupted state set at that point so exit_code/exited_at are preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/embers-client/src/scripting/runtime.rs`:
- Around line 1295-1302: Update the doc comment on the move_buffer_to_floating
function to enumerate all supported floating options so generated API docs
include the new key; mention the keys accepted by parse_floating_options (x, y,
width, height, anchor, title, focus, close_on_empty) and briefly state their
expected types/semantics in the comment above move_buffer_to_floating to match
parse_floating_options behavior.
---
Duplicate comments:
In `@crates/embers-server/src/server.rs`:
- Around line 1587-1598: The code currently inserts buffer_id into
buffer_shutdown_intents while building the runtimes list, which marks buffers as
"Interrupted" even if they already exited normally; instead, only record a
shutdown intent when the shutdown actually caused termination. Remove the
shutdown_intents.insert(buffer_id) from the mapping over buffer_runtimes; then,
where the runtime shutdown is awaited/handled (the code path that inspects the
JoinHandle result or the abort outcome), call
buffer_shutdown_intents.insert(buffer_id) only when that join/abort indicates
the runtime was terminated by our shutdown logic. Ensure the delayed
record_buffer_exit() callback uses the existing Exited vs Interrupted state set
at that point so exit_code/exited_at are preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c0b2f35c-4248-484c-a927-90c3e7839cf8
📒 Files selected for processing (43)
crates/embers-client/src/configured_client.rscrates/embers-client/src/scripting/engine.rscrates/embers-client/src/scripting/model.rscrates/embers-client/src/scripting/runtime.rscrates/embers-client/tests/repo_config.rscrates/embers-client/tests/script_actions.rscrates/embers-server/src/persist.rscrates/embers-server/src/server.rscrates/embers-server/src/state.rsdocs/config-api-book/404.htmldocs/config-api-book/action.htmldocs/config-api-book/buffer-ref.htmldocs/config-api-book/context.htmldocs/config-api-book/defs/registration.rhaidocs/config-api-book/defs/runtime.rhaidocs/config-api-book/event-info.htmldocs/config-api-book/example.htmldocs/config-api-book/floating-ref.htmldocs/config-api-book/index.htmldocs/config-api-book/mouse.htmldocs/config-api-book/mux.htmldocs/config-api-book/node-ref.htmldocs/config-api-book/print.htmldocs/config-api-book/registration-action.htmldocs/config-api-book/registration-globals.htmldocs/config-api-book/registration-system.htmldocs/config-api-book/registration-tree.htmldocs/config-api-book/registration-ui.htmldocs/config-api-book/runtime-theme.htmldocs/config-api-book/searcher-c2a407aa.jsdocs/config-api-book/searchindex-9f697510.jsdocs/config-api-book/session-ref.htmldocs/config-api-book/system-runtime.htmldocs/config-api-book/tab-bar-context.htmldocs/config-api-book/tab-info.htmldocs/config-api-book/tabbar.htmldocs/config-api-book/theme.htmldocs/config-api-book/tree.htmldocs/config-api-book/ui.htmldocs/config-api/defs/registration.rhaidocs/config-api/defs/runtime.rhaidocs/config-api/registration-tree.mddocs/config-api/tree.md
Comment on lines
+1295
to
+1302
| /// Move a buffer into a new floating window. | ||
| #[rhai_fn(return_raw, name = "move_buffer_to_floating")] | ||
| pub fn move_buffer_to_floating( | ||
| ctx: NativeCallContext, | ||
| _: &mut ActionApi, | ||
| buffer_id: i64, | ||
| options: Map, | ||
| ) -> RhaiResultOf<Action> { |
There was a problem hiding this comment.
Document the floating options keys on this builder.
parse_floating_options now exposes close_on_empty, but this doc block still doesn't enumerate any supported keys. The generated API docs will miss the new option entirely unless x, y, width, height, anchor, title, focus, and close_on_empty are spelled out here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/embers-client/src/scripting/runtime.rs` around lines 1295 - 1302,
Update the doc comment on the move_buffer_to_floating function to enumerate all
supported floating options so generated API docs include the new key; mention
the keys accepted by parse_floating_options (x, y, width, height, anchor, title,
focus, close_on_empty) and briefly state their expected types/semantics in the
comment above move_buffer_to_floating to match parse_floating_options behavior.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/embers-server/src/server.rs (1)
1317-1375:⚠️ Potential issue | 🟠 MajorRegister the runtime before exit callbacks can observe it.
BufferRuntimeHandle::spawn()can fireon_exitimmediately for a short-lived process. Because this function then awaits at Line 1361 and only inserts intobuffer_runtimesat Line 1374,record_buffer_exit()can run first, see no runtime to remove, and then this code inserts a stale handle back into the map. After that,running_buffer_runtime()can treat an already-exited buffer as live.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-server/src/server.rs` around lines 1317 - 1375, spawn_buffer_runtime currently calls BufferRuntimeHandle::spawn() then awaits mark_buffer_running and only afterward inserts the runtime into self.buffer_runtimes, which lets on_exit observe no runtime and race to reinsert a stale handle; fix by registering the runtime in self.buffer_runtimes immediately after spawn (before calling mark_buffer_running/creating surfaces) so on_exit sees the live handle, and if mark_buffer_running fails then remove the runtime from self.buffer_runtimes and perform the existing cleanup (runtime.kill()/runtime.join()) before returning the error; update references in spawn_buffer_runtime to insert/remove from buffer_runtimes around mark_buffer_running and keep using runtime.pid(), record_buffer_exit, buffer_surfaces, and the existing cleanup logic.
♻️ Duplicate comments (1)
crates/embers-server/src/server.rs (1)
1587-1600:⚠️ Potential issue | 🟠 MajorShutdown intent is still set before the kill actually happens.
Preloading every buffer id into
buffer_shutdown_intentsbeforeruntime.kill().awaitrecreates the misclassification race from the earlier shutdown fix. If a process exits on its own after this snapshot ofbuffer_runtimesbut before its kill request is sent,record_buffer_exit()will consume the pre-set intent and downgrade a genuineExitedbuffer toInterrupted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-server/src/server.rs` around lines 1587 - 1600, The code pre-inserts all buffer_ids into buffer_shutdown_intents while building the runtimes snapshot, which can cause record_buffer_exit() to misclassify genuine Exited buffers as Interrupted; instead, first collect the (buffer_id, runtime) pairs without touching buffer_shutdown_intents, then loop over that collection and for each pair acquire the buffer_shutdown_intents lock and insert the buffer_id immediately before issuing runtime.kill().await (keep the lock scope minimal), then call runtime.kill().await; reference buffer_runtimes, buffer_shutdown_intents, the runtimes snapshot and runtime.kill().await (and record_buffer_exit()) when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/embers-client/src/scripting/documentation.rs`:
- Around line 192-237: The new changes to generate_config_api_docs (and the
similar block around runtime docs at lines ~541-559) change generated outputs
(action.md, registration-action.md, and defs/*.rhai), causing
checked_in_docs_are_current to fail; regenerate the docs by running the config
API doc generator (invoke generate_config_api_docs with the repo docs/config-api
output path or the project's doc generation script), commit the updated docs
under docs/config-api and the built docs under docs/config-api-book (including
action.md, registration-action.md, defs/*.rhai, SUMMARY.md, book.toml, and theme
assets), and ensure any CI/check targets that compute
checked_in_docs_are_current see the refreshed committed files.
In `@crates/embers-server/src/state.rs`:
- Around line 51-62: The constructor from_persisted currently ignores the
persisted format_version; update from_persisted to validate the extracted
format_version against the crate's CURRENT_FORMAT_VERSION (or an allowed range)
and return an Err if they differ so incompatible persisted workspaces fail fast;
locate the destructuring in pub fn from_persisted(workspace: PersistedWorkspace)
and add a check immediately after extracting format_version (and do the same for
the other from_persisted occurrence around lines 140-150) so to_persisted()'s
stamped CURRENT_FORMAT_VERSION is enforced on load.
---
Outside diff comments:
In `@crates/embers-server/src/server.rs`:
- Around line 1317-1375: spawn_buffer_runtime currently calls
BufferRuntimeHandle::spawn() then awaits mark_buffer_running and only afterward
inserts the runtime into self.buffer_runtimes, which lets on_exit observe no
runtime and race to reinsert a stale handle; fix by registering the runtime in
self.buffer_runtimes immediately after spawn (before calling
mark_buffer_running/creating surfaces) so on_exit sees the live handle, and if
mark_buffer_running fails then remove the runtime from self.buffer_runtimes and
perform the existing cleanup (runtime.kill()/runtime.join()) before returning
the error; update references in spawn_buffer_runtime to insert/remove from
buffer_runtimes around mark_buffer_running and keep using runtime.pid(),
record_buffer_exit, buffer_surfaces, and the existing cleanup logic.
---
Duplicate comments:
In `@crates/embers-server/src/server.rs`:
- Around line 1587-1600: The code pre-inserts all buffer_ids into
buffer_shutdown_intents while building the runtimes snapshot, which can cause
record_buffer_exit() to misclassify genuine Exited buffers as Interrupted;
instead, first collect the (buffer_id, runtime) pairs without touching
buffer_shutdown_intents, then loop over that collection and for each pair
acquire the buffer_shutdown_intents lock and insert the buffer_id immediately
before issuing runtime.kill().await (keep the lock scope minimal), then call
runtime.kill().await; reference buffer_runtimes, buffer_shutdown_intents, the
runtimes snapshot and runtime.kill().await (and record_buffer_exit()) when
making this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1b18be85-bd45-4401-baf5-a6bb07577b50
⛔ Files ignored due to path filters (17)
Cargo.lockis excluded by!**/*.lockdocs/config-api-book/clipboard-1626706a.min.jsis excluded by!**/*.min.jsdocs/config-api-book/elasticlunr-ef4e11c1.min.jsis excluded by!**/*.min.jsdocs/config-api-book/favicon-8114d1fc.pngis excluded by!**/*.pngdocs/config-api-book/favicon-de23e50b.svgis excluded by!**/*.svgdocs/config-api-book/fonts/open-sans-v17-all-charsets-300-7736aa35.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-300italic-2c7b95c0.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-600-486c6759.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-600italic-1a3e8659.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-700-c22fe8c7.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-700italic-238ae959.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-800-3d2c812a.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-800italic-ba1521ec.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-italic-6c9463f7.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-regular-2e3b1d34.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/source-code-pro-v11-all-charsets-500-2bdd9410.woff2is excluded by!**/*.woff2docs/config-api-book/mark-09e88c2c.min.jsis excluded by!**/*.min.js
📒 Files selected for processing (128)
.cargo/config.toml.github/workflows/ci.ymlCargo.tomlcrates/embers-cli/Cargo.tomlcrates/embers-cli/tests/integration.rscrates/embers-cli/tests/interactive.rscrates/embers-cli/tests/panes.rscrates/embers-cli/tests/popups.rscrates/embers-cli/tests/sessions.rscrates/embers-cli/tests/windows.rscrates/embers-client/Cargo.tomlcrates/embers-client/src/bin/gen-docs.rscrates/embers-client/src/configured_client.rscrates/embers-client/src/scripting/documentation.rscrates/embers-client/src/scripting/engine.rscrates/embers-client/src/scripting/mod.rscrates/embers-client/src/scripting/model.rscrates/embers-client/src/scripting/runtime.rscrates/embers-client/tests/config_api_docs.rscrates/embers-client/tests/configured_client.rscrates/embers-client/tests/context.rscrates/embers-client/tests/controller.rscrates/embers-client/tests/fixtures/repository_config.rhaicrates/embers-client/tests/integration.rscrates/embers-client/tests/presentation.rscrates/embers-client/tests/renderer.rscrates/embers-client/tests/repo_config.rscrates/embers-client/tests/script_actions.rscrates/embers-client/tests/script_engine.rscrates/embers-core/Cargo.tomlcrates/embers-core/src/geometry.rscrates/embers-protocol/Cargo.tomlcrates/embers-protocol/schema/embers.fbscrates/embers-protocol/src/codec.rscrates/embers-protocol/src/types.rscrates/embers-protocol/tests/integration.rscrates/embers-server/Cargo.tomlcrates/embers-server/src/config.rscrates/embers-server/src/lib.rscrates/embers-server/src/model.rscrates/embers-server/src/persist.rscrates/embers-server/src/protocol.rscrates/embers-server/src/server.rscrates/embers-server/src/state.rscrates/embers-server/tests/integration.rscrates/embers-server/tests/persistence.rscrates/embers-test-support/Cargo.tomlcrates/embers-test-support/tests/integration.rsdocs/config-api-book/.nojekylldocs/config-api-book/404.htmldocs/config-api-book/action.htmldocs/config-api-book/ayu-highlight-3fdfc3ac.cssdocs/config-api-book/book-a0b12cfe.jsdocs/config-api-book/book.tomldocs/config-api-book/buffer-ref.htmldocs/config-api-book/context.htmldocs/config-api-book/css/chrome-ae938929.cssdocs/config-api-book/css/general-2459343d.cssdocs/config-api-book/css/print-9e4910d8.cssdocs/config-api-book/css/variables-8adf115d.cssdocs/config-api-book/defs/registration.rhaidocs/config-api-book/defs/runtime.rhaidocs/config-api-book/event-info.htmldocs/config-api-book/example.htmldocs/config-api-book/floating-ref.htmldocs/config-api-book/fonts/OPEN-SANS-LICENSE.txtdocs/config-api-book/fonts/SOURCE-CODE-PRO-LICENSE.txtdocs/config-api-book/fonts/fonts-9644e21d.cssdocs/config-api-book/highlight-493f70e1.cssdocs/config-api-book/highlight-abc7f01d.jsdocs/config-api-book/index.htmldocs/config-api-book/mouse.htmldocs/config-api-book/mux.htmldocs/config-api-book/node-ref.htmldocs/config-api-book/print.htmldocs/config-api-book/registration-action.htmldocs/config-api-book/registration-globals.htmldocs/config-api-book/registration-system.htmldocs/config-api-book/registration-tree.htmldocs/config-api-book/registration-ui.htmldocs/config-api-book/runtime-theme.htmldocs/config-api-book/searcher-c2a407aa.jsdocs/config-api-book/searchindex-9f697510.jsdocs/config-api-book/session-ref.htmldocs/config-api-book/system-runtime.htmldocs/config-api-book/tab-bar-context.htmldocs/config-api-book/tab-info.htmldocs/config-api-book/tabbar.htmldocs/config-api-book/theme.htmldocs/config-api-book/theme/rhai-autodocs-tabs-dbd60081.jsdocs/config-api-book/theme/rhai-autodocs-tabs.jsdocs/config-api-book/theme/rhai-highlight-8baa11ea.jsdocs/config-api-book/theme/rhai-highlight.jsdocs/config-api-book/toc-c8603eba.jsdocs/config-api-book/toc.htmldocs/config-api-book/tomorrow-night-4c0ae647.cssdocs/config-api-book/tree.htmldocs/config-api-book/ui.htmldocs/config-api/SUMMARY.mddocs/config-api/action.mddocs/config-api/book.tomldocs/config-api/buffer-ref.mddocs/config-api/context.mddocs/config-api/defs/registration.rhaidocs/config-api/defs/runtime.rhaidocs/config-api/event-info.mddocs/config-api/example.mddocs/config-api/floating-ref.mddocs/config-api/index.mddocs/config-api/mouse.mddocs/config-api/mux.mddocs/config-api/node-ref.mddocs/config-api/registration-action.mddocs/config-api/registration-globals.mddocs/config-api/registration-system.mddocs/config-api/registration-tree.mddocs/config-api/registration-ui.mddocs/config-api/runtime-theme.mddocs/config-api/session-ref.mddocs/config-api/system-runtime.mddocs/config-api/tab-bar-context.mddocs/config-api/tab-info.mddocs/config-api/tabbar.mddocs/config-api/theme.mddocs/config-api/theme/rhai-autodocs-tabs.jsdocs/config-api/theme/rhai-highlight.jsdocs/config-api/tree.mddocs/config-api/ui.md
💤 Files with no reviewable changes (1)
- crates/embers-client/tests/fixtures/repository_config.rhai
Comment on lines
+51
to
+62
| pub fn from_persisted(workspace: PersistedWorkspace) -> Result<Self> { | ||
| let PersistedWorkspace { | ||
| format_version: _, | ||
| sessions: persisted_sessions, | ||
| buffers: persisted_buffers, | ||
| nodes: persisted_nodes, | ||
| floating: persisted_floating, | ||
| next_session_id, | ||
| next_buffer_id, | ||
| next_node_id, | ||
| next_floating_id, | ||
| } = workspace; |
There was a problem hiding this comment.
Persisted format versions are written but never validated.
to_persisted() stamps CURRENT_FORMAT_VERSION, but from_persisted() discards format_version entirely. Direct callers can therefore load a newer or incompatible workspace and reinterpret it as the current schema instead of failing fast.
🛠️ Minimal guard
let PersistedWorkspace {
- format_version: _,
+ format_version,
sessions: persisted_sessions,
buffers: persisted_buffers,
nodes: persisted_nodes,
floating: persisted_floating,
next_session_id,
next_buffer_id,
next_node_id,
next_floating_id,
} = workspace;
+
+ if let Some(version) = format_version
+ && version != CURRENT_FORMAT_VERSION
+ {
+ return Err(MuxError::invalid_input(format!(
+ "unsupported workspace format version {version}"
+ )));
+ }Also applies to: 140-150
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/embers-server/src/state.rs` around lines 51 - 62, The constructor
from_persisted currently ignores the persisted format_version; update
from_persisted to validate the extracted format_version against the crate's
CURRENT_FORMAT_VERSION (or an allowed range) and return an Err if they differ so
incompatible persisted workspaces fail fast; locate the destructuring in pub fn
from_persisted(workspace: PersistedWorkspace) and add a check immediately after
extracting format_version (and do the same for the other from_persisted
occurrence around lines 140-150) so to_persisted()'s stamped
CURRENT_FORMAT_VERSION is enforced on load.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/embers-server/src/server.rs (1)
52-66:⚠️ Potential issue | 🟡 MinorInstall the socket cleanup guard before
set_socket_permissions().
UnixListener::bind()creates the socket path, but the cleanup guard is only created inside the spawned task. Ifset_socket_permissions()fails,start()returns early and leaves the pathname behind.🧹 Minimal fix
let restored_state = load_workspace(&self.config.workspace_path)?; let listener = UnixListener::bind(&self.config.socket_path)?; - set_socket_permissions(&self.config.socket_path)?; let socket_path = self.config.socket_path.clone(); + let cleanup = SocketCleanup::new(socket_path.clone()); + set_socket_permissions(&self.config.socket_path)?; let runtime = Arc::new(Runtime::new( restored_state.unwrap_or_default(), self.config.workspace_path.clone(), self.config.buffer_env.clone(), )); @@ let join = tokio::spawn(async move { - let _cleanup = SocketCleanup::new(socket_path.clone()); + let _cleanup = cleanup; info!(socket_path = %socket_path.display(), "mux server listening");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-server/src/server.rs` around lines 52 - 66, Create the SocketCleanup guard immediately after binding the socket so the pathname is removed if subsequent steps fail: after calling UnixListener::bind(&self.config.socket_path) create a SocketCleanup via SocketCleanup::new(socket_path.clone()) (or similar) and keep it in scope through set_socket_permissions(&self.config.socket_path)? and the spawn; then move the existing creation of SocketCleanup from inside the tokio::spawn block to this earlier point (or create a cloneable guard and pass ownership into the spawn) so failures in set_socket_permissions() do not leak the socket file. Use the existing symbols UnixListener::bind, set_socket_permissions, SocketCleanup, and the socket_path variable to locate and adjust the code.
♻️ Duplicate comments (1)
crates/embers-server/src/state.rs (1)
51-62:⚠️ Potential issue | 🟠 MajorValidate
format_versionon restore.
to_persisted()stampsCURRENT_FORMAT_VERSION, butfrom_persisted()still dropsformat_version. That lets direct callers deserialize a newer workspace schema as if it were current.🛡️ Minimal guard
let PersistedWorkspace { - format_version: _, + format_version, sessions: persisted_sessions, buffers: persisted_buffers, nodes: persisted_nodes, floating: persisted_floating, next_session_id, next_buffer_id, next_node_id, next_floating_id, } = workspace; + + if let Some(version) = format_version + && version != CURRENT_FORMAT_VERSION + { + return Err(MuxError::invalid_input(format!( + "unsupported workspace format version {version}" + ))); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/embers-server/src/state.rs` around lines 51 - 62, The from_persisted function currently ignores PersistedWorkspace.format_version; update from_persisted to capture the format_version value (instead of discarding it), compare it to the CURRENT_FORMAT_VERSION constant, and return an Err (or map to a suitable Result error) when they differ to prevent loading incompatible schemas; modify the destructuring of PersistedWorkspace and add a simple guard at the top of from_persisted that checks format_version == CURRENT_FORMAT_VERSION and bails with a clear error if not.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/embers-client/src/scripting/documentation.rs`:
- Around line 541-555: build_mdbook() currently invokes mdbook without cleaning
the destination; update it to explicitly remove and recreate the build
destination before running Command::new("mdbook"). Use the same style as
generate_config_api_docs(): check if build_dir exists and call
std::fs::remove_dir_all(&build_dir) (propagating errors via the function
Result), then recreate the directory with std::fs::create_dir_all(&build_dir) if
needed, and only then run Command::new("mdbook") with the same args; reference
the build_mdbook function and the build_dir and output_dir variables when making
the change.
---
Outside diff comments:
In `@crates/embers-server/src/server.rs`:
- Around line 52-66: Create the SocketCleanup guard immediately after binding
the socket so the pathname is removed if subsequent steps fail: after calling
UnixListener::bind(&self.config.socket_path) create a SocketCleanup via
SocketCleanup::new(socket_path.clone()) (or similar) and keep it in scope
through set_socket_permissions(&self.config.socket_path)? and the spawn; then
move the existing creation of SocketCleanup from inside the tokio::spawn block
to this earlier point (or create a cloneable guard and pass ownership into the
spawn) so failures in set_socket_permissions() do not leak the socket file. Use
the existing symbols UnixListener::bind, set_socket_permissions, SocketCleanup,
and the socket_path variable to locate and adjust the code.
---
Duplicate comments:
In `@crates/embers-server/src/state.rs`:
- Around line 51-62: The from_persisted function currently ignores
PersistedWorkspace.format_version; update from_persisted to capture the
format_version value (instead of discarding it), compare it to the
CURRENT_FORMAT_VERSION constant, and return an Err (or map to a suitable Result
error) when they differ to prevent loading incompatible schemas; modify the
destructuring of PersistedWorkspace and add a simple guard at the top of
from_persisted that checks format_version == CURRENT_FORMAT_VERSION and bails
with a clear error if not.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7e0c818e-23e8-4b55-af2f-5fb23fc5716a
⛔ Files ignored due to path filters (17)
Cargo.lockis excluded by!**/*.lockdocs/config-api-book/clipboard-1626706a.min.jsis excluded by!**/*.min.jsdocs/config-api-book/elasticlunr-ef4e11c1.min.jsis excluded by!**/*.min.jsdocs/config-api-book/favicon-8114d1fc.pngis excluded by!**/*.pngdocs/config-api-book/favicon-de23e50b.svgis excluded by!**/*.svgdocs/config-api-book/fonts/open-sans-v17-all-charsets-300-7736aa35.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-300italic-2c7b95c0.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-600-486c6759.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-600italic-1a3e8659.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-700-c22fe8c7.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-700italic-238ae959.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-800-3d2c812a.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-800italic-ba1521ec.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-italic-6c9463f7.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/open-sans-v17-all-charsets-regular-2e3b1d34.woff2is excluded by!**/*.woff2docs/config-api-book/fonts/source-code-pro-v11-all-charsets-500-2bdd9410.woff2is excluded by!**/*.woff2docs/config-api-book/mark-09e88c2c.min.jsis excluded by!**/*.min.js
📒 Files selected for processing (128)
.cargo/config.toml.github/workflows/ci.ymlCargo.tomlcrates/embers-cli/Cargo.tomlcrates/embers-cli/tests/integration.rscrates/embers-cli/tests/interactive.rscrates/embers-cli/tests/panes.rscrates/embers-cli/tests/popups.rscrates/embers-cli/tests/sessions.rscrates/embers-cli/tests/windows.rscrates/embers-client/Cargo.tomlcrates/embers-client/src/bin/gen-docs.rscrates/embers-client/src/configured_client.rscrates/embers-client/src/scripting/documentation.rscrates/embers-client/src/scripting/engine.rscrates/embers-client/src/scripting/mod.rscrates/embers-client/src/scripting/model.rscrates/embers-client/src/scripting/runtime.rscrates/embers-client/tests/config_api_docs.rscrates/embers-client/tests/configured_client.rscrates/embers-client/tests/context.rscrates/embers-client/tests/controller.rscrates/embers-client/tests/fixtures/repository_config.rhaicrates/embers-client/tests/integration.rscrates/embers-client/tests/presentation.rscrates/embers-client/tests/renderer.rscrates/embers-client/tests/repo_config.rscrates/embers-client/tests/script_actions.rscrates/embers-client/tests/script_engine.rscrates/embers-core/Cargo.tomlcrates/embers-core/src/geometry.rscrates/embers-protocol/Cargo.tomlcrates/embers-protocol/schema/embers.fbscrates/embers-protocol/src/codec.rscrates/embers-protocol/src/types.rscrates/embers-protocol/tests/integration.rscrates/embers-server/Cargo.tomlcrates/embers-server/src/config.rscrates/embers-server/src/lib.rscrates/embers-server/src/model.rscrates/embers-server/src/persist.rscrates/embers-server/src/protocol.rscrates/embers-server/src/server.rscrates/embers-server/src/state.rscrates/embers-server/tests/integration.rscrates/embers-server/tests/persistence.rscrates/embers-test-support/Cargo.tomlcrates/embers-test-support/tests/integration.rsdocs/config-api-book/.nojekylldocs/config-api-book/404.htmldocs/config-api-book/action.htmldocs/config-api-book/ayu-highlight-3fdfc3ac.cssdocs/config-api-book/book-a0b12cfe.jsdocs/config-api-book/book.tomldocs/config-api-book/buffer-ref.htmldocs/config-api-book/context.htmldocs/config-api-book/css/chrome-ae938929.cssdocs/config-api-book/css/general-2459343d.cssdocs/config-api-book/css/print-9e4910d8.cssdocs/config-api-book/css/variables-8adf115d.cssdocs/config-api-book/defs/registration.rhaidocs/config-api-book/defs/runtime.rhaidocs/config-api-book/event-info.htmldocs/config-api-book/example.htmldocs/config-api-book/floating-ref.htmldocs/config-api-book/fonts/OPEN-SANS-LICENSE.txtdocs/config-api-book/fonts/SOURCE-CODE-PRO-LICENSE.txtdocs/config-api-book/fonts/fonts-9644e21d.cssdocs/config-api-book/highlight-493f70e1.cssdocs/config-api-book/highlight-abc7f01d.jsdocs/config-api-book/index.htmldocs/config-api-book/mouse.htmldocs/config-api-book/mux.htmldocs/config-api-book/node-ref.htmldocs/config-api-book/print.htmldocs/config-api-book/registration-action.htmldocs/config-api-book/registration-globals.htmldocs/config-api-book/registration-system.htmldocs/config-api-book/registration-tree.htmldocs/config-api-book/registration-ui.htmldocs/config-api-book/runtime-theme.htmldocs/config-api-book/searcher-c2a407aa.jsdocs/config-api-book/searchindex-72423136.jsdocs/config-api-book/session-ref.htmldocs/config-api-book/system-runtime.htmldocs/config-api-book/tab-bar-context.htmldocs/config-api-book/tab-info.htmldocs/config-api-book/tabbar.htmldocs/config-api-book/theme.htmldocs/config-api-book/theme/rhai-autodocs-tabs-dbd60081.jsdocs/config-api-book/theme/rhai-autodocs-tabs.jsdocs/config-api-book/theme/rhai-highlight-8baa11ea.jsdocs/config-api-book/theme/rhai-highlight.jsdocs/config-api-book/toc-c8603eba.jsdocs/config-api-book/toc.htmldocs/config-api-book/tomorrow-night-4c0ae647.cssdocs/config-api-book/tree.htmldocs/config-api-book/ui.htmldocs/config-api/SUMMARY.mddocs/config-api/action.mddocs/config-api/book.tomldocs/config-api/buffer-ref.mddocs/config-api/context.mddocs/config-api/defs/registration.rhaidocs/config-api/defs/runtime.rhaidocs/config-api/event-info.mddocs/config-api/example.mddocs/config-api/floating-ref.mddocs/config-api/index.mddocs/config-api/mouse.mddocs/config-api/mux.mddocs/config-api/node-ref.mddocs/config-api/registration-action.mddocs/config-api/registration-globals.mddocs/config-api/registration-system.mddocs/config-api/registration-tree.mddocs/config-api/registration-ui.mddocs/config-api/runtime-theme.mddocs/config-api/session-ref.mddocs/config-api/system-runtime.mddocs/config-api/tab-bar-context.mddocs/config-api/tab-info.mddocs/config-api/tabbar.mddocs/config-api/theme.mddocs/config-api/theme/rhai-autodocs-tabs.jsdocs/config-api/theme/rhai-highlight.jsdocs/config-api/tree.mddocs/config-api/ui.md
💤 Files with no reviewable changes (1)
- crates/embers-client/tests/fixtures/repository_config.rhai
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
1 participant
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.
Summary by CodeRabbit
New Features
Documentation
Behavior Changes
Tests
Stack
impl Initial implementation #1(merged)persistance Persist workspace state across clean restarts #3(merged)