(MOT-4274) feat(editor): add the editor worker - #623
Open
rohitg00 wants to merge 8 commits into
Open
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
Comment on lines
+244
to
+274
| const refreshBuffers = useCallback(async () => { | ||
| try { | ||
| const ws = await api.workspace() | ||
| setBuffers(ws.buffers) | ||
| for (const buffer of ws.buffers) { | ||
| const local = draftsRef.current[buffer.path] | ||
| if (!local) continue | ||
| const file = await api.open(buffer.path) | ||
| if (file.content === local.base) continue | ||
| const edited = local.draft !== local.base | ||
| setDrafts((prev) => ({ | ||
| ...prev, | ||
| [buffer.path]: edited | ||
| ? { ...local, stale: true } | ||
| : { base: file.content, draft: file.content, truncated: file.truncated, stale: false }, | ||
| })) | ||
| setFlashed((prev) => ({ ...prev, [buffer.path]: Date.now() })) | ||
| } | ||
| } catch { | ||
| // Transient; the next tick tries again. | ||
| } | ||
| }, [api]) | ||
|
|
||
| useEffect(() => { | ||
| const id = setInterval(() => { | ||
| void refreshGit() | ||
| void refreshBuffers() | ||
| }, POLL_MS) | ||
| void refreshGit() | ||
| return () => clearInterval(id) | ||
| }, [refreshGit, refreshBuffers]) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Every open tab is fully re-read from disk every 3 s.
refreshBuffers calls api.open(buffer.path) for each open buffer on every poll tick, so N tabs cost N bus round-trips plus N full file bodies every 3 seconds — and each editor::open also has a 30 s client timeout, so a slow tick can overlap the next. The workspace already returns each buffer's mtime; compare that first and only re-read the paths whose mtime advanced.
♻️ Re-read only when mtime moved
const ws = await api.workspace()
setBuffers(ws.buffers)
for (const buffer of ws.buffers) {
const local = draftsRef.current[buffer.path]
if (!local) continue
+ if (buffer.mtime === seenMtimeRef.current[buffer.path]) continue
+ seenMtimeRef.current[buffer.path] = buffer.mtime
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 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.
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
Comment on lines
+102
to
+126
| let deadline = std::time::Instant::now() + budget; | ||
| let mut last: Option<String> = None; | ||
| while std::time::Instant::now() < deadline { | ||
| let call = client.trigger(TriggerRequest { | ||
| function_id: "editor::diff".into(), | ||
| payload: json!({ | ||
| "before": "a\nb\nc\n", | ||
| "after": "a\nB\nc\n", | ||
| "path": "sample.txt", | ||
| }), | ||
| action: None, | ||
| timeout_ms: Some(5_000), | ||
| }); | ||
| match timeout(Duration::from_secs(10), call).await { | ||
| Ok(Ok(value)) => return Some(value), | ||
| Ok(Err(e)) => { | ||
| let text = e.to_string(); | ||
| if !text.contains("not found") { | ||
| panic!("editor::diff failed: {text}"); | ||
| } | ||
| last = Some(text); | ||
| } | ||
| Err(_) => last = Some("trigger timed out".to_string()), | ||
| } | ||
| sleep(Duration::from_millis(250)).await; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Keep polling within the requested budget.
The final attempt always gets 10 seconds plus a 250 ms sleep, so a 30-second budget can take roughly 40 seconds. Cap both durations by the remaining time.
Proposed fix
while std::time::Instant::now() < deadline {
+ let remaining = deadline.saturating_duration_since(std::time::Instant::now());
let call = client.trigger(TriggerRequest {
function_id: "editor::diff".into(),
payload: json!({
@@
- match timeout(Duration::from_secs(10), call).await {
+ match timeout(remaining.min(Duration::from_secs(10)), call).await {
@@
- sleep(Duration::from_millis(250)).await;
+ let remaining = deadline.saturating_duration_since(std::time::Instant::now());
+ sleep(remaining.min(Duration::from_millis(250))).await;
}📝 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
| let deadline = std::time::Instant::now() + budget; | |
| let mut last: Option<String> = None; | |
| while std::time::Instant::now() < deadline { | |
| let call = client.trigger(TriggerRequest { | |
| function_id: "editor::diff".into(), | |
| payload: json!({ | |
| "before": "a\nb\nc\n", | |
| "after": "a\nB\nc\n", | |
| "path": "sample.txt", | |
| }), | |
| action: None, | |
| timeout_ms: Some(5_000), | |
| }); | |
| match timeout(Duration::from_secs(10), call).await { | |
| Ok(Ok(value)) => return Some(value), | |
| Ok(Err(e)) => { | |
| let text = e.to_string(); | |
| if !text.contains("not found") { | |
| panic!("editor::diff failed: {text}"); | |
| } | |
| last = Some(text); | |
| } | |
| Err(_) => last = Some("trigger timed out".to_string()), | |
| } | |
| sleep(Duration::from_millis(250)).await; | |
| let deadline = std::time::Instant::now() + budget; | |
| let mut last: Option<String> = None; | |
| while std::time::Instant::now() < deadline { | |
| let remaining = deadline.saturating_duration_since(std::time::Instant::now()); | |
| let call = client.trigger(TriggerRequest { | |
| function_id: "editor::diff".into(), | |
| payload: json!({ | |
| "before": "a\nb\nc\n", | |
| "after": "a\nB\nc\n", | |
| "path": "sample.txt", | |
| }), | |
| action: None, | |
| timeout_ms: Some(5_000), | |
| }); | |
| match timeout(remaining.min(Duration::from_secs(10)), call).await { | |
| Ok(Ok(value)) => return Some(value), | |
| Ok(Err(e)) => { | |
| let text = e.to_string(); | |
| if !text.contains("not found") { | |
| panic!("editor::diff failed: {text}"); | |
| } | |
| last = Some(text); | |
| } | |
| Err(_) => last = Some("trigger timed out".to_string()), | |
| } | |
| let remaining = deadline.saturating_duration_since(std::time::Instant::now()); | |
| sleep(remaining.min(Duration::from_millis(250))).await; |
🤖 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 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.
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
Comment on lines
+188
to
+200
| pub fn register_changed_trigger(iii: &Arc<IIIClient>) -> ChangedEmitter { | ||
| let subscribers = SubscriberSet::new(); | ||
| let _ = iii.register_trigger_type(RegisterTriggerType::new( | ||
| CHANGED, | ||
| "Fires when a file in the workspace changes, whoever changed it — \ | ||
| including an agent that never called this worker.", | ||
| ChangedTriggerHandler { | ||
| subscribers: subscribers.clone(), | ||
| }, | ||
| )); | ||
| tracing::info!(trigger_type = CHANGED, "registered trigger type"); | ||
| ChangedEmitter::new(iii.clone(), subscribers) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Swallowed register_trigger_type result hides a fully broken push-event feature.
The Result is discarded and "registered trigger type" is logged unconditionally, so if registration fails (e.g. a restart race re-registering the same type), editor::changed becomes silently undeliverable — ChangedEmitter::emit will just no-op (line 141-144) with only a debug! never seen in production, and there is no error signal anywhere. This is the trigger type the entire push-based UI feature depends on. observe::bind right next to this already follows the correct pattern (matching on the Result and logging warn on failure) — this call should do the same.
🐛 Proposed fix
pub fn register_changed_trigger(iii: &Arc<IIIClient>) -> ChangedEmitter {
let subscribers = SubscriberSet::new();
- let _ = iii.register_trigger_type(RegisterTriggerType::new(
+ match iii.register_trigger_type(RegisterTriggerType::new(
CHANGED,
"Fires when a file in the workspace changes, whoever changed it — \
including an agent that never called this worker.",
ChangedTriggerHandler {
subscribers: subscribers.clone(),
},
- ));
- tracing::info!(trigger_type = CHANGED, "registered trigger type");
+ )) {
+ Ok(_) => tracing::info!(trigger_type = CHANGED, "registered trigger type"),
+ Err(e) => tracing::warn!(
+ error = %e,
+ trigger_type = CHANGED,
+ "failed to register the editor::changed trigger type; push updates will not work"
+ ),
+ }
ChangedEmitter::new(iii.clone(), subscribers)
}📝 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
| pub fn register_changed_trigger(iii: &Arc<IIIClient>) -> ChangedEmitter { | |
| let subscribers = SubscriberSet::new(); | |
| let _ = iii.register_trigger_type(RegisterTriggerType::new( | |
| CHANGED, | |
| "Fires when a file in the workspace changes, whoever changed it — \ | |
| including an agent that never called this worker.", | |
| ChangedTriggerHandler { | |
| subscribers: subscribers.clone(), | |
| }, | |
| )); | |
| tracing::info!(trigger_type = CHANGED, "registered trigger type"); | |
| ChangedEmitter::new(iii.clone(), subscribers) | |
| } | |
| pub fn register_changed_trigger(iii: &Arc<IIIClient>) -> ChangedEmitter { | |
| let subscribers = SubscriberSet::new(); | |
| match iii.register_trigger_type(RegisterTriggerType::new( | |
| CHANGED, | |
| "Fires when a file in the workspace changes, whoever changed it — \ | |
| including an agent that never called this worker.", | |
| ChangedTriggerHandler { | |
| subscribers: subscribers.clone(), | |
| }, | |
| )) { | |
| Ok(_) => tracing::info!(trigger_type = CHANGED, "registered trigger type"), | |
| Err(e) => tracing::warn!( | |
| error = %e, | |
| trigger_type = CHANGED, | |
| "failed to register the editor::changed trigger type; push updates will not work" | |
| ), | |
| } | |
| ChangedEmitter::new(iii.clone(), subscribers) | |
| } |
🤖 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/events.rs` around lines 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.
Comment on lines
+268
to
+299
| const { onState, onChanged } = useWorkspaceEvents(host) | ||
|
|
||
| // The workspace record lives in `state`, so every buffer and expansion change | ||
| // arrives as an event. One read on mount seeds the git overlay; after that | ||
| // nothing is polled. | ||
| useEffect(() => { | ||
| void refreshGit() | ||
| }, [refreshGit]) | ||
|
|
||
| // A change to this worker's `state` scope means the shared workspace moved: | ||
| // an agent opened or closed something, or saved. Re-read it once per event | ||
| // rather than on a timer. | ||
| useEffect( | ||
| () => | ||
| onState(() => { | ||
| void refreshBuffers() | ||
| }), | ||
| [onState, refreshBuffers], | ||
| ) | ||
|
|
||
| // A file changed, however it changed. The event carries enough to light the | ||
| // row up immediately; the git overlay is refreshed for the marks. | ||
| useEffect( | ||
| () => | ||
| onChanged((event: ChangedEvent) => { | ||
| setFlashed((prev) => ({ ...prev, [event.path]: Date.now() })) | ||
| setLastChange(event) | ||
| void refreshBuffers() | ||
| void refreshGit() | ||
| }), | ||
| [onChanged, refreshBuffers, refreshGit], | ||
| ) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Push events removed the implicit throttle on refreshBuffers/refreshGit without adding a replacement.
Previously these ran at most once per 3s poll tick. Now onState and onChanged invoke them directly per event, with no debounce/coalescing and no in-flight guard. refreshBuffers already does a full api.open() round trip per open tab (flagged in prior review); for the scenario this feature explicitly targets — "an agent writing files in a loop" — this can now fire many times per second, and unguarded overlapping calls can also let a stale response overwrite a fresher one.
♻️ Proposed fix: coalesce bursts with a short trailing debounce
+ const refreshTimer = useRef<ReturnType<typeof setTimeout> | null>(null)
+ const scheduleRefresh = useCallback(() => {
+ if (refreshTimer.current) clearTimeout(refreshTimer.current)
+ refreshTimer.current = setTimeout(() => {
+ void refreshBuffers()
+ void refreshGit()
+ }, 200)
+ }, [refreshBuffers, refreshGit])
+
useEffect(
() =>
onState(() => {
- void refreshBuffers()
+ scheduleRefresh()
}),
- [onState, refreshBuffers],
+ [onState, scheduleRefresh],
)
useEffect(
() =>
onChanged((event: ChangedEvent) => {
setFlashed((prev) => ({ ...prev, [event.path]: Date.now() }))
setLastChange(event)
- void refreshBuffers()
- void refreshGit()
+ scheduleRefresh()
}),
- [onChanged, refreshBuffers, refreshGit],
+ [onChanged, scheduleRefresh],
)📝 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
| const { onState, onChanged } = useWorkspaceEvents(host) | |
| // The workspace record lives in `state`, so every buffer and expansion change | |
| // arrives as an event. One read on mount seeds the git overlay; after that | |
| // nothing is polled. | |
| useEffect(() => { | |
| void refreshGit() | |
| }, [refreshGit]) | |
| // A change to this worker's `state` scope means the shared workspace moved: | |
| // an agent opened or closed something, or saved. Re-read it once per event | |
| // rather than on a timer. | |
| useEffect( | |
| () => | |
| onState(() => { | |
| void refreshBuffers() | |
| }), | |
| [onState, refreshBuffers], | |
| ) | |
| // A file changed, however it changed. The event carries enough to light the | |
| // row up immediately; the git overlay is refreshed for the marks. | |
| useEffect( | |
| () => | |
| onChanged((event: ChangedEvent) => { | |
| setFlashed((prev) => ({ ...prev, [event.path]: Date.now() })) | |
| setLastChange(event) | |
| void refreshBuffers() | |
| void refreshGit() | |
| }), | |
| [onChanged, refreshBuffers, refreshGit], | |
| ) | |
| const { onState, onChanged } = useWorkspaceEvents(host) | |
| const refreshTimer = useRef<ReturnType<typeof setTimeout> | null>(null) | |
| const scheduleRefresh = useCallback(() => { | |
| if (refreshTimer.current) clearTimeout(refreshTimer.current) | |
| refreshTimer.current = setTimeout(() => { | |
| void refreshBuffers() | |
| void refreshGit() | |
| }, 200) | |
| }, [refreshBuffers, refreshGit]) | |
| // The workspace record lives in `state`, so every buffer and expansion change | |
| // arrives as an event. One read on mount seeds the git overlay; after that | |
| // nothing is polled. | |
| useEffect(() => { | |
| void refreshGit() | |
| }, [refreshGit]) | |
| // A change to this worker's `state` scope means the shared workspace moved: | |
| // an agent opened or closed something, or saved. Re-read it once per event | |
| // rather than on a timer. | |
| useEffect( | |
| () => | |
| onState(() => { | |
| scheduleRefresh() | |
| }), | |
| [onState, scheduleRefresh], | |
| ) | |
| // A file changed, however it changed. The event carries enough to light the | |
| // row up immediately; the git overlay is refreshed for the marks. | |
| useEffect( | |
| () => | |
| onChanged((event: ChangedEvent) => { | |
| setFlashed((prev) => ({ ...prev, [event.path]: Date.now() })) | |
| setLastChange(event) | |
| scheduleRefresh() | |
| }), | |
| [onChanged, scheduleRefresh], | |
| ) |
🤖 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 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.
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
Comment on lines
+114
to
+119
| let cfg = loaded.ok_or_else(|| { | ||
| anyhow::anyhow!( | ||
| "{}", | ||
| last_err.unwrap_or_else(|| "editor configuration unavailable".to_string()) | ||
| ) | ||
| })?; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Avoid terminating the worker when configuration bootstrap fails.
After five retries, this path exits instead of serving with WorkerConfig::default(). A temporary configuration-worker outage can therefore turn into an editor restart loop. Preserve the retry loop, but fall back to a safe default unless this worker is intentionally exempt from the repository-wide convention.
Based on learnings, worker binaries in this repository should warn and fall back to WorkerConfig::default() when configuration loading fails.
🤖 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 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.
Source: Learnings
Comment on lines
+876
to
+883
| function FileView({ path, contents, themeType }: { path: string; contents: string; themeType: 'light' | 'dark' }) { | ||
| const file = useMemo(() => ({ name: path, contents, cacheKey: `${path}:${contents.length}` }), [contents, path]) | ||
| return ( | ||
| <div className="ed-reader"> | ||
| <File file={file} options={{ ...READ_OPTIONS, themeType }} disableWorkerPool /> | ||
| </div> | ||
| ) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Pierre cache keys are derived from content length alone in all three wrappers. Every key handed to pierre's render cache uses contents.length / patch.length as its only content-dependent component, so two distinct payloads of equal length alias to the same cache entry and pierre serves the earlier render. Introduce one cheap content-hash helper (a 32-bit rolling hash is O(n) once per memo recompute) and use it at each site.
editor/ui/src/page/index.tsx#L876-L883: add the helper here and buildFileView'scacheKeyas${path}:${hash(contents)}so an equal-length edit invalidates the cached render.editor/ui/src/page/index.tsx#L898-L918: passp${hash(patch)}asPatchView'scacheKeyPrefixinstead ofp${patch.length}.editor/ui/src/function-trigger-message/index.tsx#L79-L103: same substitution inPatch, where equal-length patches from different feed messages are the most likely collision.
📍 Affects 2 files
editor/ui/src/page/index.tsx#L876-L883(this comment)editor/ui/src/page/index.tsx#L898-L918editor/ui/src/function-trigger-message/index.tsx#L79-L103
🤖 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 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.
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.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.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.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.Filelayout inside the[data-iii-ui="editor"]wrapper at narrow viewport widths. The container is a best guess and is the first thing to check.Fixes MOT-4274
Summary by CodeRabbit
New Features
Documentation
Permissions