fix(triggers): route to a window that already exists, and keep a highlight through a rewrite - #28
Merged
HarryCordewener merged 3 commits intoAug 11, 2026
Conversation
WalkthroughThe change adds existing-window trigger routing with session and ownership precedence. It also applies highlighting after rewrites across the complete rewritten text. Trigger configuration now suggests realised workspace destinations. ChangesTrigger rewrite and routing
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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: 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 `@src/SharpMUTerm.Core/Workspace/Workspace.cs`:
- Around line 205-232: Guard the renamed-window fallback in FindRouteTarget by
returning the SpawnWindowId(sessionKey, target) result only when
Layout.FindWindow(renamed.Id) is not null; otherwise return null. Add a
regression test covering an unplaced or removed renamed spawn window and verify
RouteLine does not route to its invisible buffer.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8f6b601f-3643-4de1-b440-ff8e760c3b62
📒 Files selected for processing (10)
src/SharpMUTerm.Core/Automation/Trigger.cssrc/SharpMUTerm.Core/Automation/TriggerEngine.cssrc/SharpMUTerm.Core/Workspace/Workspace.cssrc/SharpMUTerm.Tui/SharpMUTermApp.cssrc/SharpMUTerm.Tui/TriggersScreenRenderer.cssrc/SharpMUTerm.Tui/TriggersScreenView.cstests/SharpMUTerm.Core.Tests/Automation/HighlightRewriteTests.cstests/SharpMUTerm.Core.Tests/Workspace/RouteToExistingWindowTests.cstests/SharpMUTerm.Tui.Tests/TriggerRouteDestinationTests.cstests/SharpMUTerm.Tui.Tests/TriggersScreenRendererTests.cs
HarryCordewener
force-pushed
the
fix/trigger-routing-and-highlights
branch
from
August 11, 2026 20:50
d865978 to
7addac7
Compare
HarryCordewener
force-pushed
the
feat/pane-search
branch
from
August 11, 2026 21:00
31ffd6f to
bee7031
Compare
The reported defect was "highlight colours don't seem to actually work". They work alone — `Highlight_RecoloursMatchedRegion` has always passed — and they were destroyed by the rule's *own* rewrite. `Process` applied the highlight to the matched region and then, four lines later, replaced the whole line with `StyledLine.FromText(text, TextStyle.Default)`: no colour, no attributes, no left rule. That combination is not exotic, it is what a channel rule looks like — route it, tidy it to `» $1`, colour it — and it is the shape of the demo configuration's own headline rule. The F2 screen badged such a rule `H` and painted both swatches, so the client promised a highlight it then threw away, and the only way to find one was to discover that deleting the rewrite brought it back. The rewrite now runs first and the highlight covers the whole of what it produced. It cannot cover the match's own offsets, because after a rewrite those address a string that no longer exists; the rewritten text is the rule's product in its entirety, so colouring all of it is the only reading that means anything. Without a rewrite nothing moves: the highlight covers the match and only the match, as it always has. A *later* rule's rewrite still replaces an earlier rule's highlighted text, and that is correct rather than the same bug one rule over — those characters are gone. Pinned, so the ordering fix is not later generalised into re-colouring text the first rule never saw. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015nuKnWthnELNkrd86q5KWN
…ly to a spawn "Triggers should not be restricted to only be able to go to spawns." They were, and the restriction was one method deep: `Workspace.RouteSpawn` was the only destination resolver a matched rule had, and it computes `SpawnWindowId(sessionKey, target)` and registers a new `WindowKind.Spawn` window when nothing answers to that id. There is no branch in it that can reach a window that already exists under any other name, so a route naming a window on the screen opened a second one beside it wearing the same label, and the one the user was looking at stayed empty. `Workspace.RouteLine` is the resolver now: a window the target already names wins, and creating a capture pane is what happens when nothing does. `FindRouteTarget` is the same lookup without the side effects, so the shell can still tell "this line opened a pane" from "this line went to one that was already there" without routing twice. What a target may reach is deliberately narrower than "any window with that title": this session's own windows, the windows nobody owns, and another character's *main* window — one alt's channel collected into the pane you actually read. It is not another session's spawn or auxiliary window, because two characters running one capture rule get a pane each and a bare title lookup would collapse them back into one, which is the exact defect `SpawnWindowId` was given an owner to fix. Only a placed window is a destination: appending to a window no pane holds is indistinguishable from the rule not firing. And finding never creates, which is the property that bounds a capture-derived target — that arm can only ever land in a window the user already has. Two consequences in the shell. `OwnerLabel` is stamped on this session's own capture panes only; writing our name onto a window somebody else owns would rename their pane after whoever last routed a line into it. And the F2 `route` field now suggests the workspace's own windows as well as the other rules' targets — while the list was the rules' targets alone, the one place a user reads what a route may say could not name any window they had open. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015nuKnWthnELNkrd86q5KWN
Review catch. The title lookup already required a *placed* window, and the renamed-spawn fallback beside it did not — so a spawn window whose pane the user closed could be returned as a destination and the channel written into a buffer nothing draws. That is reachable rather than theoretical: the registry outlives the layout, and a restored workspace registers windows a saved layout no longer places. It also contradicted RouteLine's own doc comment, which says a closed window is passed over and the line goes somewhere visible. Both halves, because guarding only the fallback moves the same defect one call deeper: RouteSpawn returned a registered-but-unplaced window untouched, since it only placed one it had just created. It now places on the way past whether or not the window is new — which is what lets FindRouteTarget decline a closed window and fall through, reopening the pane under the same id with its history in it. The regression test was checked against the unfixed code and fails there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015nuKnWthnELNkrd86q5KWN
HarryCordewener
force-pushed
the
fix/trigger-routing-and-highlights
branch
from
August 11, 2026 21:01
2bbe154 to
ee02d8d
Compare
Base automatically changed from
feat/pane-search
to
feat/window-activity-boundary
August 11, 2026 23:21
HarryCordewener
merged commit Aug 11, 2026
d9c5815
into
feat/window-activity-boundary
3 checks passed
This was referenced Aug 11, 2026
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.
Two reported defects. They are independent, and each is a single wrong line
in a different file.
1. "Triggers should not be restricted to only be able to go to spawns."
Root cause.
Workspace.RouteSpawn(Workspace.cs:172-184onmain) was theonly destination resolver a matched rule had —
SharpMUTermApp.OnSpawnLine(
SharpMUTermApp.cs:2905-2931) is its one caller. It computesSpawnWindowId(sessionKey, target)and, when nothing answers to that id,registers a brand-new
WindowKind.Spawnwindow. There is no branch in it thatcan reach a window that already exists under any other name, so "route this
into the window I already have open" was not a thing a rule could ask for
however it was spelt: a route naming a window on the screen opened a second
one beside it wearing the same label, and the one the user was looking at
stayed empty.
The screen could not express it either.
SharpMUTermApp.SpawnTargets()(
SharpMUTermApp.cs:4942-4948) built the F2routesuggestion list out of theother rules'
SpawnTargets and nothing else, so the one place a user readswhat a route may say could name only spawn panes.
One arm of the report was already there and is worth stating precisely:
mainin the route list (
TriggersScreenRenderer.cs:44-47) is a real choice meaningSpawnTarget = null, which does reach the main window — but only for a linethe rule does not also gag, because it is not routing rather than routing
there. A gagging rule aimed at the main window deleted the line.
Change.
Workspace.RouteLineis the resolver now: a window the targetalready names wins, and creating a capture pane is what happens when nothing
does.
Workspace.FindRouteTargetis the same lookup with no side effects, sothe shell can still tell "this line opened a pane" from "this line went to one
that was already there" without routing twice.
What a target may reach is deliberately narrower than "any window with that
title", and the scoping is the design rather than caution:
pane you actually read. A main window is admitted across the owner boundary
because it is a window the user opened by connecting, rather than one a
capture rule conjured;
one capture rule get a pane each, and a bare title lookup would collapse them
back into one and file the second character's channel under the first — the
exact defect
SpawnWindowIdwas given an owner to fix.SpawnWindowPerSessionTestsand
TwoCharactersCapturingOneNameStillGetAPaneEachhold both ends of that.writes into a buffer nothing can draw, which from the reader's side is
indistinguishable from the rule not firing at all.
it (
Channel $1), so the name can be the server's text; the property thatbounds that is that this arm can only ever land in a window the user already
has. Making one out of a captured name still goes through
RouteSpawn, whichputs the matching session's own key on it.
Two consequences in the shell.
OwnerLabelis stamped on this session's owncapture panes only — it prefixes a tab as
Owner: Name, and writing our nameonto a window somebody else owns would rename their pane after whoever last
routed a line into it. And
RouteTargets()(the oldSpawnTargets()) nowoffers the workspace's own windows as well as the other rules' targets, so the
route field can name them; they are suggestions and not the permitted set, so
this widens what is discoverable rather than what is legal.
2. "Highlight colours don't seem to actually work."
Root cause.
TriggerEngine.Process(TriggerEngine.cs:256-267onmain)applied the highlight to the matched region and then, four lines later,
replaced the whole line with
StyledLine.FromText(text, TextStyle.Default)—no colour, no attributes, no left rule. A rule that both rewrote and
highlighted therefore threw its own highlight away on the very next statement.
Highlights work on their own;
TriggerEngineTests.Highlight_RecoloursMatchedRegionhas always passed, and an end-to-end check of a highlight-only rule shows the
colour reaching the pane's markup. The combination is what fails, and it is not
an exotic one — it is what a channel rule looks like (route it, tidy it to
» $1, colour it), and it is the shape of this repository's own demo headlinerule (
DemoScene.cs:161-174: teal, bold,Rewrite = "» $1"). The F2 screenbadges such a rule
Hand paints both swatches, so the client promised ahighlight it then discarded, and the only way to find one was to discover that
deleting the rewrite brought it back.
Change. The rewrite runs first, and the highlight then covers the whole of
what it produced. It cannot cover the match's own offsets, because after a
rewrite those address a string that no longer exists; the rewritten text is the
rule's product in its entirety, so colouring all of it is the only reading that
means anything. Without a rewrite nothing moves — the highlight covers the
match and only the match, as it always has, which is the property the
reordering could most easily have broken and is pinned separately.
A later rule's rewrite still replaces an earlier rule's highlighted text, and
that is correct rather than the same bug one rule over: those characters are
gone. Also pinned, so the ordering fix is not later "generalised" into
re-colouring text the first rule never saw.
On the snapshot.
--view highlightis the F2 colour-picker screen, notpainted output, so it cannot show this defect — its frame is byte-identical
before and after the fix, which is itself the evidence that the screen was
never where the colour was lost. The defect is in the output path, and it is
proven where the colour actually has to survive:
ARewrittenLineReachesThePaneWearingItsHighlightreads the Spectre markup a pane is fed and asserts
#ffd700is in it.--view route-editdoes move, and correctly: the route field's suggestion listgrows from four entries to five as the workspace's own window joins it.
Tests
Written before the fixes, and each verified to fail on the unfixed behaviour —
3 of the 6 end-to-end tests fail without them (two routing, one highlight); the
other three are regression pins that pass either way by design.
HighlightRewriteTests(Core) — 7 tests over the rewrite/highlight ordering,including the two properties that must not move.
RouteToExistingWindowTests(Core) — 10 tests overRouteLine, half of themabout what it must not reach.
TriggerRouteDestinationTests(Tui) — 6 end-to-end tests over a liveconnection with two characters, asserting the pane the line actually lands in
and the markup it lands there wearing.
Verification
🤖 Generated with Claude Code
https://claude.ai/code/session_015nuKnWthnELNkrd86q5KWN
Summary by CodeRabbit
New Features
Bug Fixes
Tests