Settings screens, keyboard interaction, mouse drag-to-split, and inline images - #7
Merged
Merged
Conversation
Decomposes TriggersScreenRenderer into pure blocks (HeaderLine, FooterLine, RulesColumn, EditorColumn) and adds TriggersScreenView to compose them into a real control tree: full-width header band with keyboard hints, two column panels split by a vertical rule, and a Cancel/Save action bar pinned to the last row instead of floating mid-screen after the content. Render(...) still merges the same blocks, so the existing renderer tests pass untouched. Also fixes a latent bug in WorldsScreenView: VerticalRule() built a MarkupControl over an empty line list, which measures to zero and so never painted its background -- F5's column rule has never actually rendered. An empty grid with a background covers its arranged area. Baseline worlds frame contained 0 cells of the rule colour; it now has 22, with the rest of the frame unchanged. Generalises the settings wiring rather than special-casing a second screen: SettingsView returns a control factory for every screen, so the snapshot path needs no per-screen branch as the remaining six convert. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both screens decomposed into pure blocks with a *ScreenView composing them into header band / column panels / pinned action bar. Render(...) still merges the blocks, so the renderer tests pass untouched. F4's body splits naturally: the numpad grid has a fixed natural width and the hotkey list takes the remainder. Unifies the settings wiring. RegisterSettingsShortcuts and SettingsView carried identical factories keyed two different ways and had to be edited in lockstep for every conversion; they now read one SettingsScreens() table of (Key, Views, Control). The --view name was the only thing that ever differed, and it is now a Views array -- F5 keeps both "worlds" and "settings". The remaining conversions become a one-line edit each. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Decomposed into pure blocks with TimersScreenView composing header band / list column / rule / editor column / pinned action bar. Render(...) still merges the blocks, so TimersScreenRendererTests passes untouched. Also corrects VisibleLength, which stripped "[[" instead of counting it as one literal character -- contradicting its own doc comment and under-padding the list column for any row containing escaped brackets. No test covers padding, so the suite never caught it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Completes backlog item 1 -- all eight settings screens now share the F5 treatment. The three options screens share one renderer, so they share one view: OptionsScreenView.Build(title, fkey, rows). They are a single options list rather than two panes, so the body is one elevated card sized to its content instead of a column split with a rule -- inventing a division that isn't there would have been the wrong kind of consistency. With every screen converted, the deferred consolidation pass follows. Six copies of the palette and helpers collapse into ScreenPalette, ScreenChrome, and MarkupText. That last one goes wider than the settings screens: nine copies of Escape and two independently hand-rolled VisibleLength scanners across seven files, all now one implementation. Two divergent width-measuring routines was the duplication most likely to silently mis-pad a column. Verified the main UI is untouched by that reach: the default workspace, menu, split and spawn frames are byte-identical before and after. SharpMUTermApp.Markup and SettingsOverlay.MarkupPanel are deleted -- with no markup screens left, the adapter has no callers. Two inconsistencies surfaced by putting the fragments in one place: F2 and F5 never showed their F-key in the header hints, and F5's action bar said lowercase "cancel". Both now match the other six. Net -398 lines. 514 tests pass, unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
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:
WalkthroughF2–F9 settings screens now use composed panel layouts, shared markup and chrome, keyboard sessions, editable configuration models, and centralized overlay wiring. Pane drag-and-drop and inline web-image rendering add policy selection, decoding, composition, previews, and headless coverage. ChangesSettings screens and workspace interactions
Estimated code review effort: 5 (Critical) | ~100 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
NumpadCell produced a variable-width string -- the unbound placeholder
is one visible character, a command up to ten -- and NumpadRow joined
cells with a fixed three-space gap. So any bound key widened its own
cell and shunted the rest of that row right, which is why the grid only
looked square while nothing was bound.
Each cell is now padded to a fixed visible width ("[N] " plus the
longest command it can hold); the last cell in a row is left unpadded to
avoid trailing whitespace. All three rows now start their cells at
columns 0, 17, 34 regardless of content.
The demo scene bound a single numpad key, which can't show a keypad
doing anything. It now carries a movement layout with commands of
differing lengths, one long enough to be ellipsised -- so the snapshot
actually exercises the grid.
Adds a regression test asserting the three rows share cell offsets.
Confirmed it fails without the padding and passes with it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/SharpMUTerm.Tui/OptionsScreenRenderer.cs (1)
91-101: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse
PadVisible, not rawPadRight, on the escaped label.
Escape(row.Label)can inflate the raw string length past its rendered width ([→[[), soPadRight(LabelWidth)pads against the wrong measurement and would misalign the value column for any label containing a bracket.PadVisible(already imported viausing static SharpMUTerm.Tui.MarkupText;) is built for exactly this.🐛 Proposed fix
- var label = Escape(row.Label).PadRight(LabelWidth); + var label = PadVisible(Escape(row.Label), LabelWidth); return $"[dim]{label}[/] {Escape(row.Value ?? string.Empty)}{hint}";🤖 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 `@src/SharpMUTerm.Tui/OptionsScreenRenderer.cs` around lines 91 - 101, The escaped label in the non-toggle rendering path is padded using raw string length, misaligning values when the label contains markup-sensitive characters. In the method containing the row rendering logic, replace the Escape(row.Label).PadRight(LabelWidth) call with the existing PadVisible helper, preserving the current escaped label and LabelWidth inputs.src/SharpMUTerm.Tui/TriggersScreenRenderer.cs (1)
230-232: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Hex(TerminalColor)duplicated with WorldsScreenRenderer.
See consolidated comment.🤖 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 `@src/SharpMUTerm.Tui/TriggersScreenRenderer.cs` around lines 230 - 232, Remove the duplicated Hex(TerminalColor) implementation from SharpMUTerm.Tui’s TriggersScreenRenderer and reuse the existing shared or WorldsScreenRenderer implementation identified by the consolidated comment. Update callers as needed while preserving RGB formatting and the Accent fallback.
🤖 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.Tui/OptionsScreenRenderer.cs`:
- Around line 103-108: Replace the rendered-glyph check in IsSection with an
explicit OptionRow discriminator, such as its existing kind or section flag, and
update section-row construction to set that discriminator. Preserve normal
labels beginning with “├ ” as ordinary option rows.
In `@src/SharpMUTerm.Tui/TriggersScreenRenderer.cs`:
- Line 1: Remove the duplicated private Hex(TerminalColor) helpers from
TriggersScreenRenderer and WorldsScreenRenderer, and add one shared
TerminalColor-to-#rrggbb helper in the existing MarkupText or ScreenPalette
utility area. Update both renderers to call the shared helper, preserving the
current Accent fallback and output format.
In `@src/SharpMUTerm.Tui/WorldsScreenRenderer.cs`:
- Around line 260-261: Remove the duplicate Hex(TerminalColor) implementation
from WorldsScreenRenderer and reuse the shared helper established for both
WorldsScreenRenderer and TriggersScreenRenderer. Preserve the existing RGB
formatting and Accent fallback through that consolidated implementation.
---
Outside diff comments:
In `@src/SharpMUTerm.Tui/OptionsScreenRenderer.cs`:
- Around line 91-101: The escaped label in the non-toggle rendering path is
padded using raw string length, misaligning values when the label contains
markup-sensitive characters. In the method containing the row rendering logic,
replace the Escape(row.Label).PadRight(LabelWidth) call with the existing
PadVisible helper, preserving the current escaped label and LabelWidth inputs.
In `@src/SharpMUTerm.Tui/TriggersScreenRenderer.cs`:
- Around line 230-232: Remove the duplicated Hex(TerminalColor) implementation
from SharpMUTerm.Tui’s TriggersScreenRenderer and reuse the existing shared or
WorldsScreenRenderer implementation identified by the consolidated comment.
Update callers as needed while preserving RGB formatting and the Accent
fallback.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 71b1608b-4006-4c16-9b23-60017519f42e
📒 Files selected for processing (23)
docs/HANDOFF.mdsrc/SharpMUTerm.Tui/AliasesScreenRenderer.cssrc/SharpMUTerm.Tui/AliasesScreenView.cssrc/SharpMUTerm.Tui/CaptureLineRenderer.cssrc/SharpMUTerm.Tui/CommandSurfaceRenderer.cssrc/SharpMUTerm.Tui/KeypadScreenRenderer.cssrc/SharpMUTerm.Tui/KeypadScreenView.cssrc/SharpMUTerm.Tui/MarkupFormatter.cssrc/SharpMUTerm.Tui/MarkupText.cssrc/SharpMUTerm.Tui/OptionsScreenRenderer.cssrc/SharpMUTerm.Tui/OptionsScreenView.cssrc/SharpMUTerm.Tui/Powerline.cssrc/SharpMUTerm.Tui/RailRenderer.cssrc/SharpMUTerm.Tui/ScreenChrome.cssrc/SharpMUTerm.Tui/ScreenPalette.cssrc/SharpMUTerm.Tui/SettingsOverlay.cssrc/SharpMUTerm.Tui/SharpMUTermApp.cssrc/SharpMUTerm.Tui/TimersScreenRenderer.cssrc/SharpMUTerm.Tui/TimersScreenView.cssrc/SharpMUTerm.Tui/TriggersScreenRenderer.cssrc/SharpMUTerm.Tui/TriggersScreenView.cssrc/SharpMUTerm.Tui/WorldsScreenRenderer.cssrc/SharpMUTerm.Tui/WorldsScreenView.cs
Comment on lines
+103
to
+108
| /// <summary>A blank separator carrying no label, value, or toggle.</summary> | ||
| private static bool IsSpacer(OptionRow row) => | ||
| row.Label.Length == 0 && row.Value is null && row.Toggle is null; | ||
|
|
||
| /// <summary>The F8 "Input & spellcheck" screen body.</summary> | ||
| public static List<string> InputSpellcheck() | ||
| { | ||
| var rows = new List<OptionRow> | ||
| { | ||
| new("├ INPUT", null, null), | ||
| new("local echo", null, true), | ||
| new("keep per-tab drafts", null, true), | ||
| new("newline key", "Shift+Enter", null), | ||
| new(string.Empty, null, null), | ||
| new("├ SPELLCHECK", null, null), | ||
| new("check spelling", null, true), | ||
| new("dictionary", "en_US", null), | ||
| }; | ||
|
|
||
| return Render("Input & spellcheck", "F8", rows); | ||
| } | ||
| /// <summary>A dim group heading, marked by the branch glyph the screens prefix them with.</summary> | ||
| private static bool IsSection(OptionRow row) => row.Label.StartsWith("├ ", StringComparison.Ordinal); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
IsSection relies on a display-glyph prefix as its type discriminator.
A label that happens to start with "├ " would be silently treated as a section header. Since OptionRow already carries Value/Toggle to discriminate other kinds, consider an explicit discriminator (e.g. a Kind field or bool IsSection) instead of matching on the rendered glyph.
🤖 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 `@src/SharpMUTerm.Tui/OptionsScreenRenderer.cs` around lines 103 - 108, Replace
the rendered-glyph check in IsSection with an explicit OptionRow discriminator,
such as its existing kind or section flag, and update section-row construction
to set that discriminator. Preserve normal labels beginning with “├ ” as
ordinary option rows.
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 `@tests/SharpMUTerm.Tui.Tests/KeypadScreenRendererTests.cs`:
- Line 42: Replace the obsolete HasCount() assertion in the keypad screen
renderer test with the current Count() assertion, preserving the expected count
of 3.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: f2502254-7ac6-4f1a-88d9-9c6a9e89fa6c
📒 Files selected for processing (3)
src/SharpMUTerm.Tui/DemoScene.cssrc/SharpMUTerm.Tui/KeypadScreenRenderer.cstests/SharpMUTerm.Tui.Tests/KeypadScreenRendererTests.cs
Backlog item 3. The F2-F9 screens were display-only projections whose header hints advertised behaviour that did not exist. Now ↑↓ moves the selection, ⇥ crosses panes, Space toggles the boolean rows, Esc cancels and ⏎ saves -- on all eight screens. Field editing is deliberately NOT built, and no header claims it is: the hints now read "↑↓ select · ⇥ pane · Space toggle", with a test asserting no screen advertises a key its ScreenModel doesn't offer. A hint that lies is worse than a missing feature. Interaction state lives in four pure types (ScreenSelection, ScreenModel, ScreenEdits, SettingsSession) so the rules are testable without a terminal; SettingsOverlay stays the only UI-aware piece. Core remains UI-agnostic. Cancel replays an undo log rather than restoring a cloned config -- cloning AppConfiguration would drop [JsonIgnore] fields such as a character's in-memory password. A toggle snapshots the value, not the boolean, because F9's checkbox is really a LogFormat: cancelling puts Html back, not Plain. Supporting changes worth review: - Alias.CaseSensitive, Trigger.StopProcessing, TriggerActions.Gag and TimerDefinition.OneShot become settable. Alias.CaseSensitive drops the cached compiled Regex, or the matcher would silently keep the old casing. - New AppConfiguration.Text/.Input. F7/F8 rendered hardcoded literals with nothing behind them, so "toggle and save" would have been a lie. Purely additive: defaults match the old constants, no schema bump. - ActiveLogging() falls back to the first configured character instead of returning a throwaway LoggingSettings, which made F9's toggle a silent no-op while disconnected. Also unifies the left-column width: the renderer padded cursor bars to 54 while the view laid the column out at 56, leaving a visible gap before the rule. One constant now, so they cannot drift. Builds SharpConsoleUI from source when the framework is cloned beside this repo, so it can be read and stepped into instead of decompiled. CI and anyone without the clone fall through to the NuGet package unchanged; -p:UseSharpConsoleUIPackage=true forces the package. Both paths verified. 574 tests pass, up from 515. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/SharpMUTerm.Tui/TimersScreenRenderer.cs (1)
170-191: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCheckbox-row markup (
[x]/[ ]+ label) is implemented three separate times.TimersScreenRendererandTriggersScreenRenderereach define a byte-identical privateCheckbox(label, value)helper, andAliasesScreenRenderer.BuildEditorinlines the same pattern (minus theEscape(label)call). This PR already centralizesEscape/PadVisible/SpreadLR/Accentinto sharedMarkupText/ScreenPalettehelpers specifically to prevent this kind of drift —Checkboxwas missed.
src/SharpMUTerm.Tui/TimersScreenRenderer.cs#L170-L191: extract thisCheckboxinto a shared helper (e.g.MarkupText.CheckboxorScreenChrome) and call it here.src/SharpMUTerm.Tui/TriggersScreenRenderer.cs#L263-L265: remove this duplicate and call the same shared helper.src/SharpMUTerm.Tui/AliasesScreenRenderer.cs#L169-L188: replace the inlinedcaseRowternary with a call to the shared helper (also fixes the missingEscape(label)here).🤖 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 `@src/SharpMUTerm.Tui/TimersScreenRenderer.cs` around lines 170 - 191, Centralize the checkbox-row markup in a shared helper, such as MarkupText.Checkbox or ScreenChrome, preserving accent styling for checked values, dim styling for unchecked values, and escaped labels. Update TimersScreenRenderer.BuildEditor and TriggersScreenRenderer to call it and remove their private Checkbox helpers; update AliasesScreenRenderer.BuildEditor to replace its inline caseRow ternary with the shared helper. Apply these changes in src/SharpMUTerm.Tui/TimersScreenRenderer.cs#L170-L191, src/SharpMUTerm.Tui/TriggersScreenRenderer.cs#L263-L265, and src/SharpMUTerm.Tui/AliasesScreenRenderer.cs#L169-L188.
♻️ Duplicate comments (2)
src/SharpMUTerm.Tui/OptionsScreenRenderer.cs (1)
142-147: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
IsSectionstill relies on the rendered glyph prefix as a type discriminator.Same concern from the prior review: a label incidentally starting with
"├ "would be silently treated as a section header. Consider an explicitOptionRowdiscriminator instead of matching on the glyph.🤖 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 `@src/SharpMUTerm.Tui/OptionsScreenRenderer.cs` around lines 142 - 147, The IsSection method incorrectly classifies rows by matching the rendered “├ ” label prefix. Add or reuse an explicit OptionRow discriminator for section headings and update IsSection to check that discriminator, preserving spacer detection and avoiding label-based classification.src/SharpMUTerm.Tui/WorldsScreenRenderer.cs (1)
339-346: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Hex(TerminalColor)duplication still unresolved.This mirrors a prior review comment:
Hexis still duplicated withTriggersScreenRenderer's implementation. Consider extracting the shared helper (e.g. intoMarkupText/ScreenPalette, alreadyusing static-imported here) instead of keeping a local copy in each renderer.🤖 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 `@src/SharpMUTerm.Tui/WorldsScreenRenderer.cs` around lines 339 - 346, Remove the local Hex(TerminalColor) implementation from the renderer and reuse a shared Hex helper extracted into the existing shared utility such as MarkupText or ScreenPalette. Update both SharpMUTerm renderers to call that single helper while preserving the current RGB formatting and Accent fallback 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 `@src/SharpMUTerm.Tui/OptionsScreenRenderer.cs`:
- Around line 204-215: Update OptionsScreenRenderer.LoggingScreen so the last
non-None LogFormat is stored separately and survives redraws; only update that
stored value when logging.Format is non-None. Use it when toggling auto-start
back on, while preserving the existing Plain fallback when no prior format
exists.
In `@src/SharpMUTerm.Tui/SharpMUTermApp.cs`:
- Around line 755-771: Move the blocking save operation in SaveConfiguration off
the UI thread by making the method asynchronous and awaiting an asynchronous
configuration-store save. Update the synchronous caller in SettingsOverlay.OnKey
to await or otherwise schedule SaveConfiguration without blocking the
render/input loop, while preserving session capture and existing
exception/status handling.
---
Outside diff comments:
In `@src/SharpMUTerm.Tui/TimersScreenRenderer.cs`:
- Around line 170-191: Centralize the checkbox-row markup in a shared helper,
such as MarkupText.Checkbox or ScreenChrome, preserving accent styling for
checked values, dim styling for unchecked values, and escaped labels. Update
TimersScreenRenderer.BuildEditor and TriggersScreenRenderer to call it and
remove their private Checkbox helpers; update AliasesScreenRenderer.BuildEditor
to replace its inline caseRow ternary with the shared helper. Apply these
changes in src/SharpMUTerm.Tui/TimersScreenRenderer.cs#L170-L191,
src/SharpMUTerm.Tui/TriggersScreenRenderer.cs#L263-L265, and
src/SharpMUTerm.Tui/AliasesScreenRenderer.cs#L169-L188.
---
Duplicate comments:
In `@src/SharpMUTerm.Tui/OptionsScreenRenderer.cs`:
- Around line 142-147: The IsSection method incorrectly classifies rows by
matching the rendered “├ ” label prefix. Add or reuse an explicit OptionRow
discriminator for section headings and update IsSection to check that
discriminator, preserving spacer detection and avoiding label-based
classification.
In `@src/SharpMUTerm.Tui/WorldsScreenRenderer.cs`:
- Around line 339-346: Remove the local Hex(TerminalColor) implementation from
the renderer and reuse a shared Hex helper extracted into the existing shared
utility such as MarkupText or ScreenPalette. Update both SharpMUTerm renderers
to call that single helper while preserving the current RGB formatting and
Accent fallback 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: ASSERTIVE
Plan: Pro
Run ID: 05f147da-572b-4bbb-bd21-f96603407865
📒 Files selected for processing (36)
CLAUDE.mddocs/HANDOFF.mdsrc/SharpMUTerm.Core/Automation/Alias.cssrc/SharpMUTerm.Core/Automation/TimerDefinition.cssrc/SharpMUTerm.Core/Automation/Trigger.cssrc/SharpMUTerm.Core/Configuration/AppConfiguration.cssrc/SharpMUTerm.Core/Configuration/PreferenceSettings.cssrc/SharpMUTerm.Tui/AliasesScreenRenderer.cssrc/SharpMUTerm.Tui/AliasesScreenView.cssrc/SharpMUTerm.Tui/KeypadScreenRenderer.cssrc/SharpMUTerm.Tui/KeypadScreenView.cssrc/SharpMUTerm.Tui/OptionsScreenRenderer.cssrc/SharpMUTerm.Tui/OptionsScreenView.cssrc/SharpMUTerm.Tui/ScreenChrome.cssrc/SharpMUTerm.Tui/ScreenEdits.cssrc/SharpMUTerm.Tui/ScreenFocus.cssrc/SharpMUTerm.Tui/ScreenModel.cssrc/SharpMUTerm.Tui/ScreenPalette.cssrc/SharpMUTerm.Tui/ScreenSelection.cssrc/SharpMUTerm.Tui/SettingsOverlay.cssrc/SharpMUTerm.Tui/SettingsSession.cssrc/SharpMUTerm.Tui/SharpMUTerm.Tui.csprojsrc/SharpMUTerm.Tui/SharpMUTermApp.cssrc/SharpMUTerm.Tui/TimersScreenRenderer.cssrc/SharpMUTerm.Tui/TimersScreenView.cssrc/SharpMUTerm.Tui/TriggersScreenRenderer.cssrc/SharpMUTerm.Tui/TriggersScreenView.cssrc/SharpMUTerm.Tui/WorldsScreenRenderer.cssrc/SharpMUTerm.Tui/WorldsScreenView.cstests/SharpMUTerm.Core.Tests/Configuration/ConfigurationTests.cstests/SharpMUTerm.Tui.Tests/KeypadScreenRendererTests.cstests/SharpMUTerm.Tui.Tests/ScreenCursorTests.cstests/SharpMUTerm.Tui.Tests/ScreenEditsTests.cstests/SharpMUTerm.Tui.Tests/ScreenModelTests.cstests/SharpMUTerm.Tui.Tests/ScreenSelectionTests.cstests/SharpMUTerm.Tui.Tests/SettingsSessionTests.cs
Comment on lines
+204
to
+215
| // "Auto-start" is really the log format: off means None, on means whatever format was last | ||
| // chosen (Plain when there isn't one). The binding's snapshot restores the *format*, not the | ||
| // boolean, so cancelling a toggle-off puts Html back rather than downgrading it to Plain. | ||
| var chosen = logging.Format == LogFormat.None ? LogFormat.Plain : logging.Format; | ||
| var autoStart = new ScreenToggle( | ||
| () => logging.Format != LogFormat.None, | ||
| () => logging.Format = logging.Format == LogFormat.None ? chosen : LogFormat.None, | ||
| () => | ||
| { | ||
| var previous = logging.Format; | ||
| return () => logging.Format = previous; | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect LoggingSettings to see if a preferred/last-format field already exists.
fd -HI 'LoggingSettings.cs' | xargs -r cat -n
rg -n 'LogFormat' src/SharpMUTerm.Core/Configuration/PreferenceSettings.cs src -g '*.cs' -A2 -B2Repository: SharpMUSH/SharpMUTerm
Length of output: 3982
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the toggle implementation and the surrounding renderer code.
fd -HI 'ScreenToggle.cs' 'src' | xargs -r cat -n
printf '\n--- OptionsScreenRenderer excerpt ---\n'
sed -n '190,230p' src/SharpMUTerm.Tui/OptionsScreenRenderer.csRepository: SharpMUSH/SharpMUTerm
Length of output: 2231
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect ScreenToggle implementation.
fd -HI 'ScreenToggle.cs' src | head -n 20Repository: SharpMUSH/SharpMUTerm
Length of output: 159
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- ScreenToggle.cs ---'
fd -HI 'ScreenToggle.cs' src | xargs -r cat -n
echo
echo '--- OptionsScreenRenderer.cs excerpt ---'
sed -n '190,230p' src/SharpMUTerm.Tui/OptionsScreenRenderer.csRepository: SharpMUSH/SharpMUTerm
Length of output: 2258
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- files named ScreenToggle.cs / OptionsScreen*.cs / ScreenModel.cs ---'
fd -a 'ScreenToggle.cs|OptionsScreen.*\.cs|ScreenModel\.cs' src
echo
echo '--- references to LoggingScreen / ScreenToggle / OptionsScreen ---'
rg -n 'LoggingScreen\(|ScreenToggle|class OptionsScreen|record OptionsScreen|struct OptionsScreen|new OptionsScreen\(' src/SharpMUTerm.Tui src/SharpMUTerm.Core -g '*.cs' -A3 -B3Repository: SharpMUSH/SharpMUTerm
Length of output: 20811
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- candidate files ---'
fd -HI 'ScreenToggle.cs' src
fd -HI 'OptionsScreen.*\.cs' src
fd -HI 'ScreenModel.cs' src
echo
echo '--- logging screen call sites / model definitions ---'
rg -n 'LoggingScreen\(|ScreenToggle|class OptionsScreen|record OptionsScreen|new OptionsScreen\(' src -g '*.cs' -A3 -B3Repository: SharpMUSH/SharpMUTerm
Length of output: 20425
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- candidate files ---'
fd -HI 'ScreenToggle.cs' src
fd -HI 'OptionsScreen*.cs' src
fd -HI 'ScreenModel.cs' src
echo
echo '--- logging screen call sites / model definitions ---'
rg -n 'LoggingScreen\(|ScreenToggle|class OptionsScreen|record OptionsScreen|new OptionsScreen\(' src -g '*.cs' -A3 -B3Repository: SharpMUSH/SharpMUTerm
Length of output: 20339
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- ScreenBinding definition and OptionsScreen call path ---'
rg -n 'class ScreenBinding|record ScreenBinding|struct ScreenBinding|OptionsScreen\(' src/SharpMUTerm.Tui -g '*.cs' -A4 -B4
echo
echo '--- surrounding SharpMUTermApp logging screen code ---'
sed -n '880,930p' src/SharpMUTerm.Tui/SharpMUTermApp.csRepository: SharpMUSH/SharpMUTerm
Length of output: 10257
Preserve the last non-None log format across redraws In OptionsScreenRenderer.LoggingScreen(...), chosen is recomputed from logging.Format on every repaint, so an off→on toggle in the same session falls back to Plain instead of restoring the prior format. Store the last non-None format separately.
🤖 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 `@src/SharpMUTerm.Tui/OptionsScreenRenderer.cs` around lines 204 - 215, Update
OptionsScreenRenderer.LoggingScreen so the last non-None LogFormat is stored
separately and survives redraws; only update that stored value when
logging.Format is non-None. Use it when toggling auto-start back on, while
preserving the existing Plain fallback when no prior format exists.
Comment on lines
+755
to
771
| /// <summary> | ||
| /// Persists the configuration the settings screens edit — the ⏎ Save action. The workspace layout | ||
| /// is captured alongside it so a save never rolls back the resumed session; a failed write is | ||
| /// swallowed for the same reason startup's is (the config is a convenience, not the session). | ||
| /// </summary> | ||
| private void SaveConfiguration() | ||
| { | ||
| try | ||
| { | ||
| _config.LastSession = CaptureSession(); | ||
| ConfigurationStore.Save(ConfigurationStore.DefaultPath, _config); | ||
| } | ||
| catch (Exception ex) when (ex is IOException or UnauthorizedAccessException or NotSupportedException) | ||
| { | ||
| SetStatus($"[red]could not save settings:[/] {Escape(ex.Message)}"); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check ConfigurationStore.Save signature and other call sites.
fd -i ConfigurationStore.cs | xargs -I{} sed -n '1,100p' {}
rg -n 'ConfigurationStore\.Save' --type=cs -C3Repository: SharpMUSH/SharpMUTerm
Length of output: 2975
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the implementation and callers of SaveConfiguration.
ast-grep outline src/SharpMUTerm.Tui/SharpMUTermApp.cs --view expanded >/tmp/outline.txt
sed -n '1,220p' /tmp/outline.txt
printf '\n--- SaveConfiguration block ---\n'
rg -n 'SaveConfiguration|ConfigurationStore\.Save|EnqueueOnUIThread|Save action|keyboard' src/SharpMUTerm.Tui/SharpMUTermApp.cs -C6
printf '\n--- surrounding lines near SaveConfiguration ---\n'
sed -n '720,790p' src/SharpMUTerm.Tui/SharpMUTermApp.csRepository: SharpMUSH/SharpMUTerm
Length of output: 14516
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect SettingsOverlay and any async save-related APIs.
fd -a 'SettingsOverlay.cs' src
printf '\n--- SettingsOverlay outline ---\n'
ast-grep outline src/SharpMUTerm.Tui/SettingsOverlay.cs --view expanded
printf '\n--- save-related symbols and async APIs ---\n'
rg -n 'SaveConfiguration|EnqueueOnUIThread|ConfigurationStore\.Save|Task<|async Task|async void|Save\(' src/SharpMUTerm.Tui src/SharpMUTerm.Core -C3Repository: SharpMUSH/SharpMUTerm
Length of output: 14741
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect SettingsOverlay's key handling and save invocation.
sed -n '1,220p' src/SharpMUTerm.Tui/SettingsOverlay.csRepository: SharpMUSH/SharpMUTerm
Length of output: 5370
Move the settings save off the UI thread. SaveConfiguration() is invoked synchronously from SettingsOverlay.OnKey, and ConfigurationStore.Save(...) is just File.WriteAllText(...), so a slow disk write can stall the render/input loop. src/SharpMUTerm.Tui/SharpMUTermApp.cs:760-765
🤖 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 `@src/SharpMUTerm.Tui/SharpMUTermApp.cs` around lines 755 - 771, Move the
blocking save operation in SaveConfiguration off the UI thread by making the
method asynchronous and awaiting an asynchronous configuration-store save.
Update the synchronous caller in SettingsOverlay.OnKey to await or otherwise
schedule SaveConfiguration without blocking the render/input loop, while
preserving session capture and existing exception/status handling.
Source: Coding guidelines
Backlog item 5. DropZones.Resolve existed in Core and was tested, but nothing in the TUI ever called it. Dragging a pane's tab onto another pane now splits or re-tabs it: drop within 25% of the left or right edge to split side by side, top or bottom to stack, or in the middle to move the tab into that pane. The wiring sits at the driver, not on a control, and that is forced by the framework: WindowEventDispatcher captures the pressed control and routes every later drag frame back to it, so a control-level handler would only ever see the source pane and never the one being dragged onto. Most of the logic is pure and tested -- the gesture state machine, flag decoding, hit-testing, preview geometry -- leaving the thinnest possible adapter over framework events. PaneDragEndToEndTests additionally drives real press/drag/release frames through the app on a headless driver, parameterised over all four edges, asserting both split direction and child ordering. Move mode's status bar has always advertised "←↑↓→ edge" with no arrow handling behind it, so only the tab-drop half was reachable and SplitWithWindow was never called from the TUI. Both paths now commit through one PaneDrop, so keyboard and mouse cannot drift. 649 tests pass, up from 574. Unverified until someone uses a real mouse: whether the terminal reports drags at all, and whether the preview keeps up at pointer speed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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 `@src/SharpMUTerm.Tui/SharpMUTermApp.cs`:
- Around line 1873-1927: Marshal the entire OnDriverMouseEvent callback onto the
UI thread, including overlay checks, PaneSnapshot, _paneDrag.Handle, and
ApplyDragResult. Ensure the driver callback only schedules this work through
OnUiThread, so all _paneDrag mutations and PaneSnapshot layout reads occur on
the same UI thread as BuildDragPane and the Escape handler.
In `@tests/SharpMUTerm.Core.Tests/Workspace/PaneDropTests.cs`:
- Line 25: Update the tab assertions in the affected tests, including the
assertions near lines 25 and 66, to pass CollectionOrdering.Matching to
IsEquivalentTo so the expected sequence order is enforced while retaining the
existing tab values.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 41fb1442-3620-4e8e-b74f-d3f47b7c7878
📒 Files selected for processing (13)
CLAUDE.mddocs/HANDOFF.mdsrc/SharpMUTerm.Core/Workspace/PaneDrop.cssrc/SharpMUTerm.Tui/PaneDragSurface.cssrc/SharpMUTerm.Tui/PaneDragTracker.cssrc/SharpMUTerm.Tui/PaneDropRenderer.cssrc/SharpMUTerm.Tui/Program.cssrc/SharpMUTerm.Tui/SharpMUTermApp.cstests/SharpMUTerm.Core.Tests/Workspace/PaneDropTests.cstests/SharpMUTerm.Tui.Tests/PaneDragEndToEndTests.cstests/SharpMUTerm.Tui.Tests/PaneDragSurfaceTests.cstests/SharpMUTerm.Tui.Tests/PaneDragTrackerTests.cstests/SharpMUTerm.Tui.Tests/PaneDropRendererTests.cs
Comment on lines
+1873
to
+1927
| private void OnDriverMouseEvent(object sender, List<MouseFlags> flags, System.Drawing.Point point) | ||
| { | ||
| // Overlays own the whole screen while they're up; a drag underneath them would target panes | ||
| // the user can't even see. | ||
| if (_palette.IsOpen || _settings.IsOpen || _moveMode) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| var result = _paneDrag.Handle(flags, point.X, point.Y, PaneSnapshot); | ||
| if (result.Action == PaneDragAction.None) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| OnUiThread(() => ApplyDragResult(result)); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Reads the pane area's live geometry back out of the framework's arranged layout, in desktop | ||
| /// cells. A control's <see cref="SharpConsoleUI.Layout.LayoutNode.AbsoluteBounds"/> is in | ||
| /// window-content space, so the window's own origin and inset are added back on. | ||
| /// Internal so a headless test can check the mapping against the framework's own hit testing — | ||
| /// it is the one part of the drag that no pure unit test can pin down. | ||
| /// </summary> | ||
| internal PaneDragSurface PaneSnapshot() | ||
| { | ||
| var origin = ContentOrigin(); | ||
| var rects = new Dictionary<string, PaneRect>(StringComparer.Ordinal); | ||
| var windows = new Dictionary<string, string>(StringComparer.Ordinal); | ||
|
|
||
| KeyValuePair<string, TabControl>[] realised; | ||
| lock (_paneTabsLock) | ||
| { | ||
| realised = _paneTabs.ToArray(); | ||
| } | ||
|
|
||
| foreach (var (paneId, tabs) in realised) | ||
| { | ||
| if (_window.GetLayoutNode(tabs) is not { } node) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| var bounds = node.AbsoluteBounds; | ||
| rects[paneId] = new PaneRect(origin.X + bounds.X, origin.Y + bounds.Y, bounds.Width, bounds.Height); | ||
|
|
||
| if (_workspace.Layout.FindPane(paneId)?.ActiveTab is { } windowId) | ||
| { | ||
| windows[paneId] = windowId; | ||
| } | ||
| } | ||
|
|
||
| return new PaneDragSurface(rects, windows); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Is SharpConsoleUI's Window/LayoutNode API thread-safe for concurrent access from a background input thread while the UI thread renders?
💡 Result:
SharpConsoleUI's Window and LayoutNode API are not designed for direct, thread-safe concurrent access from a background thread while the UI thread is rendering [1][2]. The framework enforces a strict threading model to ensure stability [1][2]: 1. Threading Model: All UI rendering operations (Measure, Arrange, Paint) occur exclusively on the main UI thread, protected by a render lock (_renderLock) [3][1][4][2]. Background threads are not permitted to modify window state or layout tree nodes directly [1][2]. 2. Communication: Background threads, such as an input loop, must communicate with the UI thread by enqueuing actions or data (e.g., via an input queue) rather than accessing UI objects directly [1][2][5]. The library provides synchronization mechanisms specifically for this, such as _inputQueue or marshalling via EnqueueOnUIThread [1][2][6][5]. 3. Lock Hierarchy: The library maintains a strict internal lock hierarchy (_renderLock -> window._lock -> _consoleLock) to prevent deadlocks [1][2]. Accessing or modifying internal state from outside these established patterns can lead to race conditions or thread safety violations [1][2][5]. In summary, you must marshal any changes to LayoutNode or Window state to the UI thread [1][2][6]. Attempting to manipulate these objects directly from a background thread while the UI thread is rendering is unsafe and violates the library's concurrency architecture [1][2].
Citations:
- 1: https://github.com/nickprotop/ConsoleEx/blob/v2.5.12/docs/RENDERING_PIPELINE.md
- 2: https://nickprotop.github.io/ConsoleEx/docfx/_site/RENDERING_PIPELINE.html
- 3: https://nickprotop.github.io/ConsoleEx/docfx/_site/DOM_LAYOUT_SYSTEM.html
- 4: https://github.com/nickprotop/ConsoleEx/blob/v2.5.12/docs/DOM_LAYOUT_SYSTEM.md
- 5: https://github.com/nickprotop/ConsoleEx/blob/v2.4.77/SharpConsoleUI/ConsoleWindowSystem.cs
- 6: https://nickprotop.github.io/ConsoleEx/docfx/_site/DIALOGS.html
🏁 Script executed:
# Inspect the relevant code paths and surrounding threading assumptions.
git ls-files | rg 'SharpMUTermApp\.cs|PaneDrag|OnDriverMouseEvent|OnWindowKey|BuildDragPane|RebuildPaneArea|OnUiThread|GetLayoutNode|AbsoluteBounds'Repository: SharpMUSH/SharpMUTerm
Length of output: 432
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in src/SharpMUTerm.Tui/SharpMUTermApp.cs src/SharpMUTerm.Tui/PaneDragTracker.cs src/SharpMUTerm.Tui/PaneDragSurface.cs tests/SharpMUTerm.Tui.Tests/PaneDragTrackerTests.cs tests/SharpMUTerm.Tui.Tests/PaneDragSurfaceTests.cs tests/SharpMUTerm.Tui.Tests/PaneDragEndToEndTests.cs; do
echo "===== $f ====="
wc -l "$f"
done
echo "===== SharpMUTermApp relevant symbols ====="
ast-grep outline src/SharpMUTerm.Tui/SharpMUTermApp.cs --view expanded | sed -n '1,260p'
echo "===== PaneDragTracker outline ====="
ast-grep outline src/SharpMUTerm.Tui/PaneDragTracker.cs --view expanded | sed -n '1,260p'
echo "===== OnDriverMouseEvent / OnWindowKey / BuildDragPane / RebuildPaneArea / OnUiThread references ====="
rg -n -A 6 -B 6 'OnDriverMouseEvent|OnWindowKey|BuildDragPane|RebuildPaneArea|OnUiThread|GetLayoutNode|AbsoluteBounds|PaneSnapshot|_paneDrag' src/SharpMUTerm.Tui/SharpMUTermApp.cs src/SharpMUTerm.Tui/PaneDragTracker.csRepository: SharpMUSH/SharpMUTerm
Length of output: 50377
Marshal the whole mouse-drag callback onto the UI thread. _paneDrag is mutated in OnDriverMouseEvent while BuildDragPane and the Escape handler touch the same tracker on the UI thread, and PaneSnapshot() also reads Window.GetLayoutNode/LayoutNode.AbsoluteBounds directly. Moving only ApplyDragResult still leaves those races in place.
🤖 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 `@src/SharpMUTerm.Tui/SharpMUTermApp.cs` around lines 1873 - 1927, Marshal the
entire OnDriverMouseEvent callback onto the UI thread, including overlay checks,
PaneSnapshot, _paneDrag.Handle, and ApplyDragResult. Ensure the driver callback
only schedules this work through OnUiThread, so all _paneDrag mutations and
PaneSnapshot layout reads occur on the same UI thread as BuildDragPane and the
Escape handler.
|
|
||
| var pane = layout.FindWindow("b")!; | ||
| await Assert.That(pane.Id).IsEqualTo(target); | ||
| await Assert.That(pane.Tabs).IsEquivalentTo(new[] { "c", "b" }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C2 'IsEquivalentTo\(' tests/SharpMUTerm.Core.Tests/Workspace/PaneDropTests.cs
rg -n --glob '*.cs' 'CollectionOrdering\.Matching' .Repository: SharpMUSH/SharpMUTerm
Length of output: 1190
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
sed -n '1,140p' tests/SharpMUTerm.Core.Tests/Workspace/PaneDropTests.csRepository: SharpMUSH/SharpMUTerm
Length of output: 4810
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
web_search
TUnit IsEquivalentTo collection ordering default behaviorRepository: SharpMUSH/SharpMUTerm
Length of output: 207
🌐 Web query:
TUnit IsEquivalentTo collection ordering default behavior
💡 Result:
In the TUnit framework, the default behavior of the IsEquivalentTo assertion is to ignore the order of elements [1][2]. It focuses solely on whether the collections contain the same items with the same counts [1]. To require that elements must appear in the same order, you must explicitly pass the CollectionOrdering.Matching parameter to the method [1][2]. Example of default behavior (order-independent): await Assert.That(new[] { 1, 2, 3 }).IsEquivalentTo(new[] { 3, 2, 1 }); // Passes Example of order-sensitive behavior: await Assert.That(new[] { 1, 2, 3 }).IsEquivalentTo(new[] { 1, 2, 3 }, CollectionOrdering.Matching); // Passes await Assert.That(new[] { 1, 2, 3 }).IsEquivalentTo(new[] { 3, 2, 1 }, CollectionOrdering.Matching); // Fails There is no global configuration option to change this default; the explicit parameter approach is maintained to ensure clarity and consistency [2].
Citations:
- 1: https://tunit.dev/docs/assertions/collections/
- 2: Fix IsEquivalentTo documentation - remove non-existent IgnoringOrder method thomhurst/TUnit#3718
Assert tab order explicitly. Lines 25 and 66 use IsEquivalentTo(...), which ignores element order, so a reordered Tabs sequence would still pass. Pass CollectionOrdering.Matching here if the test is meant to protect tab order.
🤖 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 `@tests/SharpMUTerm.Core.Tests/Workspace/PaneDropTests.cs` at line 25, Update
the tab assertions in the affected tests, including the assertions near lines 25
and 66, to pass CollectionOrdering.Matching to IsEquivalentTo so the expected
sequence order is enforced while retaining the existing tab values.
Backlog item 2, the last unwired piece. SharpMUTerm.Graphics existed and was tested but nothing in the TUI ever called it, so "inline graphics, in scope from day one" had never once put a picture on screen. Wired at the <img> seam in the web view, which is the only place in the codebase where an image URL survives: MXP and Pueblo both parse <IMG> and discard it, and HtmlStyledRenderer already produced a placeholder. HtmlStyledRenderer now indexes each placeholder line, WebPage carries the images, and WebViewComposer splits the page into text and image blocks for the framework's ImageControl. Rendering goes through the framework rather than our own encoder, and that is forced rather than preferred: our Kitty and Sixel encoders emit escape-sequence strings, but a compositor owns every cell, Cell has no raw-escape field, and AppendCombiner strips escapes as an anti-injection defence. There is nowhere to put an escape blob. The framework's renderer works because it writes U+10EEEE placeholder cells -- images become real cells, which is what docs/PLAN.md committed to. Negative finding worth recording: the framework has no Sixel back-end at v2.5.14, and IImageRenderer is internal with ResolveRenderer private, so one cannot be injected. A Sixel-only terminal therefore degrades to half-blocks. That is stated in the policy's own Describe() output rather than failing silently. Lifting it needs an upstream PR. Degradation is Kitty -> half-block -> text placeholder, and when nothing is supported no image is fetched at all. That chain is the part verifiable without a graphics terminal, so it is tested exhaustively: every protocol x surface combination, plus an invariant that no combination ever upgrades past what was detected. SharpMUTerm.Graphics keeps its own encoders, still reachable for a raw-terminal host; nothing deleted in this pass. 764 tests pass, up from 649. Not verified: that a Kitty image actually appears. The transmission, encode and placement are framework paths only a real GPU terminal exercises. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The backlog had become a write-up of finished work -- four of six items were descriptions of things already built, so a session looking for remaining work had to read ~180 lines to find the few paragraphs that still matter. A handoff should say what is true now, not narrate how it got here. What is left is now five items: field editing on the config screens, the real-terminal verification still owed, Sixel inside the compositor (blocked upstream), MXP/Pueblo <IMG> routing, and the CodeRabbit nitpicks a future agent must not "fix". The durable framework knowledge those items carried moves into Critical Gotchas, where it applies regardless of what is built: why SupportsKittyGraphics reads false in a constructor, why our escape-string encoders cannot go into a compositor, why drag wiring lives at the driver rather than on a control, and the settings-screen architecture. Corrects the IImageRenderer path, which is in Imaging/, not Controls/ImageControl/. Framework citations re-checked against the v2.5.14 clone: ImageControl.cs:375 is ResolveRenderer, Cell.cs:122 is AppendCombiner, and Sixel appears only in a forward-looking comment and a docs comparison table. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
HarryCordewener
changed the title
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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.Graphics/InlineImagePolicy.cs`:
- Around line 127-130: Replace the numeric enum comparison in Describe with an
explicit GraphicsProtocol-to-InlineImagePresentation mapping, using a helper
such as PresentationFor and handling unsupported protocols with TextPlaceholder.
Compare chosen against the mapped detected value, keeping the existing match
description behavior.
In `@src/SharpMUTerm.Tui/SharpMUTermApp.cs`:
- Around line 1368-1384: Update the web-page navigation flow around
RebuildPaneArea, BuildTabContent, and BuildWebContent so the pane is rebuilt
whenever the outgoing page has composed image content, not only when isNew is
true. Ensure the old ScrollablePanel and its extra MarkupControl/ImageControl
children are replaced before activating the new page, while preserving the
existing markup update and image-loading behavior.
- Around line 1369-1372: In the cleanup flow around _webImageCts, cancel the
source and clear the reference but do not dispose it there. Move disposal into
the finally block of LoadWebImagesAsync, after the background load has exited,
so its token remains valid through _imageLoader.LoadAsync and
HttpClient.GetAsync.
In `@src/SharpMUTerm.Tui/WebImageLoader.cs`:
- Around line 152-158: Replace the full-body ReadAsByteArrayAsync usage in the
image-loading method with the existing WebPageFetcher.ReadCappedAsync helper,
passing MaxImageBytes and the cancellation token so the response is capped while
streaming. Retain the Content-Length fast rejection and return the capped result
without materializing oversized responses.
In `@src/SharpMUTerm.Web/HtmlStyledRenderer.cs`:
- Around line 184-202: Update EmitImage so the placeholder label is written
through the non-wrapping/preformatted path, while preserving its existing
styling, link, and image index behavior. Ensure long alt text remains on the
image’s recorded line and add a WebImageIndexTests case using a narrow width
with sufficiently long alt text to verify it does not wrap.
In `@tests/SharpMUTerm.Tui.Tests/WebImageLoaderTests.cs`:
- Around line 161-175: Add a test alongside NonHttpSources_AreNeverFetched that
injects a stub HttpClient handler into WebImageLoader, returns an oversized
image/png response without a Content-Length header, and verifies LoadAsync
rejects it at the configured byte cap. Keep the test entirely in-memory and
cover the HTTP fetch path rather than the existing data: handling.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: f283e9d0-2fdf-4bb4-8f6f-72df278bc51c
📒 Files selected for processing (18)
CLAUDE.mddocs/HANDOFF.mdsrc/SharpMUTerm.Graphics/InlineImagePolicy.cssrc/SharpMUTerm.Tui/SharpMUTermApp.cssrc/SharpMUTerm.Tui/WebImageLayout.cssrc/SharpMUTerm.Tui/WebImageLoader.cssrc/SharpMUTerm.Tui/WebViewComposer.cssrc/SharpMUTerm.Web/HtmlStyledRenderer.cssrc/SharpMUTerm.Web/LineWriter.cssrc/SharpMUTerm.Web/WebImage.cssrc/SharpMUTerm.Web/WebPage.cssrc/SharpMUTerm.Web/WebPageFetcher.cstests/SharpMUTerm.Graphics.Tests/InlineImagePolicyTests.cstests/SharpMUTerm.Tui.Tests/WebImageLayoutTests.cstests/SharpMUTerm.Tui.Tests/WebImageLoaderTests.cstests/SharpMUTerm.Tui.Tests/WebInlineImageEndToEndTests.cstests/SharpMUTerm.Tui.Tests/WebViewComposerTests.cstests/SharpMUTerm.Web.Tests/WebImageIndexTests.cs
Comment on lines
+127
to
+130
| if ((int)chosen == (int)detected) | ||
| { | ||
| return $"inline images render via {chosen}"; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Coupling InlineImagePresentation to GraphicsProtocol by numeric value is fragile. (int)chosen == (int)detected only works while the two enums stay numerically aligned; adding a rung to either (e.g. an iTerm protocol) silently makes Describe claim a match that isn't one. An explicit mapping keeps the intent local.
♻️ Suggested mapping instead of a numeric cast
- if ((int)chosen == (int)detected)
+ if (chosen == PresentationFor(detected))
{
return $"inline images render via {chosen}";
}private static InlineImagePresentation PresentationFor(GraphicsProtocol protocol) => protocol switch
{
GraphicsProtocol.Kitty => InlineImagePresentation.Kitty,
GraphicsProtocol.Sixel => InlineImagePresentation.Sixel,
GraphicsProtocol.HalfBlock => InlineImagePresentation.HalfBlock,
_ => InlineImagePresentation.TextPlaceholder,
};📝 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
| if ((int)chosen == (int)detected) | |
| { | |
| return $"inline images render via {chosen}"; | |
| } | |
| if (chosen == PresentationFor(detected)) | |
| { | |
| return $"inline images render via {chosen}"; | |
| } | |
| private static InlineImagePresentation PresentationFor(GraphicsProtocol protocol) => protocol switch | |
| { | |
| GraphicsProtocol.Kitty => InlineImagePresentation.Kitty, | |
| GraphicsProtocol.Sixel => InlineImagePresentation.Sixel, | |
| GraphicsProtocol.HalfBlock => InlineImagePresentation.HalfBlock, | |
| _ => InlineImagePresentation.TextPlaceholder, | |
| }; |
🤖 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 `@src/SharpMUTerm.Graphics/InlineImagePolicy.cs` around lines 127 - 130,
Replace the numeric enum comparison in Describe with an explicit
GraphicsProtocol-to-InlineImagePresentation mapping, using a helper such as
PresentationFor and handling unsupported protocols with TextPlaceholder. Compare
chosen against the mapped detected value, keeping the existing match description
behavior.
Comment on lines
+1369
to
+1372
| _webImageCts?.Cancel(); | ||
| _webImageCts?.Dispose(); | ||
| _webImageCts = null; | ||
| _webImages.Clear(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does disposing a CancellationTokenSource while HttpClient.SendAsync is using its token throw ObjectDisposedException?
💡 Result:
Disposing a CancellationTokenSource (CTS) while an HttpClient request is using its token can indeed lead to an ObjectDisposedException, although the behavior depends on the specific timing and the internal state of the operation [1][2]. When you pass a CancellationToken to HttpClient.SendAsync, the client often creates a linked token source internally if a timeout is configured or if the provided token can be canceled [3][4]. If you dispose of the CTS that you provided to the request while that request is still active, any subsequent attempt by HttpClient or its underlying handlers to access the token—or to perform operations that check the state of the CTS—may throw an ObjectDisposedException [5][1]. This occurs because: 1. Accessing the Token property of a disposed CTS throws an ObjectDisposedException [5][1]. 2. Internal mechanisms within the HttpClient stack or the underlying network handlers may perform registration or state checks on the provided token or the linked token source after the request has been initiated [5][4]. If these operations occur after the object is disposed, the exception is triggered [1][2]. 3. Disposing the CTS does not inherently cancel the operation; it simply marks the source as disposed [6]. If you intend to cancel an ongoing request, you should call Cancel on the CancellationTokenSource instead of disposing it [1][6]. It is a best practice to keep the CancellationTokenSource alive until the HttpClient operation has completed (i.e., until the returned Task has finished) to avoid these race conditions [2][6]. If you are using a linked token source, ensure it remains in scope for the duration of the asynchronous operation [7].
Citations:
- 1: Make
CancellationTokenSource.Tokenaccessible after dispose dotnet/runtime#105811 - 2: https://stackoverflow.com/questions/68829171/system-objectdisposedexception-the-cancellationtokensource-has-been-disposed
- 3: https://source.dot.net/System.Net.Http/System/Net/Http/HttpClient.cs.html
- 4: https://github.com/dotnet/runtime/blob/bbdd508a/src/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
- 5: https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.CoreLib/src/System/Threading/CancellationTokenSource.cs
- 6: CancellationTokenSource is hard to use correctly dotnet/runtime#29970
- 7: https://www.danielcrabtree.com/blog/33/gotchas-with-httpclients-cancelpendingrequests-and-timeout-in-net
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file and inspect the relevant symbols/regions.
ast-grep outline src/SharpMUTerm.Tui/SharpMUTermApp.cs --view expanded || true
echo
echo "=== nearby lines around 1369-1372 ==="
sed -n '1320,1415p' src/SharpMUTerm.Tui/SharpMUTermApp.cs
echo
echo "=== search for web image CTS lifecycle ==="
rg -n "_webImageCts|StartWebImageLoad|LoadWebImagesAsync|WebImageLoader.FetchAsync" src/SharpMUTerm.Tui/SharpMUTermApp.csRepository: SharpMUSH/SharpMUTerm
Length of output: 14207
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the related loader implementation if it lives in the repo.
fd -a "WebImageLoader*.cs" src || true
fd -a "SharpMUTermApp.cs" src || true
rg -n "class WebImageLoader|FetchAsync|LoadWebImagesAsync|StartWebImageLoad" src || trueRepository: SharpMUSH/SharpMUTerm
Length of output: 1290
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# If there is a separate file for the loader, inspect relevant sections.
for f in $(fd -a "WebImageLoader*.cs" src); do
echo "=== $f ==="
wc -l "$f"
ast-grep outline "$f" --view expanded || true
doneRepository: SharpMUSH/SharpMUTerm
Length of output: 982
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== LoadWebImagesAsync region ==="
sed -n '1411,1495p' src/SharpMUTerm.Tui/SharpMUTermApp.cs
echo
echo "=== DisposeAsync region ==="
sed -n '2488,2525p' src/SharpMUTerm.Tui/SharpMUTermApp.cs
echo
echo "=== WebImageLoader.FetchAsync / LoadAsync region ==="
sed -n '40,150p' src/SharpMUTerm.Tui/WebImageLoader.csRepository: SharpMUSH/SharpMUTerm
Length of output: 8257
🌐 Web query:
.NET CancellationTokenSource dispose while HttpClient request with its token is still running ObjectDisposedException token registration
💡 Result:
In.NET, disposing a CancellationTokenSource (CTS) while an HttpClient request is still actively using its token is a race condition that can lead to an ObjectDisposedException [1]. This occurs because the HttpClient internal implementation may attempt to access the CTS—such as when triggering cancellation or cleaning up resources—after it has been explicitly disposed by your code [2][3][1]. The documentation explicitly states that Dispose must only be called when all other operations on the CTS have completed [4]. Because the lifetime of an asynchronous HTTP request is often difficult to track precisely, disposing the CTS while a request is in flight is generally considered unsafe [5][4]. Key points regarding this issue include: 1. Race Condition: When you call Dispose on a CTS, it enters an unusable state [6]. If the HttpClient (or another component) later attempts to interact with the CTS or its token, it will throw an ObjectDisposedException [3][7]. This is common in asynchronous flows where a finally block disposes the CTS while the underlying network operation is still technically finishing [3][1]. 2. Best Practices for Cleanup: - For standard requests, you generally do not need to dispose of a CTS immediately after the task completes if you are in a high-throughput environment, as the overhead is minimal unless the CTS was created with a linked token or a long-running timer [5]. - If you must dispose of the CTS, ensure the associated Task has fully completed (e.g., by awaiting the HttpClient request) before calling Dispose [4]. - If you are dealing with "fire-and-forget" or complex concurrent scenarios where you cannot guarantee the task completion, some developers choose to leave the CTS undisposed to avoid the race condition, allowing the garbage collector to reclaim the resources naturally [5][4]. 3. HttpClient Implementation: HttpClient is designed to consume the CancellationToken provided to it [8]. It does not "own" the CTS lifetime, meaning it is the caller's responsibility to ensure the token remains valid for the entire duration of the request [5]. If you are experiencing this exception, ensure that your disposal logic is strictly ordered after the await of the HttpClient call [3][4]. If your architecture makes this impossible, you may need to reconsider the CTS ownership or accept that not disposing of it is a safe trade-off to avoid the exception [5][4].
Citations:
- 1: HTTP/3: Cancellation causing ObjectDisposedException dotnet/runtime#56683
- 2: HttpClient ObjectDisposed after SDK upgrade from 34.0.95 -> 34.0.113 dotnet/runtime#115611
- 3: https://stackoverflow.com/questions/68829171/system-objectdisposedexception-the-cancellationtokensource-has-been-disposed
- 4: https://stackoverflow.com/questions/61359443/correct-pattern-to-dispose-of-cancellation-token-source
- 5: CancellationTokenSource is hard to use correctly dotnet/runtime#29970
- 6: https://learn.microsoft.com/en-us/dotnet/api/system.threading.cancellationtokensource.dispose?view=net-10.0
- 7: https://learn.microsoft.com/en-us/dotnet/api/system.threading.cancellationtokensource.cancel?view=netframework-4.8.1
- 8: How to use CancellationToken with HttpClient dotnet/runtime#106558
Keep _webImageCts alive until LoadWebImagesAsync exits. Cancel here, but dispose it in the background task’s finally; this fire-and-forget load can still pass the token into _imageLoader.LoadAsync/HttpClient.GetAsync, and disposing the source mid-flight can throw ObjectDisposedException.
🤖 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 `@src/SharpMUTerm.Tui/SharpMUTermApp.cs` around lines 1369 - 1372, In the
cleanup flow around _webImageCts, cancel the source and clear the reference but
do not dispose it there. Move disposal into the finally block of
LoadWebImagesAsync, after the background load has exited, so its token remains
valid through _imageLoader.LoadAsync and HttpClient.GetAsync.
Comment on lines
+152
to
+158
| if (response.Content.Headers.ContentLength > MaxImageBytes) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| var bytes = await response.Content.ReadAsByteArrayAsync(cancellationToken).ConfigureAwait(false); | ||
| return bytes.Length > MaxImageBytes ? null : bytes; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unbounded buffering when Content-Length is absent or wrong. The header check is skipped for chunked/null-length responses, and ReadAsByteArrayAsync then materialises the whole body before line 158 can reject it — so the 8 MB cap only applies after the allocation. WebPageFetcher.ReadCappedAsync already streams with a hard cap; do the same (or set MaxResponseContentBufferSize on the owned client).
🛡️ Cap while reading instead of after
- var bytes = await response.Content.ReadAsByteArrayAsync(cancellationToken).ConfigureAwait(false);
- return bytes.Length > MaxImageBytes ? null : bytes;
+ await using var stream = await response.Content
+ .ReadAsStreamAsync(cancellationToken).ConfigureAwait(false);
+ using var buffered = new MemoryStream();
+ var chunk = new byte[81920];
+ int read;
+ while ((read = await stream.ReadAsync(chunk, cancellationToken).ConfigureAwait(false)) > 0)
+ {
+ if (buffered.Length + read > MaxImageBytes)
+ {
+ return null; // oversized: the placeholder stays
+ }
+
+ buffered.Write(chunk, 0, read);
+ }
+
+ return buffered.ToArray();📝 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
| if (response.Content.Headers.ContentLength > MaxImageBytes) | |
| { | |
| return null; | |
| } | |
| var bytes = await response.Content.ReadAsByteArrayAsync(cancellationToken).ConfigureAwait(false); | |
| return bytes.Length > MaxImageBytes ? null : bytes; | |
| if (response.Content.Headers.ContentLength > MaxImageBytes) | |
| { | |
| return null; | |
| } | |
| await using var stream = await response.Content | |
| .ReadAsStreamAsync(cancellationToken).ConfigureAwait(false); | |
| using var buffered = new MemoryStream(); | |
| var chunk = new byte[81920]; | |
| int read; | |
| while ((read = await stream.ReadAsync(chunk, cancellationToken).ConfigureAwait(false)) > 0) | |
| { | |
| if (buffered.Length + read > MaxImageBytes) | |
| { | |
| return null; // oversized: the placeholder stays | |
| } | |
| buffered.Write(chunk, 0, read); | |
| } | |
| return buffered.ToArray(); |
🤖 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 `@src/SharpMUTerm.Tui/WebImageLoader.cs` around lines 152 - 158, Replace the
full-body ReadAsByteArrayAsync usage in the image-loading method with the
existing WebPageFetcher.ReadCappedAsync helper, passing MaxImageBytes and the
cancellation token so the response is capped while streaming. Retain the
Content-Length fast rejection and return the capped result without materializing
oversized responses.
Comment on lines
+184
to
+202
| private void EmitImage(IElement element, LineWriter writer, TextStyle style, bool preformatted) | ||
| { | ||
| var alt = element.GetAttribute("alt"); | ||
| var label = string.IsNullOrWhiteSpace(alt) ? "[image]" : $"[image: {alt}]"; | ||
| var src = element.GetAttribute("src"); | ||
| var hasSource = !string.IsNullOrWhiteSpace(src); | ||
| var resolved = hasSource ? Resolve(src!) : null; | ||
|
|
||
| writer.EndLine(); | ||
| var lineIndex = writer.LineIndex; | ||
| var imgLink = resolved is null ? null : SpanInteraction.Link(resolved); | ||
| writer.AddText(label, style.AddAttribute(TextAttributes.Italic), imgLink, preformatted); | ||
| writer.EndLine(); | ||
|
|
||
| if (resolved is not null) | ||
| { | ||
| _images.Add(new WebImage(lineIndex, resolved, alt?.Trim() is { Length: > 0 } a ? a : null, label)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Long alt text can wrap the placeholder across two lines, breaking the LineIndex contract.
EmitImage captures lineIndex before calling writer.AddText(label, ..., preformatted) with preformatted: false for ordinary body content. LineWriter.AddText word-wraps when content exceeds _width (clamped down to as low as 20), so a placeholder like "[image: <long alt text>]" can split into two physical lines. The recorded WebImage.LineIndex then only points at the first fragment; WebViewComposer.Compose will start its trailing text block right after that line, leaking the placeholder's second half into the surrounding text once the image is drawn — producing a visibly broken page for any image with alt text that doesn't fit on one row.
Force the placeholder write path to never wrap, mirroring the width-agnostic behavior AddPreformatted already gives:
🐛 Proposed fix
writer.EndLine();
var lineIndex = writer.LineIndex;
var imgLink = resolved is null ? null : SpanInteraction.Link(resolved);
- writer.AddText(label, style.AddAttribute(TextAttributes.Italic), imgLink, preformatted);
+ // Never let the placeholder wrap: it must land on exactly one line so the recorded
+ // lineIndex stays valid for image-capable views to swap it for a picture.
+ writer.AddText(label, style.AddAttribute(TextAttributes.Italic), imgLink, preformatted: true);
writer.EndLine();Also worth adding a WebImageIndexTests case with alt text long enough to wrap at a narrow width, since none of the current tests exercise this path.
📝 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
| private void EmitImage(IElement element, LineWriter writer, TextStyle style, bool preformatted) | |
| { | |
| var alt = element.GetAttribute("alt"); | |
| var label = string.IsNullOrWhiteSpace(alt) ? "[image]" : $"[image: {alt}]"; | |
| var src = element.GetAttribute("src"); | |
| var hasSource = !string.IsNullOrWhiteSpace(src); | |
| var resolved = hasSource ? Resolve(src!) : null; | |
| writer.EndLine(); | |
| var lineIndex = writer.LineIndex; | |
| var imgLink = resolved is null ? null : SpanInteraction.Link(resolved); | |
| writer.AddText(label, style.AddAttribute(TextAttributes.Italic), imgLink, preformatted); | |
| writer.EndLine(); | |
| if (resolved is not null) | |
| { | |
| _images.Add(new WebImage(lineIndex, resolved, alt?.Trim() is { Length: > 0 } a ? a : null, label)); | |
| } | |
| } | |
| private void EmitImage(IElement element, LineWriter writer, TextStyle style, bool preformatted) | |
| { | |
| var alt = element.GetAttribute("alt"); | |
| var label = string.IsNullOrWhiteSpace(alt) ? "[image]" : $"[image: {alt}]"; | |
| var src = element.GetAttribute("src"); | |
| var hasSource = !string.IsNullOrWhiteSpace(src); | |
| var resolved = hasSource ? Resolve(src!) : null; | |
| writer.EndLine(); | |
| var lineIndex = writer.LineIndex; | |
| var imgLink = resolved is null ? null : SpanInteraction.Link(resolved); | |
| // Never let the placeholder wrap: it must land on exactly one line so the recorded | |
| // lineIndex stays valid for image-capable views to swap it for a picture. | |
| writer.AddText(label, style.AddAttribute(TextAttributes.Italic), imgLink, preformatted: true); | |
| writer.EndLine(); | |
| if (resolved is not null) | |
| { | |
| _images.Add(new WebImage(lineIndex, resolved, alt?.Trim() is { Length: > 0 } a ? a : null, label)); | |
| } | |
| } |
🤖 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 `@src/SharpMUTerm.Web/HtmlStyledRenderer.cs` around lines 184 - 202, Update
EmitImage so the placeholder label is written through the
non-wrapping/preformatted path, while preserving its existing styling, link, and
image index behavior. Ensure long alt text remains on the image’s recorded line
and add a WebImageIndexTests case using a narrow width with sufficiently long
alt text to verify it does not wrap.
Backlog item 1's core. The screens could navigate and toggle but nothing let you type -- no new host, port, interval or pattern. Key scheme: ⏎ activates the focused row when it is activatable, and saves when it is not, so ⏎ still means Save everywhere it did before. ⌃S always saves. Esc cancels the screen, except while a field edit is open, where it abandons that edit and leaves the screen up. A ScreenRow is now a stop, a checkbox, an ordered record of fields, or both at once -- a keypad binding is Space-enables plus ⏎-edits-the- command. Fields hang off rows that already exist rather than adding rows of their own, which is what let every existing pane-count and cursor-index assertion keep passing untouched. Validation happens at commit, not per keystroke: any character types into the buffer, and ⏎/⇥/⌃S validate. A rejected value keeps the edit open and marks the field inline rather than being silently dropped. Rejecting keystrokes instead would mean never being able to clear a port to retype it. ScreenEdits.Apply is the single path from buffer to config and refuses before snapshotting, so an invalid value reaches neither config nor the undo log. Trigger.Pattern and Alias.Pattern become settable and both drop the cached compiled Regex -- the same trap Alias.CaseSensitive had, now with a Core test each asserting the engine stops matching the old pattern. The hint-honesty test is inverted rather than deleted: a screen must advertise ⏎ edit if and only if its model actually offers an editable row, asserted across all six screens in both a populated and a bare configuration so it cannot pass vacuously. Also fixes a flaky test that predates this work. PaneDragEndToEndTests ran in parallel while RenderSnapshot redirects Console.Out and the harness redirects Console.In -- both process-global, so two overlapping renders swapped each other's streams and one got a truncated frame, yielding pane rects that didn't match the screen. It failed roughly one run in ten; the class is now serialised, and 15 consecutive runs are clean. 811 tests pass, up from 764. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Finishes backlog item 1. The add/remove buttons were painted but inert,
and F2's route list and highlight row were read-only indicators.
A ScreenRow may now carry a button whose action returns its own undo,
rather than being snapshotted beforehand like a toggle or field: the
undo for an insertion cannot be described until the thing exists.
Deleting restores at the original index on cancel, not merely
re-appends. Buttons that would act on nothing are not drawn, so ⏎ never
lands on a silent no-op, and destructive buttons name their target on
screen ([- del] Aetherfall) so the row says what it will remove.
Delete is undo-only with no confirmation step: nothing reaches disk
until Save, Esc already replays the undo log, and a second modal state
inside a screen that already has one would double the key-routing rules
for a change that is reversible anyway.
Route radios and the highlight swatches are fields on the rule's own
row, not new rows -- a radio group and a swatch pair are one setting
each, so giving them a row apiece would put N cursor stops in front of
one value. The radios follow the buffer rather than config so ↑↓ visibly
move the dot before ⏎, instead of the group looking inert exactly while
in use.
Colour is a named palette rather than an RGB picker, but the validator
deliberately accepts more than the palette offers -- #rrggbb, idx:N and
none all commit -- because a TerminalColor already in config may be a
colour no short palette names, and a picker that refused the value it
was displaying would make an existing highlight uneditable.
Fixes the footer lying mid-edit: it read [⏎] Save while ⏎ was committing
a field. Focus is threaded through all six FooterLines, and a new test
pins header and footer agreeing about whether an edit is open, in both
directions so it cannot pass by naming no key.
Two pinned row counts in ScreenModelTests legitimately changed --
{2,2,1} to {4,5,1} and {2,0,0} to {4,1,0} -- because button rows are
real navigable rows. Buttons append after each list, so every index
those tests address still means what it did.
Also fixes the cursor doubling as the selection: moving onto [+ world]
pushed the selection past the list, blanking the detail column. Cursor
and selection anchor are now separate.
844 tests pass, up from 811. Eight consecutive Tui runs clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The route field was a closed Choice over SpawnTargets(), which is "distinct spawn-window targets referenced by any trigger" -- so the only windows offerable were ones some trigger already routed to. There was no way to create a new spawn window from the settings screen at all. It is now free text with the known windows as ↑↓ suggestions, following the pattern the colour picker already set: the palette is what ↑↓ steps through, but the validator deliberately accepts more than the palette offers. A window name is a tab title, so it is refused when blank or carrying control characters, and is otherwise whatever the user calls it. Fixes the display bug that fell out of the change: the radios light the row whose label equals the buffer, so a name being typed for the first time matched none of them and was invisible -- the group sat with no dot lit while the keyboard was plainly doing something. A buffer matching no known window now gets its own row carrying the caret, so what is being typed appears where the committed value will. Replaces the test that pinned the old rule (an unknown route refused) with three: an unknown name commits, blank and control characters are refused, and a first-time name is visible while being typed. 846 tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every screen drew "label value" identically whether or not you could change it, so the only way to learn what was editable was to walk the cursor into it. F5's security, password and session rows looked exactly like host and port; F2's highlight indicator and F9's auto-start box looked like toggles you could press and were not. The field well is now the affordance, drawn at rest rather than only mid-edit: an editable value sits on FieldBg, a value you cannot change where it is drawn gets muted ink and no well. Defined once in ScreenChrome.Field/ReadOnly so no renderer invents its own. Opening a field keeps the same well and adds the caret, so ⏎ deepens what is already on screen instead of conjuring it, and nothing shifts. Scoped to label/value rows -- checkboxes and radio groups already carry their own affordance. The two derived indicators stop pretending to be checkboxes: - F2's "highlight line" becomes a muted caption on the highlight section, and moves above the two swatch rows it summarises. It sat below the values it derived from, which read backwards. - F9's "auto-start on connect" merges into the format row. That one was genuinely pressable; the bug was two rows owning one stored value, so setting format to None silently unchecked a box three lines down. It is now one row: Space starts and stops logging, ⏎ picks the format. Drops the "‹ back" affordance, which appeared only on F7-F9 and pointed nowhere -- there is no navigation stack, Esc closes. Footer context lines are now one shape across all eight screens: position plus the one qualifier identifying the selection, with a test pinning it. 857 tests pass, up from 846. Four existing assertions were re-aimed rather than deleted, each pinning behaviour this change removes on purpose; three are inverted so the old behaviour cannot return. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two gaps from the design review. Every list screen drew the item's name as its primary identifier and none let you change it -- you could edit a trigger's regex but not what it was called, four times over. Name is now the first field of every list row on all five screens, so ⏎ on a row (including one just created) opens the value that identifies it. Names are deliberately not unique: nothing keys off them, the engines match on patterns and are keyed by Macro.Key, and two sets may each hold a rule called Tell. Refusing that would be a rule the config doesn't have. Blank and control characters are refused, since a name is drawn into one row of a fixed-width list. Add/remove existed only on F5, so you could create a world but not a trigger, alias, timer or macro. All four now have it, following F5's shape: the action returns its own undo, deleting restores at the original index, buttons that would act on nothing aren't drawn, and destructive ones name their target. Per-screen judgement rather than uniformity: F2 and F3 get duplicate (a rule is a regex plus route plus two colours plus three flags -- get one right, copy it per channel), F6 doesn't (three values, two of which you would change anyway), and F4 must not -- a macro is identified by its Key, MacroEngine is a dictionary, so a copy would land on the original's key and never fire. A new timer starts disabled, being the only one of the four that acts unprompted. Replaces the undiscoverable End binding with Delete on the row itself, running that pane's own remove button, and advertises it: "Del remove" is derived from the model, pinned if-and-only-if so a screen cannot claim the key without offering it. Also makes snapshots deterministic. --snapshot rendered whatever config was on the machine, falling back to the demo only when no worlds existed, so a saved config silently replaced the demo data -- a golden file that changes with the developer's own worlds isn't one. It now always renders the demo; --live-config opts into the real config for debugging. Verified the same command produces an identical frame with and without a user config present. 894 tests pass, up from 857. Four pinned row counts changed and were strengthened rather than renumbered: each now asserts ListSizes, which still carries the original meaning, alongside the new total. Field ordinals moved behind named constants so they cannot drift silently. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Half of what a trigger can do was unreachable. TriggerActions.Rewrite, SendResponse, ScriptCallback and AddAttributes had no UI at all, and Trigger.CaseSensitive had none either despite F3 exposing exactly the same setting on aliases. The README advertises "gag / highlight / rewrite / respond / spawn-route / script"; three of those six could not be reached. All five are now editable, grouped into captioned sections so the fuller pane stays readable: name and pattern, route, highlight (colours plus attributes), actions (rewrite, respond, script), then the flags. AddAttributes is a multi-select rather than a cycling choice, since bold-and-underline is not one-of-N, and it carries no Choices precisely so the chrome does not advertise ↑↓ that would do nothing. A drawn legend names the legal words instead and follows the buffer while open. Script callbacks cannot be enumerated from the scripting host -- its callbacks are anonymous Lua functions keyed by generated ids, written into runtime triggers rather than the config this screen edits -- so the field suggests the callbacks the configuration already names. Trigger.CaseSensitive drops the cached compiled Regex in its setter, the same trap Alias.CaseSensitive and Trigger.Pattern had; pinned in both directions in Core and through the screen. Also replaces the route-to radio group with an editable value, as asked. The window is a name you type -- the spawn windows that exist are defined by what routes to them -- so a radio group was the wrong shape for it: five rows, the only control in the pane not drawn like its neighbours, and unable to show a name typed for the first time without a row invented for the purpose. The windows already in use remain ↑↓ suggestions while the field is open, which is what the group was really offering. RouteRow is deleted rather than left unused. Two assertions pinning the radio rendering were re-aimed at the field: one still asserts the drawn value follows the buffer rather than config, the other that suggestions are not drawn at rest. Corrects the earlier snapshot flag: --snapshot renders your own config, and --demo-config opts into the demo worlds. Defaulting to the demo made the app's default its demo state. CI and tools/make-screenshots.sh now pass the flag explicitly, since both depend on the demo data. 912 tests pass, up from 894. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two fixes from the design review, both about F5 holding settings that were misplaced elsewhere or not reachable at all. F9 edited one specific character's logging and never said which: ActiveLogging() resolved the active character or else the first one configured, so the same screen changed a different character's log depending on what happened to be connected. Log format and folder are now fields on the character's own row, drawn in the CHARACTER form under the heading that names them, with the row itself saying "this character only". F9 still opens -- it seeds F5 on the character pane rather than being a second surface to keep in sync, so the key keeps working without duplicating a screen. HeaderLine takes the key it was opened with, since a header hard-coded to F5 would name a key that reopens rather than closes an F9-opened screen. ActiveLogging()'s fallback also made a disconnected client show the first character's LOG html in the status bar; it now reports off. The security row was a read-only summary of two booleans with no UI. They are now real checkboxes in a fourth pane, appended rather than inserted beside the world they describe, because a pane index is a cursor coordinate and inserting one renumbers every stop the screen and its tests navigate by. "accept invalid certificates" is drawn in the warning colour, with the marker a refused value gets, only while it is both checked and encrypting -- and it names the consequence rather than restating the label. A warning that also fired on an unencrypted connection would train the eye to skip the one case that matters. 929 tests pass, up from 912. Eight pinned assertions changed, each reported and each strengthened rather than renumbered: the pane-shape ones now assert ListSizes alongside the new totals. Five tests covering the deleted F9 options screen were migrated to F5, not dropped. Known limitation: at 100x24 F5 is over-full and the character table clips. The edit band already crowded that column; this made it two rows worse. Collapsing or scrolling that band is its own piece of work. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Replacing F2's route radios with a plain field fixed a closed-list problem and created a discoverability one: at rest the field showed only "Chat", and ↑↓ cycled the known windows one at a time without ever revealing that main, pages and trade existed. Radios over-constrained; a bare field under-shows. A field with known options now draws them while it is open. Built once in ScreenChrome, so all eleven such fields gained it -- route, both highlight colours, script, log format, encoding, ambiguous width, newline key and dictionary (the last two turned out to carry no choices at all, so Text gained an optional list). Typing filters, but a buffer that names a choice keeps the whole list: a field opens on its committed value, so a plain filter would collapse to one entry the instant it was drawn. A filter matching nothing says "a new value is allowed" on an open list and "nothing matches" on a closed one, in neither case using the warning ink -- refusal belongs to the validator at ⏎. ↑↓ walk the narrowed list rather than cycling blind, and the highlight and the buffer are one thing rather than two cursors, which would give ⏎ two meanings inside the only modal state these screens have. It overlays rather than pushes rows down, which was forced: the worlds view sizes a grid row from its line count, so a growing list would resize the screen on ⏎, and F2's editor is already tall enough that pushed rows fall off the bottom. It opens upward when there is no room below. Closed lists say "these values only", open ones "suggestions", drawn from the field's own validator rather than a flag a renderer sets. Checked the framework first, as asked: SharpConsoleUI does have a DropdownControl, and it cannot serve this. Its value is an index into its items, typing is a timed prefix jump rather than entry, and there is no path to a value outside the list -- it is a picker, not a combobox. Route exists precisely to name a window that does not exist yet. Using it for the closed fields alone would also mean rebuilding those panes as control trees, when the whole settings architecture is pure renderers producing markup with Render(...) merging for tests. 946 tests pass, up from 929. Two pinned assertions changed, both direct consequences of ↑↓ walking a filtered list, and both strengthened: one now asserts which entry is marked rather than merely present. Known cosmetic wart: a dropdown covering through F2's attrs row leaves the second line of its two-row legend orphaned below the shadow. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Audited every editable field on F2-F9 against its runtime consumer rather than against whether it persists. A checkbox that saves a bool nothing reads is still a lie, just a durable one. The systemic finding: StartAsync opened an anonymous session -- no character, no trigger sets, no log sink -- so F2's triggers, F3's aliases, F6's timers and every character setting were unreachable at runtime however correct Core was. The shell now connects as the world's first configured character, which is what made most of the rest reachable at all. Wired nine: strip incoming ANSI colour, allow blink, underline hyperlinks, emoji substitution, local echo, keep per-tab drafts, world encoding (now reaching CHARSET negotiation rather than a status-bar label), the log sink, and timers -- nothing had ever realised a TimerDefinition, so a configured timer had never fired. Settings are held by reference and read per line rather than copied at construction; copying them is exactly how a checkbox comes to need a restart. Removed four controls whose features do not exist: ambiguous width (all width measurement is the framework's Wcwidth, which exposes no East-Asian policy), newline key (the prompt is single-line), check spelling and dictionary (there is no spellchecker anywhere in the repo). A control for a feature that doesn't exist should go, not sit there looking operable. Old configs carrying those keys still load. Two left, with reasons rather than pretence. F4's macros cannot fire: SharpConsoleUI's input parser never produces NumPad keys, so that half needs DECKPAM/SS3 decoding upstream. Keepalive has no mechanism at all -- a correct one needs a raw IAC NOP path the telnet session doesn't expose. 999 tests pass, up from 946, with 53 added. Every wired field has a test that changing it changes behaviour, most flipping the setting on an already-connected session, which is what tells Live apart from applied-at-startup. Six pinned assertions changed, each reported: the removed rows' counts, and label assertions turned into absence assertions so the controls cannot drift back silently. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
MacroEngine.Resolve and HandleKeyAsync existed and were unit-tested, and nothing in the app had ever called either -- so no configured macro had ever sent a command, on any key. Dispatch now runs from the main window's preview handler: before the focused control so a binding beats the prompt, not raised while a modal holds the keyboard, and after global shortcuts so app-claimed chords never arrive. MacroEngine keyed its dictionary on the descriptor string, which is a cache of Macro.Key -- a rebound macro would have answered to its old key until reconnect. It now reads Key per lookup. Which chords can actually arrive is read out of the framework's input parser rather than assumed, and the screen says so: the whole numpad never arrives (no DECKPAM is sent and SS3 decodes only a fixed set), so those rows are drawn muted behind a warning marker with captions naming the reason. Ctrl+F1 survives the parser fully and is the one live binding in the demo config. A key is now rebindable through a capture mode -- the next keypress becomes the binding -- rather than by typing a key name as text. Esc is the only non-candidate so capture cannot trap you, and a duplicate is refused with capture still armed rather than created, because a second macro on one key is the one dead row with no symptom: both look alive. Also fixes the add button, which the previous pass flagged rather than changed. It claimed the lowest free numpad digit -- provably dead now that the delivery verdicts exist -- so a new binding was born unable to fire. It claims from MacroKeys.Bindable instead, whose entries are filtered through the same verdicts the screen draws from, so the candidate list cannot drift from what actually works. 1071 tests pass, up from 999. Three pinned assertions changed, all predicted and all pinning the numpad claim this removes; they now assert against Bindable and additionally that the claimed key fires. Still owed: none of this has been seen in a real terminal. The verdicts are read from the parser and asserted, the frames are headless.
TriggerSet is the organising unit of the automation config -- triggers, aliases, timers and macros all live in one, and characters are assigned them by name -- yet nothing created, renamed or deleted a set, and nothing moved an item between them. The only set operation in the whole UI was toggling a character's membership. Rejected the obvious shape, a set switcher on F2/F3/F6/F4. A new pane on four screens renumbers every pane index those screens, their snapshot key scripts and their tests navigate by; "the current set" has no natural home; and it hides something true, because a session runs the union of a character's sets, so the flattened list is the only view showing every rule that can actually fire. It also would not have solved moving an item, which is what actually made a mis-filed trigger permanent. Instead: sets are objects on F5, and "which set" is a field everywhere else. F5's third pane was assignment-only and now owns the sets -- Space still assigns, ⏎ renames, ⇥ edits the description, and it has add/remove. The other four screens get a set field appended last on each item's row, so moving something is an edit like any other, and nothing was renumbered. A set name is the one unique name on these screens, because it is a key: ResolveTriggerSets takes the first match, so the second of two identical names could never be assigned. Renaming rewrites every character's assignment in place, keeping the order that decides which set wins a conflict. Deleting strips the references too, and its undo restores the set at its index and each reference at its own index inside the character that held it -- walking detach backwards and reattach forwards so two references in one character survive. An empty set is visible in both views: a row on F5, and a muted line naming it on each flattened screen, drawn as markup rather than a row so it costs no cursor stop. Also fixes a clipping defect the screenshots caught: with the set cell added, F4's armed key-capture row ran off the right edge, its prompt being twice the width of the key it replaces. 1101 tests pass, up from 1071. Four pinned assertions changed, each reported and each asserting more than before; the pane-shape one keeps its original ListSizes assertion alongside the new total. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The last item of the design review, re-judged against the current screens rather than the ones the review described -- the findings had partly inverted. The dead space was real but the sharper problem was that the fixed 56/64 split now starves the other column: at 100x24 F2's editor clipped at "respond (off)", losing three cursor stops, and F5 clipped one line below its CHARACTERS heading, hiding both characters and all three buttons, while the list column beside them sat empty. Three pure rules in ScreenChrome, and a frame the four list screens share: a column keeps its designed width when the screen affords it and gives cells back when it doesn't; blocks drop their blank separators before anything else; and what still doesn't fit is windowed around the cursor with the edges naming what is off-screen. Bodies size to their content with a spacer beneath, so the hairline ends where the columns do, as F7/F8's card already did. F5 loses its title strip and its CHARACTERS table becomes a selector. Both restated what the rows below them already showed -- the strip's every token appeared in the five editable fields under it, and the table's columns were the character form and the trigger-set pane drawn again. F4 sizes its numpad from the longest command it actually holds instead of truncating to a fixed cell while the column beside it showed the same value in full, and yields that width while a key capture is armed, the prompt being twice as wide as the well it replaces. Each list screen gained a key for its flag marks, which had none anywhere; the marks the selected row carries are lit, so it reads as both a legend and a gloss on the cursor's row -- which also answers the cryptic "on" column header. F5 at 100x24 is correct rather than complete: 17 rows of detail still don't fit in 13, but no cursor stop goes undrawn now, which was the actual bug. A genuinely complete view needs collapsing panes, which remains its own piece of work. Kept F2's attribute legend at rest rather than making it conditional. The orphaned-legend-line case needs a buffer matching nothing, is transient and self-heals on the next keystroke; the legend is the only written record of the vocabulary that field accepts, since it carries no dropdown of its own. 1119 tests pass, up from 1101. Two pinned assertions changed, both strengthened: one now asserts TLS is stated exactly once and in the control that sets it, the other pins where each character fact lives rather than merely that it exists. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The audit recorded keepalive as inert with no mechanism to build on. That is no longer true: TelnetNegotiationCore PR #52 adds an opt-in idle-reset IAC NOP keepalive with a configurable TimeSpan interval. Records why the field still cannot be wired -- the API doesn't exist in the pinned 2.5.3, and unlike SharpConsoleUI it can't be reached through a conditional project reference, because that only works when both paths expose the same API and here the package path wouldn't compile. So it waits on the release rather than on more design, and the note says what to do when 2.6.0 lands. Also records the limitation the eventual wiring inherits: IAC NOP proves the socket write succeeded, not that the peer answered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
TelnetNegotiationCore 2.6.0 shipped the keepalive, so the field that had been inert since it was drawn now does something: a world's keepalive seconds becomes the idle window an IAC NOP goes out on, keeping a NAT or load balancer from evicting a connection that is merely quiet. ResolveKeepalive turns configured seconds into the interval. Zero is how the config spells "off". Anything past the library's 24-hour maximum is clamped rather than thrown, because the value can be hand-edited and refusing to connect over it would be the worse answer. There is deliberately no clamp against the library's one-second minimum. Writing the test found the guard could never fire -- this setting is a whole number of seconds, so every value that isn't already "off" is at least one -- and an unreachable branch claiming to be a safety net is worse than none. The test now pins the property the resolver relies on instead of the clamp it doesn't need. Applies at connect rather than live, and the docs say so: the library's KeepAliveInterval is init-only because the idle loop reads it once when it starts. That puts it with host, port, TLS and encoding rather than with the settings held by reference and read per line. What it does not do is also recorded: IAC NOP proves our write succeeded, not that the server answered, and being unnegotiated there is no way to detect a peer that mishandles it. TIMING-MARK (RFC 860) is the option that would verify the peer and is not implemented upstream. 1129 tests pass, up from 1119. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Jul 31, 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.
Works through the whole
docs/HANDOFF.mdbacklog. Nine commits, each independently verified.515 → 764 tests. No existing test was weakened or deleted at any point.
What landed
<img>renders in the web viewItem 4 is verification-only and needs your terminal.
Bugs found along the way
None of these were the task; all were things the work walked into.
VerticalRule()built aMarkupControlover an empty line list, which measures to zero and never paints. Baseline frame had 0 cells of the rule colour.VisibleLengthstripped[[instead of counting it as one literal character — contradicting its own doc comment and under-padding any column with escaped brackets.←↑↓→ edgewith no arrow handling behind it — only the tab-drop half was reachable andSplitWithWindowhad never been called from the TUI.ActiveLogging()returned a throwaway object).Notable decisions
⏎ edit. A test pins that no screen advertises a key itsScreenModeldoesn't offer.AppConfigurationwould drop[JsonIgnore]fields like a character's in-memory password. A toggle snapshots the value, not the boolean, because F9's checkbox is really aLogFormat.WindowEventDispatchercaptures the pressed control and routes later drag frames back to it, so a control-level handler would only ever see the source pane.Cellhas no raw-escape field, andAppendCombinersanitises escapes. The framework writes U+10EEEE placeholder cells, which is whatdocs/PLAN.mdcommitted to.SharpConsoleUIbuilds from source when cloned beside the repo, so it can be read and stepped into instead of decompiled. CI falls through to the NuGet package unchanged; both paths verified.Negative finding
Sixel cannot work inside the compositor at SharpConsoleUI 2.5.14.
IImageRendererisinternalandResolveRenderer()private, so no back-end can be injected. A Sixel-only terminal degrades to half-blocks, andInlineImagePolicy.Describe()states that reason rather than failing silently. Lifting it needs an upstream PR.Verification
MarkupFormatter/Powerline/RailRenderer: identical.What needs your terminal
Nothing below has been seen by a human:
/graphicsshould report Kitty;/web <page with images>should show them inline.CSI Z, which not all do.🤖 Generated with Claude Code