Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
# TimeWarp.Terminal — Release Readiness Code Review

## Executive Summary
The core API surface is coherent, testable, and well-documented, with strong widget rendering and Unicode/ANSI handling backed by targeted tests. CI/CD automation and package metadata are in place, but the repo still signals “beta” status via versioning and upstream dependencies, and there are a few release-hardening gaps around compatibility, ANSI parsing completeness, and NuGet packaging polish. Addressing the recommendations below will improve confidence for an official release.

## Scope
Focused review of the primary library surface (`IConsole`, `ITerminal`, `Terminal` static facade, ANSI/Unicode utilities, widgets), test doubles, and release tooling/metadata. The goal is to assess code quality and readiness for an official (non-beta) release.

## Methodology
- Read core source files in `source/timewarp-terminal/**`
- Reviewed build/pack configuration (`Directory.Build.props`, `Directory.Packages.props`, `source/Directory.Build.props`)
- Reviewed CI/CD pipeline and dev CLI for release flow (`.github/workflows/ci-cd.yml`, `tools/dev-cli/**`)
- Sampled representative tests under `tests/**`
- Searched for TODO/FIXME/Open Questions (none found)

## Findings

### Strengths
1. **Clean, testable API design**
- `IConsole`/`ITerminal` separation with explicit covariance on sync methods is implemented consistently and aligns with the documented inheritance pattern. This enables fluent chaining while preserving interface segregation. See `iconsole.cs`, `iterminal.cs`, and explicit interface implementations in `timewarp-terminal.cs` and `test-terminal.cs`.
- The `Terminal` static facade mirrors `System.Console` and allows test substitution via `Terminal.Instance`, which is a pragmatic migration path and simplifies user adoption (`terminal-static.cs`).

2. **Robust Unicode and ANSI handling**
- `UnicodeWidth` handles CJK, emoji, and grapheme cluster widths with explicit test coverage, which is essential for accurate widget layout (`unicode-width.cs`, `unicode-width-01-basic.cs`).
- `AnsiStringUtils` provides visibility-aware padding and wrapping, preserving ANSI state across wraps (`ansi-string-utils.cs`). Tests exist for emoji table alignment (`table-widget-06-emoji.cs`).

3. **Widget rendering quality**
- Table, panel, and rule widgets include builder-based configuration, alignment, truncation, and coloring with clear examples and tests (`table-widget.cs`, `panel-widget.cs`, `rule-widget.cs`, `terminal-static-05-widgets.cs`).

4. **CI/CD and release automation in place**
- The repo has a dedicated dev CLI and CI workflow that perform clean/build/test/sample verification, and a separate release pipeline with version checks and NuGet push (`tools/dev-cli/endpoints/ci-command.cs`, `.github/workflows/ci-cd.yml`).

### Release Readiness Gaps / Risks
1. **Project version and dependencies still marked beta**
- The package version is `1.0.0-beta.7` (`source/Directory.Build.props`).
- Several internal dependencies are beta versions (`TimeWarp.Builder`, `TimeWarp.Nuru`, `TimeWarp.Amuru`, etc.) in `Directory.Packages.props`.
- This signals instability to consumers and will likely block “official” perception unless explicitly justified.

2. **Limited target framework compatibility**
- The library targets `net10.0` only (`Directory.Build.props`). If you want broader adoption at release time, consider multi-targeting LTS versions (e.g., `net8.0`) or documenting the strict requirement.

3. **ANSI parsing scope may be too narrow for general usage**
- `AnsiStringUtils` only strips SGR codes and OSC 8 hyperlinks. Other common ANSI control sequences (cursor moves, erase line, etc.) are not handled. If users emit non-SGR ANSI sequences, width calculations and wrapping may be incorrect (`ansi-string-utils.cs`).

4. **Truncation drops ANSI styling**
- `Table.TruncateWithEllipsis` strips ANSI codes and returns plain text, losing styling in truncated cells (`table-widget.cs`). This is likely acceptable, but should be documented or adjusted to preserve styling where feasible.

5. **NuGet package polish**
- The README is included via `None Include` but there is no explicit `PackageReadmeFile` metadata in the project file (`timewarp-terminal.csproj`). NuGet now uses `PackageReadmeFile` to render docs in gallery and clients. Consider adding it alongside other metadata.

### Code Quality Observations
1. **Defensive I/O handling is thoughtful**
- `TimeWarpTerminal` gracefully handles `IOException` for redirected output; this is user-friendly (`timewarp-terminal.cs`).
2. **Test doubles are mature**
- `TestTerminal` and `TestConsole` cover key scenarios and helper APIs (output capture, key queues, line input) and align with the interface contracts (`test-terminal.cs`, `test-console.cs`).
3. **Build and analyzer posture is strict**
- Warnings as errors and analyzer enforcement are enabled; AOT/trim warnings are suppressed in `Directory.Build.props` (intentional, but a release audit should revisit them).

## Recommendations
1. **Finalize release versioning**
- Move `source/Directory.Build.props` to a stable version (e.g., `1.0.0`) and ensure the release pipeline/CI validates no `-beta` suffix for official release tags.
- Evaluate upstream dependency stability; either upgrade to stable versions or explicitly document beta dependencies in the README.

2. **Consider multi-targeting or document strict requirements**
- If you intend broad adoption, multi-target `net8.0`/`net9.0` alongside `net10.0`. If not, clearly document the framework requirement and rationale.

3. **Expand ANSI parsing or document constraints**
- Either expand `AnsiStringUtils` to recognize additional ANSI control sequences or document that width/line calculations assume SGR + OSC 8 only.

4. **Preserve or document ANSI styling on truncation**
- If style loss on truncation is acceptable, document it in table API docs. Otherwise, consider retaining ANSI prefix/suffix where possible for truncated content.

5. **NuGet package metadata polish**
- Add `PackageReadmeFile` (and optionally `PackageIcon`) for a more professional NuGet experience and to avoid support questions.

## References
- README: `README.md`
- Core interfaces and implementations: `source/timewarp-terminal/iconsole.cs`, `source/timewarp-terminal/iterminal.cs`, `source/timewarp-terminal/timewarp-terminal.cs`, `source/timewarp-terminal/terminal-static.cs`
- Widgets and utilities: `source/timewarp-terminal/widgets/table-widget.cs`, `panel-widget.cs`, `rule-widget.cs`, `ansi-string-utils.cs`, `unicode-width.cs`
- Tests: `tests/terminal-static-05-widgets.cs`, `tests/table-widget-06-emoji.cs`, `tests/unicode-width-01-basic.cs`
- Build & packaging: `Directory.Build.props`, `Directory.Packages.props`, `source/Directory.Build.props`, `source/timewarp-terminal/timewarp-terminal.csproj`
- CI/CD: `.github/workflows/ci-cd.yml`, `tools/dev-cli/endpoints/ci-command.cs`
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -417,3 +417,6 @@ FodyWeavers.xsd
*.msm
*.msp
tools/dev-cli/generated/

# Dev CLI AOT binary
bin/
Original file line number Diff line number Diff line change
Expand Up @@ -61,3 +61,14 @@ dotnet tools/dev-cli/dev.cs ci --mode pr
dotnet runfiles/publish-dev.cs
./dev --help
```

## Archived Reason

**Obsolete** - The dev-cli was already converted to runfile structure before this task was tracked:

- `tools/dev-cli/dev.cs` exists (runfile entry point)
- `tools/dev-cli/Directory.Build.props` contains configuration
- No `.csproj` file present
- `endpoints/` directory contains command handlers

The conversion was completed in a previous session but the task file was not updated at that time.
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
# Update TestTerminalContext to integrate with Terminal.Instance property

## Description

Update the existing `TestTerminalContext` to provide seamless integration with the new `Terminal.Instance` static property. This enables users to use the ambient test context pattern while also having access to the static `Terminal` class methods in tests.

## Checklist

- [x] Add `Terminal` property to `TestTerminalContext` that returns `TestTerminal`
- [x] Modify `TestTerminalContext.Resolve(ITerminal? terminal)` to check `Terminal.Instance` first
- [x] Update `TestTerminalContext` to set `Terminal.Instance = Context.Terminal` on initialization
- [x] Restore previous `Terminal.Instance` on disposal (for proper test isolation)
- [x] Update XML documentation with test usage patterns
- [x] Write integration tests demonstrating the test pattern
- [x] Update existing tests to use the new static API

## Notes

### Current Status (2026-03-19)

**What exists:**
- `TestTerminalContext` class in `source/timewarp-terminal/test-terminal-context.cs`
- Uses `AsyncLocal<TestTerminal?>` for test isolation
- Has `Current` property and `Resolve()` methods
- **NOT integrated with `Terminal.Instance`**
- `Terminal` static class in `source/timewarp-terminal/terminal-static.cs`
- Has `Instance` property that can be set to any `ITerminal`
- **Does NOT check `TestTerminalContext.Current`**

**Current test pattern (working):**
```csharp
ITerminal original = Terminal.Instance;
using TestTerminal testTerminal = new();
try
{
Terminal.Instance = testTerminal;
// test code
}
finally
{
Terminal.Instance = original;
}
```

**Proposed pattern (cleaner):**
```csharp
using TestTerminal terminal = new();
TestTerminalContext.Current = terminal;
// Terminal.Instance automatically set
// Terminal.WriteLine routes to testTerminal
// Automatic restoration on dispose
```

### Why This Task

The integration would provide:
1. **Cleaner test code** - No manual try/finally blocks
2. **Automatic restoration** - `Terminal.Instance` restored when context disposed
3. **AsyncLocal isolation** - Each async context gets its own terminal
4. **Backward compatibility** - Existing pattern still works

### Files to Modify

- `source/timewarp-terminal/test-terminal-context.cs` - Add integration logic
- `tests/*.cs` - Optionally update to use new pattern (low priority)

## Results

Successfully integrated `TestTerminalContext` with `Terminal.Instance` for automatic synchronization.

### Files Changed
- `source/timewarp-terminal/test-terminal-context.cs` - Added Terminal.Instance sync logic
- `source/timewarp-terminal/test-terminal.cs` - Dispose clears context if current
- `tests/test-terminal-context-01-integration.cs` - New integration tests

### Implementation Details

**TestTerminalContext.Current setter:**
- When set to non-null: saves previous `Terminal.Instance`, sets new instance
- When set to null: restores previous `Terminal.Instance`

**TestTerminal.Dispose:**
- Automatically clears `TestTerminalContext.Current` if this is the current terminal
- Triggers restoration of previous `Terminal.Instance`

**New property:**
- `TestTerminalContext.Terminal` - Direct access to current test terminal (throws if not set)

### New Test Pattern

```csharp
using TestTerminal terminal = new();
TestTerminalContext.Current = terminal;

// Terminal.Instance automatically set
Terminal.WriteLine("Hello"); // Routes to test terminal

// Automatic restoration on dispose
```

### Verification
- 6 new integration tests pass
- Existing tests continue to pass
- Build succeeds with no warnings

### Commits
- `feat: integrate TestTerminalContext with Terminal.Instance` (0ecece0)
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
# Update dev-cli to support self-install and bin prebuilt binary

## Description

Follow the Nuru repo pattern for dev-cli self-install and prebuilt binary support. Currently the dev-cli can only be invoked via `dotnet run --project tools/dev-cli` which is slow and cumbersome.

## Checklist

- [x] Fix pre-existing MissingMethodException with TimeWarp.Nuru dependency
- [x] Add `dev self-install` command to publish AOT binary to `bin/`
- [x] Add `bin/` to `.gitignore`
- [x] Set up PATH so `dev test` works from repo root (via .envrc)
- [x] Verify `dev test`, `dev build`, `dev workflow` all work from `bin/dev`

## Notes

### Implementation Details

- `dev self-install` command: `tools/dev-cli/endpoints/self-install.cs`
- AOT binary location: `bin/dev` (7.3 MB)
- Debug symbols: `bin/dev.dbg` (13 MB)
- PATH setup: `.envrc` with `export PATH="$PWD/bin:$PATH"`

### Verification

```bash
./bin/dev --help # Works
./bin/dev test # Works
./bin/dev build # Works
./bin/dev workflow # Works
```

## Results

All items completed. The dev-cli now supports:
- AOT compilation via `dev self-install`
- Fast execution from `bin/dev`
- PATH integration via `.envrc` (direnv)
- Proper gitignore for generated binaries
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
# Add CancelKeyPress event to ITerminal for Ctrl+C handling

**GitHub Issue:** https://github.com/TimeWarpEngineering/timewarp-terminal/issues/18

## Description

Add a `CancelKeyPress` event to `ITerminal` interface to allow graceful Ctrl+C handling without using `System.Console` directly. This enables testability and removes banned API usage in consumers like TimeWarp.Nuru's REPL.

## Problem

The REPL in TimeWarp.Nuru needs to handle Ctrl+C gracefully. Currently, it must use `Console.CancelKeyPress` directly, which violates the banned API rule and breaks testability.

```csharp
// Current code in repl-session.cs
Console.CancelKeyPress += OnCancelKeyPress;
Console.CancelKeyPress -= OnCancelKeyPress;
```

This triggers RS0030 warnings because `System.Console` is banned in favor of `ITerminal`.

## Checklist

- [x] Add `CancelKeyPress` event to `ITerminal` interface
- [x] Implement event in `TimeWarpConsole` (delegate to `Console.CancelKeyPress`)
- [x] Implement event in `TestConsole` (allow test simulation)
- [x] Add unit tests for event behavior
- [x] Update any relevant documentation
- [x] Verify TimeWarp.Nuru REPL can use the new event

## Notes

### Proposed Interface Change

```csharp
public interface ITerminal
{
// Existing members...

/// <summary>
/// Occurs when the Ctrl+C key combination is pressed.
/// </summary>
event ConsoleCancelEventHandler? CancelKeyPress;
}
```

### Use Case

```csharp
// In ReplSession
Terminal.CancelKeyPress += OnCancelKeyPress;

// Cleanup
Terminal.CancelKeyPress -= OnCancelKeyPress;
```

### Benefits

1. **Testability** - TestConsole can simulate Ctrl+C events
2. **Consistency** - All console interactions go through ITerminal
3. **Removes banned API usage** - REPL code no longer needs to use Console directly

### Related

- Discovered while fixing banned API warnings in timewarp-nuru
- Affects `ReplSession` class in timewarp-nuru

### Files to Modify

- `source/timewarp-terminal/ITerminal.cs` - Add event to interface
- `source/timewarp-terminal/TimeWarpConsole.cs` - Implement event
- `tests/timewarp-terminal.tests/` - Add tests for event behavior

## Results

Successfully added `CancelKeyPress` event to `ITerminal` interface for graceful Ctrl+C handling.

### Files Changed
- `source/timewarp-terminal/iterminal.cs` - Added `CancelKeyPress` event to interface
- `source/timewarp-terminal/timewarp-terminal.cs` - Implemented event (delegates to `Console.CancelKeyPress`)
- `source/timewarp-terminal/test-terminal.cs` - Implemented event with `SimulateCancelKeyPress()` method for testing
- `tests/cancel-key-press-01-basic.cs` - New test file

### Implementation Details

**ITerminal Interface:**
```csharp
event ConsoleCancelEventHandler? CancelKeyPress;
```

**TimeWarpTerminal:**
- Delegates directly to `Console.CancelKeyPress`

**TestTerminal:**
- Uses private handler field for test simulation
- `SimulateCancelKeyPress(ConsoleSpecialKey)` method uses reflection to create `ConsoleCancelEventArgs` (no public constructor)
- Supports testing Ctrl+C, Ctrl+Break, and Cancel property

### Verification
- All new tests pass
- Existing tests continue to pass
- Build succeeds with no warnings

### Commits
- `feat: add CancelKeyPress event to ITerminal for Ctrl+C handling` (f6323fd)

Closes #18
Loading