Skip to content

fix: Running CLI editors does not work when piped to rainbow, unlike when piped to the Ruby lolcat #7

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
webbrain-one wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: Running CLI editors does not work when piped to rainbow, unlike when piped to the Ruby lolcat #7

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
21 changes: 19 additions & 2 deletions rainbow/rainbow.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ import (

var (
// We remove all previous paintings to create a new rainbow.
colorMatch = regexp.MustCompile("^\033" + `\[\d+(;\d+)?(;\d+)?[mK]`)
colorMatch = regexp.MustCompile(`\033\[[0-9;]*[mK]`)

@arsham arsham Aug 11, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you document and test the intended behaviour here please? This changes the previous anchored match into a global removal, so all matching SGR and erase-line sequences are removed before the new parser has a chance to preserve them. I am not sure whether we want to remove every existing m/K sequence or only colours previously produced by rainbow. A test covering an embedded \x1b[K would help make that contract clear.


// ErrNilWriter is returned when Light.Writer is nil.
ErrNilWriter = errors.New("nil writer")
Expand Down Expand Up @@ -87,7 +87,24 @@ func (l *Light) Write(data []byte) (int, error) {
)

data = colorMatch.ReplaceAll(data, []byte(""))
for _, c := range string(data) {
for i, c := range string(data) {

@arsham arsham Aug 11, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you feel about using an explicitly controlled byte index here instead of range? After writing seq[:j], continue only advances the range loop by one rune, so the bytes inside the sequence will be visited and colourised again. For example, with \x1b[2Jx, we first write \x1b[2J unchanged and then process [2J again as visible text. An index that advances by the full sequence length would avoid duplicating the consumed bytes.

if c == '\033' {

@arsham arsham Aug 11, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you feel about accounting for escape sequences split across calls to Write? io.Copy is allowed to split the input at arbitrary byte boundaries, so one call could end with \x1b[?25 and the next begin with l. Since the parser is currently stateless per call, that sequence can't be reconstructed. I think we need either a small pending-sequence buffer or another streaming approach, together with a split-write regression test.

seq := data[i:]
j := 1
if j < len(seq) && seq[j] == '[' {
for j++; j < len(seq); j++ {
if (seq[j] >= 'A' && seq[j] <= 'Z') || (seq[j] >= 'a' && seq[j] <= 'z') {

@arsham arsham Aug 11, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you reckon we could check the complete CSI final-byte range (0x40 through 0x7e) here? Valid CSI sequences do not always end in a letter. For example, bracketed-paste sequences end in ~, so with the current condition something like \x1b[200~text may consume text as part of the escape sequence. It would be good to add a regression test for a ~-terminated sequence as well.

j++
break
}
}
} else if j < len(seq) && (seq[j] == ']' || seq[j] == 'P' || seq[j] == 'X' || seq[j] == '^') {
for j++; j < len(seq) && seq[j] != 0x1b && seq[j] != 0x07; j++ {

@arsham arsham Aug 11, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend consuming the complete terminator here. At the moment seq[:j] excludes BEL, and for an ST terminator it stops before the ESC in ESC \. This means the terminator is processed separately and the backslash may be colourised as visible text. Could we preserve both BEL and the complete ESC \ terminator, with tests for both forms?

}
}
buf.Write(seq[:j])
continue
}
switch c {
case '\n':
offset = 0
Expand Down