fix: iced-migration bug closeout sweep (plan 014) - #301
Conversation
iced's ButtonPressed carries no position, and iced_winit drains an event batch with a single batch-newest cursor — so a seam press made during pointer motion was hit-tested at a position the pointer only reached after the press, missing the ±3px grip zone (issue #295). The grip now records the last CursorMoved's own position (events process in receipt order, so at press time it holds where the pointer was when the press fired) and gates presses on that anchor, falling back to the batch cursor before any observed motion. Hardened per review: the anchor drops on CursorLeft, and only ever replaces an available batch cursor, so an unavailable-cursor press stays a no-op. The ReorderStrip half of #295 is deliberately NOT fixed: both strips sit under iced scrollables, which pass children a translated cursor but the raw untranslated event, so an event-position anchor is wrong by the scroll offset — split to #300 with the analysis. The real-input harness dwell that existed solely as the workaround (iced_clipboard_check.py, commit 2671ba1) is removed; the cage-tier run now proves the fix. Grip unit tests drive the real update() path (the precomputed in_zone test shortcut is gone) — negative controls confirmed each new test fails with its guard reverted. Cursor review: strip coordinate-space + anchor-staleness findings → strip reverted, grip hardened (this shape). Simplify pass: no changes. Closes #295 (plan 014 C1) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WLKWsLV45DAk61xG6Utj3e
PR #251's replacement grab zone was symmetric ±2px — tight enough to stop the paned stealing column-0 terminal selections, but too narrow to grab reliably ("resizing seems to only work sometimes"). The two goals only conflict on the terminal side, so the zone is now asymmetric: 4px into the sidebar, 2px past the separator's terminal edge (unchanged, selection still wins). 4px, not the 6-10px the issue floated: the claim fires on press (capture phase), and the sidebar ScrolledWindow's overlay scrollbar sits flush against the seam with a hovering interactive column of ~14-16px (verified against the compiled Adwaita stylesheet) — wider bands steal proportionally more of it. 4px doubles the grabbable sidebar-side width while leaving ~12px of scrollbar reachable. The harness layout cannot scroll the project list, so scrollbar overlap is a production-only tradeoff, recorded in plan 014. The cage-tier check gains a sidebar-side range probe (sep-6..sep-3, mirroring the terminal side's drift-buffer scan): at least one offset must resize, then the seam is restored from a press guaranteed inside the new band (review finding: undoing from the original press offset missed the band after drift-shifted hits). Cursor review: probe-undo drift bug fixed; stale fn-doc comment updated; bounds math confirmed clean. Closes #252 (plan 014 C2) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WLKWsLV45DAk61xG6Utj3e
The Swift app now answers the two optional app.window_metrics fields both Rust UIs already report (#278 added them; the Mac omission was the parity gap). A UiBridge terminalMetrics() accessor mirrors sidebarMetrics(), delegating to a pure static function so the geometry contract is unit-testable headless: terminal_top is the active terminal view's offset from the content view's TOP (bounds.height - converted maxY — AppKit content views are unflipped, so origin.y alone would be the bottom offset), and the family is the NSFont actually in use (familyName over fontName), matching the resolved-family semantics on iced/GTK. Both fields omit cleanly when no terminal is mounted, per the optional wire contract. XCTest coverage pins the flip derivation against constructed layouts (including a regression guard proving origin.y-based math would fail) plus nil-tolerance; test_sidebar_resize.py now asserts the family is a non-empty string on every target and terminal_top is positive when present (GTK legitimately omits it). ipc.md updated. Cursor review: no findings (convert() flip handling verified empirically, accessor matches the codebase's active-tab pattern, nil-omission confirmed on the wire). Closes #287 (plan 014 C3) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WLKWsLV45DAk61xG6Utj3e
…ranch Security-adjacent closeout of the drop-path divergence: both languages now reject dropped paths AND dragged URLs carrying any of LF, VT, FF, CR, NEL, LS, PS, or ESC — Swift's Character.isNewline scalar classes plus ESC, expressed in Rust as an explicit 8-scalar const (Rust ends up strictly stricter on pathological grapheme clusters, which only ever rejects more). Swift's dragged-URL branch — previously completely unfiltered, letting escape bytes reach the PTY — now applies the same predicate as the path branch; a rejected URL is treated as absent and falls through to the plain-string fallback, which stays deliberately unfiltered (reject, don't strip: #280's bracketed-paste mitigations own that boundary). Rust resolve() gains the equivalent url branch (paths → url → text, mirroring Swift) for cross-UI parity; both Rust call sites pass None — no toolkit surfaces a distinct URL drop payload yet, so the branch is production-dead and its tests are its only exercise (by design). Every rejected class has cross-pinned twin vectors in both test suites, with individual VT/FF vectors on the Rust side (the classes it newly rejects). Adversarial review's one finding is pinned as a documenting test on both sides: a drag populating .URL and .string with the same control-bearing text reaches the PTY via the string arm — the accepted #282 baseline, made explicit rather than incidental. Closes #282 (plan 014 C4) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WLKWsLV45DAk61xG6Utj3e
…n lint gate The last clippy type_complexity in roost-linux was ipc_window_metrics's six-field tuple return. Instead of a type alias over the same tuple, the function (and the shared WindowMetricsReply channel in roost-engine) now carries roost_ipc's WindowMetricsResult directly — the wire struct already names every field, and the dispatch arm's destructure-and- rebuild step disappears (encode(&result), mirroring SidebarDump). The iced handler builds the struct at its reply site for the same reason. With roost-linux clippy-clean, the gtk-build job's narrow denylist step (-A warnings -D disallowed_types -D disallowed_methods) becomes the same full -D warnings gate every other crate gets; Makefile mirrors. The GtkDnD (#236) and grab_focus (#234) guards lose nothing: both disallowed lints are warn-by-default, so -D warnings still enforces them, and clippy.toml keeps the full rationale as the source of truth. rust-lint's exclusion stays (no GTK toolchain there). Both panel reviewers independently confirmed this was the sole remaining warning, so the tightened gate is green from this commit on. Cursor CLI returned empty on review (flaking); self-review verified field mapping against the old positional order at all three construction sites, identical wire JSON via the shared struct, YAML validity, and make clippy end to end. Closes #283 (plan 014 C5) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WLKWsLV45DAk61xG6Utj3e
…g it The shed cage run proved C2's sidebar-side probe wrong by design: the separator's screen x carries an unknown CSD margin (the reason the terminal-side scan probes a range), so a fixed sb-relative range misses the 4px sidebar-side band for most margin values — a screen-coordinate probe cannot discriminate the widening. The probe is removed; the claim bounds are extracted into a pure paned_claims_press(x, pos, sep_end) and unit-pinned at all four boundary points (claims pos-4 / sep_end+2, denies pos-5 / sep_end+3), which fails deterministically on any revert to a symmetric zone. The cage tier keeps its drift-tolerant checks: seam resizes, left-edge drag still selects. Refs #252 (plan 014 C2 follow-up; shed verification finding) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WLKWsLV45DAk61xG6Utj3e
📝 WalkthroughWalkthroughThe PR adds structured terminal metrics to ChangesRoost functional updates
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant IcedApp
participant RoostLinux
participant UiBridge
participant IPCHandler
IcedApp->>RoostLinux: request app.window_metrics
RoostLinux->>UiBridge: obtain terminal metrics
UiBridge-->>RoostLinux: terminal top and font family
RoostLinux->>IPCHandler: encode WindowMetricsResult
IPCHandler-->>IcedApp: optional terminal metrics response
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
…eview) Compensating-review findings applied (the PR's CodeRabbit check was rate-limited): the terminal_top e2e assert now distinguishes an omitted key from an explicit null (the wire contract is omit-not-null, and serde emits null for non-finite f64s — exactly the arm .get() flattened away) and requires a finite value; the parity asserts moved after _live_tab so the Mac fields are read with a terminal mounted instead of racing the async first-tab spawn; ipc.md no longer overstates terminal_font_family as unconditional on Mac; the grab-zone doc names the overlay-scrollbar tradeoff the 4px bound exists for; the grip comment records the accepted overlay-staleness window; clippy.toml quotes the gate verbatim. Dispositions: scrollbar overlap remains the plan-recorded accepted tradeoff (now stated at the code site too); XCTest-vs-swift-testing findings declined per the documented Xcode 26 SIGABRT constraint. Refs #287 #252 #295 (plan 014, post-review) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WLKWsLV45DAk61xG6Utj3e
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@crates/roost-iced/src/sidebar_resize.rs`:
- Around line 98-120: Invalidate state.last_cursor at overlay boundaries so
cursor movement consumed by an iced overlay cannot leave a stale anchor for the
next press. Update the sidebar resize event/state flow around the existing
anchored cursor logic to recognize the overlay transition and fall back to the
current batch cursor, preserving normal in-grip movement behavior. Add coverage
for seam movement, overlay pointer movement, and a subsequent terminal-side
press ensuring no resize gesture starts from the stale position.
In `@crates/roost-linux/src/terminal_view.rs`:
- Around line 2531-2532: Update the drop handling around the String branch in
the terminal view to inspect the GdkDrop/URI MIME type explicitly: read
text/uri-list through GtkDropTargetAsync/GdkDrop and pass it to
drop_content::resolve as url, while retaining text/plain handling as text.
Ensure URI drops receive the URL filtering and escaping behavior in
drop_content::resolve.
In `@mac/Tests/RoostTests/ShellEscapeTests.swift`:
- Around line 98-136: The added ShellEscapeTests cases must use swift-testing
rather than XCTest. In mac/Tests/RoostTests/ShellEscapeTests.swift lines 98-136
and 173-228, convert the affected test methods to `@Test` functions and replace
XCTAssertEqual/XCTAssertNil assertions with `#expect`, preserving all existing
cases and expectations; do not add further XCTestCase coverage.
In `@mac/Tests/RoostTests/WindowMetricsTests.swift`:
- Around line 12-23: Convert WindowMetricsTests from XCTestCase to the
swift-testing framework by replacing the XCTest import and test class structure
with a `@Suite` and individual `@Test` cases, preserving the existing coverage and
`@MainActor` requirements. Do not retain the XCTest workaround or bypass the
test-target standard; address any Xcode 26.x runner crash separately.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 066e0586-ca37-4050-b7a4-ff51cf7a1528
📒 Files selected for processing (21)
.github/workflows/ci.ymlMakefileclippy.tomlcrates/roost-engine/src/ipc.rscrates/roost-iced/src/app.rscrates/roost-iced/src/app/interactions.rscrates/roost-iced/src/app/servicing.rscrates/roost-iced/src/sidebar_resize.rscrates/roost-linux/src/app.rscrates/roost-linux/src/terminal_view.rscrates/roost-ui-model/src/drop_content.rsdocs/reference/ipc.mdmac/Sources/Roost/App.swiftmac/Sources/Roost/IPCHandlerImpl.swiftmac/Sources/Roost/RoostBackend.swiftmac/Sources/Roost/TerminalView.swiftmac/Tests/RoostTests/ShellEscapeTests.swiftmac/Tests/RoostTests/WindowMetricsTests.swifttools/input/linux/iced_clipboard_check.pytools/input/linux/real_input_check.pytools/roosttest/test_sidebar_resize.py
💤 Files with no reviewable changes (1)
- tools/input/linux/iced_clipboard_check.py
| // Recorded before every early return: `ButtonPressed` carries no position | ||
| // of its own, and iced hit-tests it against the newest cursor of the batch | ||
| // it was drained with. A frame behind, that is wherever the pointer | ||
| // travelled *after* the button went down, so the last move the grip | ||
| // actually saw is the honest press anchor (issue #295). A pointer that | ||
| // left the window invalidates it — the next entry can land anywhere, and | ||
| // no move need be observed before the press. Moves consumed by an iced | ||
| // overlay never reach the grip and fire no CursorLeft, so the anchor can | ||
| // sit stale across an overlay session; accepted — the first post-overlay | ||
| // move refreshes it, and a press before that is a narrow window. | ||
| match event { | ||
| Event::Mouse(mouse::Event::CursorMoved { position, .. }) => { | ||
| state.last_cursor = Some(*position); | ||
| } | ||
| Event::Mouse(mouse::Event::CursorLeft) => state.last_cursor = None, | ||
| _ => {} | ||
| } | ||
| // Anchoring only ever *replaces* an available batch cursor. With no cursor | ||
| // at all the grip has always been a no-op, and a stale anchor must not | ||
| // start arming presses it used to ignore. | ||
| let anchored = cursor | ||
| .position() | ||
| .map(|batch| state.last_cursor.unwrap_or(batch)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Invalidate the cursor anchor at overlay boundaries.
crates/roost-iced/src/sidebar_resize.rs:105-120 retains last_cursor when an overlay consumes CursorMoved and CursorLeft. The next press then prefers that stale seam position over the available batch cursor. A first terminal press after an overlay can start a resize gesture.
Clear the anchor before an overlay can consume pointer events, or pass an overlay-generation boundary into this state. Add a test for seam movement, overlay pointer movement, then a terminal-side press.
🤖 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 `@crates/roost-iced/src/sidebar_resize.rs` around lines 98 - 120, Invalidate
state.last_cursor at overlay boundaries so cursor movement consumed by an iced
overlay cannot leave a stale anchor for the next press. Update the sidebar
resize event/state flow around the existing anchored cursor logic to recognize
the overlay transition and fall back to the current batch cursor, preserving
normal in-grip movement behavior. Add coverage for seam movement, overlay
pointer movement, and a subsequent terminal-side press ensuring no resize
gesture starts from the stale position.
| if let Ok(s) = value.get::<String>() { | ||
| return roost_ui_model::drop_content::resolve(std::iter::empty::<&Path>(), Some(&s)); | ||
| return roost_ui_model::drop_content::resolve(std::iter::empty::<&Path>(), None, Some(&s)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 'DropTarget::new|set_types|connect_drop|drop_value_to_text|drop_content::resolve' \
crates/roost-linux/src/terminal_view.rs \
crates/roost-ui-model/src/drop_content.rsRepository: charliek/roost
Length of output: 5913
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- drop_content.rs ---'
sed -n '1,180p' crates/roost-ui-model/src/drop_content.rs
printf '%s\n' '--- terminal_view.rs drop helpers ---'
sed -n '2460,2540p' crates/roost-linux/src/terminal_view.rs
printf '%s\n' '--- related tests/usages ---'
rg -n -C 5 'drop_content|drop_value_to_text|text/uri|DropTarget|URI|url' \
crates/roost-linux crates/roost-ui-model --glob '*.rs' --glob '*.toml'
printf '%s\n' '--- current diff summary ---'
git diff --statRepository: charliek/roost
Length of output: 50370
🌐 Web query:
GTK4 DropTarget connect_drop GValue String text/uri-list distinguish offered MIME type GdkDrop formats
💡 Result:
In GTK4, GtkDropTarget is designed primarily for simplified, synchronous data reception using GTypes [1][2]. When you use the GtkDropTarget::drop signal, the GValue provided to your callback has already been deserialized from the underlying MIME type into the specified GType by GTK's automatic content deserialization system [3][4]. To distinguish or handle specific MIME types that are not automatically mapped to a convenient GType, or to manage complex data transfers, you have two primary approaches: 1. Use GtkDropTargetAsync: If you need control over the data transfer process, including selecting specific MIME types or performing asynchronous reads, use GtkDropTargetAsync instead of GtkDropTarget [2]. This object allows you to handle the GdkDrop object directly [4]. 2. Access the GdkDrop Object: When using a GtkDropTarget (or GtkDropTargetAsync), you can retrieve the underlying GdkDrop object (usually accessible via the controller's state or signals). From the GdkDrop object, you can: - Use gdk_drop_get_formats to inspect the GdkContentFormats available for the drop [5][6]. - Use gdk_content_formats_contain_mime_type to check if specific MIME types (like text/uri-list) are present [7]. - Use gdk_drop_read_async to manually read the data as a specific MIME type if necessary [6][8]. For common types like text/uri-list, GtkDropTarget typically supports the GDK_TYPE_FILE_LIST GType [3]. You can set this type on your GtkDropTarget using gtk_drop_target_set_gtypes, and the GValue provided to your::drop signal handler will be a boxed GdkFileList, which you can then process using gdk_file_list_get_files [3]. In summary, you do not typically distinguish MIME types inside the standard GtkDropTarget::drop signal because that signal's GValue is the result of a deserialization process that has already committed to a GType [1][4]. To work with raw MIME types, you must step down to the GdkDrop and GdkContentFormats API [7][4].
Citations:
- 1: https://docs.gtk.org/gtk4/class.DropTarget.html
- 2: https://docs.gtk.org/gtk4/drag-and-drop.html
- 3: https://developer.gnome.org/documentation/tutorials/drag-and-drop.html
- 4: https://blogs.gnome.org/gtk/2020/01/29/data-transfer-in-gtk4/
- 5: https://docs.gtk.org/gdk4/method.Drop.get_formats.html
- 6: https://docs.gtk.org/gdk4/class.Drop.html
- 7: https://docs.gtk.org/gdk4/struct.ContentFormats.html
- 8: https://docs.gtk.org/gdk4/method.Drop.read_async.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- GTK dependency and API references in repository ---'
rg -n -C 4 'gtk4|gdk4|DropTarget|GdkContentFormats|content_formats|current_drop|GdkDrop' \
Cargo.toml crates/*/Cargo.toml crates/roost-linux/src --glob '*.rs' --glob '*.toml' \
| head -n 240
printf '%s\n' '--- all drop_content resolver call sites ---'
rg -n -C 3 'drop_content::resolve|fn drop_value_to_text|install_file_drop' \
crates --glob '*.rs'
printf '%s\n' '--- remaining drop_content URL tests ---'
sed -n '175,270p' crates/roost-ui-model/src/drop_content.rsRepository: charliek/roost
Length of output: 28630
🌐 Web query:
site:docs.gtk.org/gtk4/class.DropTarget.html current-drop get_formats GtkDropTarget drop signal value type
💡 Result:
The GtkDropTarget::drop signal does not pass the dropped data directly as a signal argument [1]. Instead, when the drop signal is emitted, the data has been retrieved and can be accessed via the GtkDropTarget:value property or the gtk_drop_target_get_value function [1]. The value returned by gtk_drop_target_get_value is a GValue [1]. The type of this GValue is determined by the GType specified when the GtkDropTarget was initialized using gtk_drop_target_new [1]. To manage data formats, gtk_drop_target_get_formats returns a GdkContentFormats object, which defines the formats the target is configured to accept [1]. The current drop operation can be retrieved via the current-drop property or the gtk_drop_target_get_drop function [1].
Citations:
🌐 Web query:
site:gtk-rs.org gtk4 0.10 DropTarget current_drop GdkDrop read_value_async
💡 Result:
In GTK4, when using Gtk::DropTarget, you can access the current drag-and-drop operation via the current_drop method [1][2]. This returns a gdk::Drop object representing the ongoing drop [1][2]. If you need to asynchronously read the data from this drop, you can use the read_value_async method provided by the Gdk::Drop struct [3][4]. Key details regarding these components: 1. Gtk::DropTarget::current_drop: This method retrieves the gdk::Drop currently being handled by the drop target [1][2]. If no drop is in progress, it returns None [1][2]. Note that in older versions of GTK4, this was accessed via a method simply called drop, which has been deprecated since version 4.4 in favor of current_drop [2][5]. 2. Gdk::Drop::read_value_async: This method is used to asynchronously request the contents of the drag operation converted to a specific GType [3][4]. For local drag-and-drop operations where the data matches the requested GType, the value is copied directly [3][4]. For other cases, GDK attempts to use content_deserialize_async to convert the data [3][4]. While Gtk::DropTarget provides a higher-level abstraction that is generally preferred for GTK applications, these underlying Gdk::Drop methods allow for more direct, asynchronous access to the transferred data when necessary [3][6].
Citations:
- 1: https://gtk-rs.org/gtk4-rs/stable/0.10/docs/gtk4/struct.DropTarget.html
- 2: https://gtk-rs.org/gtk4-rs/stable/0.10/docs/src/gtk4/auto/drop_target.rs.html
- 3: https://gtk-rs.org/gtk4-rs/stable/0.10/docs/gdk4/struct.Drop.html
- 4: https://gtk-rs.org/gtk4-rs/stable/0.10/docs/src/gdk4/auto/drop.rs.html
- 5: https://gtk-rs.org/gtk4-rs/stable/latest/docs/src/gtk4/auto/drop_target.rs.html
- 6: https://gtk-rs.org/gtk4-rs/stable/latest/docs/gdk4/struct.Drop.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- lockfile versions ---'
rg -n -A3 -B2 '^name = "(gtk4|gdk4|glib)"' Cargo.lock
printf '%s\n' '--- project policy files ---'
git ls-files | rg '(^|/)(CLAUDE\.md|clippy\.toml)$' | xargs -r -n1 sh -c 'echo "--- $0"; sed -n "1,180p" "$0"'
printf '%s\n' '--- drop target call context ---'
sed -n '540,575p' crates/roost-linux/src/terminal_view.rs
sed -n '2484,2536p' crates/roost-linux/src/terminal_view.rsRepository: charliek/roost
Length of output: 17508
Read URI drops as url, not text.
crates/roost-linux/src/terminal_view.rs:2531-2532 routes link drops delivered as String through the verbatim text branch. Use GtkDropTargetAsync/GdkDrop to read text/uri-list explicitly and pass it as url; keep text/plain on the text path. This enables URL filtering and escaping in crates/roost-ui-model/src/drop_content.rs:68-69.
🤖 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 `@crates/roost-linux/src/terminal_view.rs` around lines 2531 - 2532, Update the
drop handling around the String branch in the terminal view to inspect the
GdkDrop/URI MIME type explicitly: read text/uri-list through
GtkDropTargetAsync/GdkDrop and pass it to drop_content::resolve as url, while
retaining text/plain handling as text. Ensure URI drops receive the URL
filtering and escaping behavior in drop_content::resolve.
| /// Shared with the Rust `control_bearing_url_falls_through_to_text` vector: | ||
| /// a rejected URL is absent, not stripped, so the deliberately unfiltered | ||
| /// string fallback answers instead. | ||
| func testControlBearingURLFallsThroughToString() { | ||
| for control in ["\n", "\u{0B}", "\u{0C}", "\r", "\u{85}", "\u{2028}", "\u{2029}", "\u{1B}"] { | ||
| XCTAssertEqual( | ||
| TerminalView.dropContentString( | ||
| fileURLs: [], url: "https://example.com/\(control)evil", string: "fallback" | ||
| ), | ||
| "fallback", | ||
| "url bearing \(control.unicodeScalars.map(\.value)) should fall through" | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| /// Shared with the Rust `control_bearing_url_and_text_yields_raw_text` | ||
| /// vector. Documents the accepted #282 baseline: a drag can populate both | ||
| /// `.URL` and `.string` with the same control-bearing text, and the | ||
| /// rejected URL then falls through to the deliberately unfiltered string | ||
| /// arm, so the raw text reaches the PTY. That plain-text boundary is owned | ||
| /// by #280's bracketed-paste mitigations — and because we reject rather | ||
| /// than strip, the URL arm must not launder the payload into an escaped | ||
| /// form here either. | ||
| func testControlBearingURLAndStringYieldsRawString() { | ||
| let payload = "https://example.com/\u{1B}[201~evil" | ||
| XCTAssertEqual( | ||
| TerminalView.dropContentString(fileURLs: [], url: payload, string: payload), | ||
| payload | ||
| ) | ||
| } | ||
|
|
||
| /// Shared with the Rust `control_bearing_url_without_text_is_none` vector. | ||
| func testControlBearingURLWithoutStringIsNil() { | ||
| XCTAssertNil( | ||
| TerminalView.dropContentString( | ||
| fileURLs: [], url: "https://example.com/\u{1B}[201~evil", string: nil | ||
| ) | ||
| ) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 2 'import XCTest|XCTestCase|XCTAssert|XCTAssertNil|import Testing|`@Test`|`#expect`' \
mac/Tests/RoostTests/ShellEscapeTests.swiftRepository: charliek/roost
Length of output: 7616
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- test-file inventory ---'
git ls-files 'mac/Tests/RoostTests/*' | sed -n '1,120p'
printf '%s\n' '--- ShellEscapeTests.swift header and suite declarations ---'
sed -n '1,90p' mac/Tests/RoostTests/ShellEscapeTests.swift
rg -n '^(import (Testing|XCTest)|`@Suite`|`@Test`|final class .*Tests|struct .*Tests|XCTestCase)' mac/Tests/RoostTests
printf '%s\n' '--- project guidance ---'
find .. -name CLAUDE.md -print 2>/dev/null | sed -n '1,40p'Repository: charliek/roost
Length of output: 44911
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- CLAUDE.md ---'
cat ../git/CLAUDE.md
printf '%s\n' '--- test manifest references ---'
find mac -maxdepth 3 -type f \( -name 'Package.swift' -o -name '*.pbxproj' -o -name 'project.yml' \) -print
rg -n -i 'swift-testing|xctest|testing' mac/Package.swift mac 2>/dev/null | sed -n '1,160p'
printf '%s\n' '--- current diff for the reviewed file ---'
git diff --unified=3 -- mac/Tests/RoostTests/ShellEscapeTests.swift | sed -n '1,320p'
printf '%s\n' '--- framework classification for RoostTests ---'
python3 - <<'PY'
from pathlib import Path
import re
root = Path("mac/Tests/RoostTests")
for path in sorted(root.glob("*.swift")):
text = path.read_text()
frameworks = []
if re.search(r'^\s*import\s+XCTest\b', text, re.M) or "XCTestCase" in text:
frameworks.append("XCTest")
if re.search(r'^\s*import\s+Testing\b', text, re.M) or re.search(r'^\s*`@Test`\b', text, re.M):
frameworks.append("swift-testing")
if path.name == "ShellEscapeTests.swift" or "XCTest" in frameworks:
print(f"{path}: {', '.join(frameworks) or 'none'}")
PYRepository: charliek/roost
Length of output: 25303
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- exact cited ranges ---'
nl -ba mac/Tests/RoostTests/ShellEscapeTests.swift | sed -n '1,25p;95,140p;168,232p'
printf '%s\n' '--- XCTest exception rationale in the test suite ---'
for file in mac/Tests/RoostTests/BracketedPasteTests.swift \
mac/Tests/RoostTests/ShellEscapeTests.swift \
mac/Tests/RoostTests/WindowMetricsTests.swift; do
printf '\n[%s]\n' "$file"
nl -ba "$file" | sed -n '1,28p'
done
printf '%s\n' '--- Swift test target configuration ---'
nl -ba mac/Package.swift | sed -n '1,220p'Repository: charliek/roost
Length of output: 219
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- exact cited ranges ---'
cat -n mac/Tests/RoostTests/ShellEscapeTests.swift | sed -n '1,25p;95,140p;168,232p'
printf '%s\n' '--- XCTest exception rationale in the test suite ---'
for file in mac/Tests/RoostTests/BracketedPasteTests.swift \
mac/Tests/RoostTests/ShellEscapeTests.swift \
mac/Tests/RoostTests/WindowMetricsTests.swift; do
printf '\n[%s]\n' "$file"
cat -n "$file" | sed -n '1,28p'
done
printf '%s\n' '--- Swift test target configuration ---'
cat -n mac/Package.swift | sed -n '1,220p'Repository: charliek/roost
Length of output: 19968
Migrate the added cases to swift-testing. mac/Tests/RoostTests/ShellEscapeTests.swift:6-11 documents a runner workaround, but CLAUDE.md requires swift-testing under this directory. Convert mac/Tests/RoostTests/ShellEscapeTests.swift:101-135 and :174-228 to @Test and #expect; do not add further XCTestCase coverage.
📍 Affects 1 file
mac/Tests/RoostTests/ShellEscapeTests.swift#L98-L136(this comment)mac/Tests/RoostTests/ShellEscapeTests.swift#L173-L228
🤖 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 `@mac/Tests/RoostTests/ShellEscapeTests.swift` around lines 98 - 136, The added
ShellEscapeTests cases must use swift-testing rather than XCTest. In
mac/Tests/RoostTests/ShellEscapeTests.swift lines 98-136 and 173-228, convert
the affected test methods to `@Test` functions and replace
XCTAssertEqual/XCTAssertNil assertions with `#expect`, preserving all existing
cases and expectations; do not add further XCTestCase coverage.
Source: Coding guidelines
| // XCTest, not swift-testing: a swarm of fast value-checks in the | ||
| // swift-testing suite reliably SIGABRTs `swiftpm-testing-helper` under | ||
| // Xcode 26.x (see `ShellEscapeTests.swift`'s header for the same note). | ||
|
|
||
| import AppKit | ||
| import Foundation | ||
| import XCTest | ||
|
|
||
| @testable import Roost | ||
|
|
||
| @MainActor | ||
| final class WindowMetricsTests: XCTestCase { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Use swift-testing for this test target.
mac/Tests/RoostTests/WindowMetricsTests.swift:12-23 uses XCTestCase, but this path requires swift-testing. Replace this suite with @Suite and @Test cases. If swift-testing crashes under Xcode 26.x, track and resolve that runner problem separately instead of bypassing the test-target standard.
As per coding guidelines, “Swift tests must use swift-testing under mac/Tests/RoostTests/.”
🤖 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 `@mac/Tests/RoostTests/WindowMetricsTests.swift` around lines 12 - 23, Convert
WindowMetricsTests from XCTestCase to the swift-testing framework by replacing
the XCTest import and test class structure with a `@Suite` and individual `@Test`
cases, preserving the existing coverage and `@MainActor` requirements. Do not
retain the XCTest workaround or bypass the test-target standard; address any
Xcode 26.x runner crash separately.
Source: Coding guidelines
Backlog table catches up with PRs #298/#301: #292/#282/#283/#287 fixed, #295 fixed for the grip with the strip half split to #300 (scrollable event/cursor space mismatch), new rows for #299 (vendored-swash robustness follow-ups) and #297's deliberate park (idle-fuse analysis). M4 entry criteria annotated: the real-input criterion is met; the no-P0/P1 criterion needs a parity-inventory refresh audit (rows predate shipped slices) which should also decide #284. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WLKWsLV45DAk61xG6Utj3e
1 participant
Pass 2 of the iced bug closeout (brief BRIEF-013; pass 1 was PR #298). Closes #295, #252, #287, #282, #283. Six commits, each independently gated.
What changed
#295 —
fix(iced): sidebar-grip presses anchor at the last event-carried move. iced's ButtonPressed carries no position and a drained batch reports one batch-newest cursor, so a seam press during motion was hit-tested where the pointer ended up afterwards. The grip records the last CursorMoved's own position and gates presses on it (cleared on CursorLeft; never arms from an unavailable cursor). Scope narrowed during adversarial review: the ReorderStrip half cannot use this fix — both strips sit under iced scrollables, which pass children a translated cursor but the raw event, so an event-position anchor is off by the scroll offset — split to #300 with the analysis. The harness dwell that existed solely as the workaround is removed; the shed cage run (dwell-free) is the efficacy proof.#252 —
fix(gtk): asymmetric paned grab zone. 4px into the sidebar / 2px terminal side (selection still wins). 4px, not the issue's 6-10px: the overlay scrollbar's hovering interactive column (~14-16px, verified against the compiled Adwaita stylesheet) sits flush at the seam, and wider bands steal proportionally more of it. A follow-up commit replaced the shed-disproven screen probe with a unit-pinned pure predicate (paned_claims_press, four boundary points) — a fixed screen-coordinate probe can't discriminate the widening under unknown CSD margin.#287 —
feat(mac): window_metrics parity fields.terminal_top(flip-correct: content-view height minus converted maxY, not origin.y) +terminal_font_familyvia a newterminalMetrics()UI-bridge accessor mirroringsidebarMetrics(). XCTest pins the flip derivation (including a guard proving origin.y math would fail);test_sidebar_resize.pyasserts the fields cross-target; ipc.md updated.#282 —
fix(drop): control-char predicate convergence + Swift URL filtering. Both languages reject the identical 8-scalar class (Swift'sisNewlineclasses + ESC) on path AND URL branches; Swift's previously-unfiltered dragged-URL branch now rejects (reject-don't-strip; rejected URL falls to the deliberately-unfiltered string baseline owned by #280's mitigations — pinned explicitly by a dual-flavor documenting test on both sides). Rust gains the parity URL branch (production-dead until a toolkit surfaces URL drops; flagged). Cross-pinned twin vectors for every class in both test suites.#283 —
refactor(gtk): last clippy debt + gate tightening.ipc_window_metricsreturns the wire struct instead of the 6-tuple (the alias + iced reply site unified on it); with roost-linux clippy-clean, gtk-build's narrow denylist step becomes the same full-D warningsgate as every other crate (the #236/#234 disallowed guards are warn-by-default — nothing is lost; clippy.toml remains the source of truth). Both panel reviewers independently confirmed this was the only remaining warning.Issue closeouts without code
Verification
iced_clipboard_check.pyPASS with the Iced hit-tests positionless presses at the batch-newest cursor (affects SidebarResizeGrip + ReorderStrip) #295 dwell removed (seam-grip drag clean);real_input_check.pyPASS — its first run caught the GTK: sidebar separator resize hit zone is too narrow after paned grab-zone tightening #252 probe design flaw, which is exactly what pre-push shed verification is for.make check-iced(iced),cargo test -p roost-linux+ clippy (GTK),swift build && swift test(Mac — 693 swift-testing + XCTest incl. 7 new WindowMetrics + 15 DropContentResolver tests),cargo test --workspace(shared crates),make clippyend-to-end.Risk notes
Plan 014 (full text)
🤖 Generated with Claude Code
https://claude.ai/code/session_01WLKWsLV45DAk61xG6Utj3e
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests