API Reviewer

API Reviewer Skill

This skill captures API design preferences for the Hex1b codebase. Use it when reviewing public APIs, evaluating whether code should be public or internal, or assessing API design decisions made by AI agents.

Codebase Architecture

Hex1b is structured into distinct layers, each with different API design considerations:

Layer 1: Hex1bTerminal (Core)

The foundational layer maintaining an in-memory representation of terminal state.

Design note: Hex1bTerminal emerged from the need to properly test TUI components. The adapter pattern enables use in real terminals, unit tests, or projecting state across networks.

Layer 2: Hex1bApp/TUI

The widget/node tree layer, inspired by React/Ink but not beholden to it.

Layer 3: Automation

Testing and automation infrastructure.

Design note: Automation APIs are explicitly for external consumers, not just internal testing.

Public vs Internal Decision Framework

When reviewing accessibility modifiers, apply this framework:

Make it PUBLIC when:

1. External consumers need it - Adapters, extension points, widget customization
2. It's part of the conceptual API - Users think in terms of this abstraction
3. It enables extensibility - Custom widgets, themes, adapters
4. It's stable - You're confident the design won't need breaking changes

Make it INTERNAL when:

1. It's implementation detail - Only Hex1b itself (or agents working on it) would invoke it
2. The abstraction isn't proven - Uncertain if this is the right level of abstraction
3. It could change - Design may evolve with more usage
4. It exposes internals - Would leak implementation details to consumers

Make it PRIVATE when:

1. Single class usage - Only used within one type
2. Helper methods - Implementation helpers with no external meaning

Review Questions

When reviewing agent-generated code, ask:

• Is this API an internal implementation detail?
• Does this method/property make sense on this type?
• Is the abstraction level right?
• Would making this public lock us into a design we're not confident about?
• Can this be internal for now until we're confident in the design?

API Design Patterns

Builders: Classic With...() Pattern

For Hex1bTerminalBuilder and similar construction scenarios:

// ✅ PREFERRED: Classic builder pattern
var terminal = Hex1bTerminal.CreateBuilder()
    .WithWorkload(workload)
    .WithPresentation(presentation)
    .WithDimensions(80, 24)
    .WithHeadless()
    .Build();

Widgets: SwiftUI-Inspired Minimal Constructors

For widget APIs, use minimal constructors with essential arguments only, then fluent extension methods:

// ✅ PREFERRED: Minimal constructor + fluent methods
ctx.Table(columns, rows)
   .SelectedRow(selectedIndex)
   .OnSelectionChanged(HandleSelection)

// ✅ PREFERRED: Cross-cutting concerns as extension methods
ctx.Progress(value, max).Fill()

// ❌ AVOID: Overloaded constructors with many parameters
new TableWidget(columns, rows, selectedIndex, onSelectionChanged, sortColumn, sortDirection, ...)

Guideline: Only include the most essential arguments in the constructor. Make usage "bleeding obvious." Use extension methods to splice in extra behavior.

Stateful Widgets: Lift-State-Up via IStatefulWidget<TSelf, TState>

When a widget owns non-trivial mutable user-facing state (a textbox's buffer + cursor, an editor's document, a navigator's selection, a checkbox's checked value), expose that state to callers so composites can drive the widget from outside.

The framework-wide contract is IStatefulWidget<TSelf, TState>:

public sealed record TextBoxWidget(...)
    : Hex1bWidget, IStatefulWidget<TextBoxWidget, TextBoxState>
{
    internal TextBoxState? InjectedState { get; init; }
    public TextBoxWidget State(TextBoxState state) => this with { InjectedState = state };
}

Callers then own the state via ctx.UseState(...) and bind it:

var state = ctx.UseState(() => new TextBoxState());
return ctx.TextBox().State(state);

When to apply:

• ✅ Mutable state that a parent might read or write (buffer text, selection, history).
• ✅ Multi-instance widgets that need independent state (two textboxes, two editors) — wrap in a single state class with explicit fields, no keyed UseState needed.
• ❌ Pure visual/transient state (focus, hover, animation phase) — keep that internal to the node.

Rules:

• The state type must be a class (where TState : class) so external mutations are observed.
• Reference equality matters — .State(s) must route the same instance into the node every reconcile.
• If a widget supports both a ctor-arg "initial value" and .State(...), conflicting use must throw InvalidOperationException on reconcile (no precedence rules to memorise).
• Method name is State(...) — the analyzer (HEX1B0001) forbids With* on widget extension methods.

See TextBoxWidget/TextBoxState, EditorWidget/EditorState, NavigatorWidget/NavigatorState, CheckboxWidget/CheckboxState for the canonical patterns.

Options Types for Complex Configuration

When you start creating many overloads, use an options type:

// ✅ PREFERRED: Options type for complex configuration
public class Hex1bAppOptions
{
    public Hex1bTheme? Theme { get; init; }
    public IHex1bTerminalWorkloadAdapter? WorkloadAdapter { get; init; }
    // ... extensible without breaking changes
}

// ❌ AVOID: Many overloads
public Hex1bApp(Func<...> builder) { }
public Hex1bApp(Func<...> builder, Hex1bTheme theme) { }
public Hex1bApp(Func<...> builder, Hex1bTheme theme, IWorkloadAdapter adapter) { }

But: Don't have options classes everywhere. Be conservative with what you require.

Async Patterns

Prefer Task, Use ValueTask for Performance

// ✅ PREFERRED: Task as default
public Task<Hex1bWidget> BuildAsync(WidgetContext ctx);

// ✅ Use ValueTask when there's a performance reason (hot paths, avoiding allocations)
public ValueTask HandleInputAsync(Hex1bKeyEvent key);

Sync + Async Overloads for Callbacks

Provide both sync and async overloads for event handlers. Implement everything assuming async is possible, then wrap sync handlers:

// ✅ PREFERRED: Both overloads, sync wraps to async
public ButtonWidget OnClick(Action<ButtonClickedEventArgs> handler)
    => this with { ClickHandler = args => { handler(args); return Task.CompletedTask; } };

public ButtonWidget OnClick(Func<ButtonClickedEventArgs, Task> handler)
    => this with { ClickHandler = handler };

Note: Samples often favor sync versions for simplicity.

Naming and Nullability

Types Over Magic Strings

// ✅ PREFERRED: Strongly typed
public void SetColor(Hex1bColor color);

// ❌ AVOID: Magic strings
public void SetColor(string colorName);

Avoid Forcing Null Arguments

// ❌ AVOID: Forcing callers to pass null
public void Configure(string? requiredArg, string? optionalArg);
Configure("value", null);  // Awkward

// ✅ PREFERRED: Overloads or optional parameters
public void Configure(string requiredArg);
public void Configure(string requiredArg, string optionalArg);

Avoid Primitive Obsession

Be wary of using too many primitive types, as it makes creating overloads harder in the future:

// Consider whether a type is warranted
public void SetPosition(int x, int y);        // OK for simple cases
public void SetPosition(Point position);       // Better if Point is meaningful elsewhere