(MOT-4274) feat(editor): add the editor worker - #623
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
skill-check — worker0 verified, 50 skipped (no docs/).
Four for four. Nicely done. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new Rust ChangesEditor worker foundation
Workspace change observation
Console UI
Contracts, validation, and integration
Release and documentation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (8)
editor/src/fuzzy.rs (2)
93-98: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
PENALTY_LEADING_MAXdoubles as the trailing clamp. Naming reads as a bug at the trailing site; a separatePENALTY_TAIL_MAXconstant (same value) documents the intent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@editor/src/fuzzy.rs` around lines 93 - 98, Define a separate PENALTY_TAIL_MAX constant with the same value as PENALTY_LEADING_MAX, then update the trailing penalty calculation in the fuzzy scoring function to use PENALTY_TAIL_MAX while retaining PENALTY_LEADING_MAX for the leading calculation.
56-67: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueCase folding is ASCII-only despite the "case-insensitive" contract.
to_ascii_lowercaseleaves non-ASCII untouched, soé/É(or any non-Latin path) never fold.to_lowercase()returns an iterator, so a cheap approximation is comparingqc.to_lowercase().eq(hc.to_lowercase())only when either char is non-ASCII.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@editor/src/fuzzy.rs` around lines 56 - 67, Update the character comparison in the fuzzy-matching loop around query and haystack characters to support non-ASCII case folding while retaining the ASCII fast path. Replace the ASCII-only comparison with direct lowercase character comparison for ASCII inputs, and use lowercase iterator equality when either character is non-ASCII so pairs such as `é` and `É` match.editor/src/workspace.rs (1)
91-106: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
remapcan create duplicate buffer/expanded entries.Moving
a.rsonto a path that is already open (b.rs) leaves two buffers withpath == "b.rs";closethen removes both andupsertonly refreshes the first. Same forexpanded. A dedup pass after the rewrite keeps the invariant thatupsertmaintains.♻️ Optional dedup after remap
for path in self.expanded.iter_mut() { if let Some(next) = remapped(path, from, to) { *path = next; changed += 1; } } + let mut seen = std::collections::HashSet::new(); + self.buffers.retain(|b| seen.insert(b.path.clone())); + let mut seen_dirs = std::collections::HashSet::new(); + self.expanded.retain(|p| seen_dirs.insert(p.clone())); changed🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@editor/src/workspace.rs` around lines 91 - 106, Update Workspace::remap to deduplicate both buffers and expanded paths after applying remapped paths, preserving only one entry per path so the invariant expected by upsert and close remains intact. Keep the existing changed-count behavior for rewritten entries.editor/src/git.rs (1)
79-89: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
split_at(1)on a non-ASCII token would panic. git only ever emits+N -Nhere, so this is defensive only, butrest.split_whitespace()feedingtoken.split_at(1)panics on a multi-byte leading char.token.strip_prefix('+')/strip_prefix('-')avoids the sharp edge entirely.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@editor/src/git.rs` around lines 79 - 89, Update the branch status token parsing in the `# branch.ab ` handling to replace `split_at(1)` with safe `strip_prefix('+')` and `strip_prefix('-')` checks. Parse the remaining numeric text for ahead/behind counts, and ignore tokens with neither prefix without risking a panic on non-ASCII input.editor/src/functions/mod.rs (1)
1195-1212: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
editor::git::showreturns unbounded content. Every other read path here is capped (max_file_bytes,max_diff_bytes);git show <rev>:<path>streams a whole blob straight into the response, including binary blobs as lossy text. Consider capping againstcfg.max_file_byteswith atruncatedflag, matchingeditor::open.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@editor/src/functions/mod.rs` around lines 1195 - 1212, Update the git show handling around bus.git and GitShowOutput to cap returned content at cfg.max_file_bytes, matching editor::open’s read behavior. Track whether the blob was truncated with the existing output contract’s truncated flag, and ensure binary or oversized content does not stream unbounded data into the response.editor/tests/schemas.rs (1)
86-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winValidate descriptions per request property.
rendered.contains("description")can pass because an unrelated nested definition has a description, even when a request field loses its own documentation. Parse the schema and assert each top-level request property has a description.Suggested test adjustment
- let rendered = serde_json::to_string(&spec.request_schema).expect("schema serializes"); - if !rendered.contains("properties") { + let schema = serde_json::to_value(&spec.request_schema).expect("schema serializes"); + let Some(properties) = schema.get("properties").and_then(|v| v.as_object()) else { continue; - } - assert!( - rendered.contains("description"), - "{}: request schema lost its field descriptions", - spec.function_id - ); + }; + for (field, property) in properties { + assert!( + property.get("description").and_then(|v| v.as_str()).is_some(), + "{}.{}: request field lost its description", + spec.function_id, + field + ); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@editor/tests/schemas.rs` around lines 86 - 97, Update schemas_with_fields_carry_field_descriptions to parse each request_schema into a JSON value and inspect its top-level properties individually. For every property under the request schema’s properties object, assert that the property contains a description, while preserving the existing catalog iteration and function_id context in failures.editor/ui/src/page/index.tsx (1)
246-276: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPoll re-reads every open buffer's full contents every 3s.
refreshBufferscallsapi.openfor each buffer on every tick regardless of whether it changed, so N open tabs cost N full file reads (plus theeditor::git::hunkscall thedeltaeffect re-runs on each status change). The workspace view already carriesmtimeper buffer — gate the read on it.♻️ Suggested gating
const ws = await api.workspace() + const known = new Map(buffers.map((b) => [b.path, b.mtime])) setBuffers(ws.buffers) for (const buffer of ws.buffers) { const local = draftsRef.current[buffer.path] if (!local) continue + // Unchanged mtime means the bytes we already hold are current. + if (known.get(buffer.path) === buffer.mtime) continue const file = await api.open(buffer.path)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@editor/ui/src/page/index.tsx` around lines 246 - 276, Update refreshBuffers to use each workspace buffer’s mtime to skip api.open when the buffer has not changed since the locally tracked draft metadata. Only re-read and update the draft when the workspace mtime differs, while preserving stale and flashed handling for changed content; ensure the stored draft state retains the latest mtime for subsequent polling checks.editor/build.rs (1)
62-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
pnpm installisn't pinned to the lockfile.Running plain
pnpm installduring acargo buildlets pnpm silently updatepnpm-lock.yamlif it's out of sync withui/package.json, instead of failing fast. For a release build script (invoked across up to 9 cross-compile targets perREADME.md), that risks non-reproducible installs across jobs. Consider--frozen-lockfile(or the pnpm equivalent) so drift surfaces as a build failure rather than a silent lockfile mutation.🔧 Proposed fix
let status = Command::new(&pnpm) - .args(["install"]) + .args(["install", "--frozen-lockfile"]) .current_dir(&ui_dir)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@editor/build.rs` around lines 62 - 74, Update the pnpm invocation in the build script’s Command chain to pass the frozen-lockfile option alongside install, ensuring lockfile/package manifest drift fails the build instead of updating pnpm-lock.yaml. Preserve the existing spawn and non-success status handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@editor/README.md`:
- Line 3: Complete or remove the dangling sentence fragment directly beneath the
README title; if retained, rewrite it as a grammatically complete sentence that
clearly states the intended relationship between shell::fs::write and editor::*.
- Around line 129-165: Update the Configuration section’s statement about config
files to acknowledge the committed editor/config.yaml as a local/test seed used
by development and integration tests, while clearly distinguishing it from
production runtime configuration stored by the configuration worker. Preserve
the existing explanation that --config is an optional seed and does not
overwrite stored runtime values.
In `@editor/src/bus.rs`:
- Around line 153-184: Update read so the size obtained from the shell::fs::read
response is compared with max_bytes before constructing ChannelReader or calling
read_all(), returning the existing truncated FileRead result without opening or
buffering the channel when oversized. Preserve normal channel reading for files
within the limit and ensure the documented pre-read size-check behavior matches
the implementation.
In `@editor/src/functions/mod.rs`:
- Around line 687-692: Update the patch bounding logic in the worker around the
`patch` variable so truncation never uses a byte index inside a UTF-8 codepoint.
When `patch.len()` exceeds `cfg.max_diff_bytes`, reduce the limit to the
greatest valid char boundary at or before that byte limit, then truncate using
that boundary while preserving the existing size cap.
- Around line 803-817: Update the session persistence logic around
session.collapse so save_session runs whenever either buffers were closed or the
expanded state changed. Track whether collapse mutated the session, or otherwise
compare the relevant state before and after collapsing, while preserving the
existing close behavior.
In `@editor/src/functions/types.rs`:
- Around line 481-502: Update the mtime conflict-version flow used by
editor::open and SaveInput::expected_mtime to avoid Unix-second collisions:
return and compare nanosecond-precision metadata or an opaque content
version/hash, while preserving intentional overwrite behavior when
expected_mtime is omitted. Regenerate the associated schemas after updating the
public type definitions.
In `@editor/src/git.rs`:
- Around line 116-128: Update the unmerged-entry parsing in the status parser to
use splitn(10, ' ') and read the path from fields index 9, so conflict rows are
added to report.entries. Add a parse_status test covering an input such as “u YY
sub ... path” and verify the resulting conflicted StatusEntry.
In `@editor/src/main.rs`:
- Around line 89-94: Update Bus construction and its Git-operation methods so
Bus retains access to the live ConfigCell rather than the boot-time
git_timeout_ms value. In each Bus-mediated Git invocation, read the current
timeout from ConfigCell at call time, ensuring apply_config updates are observed
while preserving existing timeout behavior.
- Around line 99-102: Update the startup flow surrounding
configuration::register_config_trigger so a registration failure is returned or
propagated instead of merely logged with tracing::warn. Ensure main startup
aborts and exposes the error to the supervisor, preventing the worker from
serving requests with stale limits.
In `@editor/src/tree.rs`:
- Around line 26-53: Update walk so traversal is bounded by a budget charged for
every visited child, including directories, rather than relying only on
out.len(). Use the existing limit as the candidate bound while adding or
propagating a separate node-budget counter, and stop recursion once that budget
is exhausted without changing file candidate emission.
In `@editor/tests/golden/schemas/editor.git.hunks.json`:
- Around line 44-52: Update the context_lines description in the
editor.git.hunks schema to state that omitted values use the configured
diff_context_lines setting, rather than documenting 3 as the default. Keep the
existing guidance about passing 0 and hunk range widening, while describing 3
only as the shipped initial value if needed.
In `@editor/tests/integration.rs`:
- Around line 52-87: Update boot() to retain the spawned iii Child and
explicitly terminate and reap it if worker creation fails, instead of
propagating through .ok()? with the engine still running. Replace the fixed
startup sleeps in boot() with polling that waits until the engine and worker are
ready, using bounded retries or a timeout so readiness is deterministic under
load.
In `@editor/tests/schemas.rs`:
- Around line 1-12: The module documentation in the schemas test incorrectly
states there are six editor functions; update that wording to reflect all 19
functions registered by editor::surface::catalog(), or describe them as all
registered editor::* functions. Keep the existing snapshot and regeneration
guidance unchanged.
In `@editor/ui/src/page/index.tsx`:
- Around line 25-39: Remove the unused Diff named import from the static
`@iii-dev/console-ui` import. Preserve DiffPane’s runtime resolution through
host.components.Diff so older console shims can load the page without
module-linking failures.
In `@editor/ui/styles.css`:
- Around line 305-319: Update the `.ed-close` visibility rules so an inactive
tab reveals its close button when the tab receives keyboard focus within, by
adding a `.ed-tab:focus-within .ed-close` selector alongside the existing hover
and active-tab selectors.
---
Nitpick comments:
In `@editor/build.rs`:
- Around line 62-74: Update the pnpm invocation in the build script’s Command
chain to pass the frozen-lockfile option alongside install, ensuring
lockfile/package manifest drift fails the build instead of updating
pnpm-lock.yaml. Preserve the existing spawn and non-success status handling.
In `@editor/src/functions/mod.rs`:
- Around line 1195-1212: Update the git show handling around bus.git and
GitShowOutput to cap returned content at cfg.max_file_bytes, matching
editor::open’s read behavior. Track whether the blob was truncated with the
existing output contract’s truncated flag, and ensure binary or oversized
content does not stream unbounded data into the response.
In `@editor/src/fuzzy.rs`:
- Around line 93-98: Define a separate PENALTY_TAIL_MAX constant with the same
value as PENALTY_LEADING_MAX, then update the trailing penalty calculation in
the fuzzy scoring function to use PENALTY_TAIL_MAX while retaining
PENALTY_LEADING_MAX for the leading calculation.
- Around line 56-67: Update the character comparison in the fuzzy-matching loop
around query and haystack characters to support non-ASCII case folding while
retaining the ASCII fast path. Replace the ASCII-only comparison with direct
lowercase character comparison for ASCII inputs, and use lowercase iterator
equality when either character is non-ASCII so pairs such as `é` and `É` match.
In `@editor/src/git.rs`:
- Around line 79-89: Update the branch status token parsing in the `# branch.ab
` handling to replace `split_at(1)` with safe `strip_prefix('+')` and
`strip_prefix('-')` checks. Parse the remaining numeric text for ahead/behind
counts, and ignore tokens with neither prefix without risking a panic on
non-ASCII input.
In `@editor/src/workspace.rs`:
- Around line 91-106: Update Workspace::remap to deduplicate both buffers and
expanded paths after applying remapped paths, preserving only one entry per path
so the invariant expected by upsert and close remains intact. Keep the existing
changed-count behavior for rewritten entries.
In `@editor/tests/schemas.rs`:
- Around line 86-97: Update schemas_with_fields_carry_field_descriptions to
parse each request_schema into a JSON value and inspect its top-level properties
individually. For every property under the request schema’s properties object,
assert that the property contains a description, while preserving the existing
catalog iteration and function_id context in failures.
In `@editor/ui/src/page/index.tsx`:
- Around line 246-276: Update refreshBuffers to use each workspace buffer’s
mtime to skip api.open when the buffer has not changed since the locally tracked
draft metadata. Only re-read and update the draft when the workspace mtime
differs, while preserving stale and flashed handling for changed content; ensure
the stored draft state retains the latest mtime for subsequent polling checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b60aef25-b7af-4d01-8746-716f58080c91
⛔ Files ignored due to path filters (2)
editor/Cargo.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (65)
.github/workflows/create-tag.yml.github/workflows/release.ymlREADME.mdconsole/web/public/vendor/console-ui.jsconsole/web/src/components/ui/Diff.tsxconsole/web/src/lib/console-api.tsconsole/web/src/lib/console-ui-conformance.test.tseditor/Cargo.tomleditor/README.mdeditor/build.rseditor/config.yamleditor/iii.worker.yamleditor/skills/SKILL.mdeditor/src/bus.rseditor/src/config.rseditor/src/configuration.rseditor/src/diff.rseditor/src/functions/mod.rseditor/src/functions/types.rseditor/src/fuzzy.rseditor/src/git.rseditor/src/lang.rseditor/src/lib.rseditor/src/main.rseditor/src/manifest.rseditor/src/surface.rseditor/src/tree.rseditor/src/ui.rseditor/src/workspace.rseditor/tests/golden/schemas/editor.buffers.close.jsoneditor/tests/golden/schemas/editor.buffers.list.jsoneditor/tests/golden/schemas/editor.create.jsoneditor/tests/golden/schemas/editor.delete.jsoneditor/tests/golden/schemas/editor.diff.jsoneditor/tests/golden/schemas/editor.find.jsoneditor/tests/golden/schemas/editor.git.commit.jsoneditor/tests/golden/schemas/editor.git.hunks.jsoneditor/tests/golden/schemas/editor.git.show.jsoneditor/tests/golden/schemas/editor.git.stash.jsoneditor/tests/golden/schemas/editor.git.status.jsoneditor/tests/golden/schemas/editor.git.sync.jsoneditor/tests/golden/schemas/editor.git.undo-commit.jsoneditor/tests/golden/schemas/editor.move.jsoneditor/tests/golden/schemas/editor.open.jsoneditor/tests/golden/schemas/editor.save.jsoneditor/tests/golden/schemas/editor.search.jsoneditor/tests/golden/schemas/editor.tree.jsoneditor/tests/golden/schemas/editor.workspace.get.jsoneditor/tests/golden/schemas/editor.workspace.open.jsoneditor/tests/integration.rseditor/tests/manifest.rseditor/tests/schemas.rseditor/tests/support/mod.rseditor/ui/build.mjseditor/ui/package.jsoneditor/ui/page.tsxeditor/ui/src/function-trigger-message/index.tsxeditor/ui/src/lib/api.tseditor/ui/src/page/index.tsxeditor/ui/styles.csseditor/ui/tsconfig.jsoniii-permissions.yamlpackages/console-ui/component-names.mjspackages/console-ui/index.d.tspnpm-workspace.yaml
Comment on lines
+153
to
+184
| /// Read a file's text through the channel `shell::fs::read` returns. | ||
| /// | ||
| /// The size check runs on the stat that comes back with the channel ref, | ||
| /// before a byte is pulled, so an oversized file costs one round trip | ||
| /// rather than streaming megabytes we intend to throw away. | ||
| pub async fn read(&self, path: &str, max_bytes: usize) -> Result<FileRead, Error> { | ||
| let value = self | ||
| .call("shell::fs::read", json!({ "path": path }), 30_000) | ||
| .await?; | ||
|
|
||
| let size = value.get("size").and_then(Value::as_u64).unwrap_or(0); | ||
| let mtime = value.get("mtime").and_then(Value::as_i64).unwrap_or(0); | ||
| let channel_ref: StreamChannelRef = value | ||
| .get("content") | ||
| .cloned() | ||
| .ok_or_else(|| Error::Handler("shell::fs::read returned no content channel".into())) | ||
| .and_then(|v| { | ||
| serde_json::from_value(v).map_err(|e| { | ||
| Error::Handler(format!("shell::fs::read content is not a channel ref: {e}")) | ||
| }) | ||
| })?; | ||
|
|
||
| let reader = ChannelReader::new(&self.ws_url, &channel_ref); | ||
| let bytes = reader.read_all().await?; | ||
| let _ = reader.close().await; | ||
|
|
||
| let truncated = bytes.len() > max_bytes; | ||
| let slice = if truncated { | ||
| &bytes[..max_bytes] | ||
| } else { | ||
| &bytes[..] | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
The documented pre-read size check does not exist; the whole file is streamed into memory.
The doc comment claims the size check runs on the stat "before a byte is pulled", but size (Line 163) is never compared to max_bytes — read_all() buffers the entire file and the cap is applied afterwards. A multi-GB blob is fully materialized before being thrown away.
🛡️ Bail out on the stat before opening the channel
let size = value.get("size").and_then(Value::as_u64).unwrap_or(0);
let mtime = value.get("mtime").and_then(Value::as_i64).unwrap_or(0);
+ // Refuse before streaming: the stat already told us it is too big.
+ if size > max_bytes as u64 {
+ let reader = ChannelReader::new(&self.ws_url, &serde_json::from_value::<StreamChannelRef>(
+ value.get("content").cloned().unwrap_or(Value::Null),
+ ).map_err(|e| Error::Handler(format!("shell::fs::read content is not a channel ref: {e}")))?);
+ let _ = reader.close().await;
+ return Err(Error::Handler(format!(
+ "{path} is {size} bytes, over the {max_bytes}-byte read cap"
+ )));
+ }If truncated reads must stay supported, keep read_all() but at least reconcile the comment with the behaviour.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// Read a file's text through the channel `shell::fs::read` returns. | |
| /// | |
| /// The size check runs on the stat that comes back with the channel ref, | |
| /// before a byte is pulled, so an oversized file costs one round trip | |
| /// rather than streaming megabytes we intend to throw away. | |
| pub async fn read(&self, path: &str, max_bytes: usize) -> Result<FileRead, Error> { | |
| let value = self | |
| .call("shell::fs::read", json!({ "path": path }), 30_000) | |
| .await?; | |
| let size = value.get("size").and_then(Value::as_u64).unwrap_or(0); | |
| let mtime = value.get("mtime").and_then(Value::as_i64).unwrap_or(0); | |
| let channel_ref: StreamChannelRef = value | |
| .get("content") | |
| .cloned() | |
| .ok_or_else(|| Error::Handler("shell::fs::read returned no content channel".into())) | |
| .and_then(|v| { | |
| serde_json::from_value(v).map_err(|e| { | |
| Error::Handler(format!("shell::fs::read content is not a channel ref: {e}")) | |
| }) | |
| })?; | |
| let reader = ChannelReader::new(&self.ws_url, &channel_ref); | |
| let bytes = reader.read_all().await?; | |
| let _ = reader.close().await; | |
| let truncated = bytes.len() > max_bytes; | |
| let slice = if truncated { | |
| &bytes[..max_bytes] | |
| } else { | |
| &bytes[..] | |
| }; | |
| /// Read a file's text through the channel `shell::fs::read` returns. | |
| /// | |
| /// The size check runs on the stat that comes back with the channel ref, | |
| /// before a byte is pulled, so an oversized file costs one round trip | |
| /// rather than streaming megabytes we intend to throw away. | |
| pub async fn read(&self, path: &str, max_bytes: usize) -> Result<FileRead, Error> { | |
| let value = self | |
| .call("shell::fs::read", json!({ "path": path }), 30_000) | |
| .await?; | |
| let size = value.get("size").and_then(Value::as_u64).unwrap_or(0); | |
| let mtime = value.get("mtime").and_then(Value::as_i64).unwrap_or(0); | |
| // Refuse before streaming: the stat already told us it is too big. | |
| if size > max_bytes as u64 { | |
| let reader = ChannelReader::new(&self.ws_url, &serde_json::from_value::<StreamChannelRef>( | |
| value.get("content").cloned().unwrap_or(Value::Null), | |
| ).map_err(|e| Error::Handler(format!("shell::fs::read content is not a channel ref: {e}")))?); | |
| let _ = reader.close().await; | |
| return Err(Error::Handler(format!( | |
| "{path} is {size} bytes, over the {max_bytes}-byte read cap" | |
| ))); | |
| } | |
| let channel_ref: StreamChannelRef = value | |
| .get("content") | |
| .cloned() | |
| .ok_or_else(|| Error::Handler("shell::fs::read returned no content channel".into())) | |
| .and_then(|v| { | |
| serde_json::from_value(v).map_err(|e| { | |
| Error::Handler(format!("shell::fs::read content is not a channel ref: {e}")) | |
| }) | |
| })?; | |
| let reader = ChannelReader::new(&self.ws_url, &channel_ref); | |
| let bytes = reader.read_all().await?; | |
| let _ = reader.close().await; | |
| let truncated = bytes.len() > max_bytes; | |
| let slice = if truncated { | |
| &bytes[..max_bytes] | |
| } else { | |
| &bytes[..] | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@editor/src/bus.rs` around lines 153 - 184, Update read so the size obtained
from the shell::fs::read response is compared with max_bytes before constructing
ChannelReader or calling read_all(), returning the existing truncated FileRead
result without opening or buffering the channel when oversized. Preserve normal
channel reading for files within the limit and ensure the documented pre-read
size-check behavior matches the implementation.
Comment on lines
+44
to
+52
| "context_lines": { | ||
| "default": null, | ||
| "description": "Unchanged lines kept around each hunk in `patch`. Defaults to 3, which reads well. Pass 0 for ranges that match a gutter exactly — with context, a hunk's reported range widens to include it.", | ||
| "format": "uint32", | ||
| "minimum": 0.0, | ||
| "type": [ | ||
| "integer", | ||
| "null" | ||
| ] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the configurable context default.
Line 46 hard-codes 3, but omitted context_lines uses the live diff_context_lines configuration. Describe that setting instead; 3 is only the shipped initial value.
Proposed fix
- "description": "Unchanged lines kept around each hunk in `patch`. Defaults to 3, which reads well. Pass 0 for ranges that match a gutter exactly — with context, a hunk's reported range widens to include it.",
+ "description": "Unchanged lines kept around each hunk in `patch`. Defaults to the editor's configured `diff_context_lines` value (3 initially). Pass 0 for ranges that match a gutter exactly — with context, a hunk's reported range widens to include it.",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "context_lines": { | |
| "default": null, | |
| "description": "Unchanged lines kept around each hunk in `patch`. Defaults to 3, which reads well. Pass 0 for ranges that match a gutter exactly — with context, a hunk's reported range widens to include it.", | |
| "format": "uint32", | |
| "minimum": 0.0, | |
| "type": [ | |
| "integer", | |
| "null" | |
| ] | |
| "context_lines": { | |
| "default": null, | |
| "description": "Unchanged lines kept around each hunk in `patch`. Defaults to the editor's configured `diff_context_lines` value (3 initially). Pass 0 for ranges that match a gutter exactly — with context, a hunk's reported range widens to include it.", | |
| "format": "uint32", | |
| "minimum": 0.0, | |
| "type": [ | |
| "integer", | |
| "null" | |
| ] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@editor/tests/golden/schemas/editor.git.hunks.json` around lines 44 - 52,
Update the context_lines description in the editor.git.hunks schema to state
that omitted values use the configured diff_context_lines setting, rather than
documenting 3 as the default. Keep the existing guidance about passing 0 and
hunk range widening, while describing 3 only as the shipped initial value if
needed.
Comment on lines
+52
to
+87
| async fn boot() -> Option<Harness> { | ||
| let iii_bin = which::which("iii").ok()?; | ||
|
|
||
| if engine_already_running() { | ||
| eprintln!( | ||
| "skipping: an engine is already listening on 127.0.0.1:49134. \ | ||
| This test spawns its own worker, which would re-register the \ | ||
| editor console assets on that engine and remove them again on \ | ||
| teardown." | ||
| ); | ||
| return None; | ||
| } | ||
|
|
||
| let iii = Command::new(&iii_bin) | ||
| .stdout(Stdio::null()) | ||
| .stderr(Stdio::null()) | ||
| .spawn() | ||
| .ok()?; | ||
|
|
||
| sleep(Duration::from_millis(800)).await; | ||
|
|
||
| let worker = Command::new(env!("CARGO_BIN_EXE_editor")) | ||
| .args(["--url", ENGINE_WS]) | ||
| .args([ | ||
| "--config", | ||
| concat!(env!("CARGO_MANIFEST_DIR"), "/config.yaml"), | ||
| ]) | ||
| .stdout(Stdio::null()) | ||
| .stderr(Stdio::null()) | ||
| .spawn() | ||
| .ok()?; | ||
|
|
||
| sleep(Duration::from_millis(1500)).await; | ||
|
|
||
| Some(Harness { iii, worker }) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
boot() can leak an orphaned iii engine process, and relies on fixed sleeps for readiness.
If Command::new(&iii_bin).spawn() succeeds (line 65-69) but the subsequent worker .spawn() at line 73-82 fails, boot() returns None via .ok()? before a Harness is ever constructed. std::process::Child::drop does not kill the child process, so the already-spawned iii engine is left running, still bound to 127.0.0.1:49134. Because engine_already_running() (line 44-50) checks that exact port, every subsequent run of this test on that machine/runner will silently self-skip forever — a rare failure mode with a very sticky, hard-to-diagnose effect (until someone manually kills the orphan).
Separately, the fixed sleep(800ms) / sleep(1500ms) / sleep(500ms) calls (lines 71, 84, 97) are a readiness-polling anti-pattern; under a loaded CI runner these could be too short and cause intermittent failures rather than a deterministic wait.
🛠️ Sketch: kill `iii` on early failure
let iii = Command::new(&iii_bin)
.stdout(Stdio::null())
.stderr(Stdio::null())
.spawn()
.ok()?;
sleep(Duration::from_millis(800)).await;
- let worker = Command::new(env!("CARGO_BIN_EXE_editor"))
+ let mut iii = iii;
+ let worker = match Command::new(env!("CARGO_BIN_EXE_editor"))
.args(["--url", ENGINE_WS])
.args([
"--config",
concat!(env!("CARGO_MANIFEST_DIR"), "/config.yaml"),
])
.stdout(Stdio::null())
.stderr(Stdio::null())
.spawn()
- .ok()?;
+ {
+ Ok(w) => w,
+ Err(_) => {
+ let _ = iii.kill();
+ let _ = iii.wait();
+ return None;
+ }
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async fn boot() -> Option<Harness> { | |
| let iii_bin = which::which("iii").ok()?; | |
| if engine_already_running() { | |
| eprintln!( | |
| "skipping: an engine is already listening on 127.0.0.1:49134. \ | |
| This test spawns its own worker, which would re-register the \ | |
| editor console assets on that engine and remove them again on \ | |
| teardown." | |
| ); | |
| return None; | |
| } | |
| let iii = Command::new(&iii_bin) | |
| .stdout(Stdio::null()) | |
| .stderr(Stdio::null()) | |
| .spawn() | |
| .ok()?; | |
| sleep(Duration::from_millis(800)).await; | |
| let worker = Command::new(env!("CARGO_BIN_EXE_editor")) | |
| .args(["--url", ENGINE_WS]) | |
| .args([ | |
| "--config", | |
| concat!(env!("CARGO_MANIFEST_DIR"), "/config.yaml"), | |
| ]) | |
| .stdout(Stdio::null()) | |
| .stderr(Stdio::null()) | |
| .spawn() | |
| .ok()?; | |
| sleep(Duration::from_millis(1500)).await; | |
| Some(Harness { iii, worker }) | |
| } | |
| async fn boot() -> Option<Harness> { | |
| let iii_bin = which::which("iii").ok()?; | |
| if engine_already_running() { | |
| eprintln!( | |
| "skipping: an engine is already listening on 127.0.0.1:49134. \ | |
| This test spawns its own worker, which would re-register the \ | |
| editor console assets on that engine and remove them again on \ | |
| teardown." | |
| ); | |
| return None; | |
| } | |
| let iii = Command::new(&iii_bin) | |
| .stdout(Stdio::null()) | |
| .stderr(Stdio::null()) | |
| .spawn() | |
| .ok()?; | |
| sleep(Duration::from_millis(800)).await; | |
| let mut iii = iii; | |
| let worker = match Command::new(env!("CARGO_BIN_EXE_editor")) | |
| .args(["--url", ENGINE_WS]) | |
| .args([ | |
| "--config", | |
| concat!(env!("CARGO_MANIFEST_DIR"), "/config.yaml"), | |
| ]) | |
| .stdout(Stdio::null()) | |
| .stderr(Stdio::null()) | |
| .spawn() | |
| { | |
| Ok(w) => w, | |
| Err(_) => { | |
| let _ = iii.kill(); | |
| let _ = iii.wait(); | |
| return None; | |
| } | |
| }; | |
| sleep(Duration::from_millis(1500)).await; | |
| Some(Harness { iii, worker }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@editor/tests/integration.rs` around lines 52 - 87, Update boot() to retain
the spawned iii Child and explicitly terminate and reap it if worker creation
fails, instead of propagating through .ok()? with the engine still running.
Replace the fixed startup sleeps in boot() with polling that waits until the
engine and worker are ready, using bounded retries or a timeout so readiness is
deterministic under load.
Comment on lines
+305
to
+319
| [data-iii-ui='editor'] .ed-close { | ||
| border: 0; | ||
| background: transparent; | ||
| color: var(--color-ink-ghost); | ||
| font-size: 13px; | ||
| line-height: 1; | ||
| padding: 0 2px; | ||
| cursor: pointer; | ||
| visibility: hidden; | ||
| } | ||
|
|
||
| [data-iii-ui='editor'] .ed-tab:hover .ed-close, | ||
| [data-iii-ui='editor'] .ed-tab[data-active='true'] .ed-close { | ||
| visibility: visible; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Close button is unreachable by keyboard on inactive tabs.
visibility: hidden removes .ed-close from the focus order, so it only becomes tabbable once the tab is hovered or activated. Add a focus-within reveal so keyboard users can reach it directly.
♿ Proposed fix
[data-iii-ui='editor'] .ed-tab:hover .ed-close,
+[data-iii-ui='editor'] .ed-tab:focus-within .ed-close,
[data-iii-ui='editor'] .ed-tab[data-active='true'] .ed-close {
visibility: visible;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| [data-iii-ui='editor'] .ed-close { | |
| border: 0; | |
| background: transparent; | |
| color: var(--color-ink-ghost); | |
| font-size: 13px; | |
| line-height: 1; | |
| padding: 0 2px; | |
| cursor: pointer; | |
| visibility: hidden; | |
| } | |
| [data-iii-ui='editor'] .ed-tab:hover .ed-close, | |
| [data-iii-ui='editor'] .ed-tab[data-active='true'] .ed-close { | |
| visibility: visible; | |
| } | |
| [data-iii-ui='editor'] .ed-close { | |
| border: 0; | |
| background: transparent; | |
| color: var(--color-ink-ghost); | |
| font-size: 13px; | |
| line-height: 1; | |
| padding: 0 2px; | |
| cursor: pointer; | |
| visibility: hidden; | |
| } | |
| [data-iii-ui='editor'] .ed-tab:hover .ed-close, | |
| [data-iii-ui='editor'] .ed-tab:focus-within .ed-close, | |
| [data-iii-ui='editor'] .ed-tab[data-active='true'] .ed-close { | |
| visibility: visible; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@editor/ui/styles.css` around lines 305 - 319, Update the `.ed-close`
visibility rules so an inactive tab reveals its close button when the tab
receives keyboard focus within, by adding a `.ed-tab:focus-within .ed-close`
selector alongside the existing hover and active-tab selectors.
rohitg00
force-pushed
the
feat/editor-worker
branch
from
July 29, 2026 15:46
640c519 to
bcb3be8
Compare
rohitg00
changed the title
Adds a shared code workspace: a folder, the buffers open against it, and which folders are expanded, held in the state worker so it is a fact on the bus rather than something a browser tab happens to remember. A file an agent opens appears in the user's tabs; a file the user opens is one the agent can see. The unit is a folder, not a repository. The tree, tabs, editor, finder and content search all work in a plain directory; git adds a branch label and change marks when the root happens to be a repo, and nothing else changes when it is not. No filesystem access of its own. Reads, writes, moves, listings, content search and git all delegate to the shell worker, so its jail and denylist stay the one boundary, and both the recursive walk and the grep are shell's rather than a second implementation. What this adds is the model on top: a unified diff (pure — two strings in, a patch out), path ranking, a save that refuses to clobber a file that moved since it was opened, a move that rewrites every open buffer beneath it, and a delete that closes them. Those last two are why moves and deletes do not go through shell::fs directly: a buffer left pointing at a path that moved or vanished writes itself back there on the next save. The git write surface is deliberately narrow — commit, fetch, ff-only pull, push, stash, undo-last-commit. Pull is --ff-only because a merge under open buffers produces a conflicted tree an editor cannot usefully show; anything beyond this set stays with shell::exec. The injected console page is a second view of the same workspace: file tree with a Files/Search switch and a git action bar, tabs, the shared Monaco editor, an unsaved-diff toggle, and the conflict guard surfaced as a dialog. Folder expansion round-trips through the worker, so it survives a reload. The page polls the working tree, so an edit landing from anywhere lights the row up and pulls an untouched tab forward. editor::* calls render as themselves in chat: a diff as a diff, a save as a file card with its line counts. --url reads III_URL, which the worker manager injects — inside a sandbox the engine is on the VM gateway, never on the VM's loopback. Reading functions are allowlisted in iii-permissions.yaml. The writes, the git write surface, the workspace repoint, and closing someone else's buffer stay at the needs_approval default.
rohitg00
force-pushed
the
feat/editor-worker
branch
from
July 29, 2026 15:57
bcb3be8 to
9225843
Compare
Unmerged porcelain v2 entries were never reported. The `u` line carries nine columns before the path, not ten, so every conflicted file was silently dropped from the status. Covered by a test. The patch cap called String::truncate on a byte index, which panics the moment a diff contains a multibyte character and the cap lands inside it. Cut back to the last char boundary instead. editor::git::hunks hard-coded three context lines rather than reading the configured diff_context_lines, and its schema documented the shipped default instead of the setting. git_timeout_ms was copied into the bus at boot, making it the one field that ignored a configuration change despite the docs promising every field hot-reloads. It is now shared through an atomic the reload path publishes to. A failed configuration trigger bind was a warning, which left the worker serving requests with limits that could never change. It is fatal now. editor::delete discarded a collapse when no buffer happened to close, because the save was gated on the buffer list rather than on the record. The read path claimed to check size before pulling bytes but streamed the whole file and truncated afterwards. It now stops draining once past the cap. The tree walk bounded emitted files but not visited nodes, so a deep tree of empty directories was traversed in full regardless of the limit. The integration test could leak an engine process when the worker failed to spawn, because nothing owned the child until both had started. Tab close buttons used visibility:hidden, which removes them from the focus order, so an inactive tab could not be closed from the keyboard. Also removes a stray line that a test write left in the worker README, and corrects a stale function count in the schema test's module doc.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
editor/src/main.rs (1)
80-88: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueFatal config boot deviates from the repo-wide worker convention.
Sibling workers (shell/storage/email) log a warning and fall back to
WorkerConfig::default()on config-load failure, registering in an inert state rather than exiting. Here bothregister_configandfetch_configfailures abort startup, so a slow-startingconfigurationworker takeseditordown with it. If the stricter behaviour is intentional, that is fine — but it is a per-worker deviation worth calling out explicitly (or aligning).Based on learnings: worker binaries should handle config-load failures by logging
tracing::warn!and falling back toWorkerConfig::default()rather than exiting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@editor/src/main.rs` around lines 80 - 88, Update the editor startup flow around configuration::register_config and configuration::fetch_config to match sibling worker behavior: log configuration failures with tracing::warn!, fall back to WorkerConfig::default(), and continue startup in the inert state instead of propagating errors and exiting.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@editor/ui/src/page/index.tsx`:
- Around line 812-836: Update the row classification logic in the shown map
callback so lines representing the “\ No newline at end of file” marker are
classified as meta rows, like the existing diff metadata prefixes. Ensure these
marker rows do not assign or increment lineNo, while preserving numbering for
actual context and added lines.
- Around line 244-274: Update refreshBuffers to use each workspace buffer’s
mtime before calling api.open: compare it with the locally tracked buffer
metadata and only re-read paths whose mtime has advanced. Preserve the existing
draft/stale/flashed updates for files that are re-read, and store the latest
mtime so unchanged tabs avoid repeated bus round-trips and full file reads.
---
Nitpick comments:
In `@editor/src/main.rs`:
- Around line 80-88: Update the editor startup flow around
configuration::register_config and configuration::fetch_config to match sibling
worker behavior: log configuration failures with tracing::warn!, fall back to
WorkerConfig::default(), and continue startup in the inert state instead of
propagating errors and exiting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e2a3bd21-5b07-4d27-91d3-a31d58b90892
⛔ Files ignored due to path filters (2)
editor/Cargo.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (59)
.github/workflows/create-tag.yml.github/workflows/release.ymlREADME.mdeditor/Cargo.tomleditor/README.mdeditor/build.rseditor/config.yamleditor/iii.worker.yamleditor/skills/SKILL.mdeditor/src/bus.rseditor/src/config.rseditor/src/configuration.rseditor/src/diff.rseditor/src/functions/mod.rseditor/src/functions/types.rseditor/src/fuzzy.rseditor/src/git.rseditor/src/lang.rseditor/src/lib.rseditor/src/main.rseditor/src/manifest.rseditor/src/surface.rseditor/src/tree.rseditor/src/ui.rseditor/src/workspace.rseditor/tests/golden/schemas/editor.buffers.close.jsoneditor/tests/golden/schemas/editor.buffers.list.jsoneditor/tests/golden/schemas/editor.create.jsoneditor/tests/golden/schemas/editor.delete.jsoneditor/tests/golden/schemas/editor.diff.jsoneditor/tests/golden/schemas/editor.find.jsoneditor/tests/golden/schemas/editor.git.commit.jsoneditor/tests/golden/schemas/editor.git.hunks.jsoneditor/tests/golden/schemas/editor.git.show.jsoneditor/tests/golden/schemas/editor.git.stash.jsoneditor/tests/golden/schemas/editor.git.status.jsoneditor/tests/golden/schemas/editor.git.sync.jsoneditor/tests/golden/schemas/editor.git.undo-commit.jsoneditor/tests/golden/schemas/editor.move.jsoneditor/tests/golden/schemas/editor.open.jsoneditor/tests/golden/schemas/editor.save.jsoneditor/tests/golden/schemas/editor.search.jsoneditor/tests/golden/schemas/editor.tree.jsoneditor/tests/golden/schemas/editor.workspace.get.jsoneditor/tests/golden/schemas/editor.workspace.open.jsoneditor/tests/integration.rseditor/tests/manifest.rseditor/tests/schemas.rseditor/tests/support/mod.rseditor/ui/build.mjseditor/ui/package.jsoneditor/ui/page.tsxeditor/ui/src/function-trigger-message/index.tsxeditor/ui/src/lib/api.tseditor/ui/src/page/index.tsxeditor/ui/styles.csseditor/ui/tsconfig.jsoniii-permissions.yamlpnpm-workspace.yaml
🚧 Files skipped from review as they are similar to previous changes (48)
- editor/tests/golden/schemas/editor.git.stash.json
- editor/ui/tsconfig.json
- editor/tests/golden/schemas/editor.buffers.list.json
- editor/ui/build.mjs
- editor/tests/golden/schemas/editor.delete.json
- editor/tests/golden/schemas/editor.git.undo-commit.json
- editor/tests/golden/schemas/editor.search.json
- editor/config.yaml
- editor/tests/golden/schemas/editor.git.sync.json
- editor/src/surface.rs
- editor/tests/golden/schemas/editor.buffers.close.json
- editor/src/manifest.rs
- editor/tests/manifest.rs
- pnpm-workspace.yaml
- editor/iii.worker.yaml
- editor/ui/page.tsx
- editor/tests/golden/schemas/editor.git.show.json
- editor/src/lib.rs
- editor/tests/golden/schemas/editor.move.json
- editor/src/lang.rs
- editor/ui/package.json
- editor/ui/styles.css
- editor/tests/support/mod.rs
- editor/tests/golden/schemas/editor.git.hunks.json
- editor/tests/schemas.rs
- editor/tests/golden/schemas/editor.git.commit.json
- editor/tests/golden/schemas/editor.diff.json
- editor/src/ui.rs
- editor/tests/golden/schemas/editor.find.json
- editor/src/diff.rs
- editor/tests/golden/schemas/editor.save.json
- editor/src/tree.rs
- editor/ui/src/lib/api.ts
- editor/tests/golden/schemas/editor.workspace.open.json
- editor/build.rs
- editor/tests/golden/schemas/editor.open.json
- iii-permissions.yaml
- editor/Cargo.toml
- editor/src/fuzzy.rs
- editor/src/git.rs
- editor/src/config.rs
- editor/tests/integration.rs
- editor/README.md
- README.md
- editor/src/workspace.rs
- editor/src/configuration.rs
- editor/src/functions/mod.rs
- editor/src/functions/types.rs
editor::git::hunks defaulted to the configured diff_context_lines after the last round of review fixes, which widened every reported range by that many lines. `Hunk` is documented as the ranges a gutter paints, so the default is back to zero and a caller wanting a readable patch asks for context explicitly. The console page does exactly that. The char-boundary truncate had no test. It has three now, including a string of nothing but multibyte characters swept across every cap. The integration test relied on a fixed sleep for worker readiness. Since the worker moved to the configuration worker it registers its functions only after a register and a fetch, both of which retry with backoff on a cold engine, so the sleep was no longer long enough and the test failed the first time it actually ran rather than skipping. It now polls until the function answers.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@editor/tests/integration.rs`:
- Around line 102-126: Update the polling loop around the trigger call to derive
the remaining time until deadline and cap both the trigger timeout and the 250
ms retry sleep to that remaining budget. Ensure the final attempt and sleep
cannot extend beyond the requested budget while preserving the existing retry
and error-handling behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fb9331e6-7ad9-48cd-aebd-bd15afca4475
📒 Files selected for processing (5)
editor/src/functions/mod.rseditor/src/functions/types.rseditor/tests/golden/schemas/editor.git.hunks.jsoneditor/tests/integration.rseditor/ui/src/lib/api.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- editor/src/functions/types.rs
Replaces the page's polling with the platform's push primitives, and adds
the remote comparison that was missing.
The page ran a three-second interval asking for git status and re-reading
every open tab. That was wrong on every axis: a git invocation per tick
whether or not anything had happened, blind to changes outside a git
repository, and reporting the settled state rather than the edit. Two
bindings replace it, both garbage-collected with the tab. The `state`
trigger type covers the workspace record, which already lived in `state`
under scope `editor`, so every buffer open, close, save and folder toggle
was already emitting an event the page ignored. A new `editor::changed`
trigger type covers file changes, fanned out to subscribers in the shape
the github worker uses for `github::called`.
The observer is what makes those events real. The workspace only ever
reflected files routed through editor::open, and agents do not do that:
they call coder::update-file and shell::fs::write, because that is what
their prompts point at. So the editor was blind to exactly the edits it
exists to show. It now binds harness::hook::post-trigger over shell::*
and coder::*, maps the write verbs to a touched path, and emits. Reads
are not changes and are filtered out; shell::exec is deliberately not
guessed at, because a command can write anything and its argv does not
say what, so inferring would produce phantom events. Those still surface
through git status.
Two things fall out of the hook payload. metadata.fs_scope.root is the
session's own workspace, so the editor follows the agent instead of
needing a root set by hand. call.function_id names the cause, so a
surface can say what did it. The hook is fail-open and always continues:
a viewer must never be able to hold or deny a write.
editor::git::hunks gains against: upstream, diffing the working tree
against @{upstream} for everything not yet pushed. git's own name for it,
so it works on any branch without the caller knowing the remote.
… the observer The observer emitted an event for every write it saw, but the event was empty. `report` computed a root-relative path and then handed that to the read, which resolves through shell's own working directory rather than the session root. For a session rooted anywhere other than shell's cwd the read missed, the error was swallowed by the fallback, and the event went out with added: 0 and no patch. That is indistinguishable from the hook never firing. Reads now go out absolute; only the reported path stays relative to the root, which is what a surface wants to display. The reason this took a rig boot and a harness turn to find is that the emitter logged only failures, so a delivered-but-empty event left no trace at all. Every branch now says what it did: no call payload, not a write, no subscribers, and a successful delivery with its counts. An observer without observability was the actual defect. Verified end to end against a live rig: the hook fires, the event is delivered to the page's subscription carrying the real line counts, and the page renders it.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@editor/src/events.rs`:
- Around line 188-200: Update register_changed_trigger to handle the Result from
iii.register_trigger_type instead of discarding it. Match the registration
outcome, log the success message only when registration succeeds, and emit a
warning with the error on failure, following the existing observe::bind pattern.
In `@editor/ui/src/page/index.tsx`:
- Around line 268-299: Replace the direct refresh calls in the onState and
onChanged handlers with a shared short trailing debounce that coalesces event
bursts into one refreshBuffers/refreshGit execution. Ensure pending timers are
cleaned up when the component unmounts, and prevent overlapping refreshes or
stale responses from overwriting newer state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7a617b69-0ee3-4e90-9a68-d14abe09a388
⛔ Files ignored due to path filters (1)
editor/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
editor/Cargo.tomleditor/src/events.rseditor/src/functions/mod.rseditor/src/functions/types.rseditor/src/lib.rseditor/src/main.rseditor/src/observe.rseditor/tests/golden/schemas/editor.git.hunks.jsoneditor/ui/src/lib/events.tseditor/ui/src/page/index.tsxeditor/ui/styles.cssobserver-proof.txt
observer-proof.txt was written by an agent while verifying the file-change observer end to end, and was committed by mistake. It is evidence of a probe, not part of the worker.
The read-only view used the console's shared Monaco editor, which is deliberately chrome-less: lineNumbers off, no minimap, no folding, no glyph margin. Code read as unstyled text and diffs carried no colour, so the page failed at the one job it exists for. Pierre's File now backs a new read view, the default on open, and FileDiff backs every patch. That replaces a hand-rolled renderer which printed diff --git, index, and @@ headers as though they were file content. Monaco stays for the edit view, since pierre is render-only. Patches in the chat function-trigger message render through the same component, at zero marginal bytes. parsePatchFiles is used rather than PatchDiff on purpose: PatchDiff throws unless the text yields exactly one file with one diff, so a clean file and a multi-file patch would each take the page down. Bundling pierre naively costs 10.3 MB, over the console's 8 MiB asset cap. But 71% of that is @shikijs/langs shipping 347 TextMate grammars and 12.5% is 65 themes; pierre itself is 350 KB. Aliasing shiki's root bundle to a 20-language allowlist, emptying the theme catalogs down to pierre-dark and pierre-light, and stubbing the unused oniguruma wasm brings the asset to 1.995 MiB, 25% of the cap. No console change was needed, and no stylesheet: pierre inlines its styles into its own shadow root. Two build guards hold that line, both verified to fail on purpose: a 2.5 MiB budget, and a smoke test that renders a TypeScript snippet and fails the build if it stops tokenising. A 4 MiB budget was measured and rejected, because moving @pierre/theming's private collection path adds 1.35 MB and still passes it, which is precisely the regression the guard exists to catch.
Registering configuration stays a required boot step, and should: with no authoritative config there is nothing to run against, and quietly guessing limits nobody chose is worse than refusing to start. Treating a timeout as permanent is the defect. A loaded engine answers the registration late, the worker exits, and the source watcher restarts it straight into the same race, so a slow boot becomes a crash loop that never converges. Seen on a live rig as "registering editor configuration: invocation timed out" repeating across reboots, with the worker never reaching the ready state. Five attempts with linear backoff, then fail carrying the last error instead of a generic one.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
editor/ui/src/page/index.tsx (1)
920-954: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
LocalDiffrebuilds its own API client. The parent already holdsapi = useMemo(() => createApi(host), [host]); passingapiin instead ofhostwould drop the duplicatecreateApiand the extra prop. Optional.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@editor/ui/src/page/index.tsx` around lines 920 - 954, Update LocalDiff to accept the parent’s existing api client instead of host, remove its local createApi/useMemo setup and unused host prop, and pass the api client from the parent while preserving the existing diff effect behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@editor/src/main.rs`:
- Around line 114-119: Update the configuration bootstrap flow around the loaded
result and retry loop to avoid propagating the final error when loading fails.
Preserve the five-retry behavior, then log a warning and use
WorkerConfig::default() as the fallback; retain the successfully loaded
configuration unchanged.
In `@editor/ui/src/page/index.tsx`:
- Around line 876-883: Replace length-only Pierre cache keys with a shared
32-bit rolling content-hash helper. In editor/ui/src/page/index.tsx lines
876-883, add the helper and use it in FileView's cacheKey as path plus
hash(contents); in lines 898-918, use p plus hash(patch) for PatchView's
cacheKeyPrefix; apply the same p plus hash(patch) substitution in
editor/ui/src/function-trigger-message/index.tsx lines 79-103 within Patch.
---
Nitpick comments:
In `@editor/ui/src/page/index.tsx`:
- Around line 920-954: Update LocalDiff to accept the parent’s existing api
client instead of host, remove its local createApi/useMemo setup and unused host
prop, and pass the api client from the parent while preserving the existing diff
effect behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 99d71d8d-2ed6-4b33-b934-04d34d10d325
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
editor/src/main.rseditor/ui/build.mjseditor/ui/package.jsoneditor/ui/src/function-trigger-message/index.tsxeditor/ui/src/page/index.tsxeditor/ui/styles.css
… tree walk Review findings against the worker, with the two that were wrong recorded rather than silently accommodated. The save guard compared Unix-second mtimes, so two writes inside one second were indistinguishable: a stale expected_mtime still matched and overwrote an edit it never saw. That is the one thing the guard exists to prevent. There is now an opaque content version alongside it, and the change is additive: expected_mtime keeps its name, type and meaning, save prefers the version when it is sent and falls back to the exact previous comparison when it is not, and a session persisted before the field existed still loads. A content hash rather than a finer timestamp because shell only exposes as_secs(), so a nanosecond mtime is not available to this worker at all. It costs no extra read: save already read the file to count added and removed lines. The find traversal budget only counted emitted files, so a tree of empty directories was walked in full however small the limit. Its test passed trivially, 400 directories against a budget of 1000. Truncation is now reported rather than silent, and a 425-file repository-shaped tree proves a normal repo still returns complete. Configuration no longer exits when the worker cannot reach it. Exiting made the source watcher restart into the same race, so a slow boot became a crash loop. It now falls back to the seed, which carries the operator's own numbers, and only to the built-in defaults when nothing was seeded. Falling back to the defaults outright was the suggestion and would have silently loosened a limit an operator had tightened. Because a failed registration means the configuration worker holds no editor entry, no reload event could ever arrive, so a background retry keeps asking and publishes through the same path a reload uses. Two findings are recorded as rejected. The suggested registration check in events.rs does not compile: register_trigger_type returns TriggerTypeRef, not Result, and swallows internally, so there is no failure to observe at that call site. The loudness moved to emit(), where an event with nowhere to go is observable, which is the signal that matters. And the suggested pre-read bail in bus.rs converts a large file into an error, when editor::open must still return a bounded body with truncated set. Reads already drain chunk by chunk and stop one chunk past the cap, so memory was never unbounded; the doc comment was the only thing that needed correcting. Also: a discarded collapse when no buffer was closed, an orphaned engine process and fixed sleeps in the integration harness, a poll that could overrun a 30 second budget by ten, and a README that documented feeding an engine config to the worker seed parser, which deny_unknown_fields rejects. A test pins that last one.
…e renders Only the read view scrolled. Opening any file longer than the viewport in edit, unsaved or head ran the content off the bottom with no scrollbar and no way to reach the rest of it, which is most files. That was a regression from introducing the pierre read view: it got a scroll pane and the other three were left with none. Two causes. A flex item defaults to min-height auto, so it refuses to shrink below its content and grows the box instead of overflowing it, which means no scrollbar ever appears. And the console's CodeEditor sets its own scrollbars to hidden and sizes its host to the content height, by design, with its own documentation saying to put it inside an overflow pane. So a bounded height would not have made it scroll. All four views now share one scroll contract; pierre wants the same arrangement, since its element clips vertically and grows too. Pierre cache keys were path plus content length. Pierre memoises on the key alone in the paths that matter, so any edit preserving byte length reused the cached render and showed stale content. Swapping a single character does exactly that. All three call sites now use one shared content hash. Pushed events had no throttle. Moving off polling removed the implicit three-second gap without replacing it, so a burst of agent writes fanned out one full refresh each with no coalescing and no in-flight guard. Refreshes now coalesce on a short trailing edge and defer rather than discard, so the last event of a burst is always the one reflected. The unconditional per-tab re-read was worse than a cost. editor::open upserts the buffer and state emits on every set without an equality check, so a refresh that re-read every tab wrote state, which emitted, which refreshed again. The gate that stops it is applied only where mtime is meaningful; editor::changed names its path and is still read unconditionally, because a buffer's mtime only advances through this worker and gating on it would make the page blind to exactly the agent edits the feature exists for. The save call now sends the new content version alongside expected_mtime. Both go out together and the worker prefers the version, so this page still works against a worker built before the field existed. The unsaved tab now appears only while something is unsaved. It diffs the draft against what was last read, so on a clean file it was guaranteed empty, and it never carried an agent's edits either: those are written to disk and arrive under head. Markdown files get a preview tab, rendered with the console's own MarkdownPreview, the documented preview counterpart to CodeEditor that iii-directory already uses for skills, prompts and registry readmes. It resolves through the import map, so it costs 687 bytes here rather than the size of a markdown renderer, and it previews the draft so it tracks what is being typed.
… the code editor::on-file-change was left agent-callable. It is a harness::hook::post-trigger target meant to be fired engine-side, and a model that called it directly could forge an editor::changed event into every subscribed console tab: path, cause, kind and patch all come straight off the payload. Every comparable internal bridge target in the file is denied, and this one now is too. editor::git::show was neither allowlisted nor in the needs-approval enumeration despite being read-only and routinely useful, so it joins git::status and git::hunks. The prefix strip in relative() and relative_to() matched by string rather than by segment, so a workspace root of /srv/app turned /srv/application/a.rs into lication/a.rs: a plausible-looking path resolving nowhere, reported as the file that changed. That is the defect Session::remap was written to avoid, and it survived because there were two copies of it. Now one, with a test that fails without the fix. The rest is documentation that had drifted behind the code. The README said the page polls the working tree, which the push rewrite removed; it described a hand-rolled diff renderer that no longer exists and a two-way toggle where there are now five views; and it used relative links that escape the worker folder, which break when the README renders on the registry. The skill taught expected_mtime as the save guard without mentioning the content version that supersedes it, and carried neither of the two sections the repo requires: a function list, and the reactive trigger this worker registers. diff_context_lines was documented as applying to hunks, which ignores it and defaults to 0; only editor::diff reads it, and that description ships into the console config panel. One comment named config.yaml as where an operator writes their intent, when that file is an engine config the seed parser rejects by design.
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.
Adds an editor to the console. Open a folder, browse it, edit files, and see what changed, in the same window as the agent you are working with.
Screen.Recording.2026-07-30.at.14.16.23.mov
The workspace is shared rather than private to the browser: the files you have open, the folders you have expanded and the version each file was read at are held by the worker, so a file the agent opens appears in your tabs and a file you open is one the agent can see. A reload does not lose it.
A folder is the unit, not a repository. The tree, tabs, editor, finder and content search all work in a plain directory. When the folder happens to be a git repository you also get a branch label, change marks, diffs against HEAD, and commit, fetch, pull, push and stash.
Saving is guarded. If a file changed underneath since it was opened, nothing is written and the difference comes back for review instead of being overwritten.
Scope
Everything is inside
editor/. No console files are touched, and the shared@iii-dev/console-uicontract is unchanged. The page is ordinary injectable UI:editor/ui/builds two assets that register overconsole:scriptandconsole:style.What the worker adds
Twenty
editor::*functions. The worker performs no filesystem access of its own: reads, writes, moves, listings, content search and git all delegate to theshellworker, so its jail and denylist remain the only filesystem boundary. The workspace record lives instate. Runtime limits live in theconfigurationworker (Path B) and hot-reload without a restart.What is left is the part a filesystem has no opinion about:
editor::diffis pure, two strings in and a patch out, so an agent can show what a write will do before making it.editor::saverefuses a write when the file moved since theeditor::openit started from, and returns the divergence as a patch. The guard compares an opaque content version rather than only an mtime, because mtime is Unix seconds: two writes inside one second were indistinguishable, so a staleexpected_mtimestill matched and overwrote an edit it never saw. The change is additive,expected_mtimekeeps its name, type and meaning, and a session persisted before the field existed still loads.editor::moverewrites every open buffer and expanded folder at or under the path, andeditor::deletecloses them. This is why moves and deletes do not go throughshell::fsdirectly: a buffer left pointing at a path that moved writes itself back there on the next save.editor::findranks paths,editor::searchgroups content matches by file.editor::git::*covers status, hunks, show, commit, fetch, ff-only pull, push, stash and undo-last-commit. Pull is--ff-onlyon purpose, because a merge under open buffers produces a conflicted tree an editor cannot usefully show.Rendering: code and diffs go through pierre
An earlier revision of this PR shipped a hand-rolled diff renderer and argued that
@pierre/diffscould not be used, because bundling it into the worker asset measured 10.3 MB against the console's 8 MiB per-asset cap. That measurement was right and the conclusion drawn from it was wrong.Almost none of that 10.3 MB is pierre. 71% is
@shikijs/langsshipping 347 TextMate grammars, 12.5% is 65 themes, and 6% is the oniguruma wasm. Pierre's own code is 350 KB, or 3.3%. The cause is one line inshiki/bundle-full.mjswhich statically references all three catalogs, so withsplittingoff every grammar is inlined.Aliasing shiki's root bundle to a fine-grained equivalent with a 20-language allowlist, emptying the theme catalogs down to
pierre-darkandpierre-light, and stubbing the unused wasm engine brings the asset to 1.995 MiB, 25% of the cap.cppis deliberately excluded:cpp.mjspluscpp-macro.mjsare 626 KB, a fifth of the whole asset, for a language no worker here is written in.So the console's own diff rendering is now reused rather than reimplemented, with no console change and no widening of the shared contract:
Filebacks a newreadview, which is the default on open. The console's shared MonacoCodeEditoris deliberately chrome-less (lineNumbers: 'off', no minimap, no folding), which is correct for its own callers but meant code here rendered as unstyled text. Monaco stays for theeditview, since pierre is render-only.FileDiffbacks every patch, replacing a renderer that printeddiff --git,indexand@@lines as though they were file content.function-trigger-messagerender through the same component, at zero marginal bytes.adoptedStyleSheets, so there is noconsole:styleasset for it and no leakage either way.parsePatchFilesis used rather thanPatchDiff, on purpose:PatchDiffthrows unless the text yields exactly one file containing one diff, so a clean file and a multi-file patch would each take the page down.Two build guards hold the size down, and both were verified to fail on purpose rather than assumed to work:
dist/page.js. A 4 MiB budget was measured and rejected: moving@pierre/theming's private collection path adds 1.35 MB and still passes it, which is exactly the regression a guard is for. The measured table for each interceptor failing alone is in a comment above the constant.shikibump silently reverting the aliases, or a language quietly leaving the allowlist.Both run in CI:
editor/build.rsand thepnpm buildscript each invoke them.Three view details worth calling out, since they are decisions rather than defaults:
readis what opens, because opening a file should show you the file rather than put a cursor in it.unsavedappears only while something is unsaved. It diffs the draft against what was last read, so on a clean file it is guaranteed empty, and it never carries an agent's edits either: those are written to disk and arrive underhead.preview, rendered with the console's ownMarkdownPreview, the documented preview counterpart toCodeEditorthatiii-directoryalready uses for skills, prompts and registry readmes. It resolves through the import map, so it costs 687 bytes rather than the size of a markdown renderer, and it previews the draft so it tracks what is being typed.All four views share one scroll contract. A flex item defaults to
min-height: auto, so it grows rather than overflows and no scrollbar appears; and the sharedCodeEditorsets its own scrollbars to hidden and sizes its host to the content height by design, its own documentation saying to place it inside an overflow pane. pierre wants the same arrangement, since its element clips vertically and grows too.Verified on a live rig
/ui/editor/page.jsreturns 2,092,017 bytes, byte-identical to the build output.shell::fs::write, with noeditor::*call at all, is picked up and rendered as a diff.configuration::setis honoured by the next call, no restart.Notes for review
iii-permissions.yaml. Writes, the git write surface, repointing the workspace and closing another surface's buffer stay at the needs-approval default.--configseed, which is the operator's own numbers, with a warning naming exactly which values are in force. The built-in defaults are used only when nothing was seeded either, because falling back to them outright would silently loosen a limit an operator had tightened. A failed registration also means the configuration worker holds noeditorentry, so no reload event could ever arrive and the fallback would be permanent; a background retry keeps asking and publishes through the same path a reload uses.tests/integration.rsrefuses to run when an engine is already listening. It spawns its own worker, which would otherwise re-register the editor console assets on a developer's live engine and remove them again on teardown.--urlreadsIII_URL. Inside a sandbox the engine is on the VM gateway, never on the VM's loopback.edit-mode cursor-into-view when typing near the bottom of a long file. It relies on the browser scrolling Monaco's focused hidden textarea into the nearest scrollable ancestor, which is the documented arrangement but needs a live rig and a keystroke to confirm.Fixes MOT-4274
Summary by CodeRabbit
New Features
Documentation
Permissions