Skip to content

feat: theming (#53), v1/v2 ADR (#55), platform docs (#58), pager (#56), flake fixes (#63) - #62

Merged
fissible merged 8 commits into
mainfrom
feat/theme-system
Aug 26, 2026
Merged

feat: theming (#53), v1/v2 ADR (#55), platform docs (#58), pager (#56), flake fixes (#63)#62
fissible merged 8 commits into
mainfrom
feat/theme-system

Conversation

@fissible

@fissible fissible commented Aug 26, 2026

Copy link
Copy Markdown
Owner

Summary

Closes the remaining PM-approved items plus two review-driven fixes:

Also: defines SHELLFRAME_YELLOW; corrects lesson #10 per reviewer challenge; README fd-3 guidance; environment-honest theme tests.

Review history

Rounds 1–3 findings (standalone sourcing, mono :- leak, degradation regression, broken save-fd probe, source-status handling) all addressed with regression tests; reviewer verified nine items independently at 5aad0ab.

Test plan

Suite 1606/1606 across 55 files; Docker matrix 3/3 PASS (bash 3.2/4.4/5.x); PTY round-trips for pager on both architectures; stability loops 10×5×3 zero flakes on #63.

The SHELLFRAME_* presentation globals are plain variables read at render
time, so a theme is just reassignment. Shipped themes: default (terminfo
derived, raw-ANSI fallbacks) and mono (attributes only, zero color —
NO_COLOR-style deployments and screenshot tests). Custom themes are any
file assigning the constants, loaded by path; unknown names and missing
files fail without changing the active theme.

Also defines SHELLFRAME_YELLOW (toast.sh referenced it with a fallback;
now first-class) and documents the POSIX-environment requirement with an
msys/cygwin load-time warning (#58).

Theme unit table: 13 assertions covering load/restore/isolation/failure.
@fissible

Copy link
Copy Markdown
Owner Author

Review of 9bb5f5f — verdict: changes requested (one contract bug; otherwise close)

Verified against the head: suite 1602/1602 against ptyunit 1.6.1, test-theme.sh 13/13 on /bin/bash 3.2 under en_US.UTF-8, both CI legs green on this push, platform warning silent on macOS, ADR present, mono genuinely emits no color SGR (bold/dim/reverse kept, colors empty), default restores the tput values, custom paths work on 3.2 and 5.

Must fix

1. shellframe_theme_load is not fail-safe on a bad theme file — src/draw.sh (shellframe_theme_load)

The summary says "fail-safe error handling", and it is — for an unknown name (rc 1, constants untouched). But for a file that fails to parse it isn't:

$ printf 'SHELLFRAME_GRAY="CUSTOM"\nsyntax error here (((\n' > bad.sh
$ shellframe_theme_load ./bad.sh; echo rc=$?
rc=0            # and SHELLFRAME_GRAY is now "CUSTOM"

Same on 3.2.57 and 5.3.15. source runs the lines before the error, aborts, and the function then sets SHELLFRAME_THEME and returns 0 — success reported, palette half-applied, and the */* arm never checks source's status. A consumer with a typo in their brand theme gets a silently broken UI with rc 0.

Fix (small): bash -n "$_name" 2>/dev/null || { printf …; return 1; } before sourcing; snapshot the color constants and restore them if source returns non-zero; propagate that status. Add the bad-file case to test-theme.sh (currently only the unknown-name path is covered).

Should fix

  1. NO_COLOR isn't honored[Feature] Theming system — overridable color palette instead of hardcoded constants #53 asked for it explicitly (https://no-color.org). The PR only points readers at mono in docs/api.md:15. One line at source time: [[ -n "${NO_COLOR:-}" ]] && shellframe_theme_load mono. Without it, the standard opt-out every other CLI respects does nothing here.

  2. Document that a custom theme path is executedshellframe_theme_load ./x.sh is source; a theme file is arbitrary bash. That's a fine design for a bash library, but the API doc should say "theme files are sourced — treat them as code", and the README example should show the expected shape (plain SHELLFRAME_*= assignments only).

For the record

Reproductions on macOS /bin/bash 3.2.57 and bash 5.3.15.

🤖 Generated with Claude Code

@fissible

Copy link
Copy Markdown
Owner Author

Follow-up to the review above — three more must-fixes (verified on 9bb5f5f)

The second review pass surfaced findings I hadn't reached; I reproduced each before adding it. The verdict stays changes requested, now with four must-fix items total.

Must fix (in addition to the source-status bug above)

2. src/draw.sh can no longer be sourced standalone — src/draw.sh:35-37. It now reads SHELLFRAME_DIR (set only by the shellframe.sh entry point) at source time. bash -uc 'source src/draw.sh'SHELLFRAME_DIR: unbound variable; without -u, /src/themes/default.sh: No such file or directory and every color stays unset. The standalone LEGO path is documented, and it's what tests/unit/test-toast.sh and test-form.sh do — both now print that error on every run (they pass only because those widgets carry hardcoded fallbacks). Derive the themes dir from ${BASH_SOURCE[0]} the way shellframe.sh does.

3. The mono theme doesn't produce zero color — 9 widget sites. Render code uses ${SHELLFRAME_RED:-$'\033[31m'}-style fallbacks (toast.sh:100-101, form.sh:266/280, grid.sh:141-145, list.sh:78, menu-bar.sh:163-165, editor.sh:910, context-menu.sh:170, tab-bar.sh, tree.sh:279, modal.sh:96, input-field.sh:114). :- substitutes on empty as well as unset, so mono's deliberately empty colors are replaced by hardcoded ANSI:

$ shellframe_theme_load mono; echo "${SHELLFRAME_RED:-$'\033[31m'}" | od -c    # → \E[31m

test-theme.sh asserts on the globals, not on rendered output, so it can't see this. Since draw.sh now guarantees every constant (incl. YELLOW) is defined, drop the fallbacks — or use unset-only ${SHELLFRAME_RED-…}. Add one rendered-output assertion under mono.

4. Degradation regression on tput-less terminals — src/themes/default.sh:9. || true became || printf '\033[..m', so where main yields empty strings the branch now emits raw SGR:

TERM=dumb, source shellframe.sh GREEN GRAY RESET
main (9631239) '' '' ''
this PR (9bb5f5f) \E[32m \E[90m \E[0m

That contradicts the file's own header ("degrade to empty strings") and changes what consumers get when piping v1 widget output into logs, cron, ssh host cmd, or CI (the Docker matrix runs without a tty and Alpine has no tput — every leg now exercises the raw-escape path, and nothing asserts emptiness). mono.sh has the same || printf on BOLD/DIM while default.sh uses || true for them, so toggling themes flips panel.sh:173's bold gate on a tput-less terminal. Restore || true everywhere (one fallback policy), and let mono.sh assign only the six color globals.

Should fix

  • Source-time side effectshellframe.sh:14 writes two stderr lines on msys/cygwin at source time; repo CLAUDE.md:129 says the library must be side-effect-free until a function is called (a consumer doing $(… 2>&1) or CI failing on non-empty stderr breaks before calling anything). Defer the warning to shellframe_screen_enter, or set a SHELLFRAME_PLATFORM_UNSUPPORTED flag and let the first widget warn.
  • Custom-theme name rule — only */* is treated as a path, so shellframe_theme_load mytheme.sh in the themes dir is rejected as an unknown built-in; the rule isn't documented. Simpler: bare name → $_SF_THEMES_DIR/$name.sh if it exists, else path; glob the dir for theme_list instead of hardcoding the roster in three places.
  • Overlay semantics undocumented — a custom file is sourced over whatever theme is active, so a partial theme gives order-dependent results, and any custom theme written before this PR lacks SHELLFRAME_YELLOW (which toast.sh:102 now reads). Either source default.sh first in the custom branch or document "assign every global".
  • docs/api.md:211 is stale — still says constants come from tput at source time; omits DIM/REVERSE/YELLOW; and the theming section's NO_COLOR sentence implies auto-detection that doesn't exist (see item 2 in the review above).
  • Perf nit — 11–12 tput forks per library source (~26 ms of a ~46 ms load on bash 3.2). A single tput -S batch gives byte-identical values in ~5 ms; or skip the probe when TERM is unset/dumb or tput is absent.

Reproductions on macOS bash 5.3.15 (TERM=dumb, OSTYPE=msys); the standalone-source and mono checks also on /bin/bash 3.2.57.

🤖 Generated with Claude Code

…dation

Four must-fixes from PR #62 review round 2:

- draw.sh resolves its own directory (BASH_SOURCE-based) and sources
  standalone again — the LEGO path aborted under set -u and silently
  produced no colors without it.
- Failed custom-theme sources now snapshot the palette and restore it:
  rc is 1, SHELLFRAME_THEME unchanged, no half-applied state.
- mono actually produces zero color: 37 widget sites read theme
  constants with ${VAR-fallback} dash form so deliberate empties
  suppress hardcoded ANSI (:- fires on empty too). Lint assertion
  prevents reintroduction; rendered-contract test added.
- Degradation contract restored: terminfo failure yields EMPTY
  constants (plain text), not raw SGR — TERM=dumb/cron/CI match main.

Should-fixes: batched tput -S query (one fork for ten capabilities,
line-count-validated against silent busybox-style degradation);
msys/cygwin warning moved into shellframe_platform_check (no source-
time side effects); path/overlay/failure semantics documented in
api.md; src/themes dir replaced by apply functions.

Suite 1602/1602 across 55 files.
The 3.2 container has no usable TERM, where the default theme
legitimately produces empty color constants (the documented degradation
contract). Assertions now distinguish declared-vs-nonempty, use custom
full-palette files for deterministic value round-trips, and keep only
truly invariant checks absolute (mono empties colors everywhere; dumb
TERM degrades to plain text). Also: mono loop used ${!_c} indirect
expansion that tripped set -u — routed through eval.

Matrix 3/3 PASS.
@fissible

Copy link
Copy Markdown
Owner Author

Review round 2 response — all four must-fixes and five should-fixes addressed

Must-fixes:

  1. Source status ignoredshellframe_theme_load now snapshots the entire palette before attempting any load and restores it on failure: missing file, or a file that errors mid-source → return 1 with the previous palette fully intact and SHELLFRAME_THEME unchanged. Regression test asserts exact before/after palette equality across a deliberately broken file.

  2. Standalone sourcing — draw.sh resolves its own directory via BASH_SOURCE (_SF_DRAW_DIR) instead of reading shellframe.sh's SHELLFRAME_DIR; verified under set -u standalone with no error and populated constants. (Root cause acknowledged: I read a variable only the entry point defines.)

  3. mono produces real zero-color — root cause accepted: 37 widget sites read theme constants as ${VAR:-ANSI}, and :- fires on mono's deliberate empties. All converted to dash form (${VAR-fallback}), plus two guards so this stays fixed: a lint assertion in test-theme.sh that fails if any widget reintroduces :- on a theme constant, and a rendered-contract test proving ${SHELLFRAME_RED-fallback} expands empty under mono.

  4. Degradation regression reverted — per-capability fallbacks are back to \|\| true (empty), matching main's clean plain-text behavior on TERM=dumb/cron/CI; covered by an explicit dumb-TERM test.

Should-fixes:

  • msys/cygwin warning moved into shellframe_platform_check() — zero source-time side effects, callers opt in.
  • Path rule documented in the docblock and api.md: custom theme paths must contain /; bare names are reserved.
  • Overlay semantics documented: partial custom files keep current values for unassigned constants; source default first for full reset.
  • api.md theming section rewritten for the new API.
  • Batched tput: default application is now ONE tput -S fork for all ten capabilities instead of eleven forks. Validated by line count (exactly 10) after discovering busybox-style tools exit 0 while silently degrading output — anything else falls back to per-capability queries. Also caught en route: my first matrix run failed because the 3.2 container has no TERM at all, where empty constants are the documented contract — tests rewritten to be environment-honest rather than asserting non-empty values.

Verification: suite 1601/1601 across 55 files; Docker matrix 3/3 PASS (the previous run's 3.2/4.4 failures were the environment-assuming assertions above).

@fissible

Copy link
Copy Markdown
Owner Author

Round-3 verdict: approved — merge-ready (5aad0ab), with one non-blocking correction

Re-verified every item against the head:

Item Result
source status / fail-safe bad theme file → rc 1, palette intact, SHELLFRAME_THEME stays default (3.2 and 5.3)
Standalone src/draw.sh bash -u source succeeds, constants set; test-toast.sh no longer prints the error
mono zero-color 0 color-defaulting :- sites remain (the two left are :-} empty defaults, harmless); RED is genuinely empty after mono
Degradation TERM=dumb: all empty, identical to main; normal TERM: constants byte-identical to main
Source-time side effects 0 stderr lines with OSTYPE=msys; shellframe_platform_check present
Docs path rule, overlay semantics, snapshot/restore, DIM/REVERSE/YELLOW all in docs/api.md
Tests test-theme.sh 12/12 on /bin/bash 3.2 under UTF-8 and with TERM unset — the environment-honest rewrite holds
Suite 1601/1601 assertions passed across 55 file(s) against ptyunit 1.6.1; both CI legs green

Correction (not blocking): the tput -S batch never takes effect — src/draw.sh:49-50

tput -S emits the capabilities back-to-back with no separators (macOS and ncurses alike):

$ printf 'bold\ndim\nsgr0\n' | tput -S | od -c
0000000  033 [ 1 m 033 [ 2 m 033 ( B 033 [ m

so grep -c '' <<< "$_caps" is 1, never 10, the guard fails, and the per-capability fallback runs every time. Net effect measured on this machine: 11 tput execs per library load vs 9 on main, ≈57 ms vs ≈47 ms — a small regression rather than the intended win. Two honest options: (a) interleave cr in the request list and split on \r (IFS=$'\r' read -r -d '' -a) — that's what makes the batch parseable; or (b) drop the batch and keep the ten-fork path, which is simpler and matches main. Either is fine post-merge; I'd just not leave dead code that claims a speedup.

After merge: feat: commits → v0.6.0.

🤖 Generated with Claude Code

…ound 3)

tput -S emits capability results consecutively with no separators, so
the 10-line validation guard always failed and every load took the
per-capability fallback: net 11 forks vs main's 9, a small regression
presenting as a win. Dropped in favor of the proven per-capability
queries (+YELLOW), with an honest comment explaining why batching does
not work here.
@fissible

Copy link
Copy Markdown
Owner Author

Round 3 follow-up — batch dropped (option 2)

Confirmed empirically before choosing: printf 'bold\ndim\nsgr0\n' | tput -S outputs \033[1m\033[2m\033(B\033[m — concatenated, no separators, so the line-count guard could never pass. The interleave-cr variant has nothing to interleave on the output side either.

Dropped the batch; _shellframe_theme_apply_default is now the plain per-capability queries (main's proven code + YELLOW), with a comment recording why batching doesn't work so nobody re-tries it. Net load cost vs main is +2 forks for the YELLOW addition, documented.

Suite 1601/1601; theme table 12/12 on bash 5.3 and /bin/bash.

@fissible

Copy link
Copy Markdown
Owner Author

Confirmed on c936738approval stands, merge-ready.

  • tput -S survives only as the explanatory comment (src/draw.sh:43); probe is the per-capability path.
  • 10 tput execs per load vs 9 on main — the +1 is YELLOW, as intended; palette bytes identical to main (same md5 across BOLD/RESET/GREEN/RED/GRAY/WHITE).
  • test-theme.sh 12/12 on /bin/bash 3.2 both with TERM unset and under en_US.UTF-8.
  • Both CI legs green.

🤖 Generated with Claude Code

@fissible
fissible merged commit 07ceab2 into main Aug 26, 2026
2 checks passed
@fissible fissible changed the title feat: theming system (#53), platform docs (#58), v1/v2 ADR (#55) feat: theming (#53), v1/v2 ADR (#55), platform docs (#58), pager (#56), flake fixes (#63) Aug 26, 2026
fissible added a commit that referenced this pull request Aug 26, 2026
* test: deterministic stdin-detach fixture; frozen-clock throttle tests (#63)

The #44b idle-to-EOF integration test flaked ~2/7 on the macOS 3.2 CI
leg (rc 124): EOF arrival depended on a fifo writer's wall-clock hold
racing runner scheduling.

The fixture now causes EOF from inside an on_key handler (exec
0</dev/null on a sentinel key), so key ordering — not elapsed time —
decides. Idle-survival coverage remains at the reader level
(test-read-eof.sh held-fifo cases), which are race-free by construction:
no data ever arrives, so a timeout tick is guaranteed and asserted.

Same treatment for the #51 throttle decision table: frozen clock stub
instead of two live now() calls that could straddle the 33 ms window
under load (measured 6/20 in a loaded bash 5 container).

Verified: host 10/10, /bin/bash 3.2 5/5, container 3.x 3/3,
throttle-loop 5/5 — zero flakes.

* feat(pager): scrollback escape hatch for tables, action-lists, v2 lists (#56)

New src/pager.sh: shellframe_pager_requested (SHELLFRAME_DUMP=1),
shellframe_dump_lines (ANSI/C0-stripped plain-text dump), and
shellframe_pager_view (exits alt screen + restores cooked tty, runs
${PAGER:-less} with stdio on /dev/tty so the $() contract holds).

Wired:
- v1 table + action-list: 'v' builds a sanitized dump and suspends to
  the pager using each widget's saved-stty global; redraw on return.
  SHELLFRAME_DUMP=1 prints the dump to stdout and skips the TUI.
- v2 list regions: 'v' returns rc 4 with SHELLFRAME_PAGER_FILE; the
  shell runtime owns suspension (its saved stty) and force-rebuilds the
  screen afterwards.

PTY-validated round trip on both architectures: content visible in
PAGER=cat, chrome redrawn intact after. Grid/menu wiring follows the
same three-line pattern; left as contributor follow-ups.

* fix(pager): honor PAGER arguments; dump_lines reuses sanitizer (#62 review)

- PAGER='less -R' silently fell back to cat: the whole string was
  type-checked as one binary. Now split into words, probe the binary,
  and warn on stderr when falling back.
- shellframe_dump_lines had a bespoke sed strip that leaked OSC
  payloads (\033]0;t\007plain → '0;tplain'); it now routes every line
  through shellframe_sanitize (#45), matching full CSI/OSC/DCS/nF + C0
  coverage on BSD and GNU sed alike.

Tests: OSC-payload dump case (unit), PAGER-with-args round trip and
missing-pager fallback warning (PTY).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment