Conversation
gtsiolis
requested review from
anisaoshafi,
carole-lavillonniere and
silv-io
as code owners
|
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:
📝 WalkthroughWalkthroughReplaces textual "Success:" prefixes with a centralized success marker and color. Adds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
internal/output/plain_format.go (1)
45-47: Consider keepingFormatEventLineANSI-free for plain output.Using
SuccessMarkerText()here (and applying color in TTY/UI renderers) avoids escape-sequence noise in redirected logs and script-consumed output.♻️ Possible adjustment
- return fmt.Sprintf("%s LocalStack ready (%s)", SuccessMarker(), e.Detail), true + return fmt.Sprintf("%s LocalStack ready (%s)", SuccessMarkerText(), e.Detail), true ... - return SuccessMarker() + " LocalStack ready", true + return SuccessMarkerText() + " LocalStack ready", true ... - return SuccessMarker() + " " + e.Text + return SuccessMarkerText() + " " + e.TextAlso applies to: 115-115
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/output/plain_format.go` around lines 45 - 47, FormatEventLine is emitting ANSI-colored markers via SuccessMarker(), which pollutes non-TTY/plain outputs; change those calls to SuccessMarkerText() so the function returns ANSI-free marker text (replace SuccessMarker() with SuccessMarkerText() in FormatEventLine and the other occurrence around the second branch), and ensure any TTY/UI renderers apply coloring themselves rather than relying on FormatEventLine to produce colored output.internal/output/plain_sink_test.go (1)
62-62: Consider avoiding raw ANSI literals in expected values.Building these expectations from
SuccessMarker()will reduce drift risk if marker styling changes.♻️ Suggested assertion cleanup
- expected: "\x1b[38;2;183;201;92m✔︎\x1b[0m LocalStack ready (abc123)\n", + expected: SuccessMarker() + " LocalStack ready (abc123)\n", ... - expected: "\x1b[38;2;183;201;92m✔︎\x1b[0m LocalStack ready\n", + expected: SuccessMarker() + " LocalStack ready\n",Also applies to: 67-67
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/output/plain_sink_test.go` at line 62, The test uses hard-coded ANSI escape sequences for the success marker; replace the raw literal (e.g. "\x1b[38;2;183;201;92m✔︎\x1b[0m") with a call to SuccessMarker() when building the expected string (for example: fmt.Sprintf("%s LocalStack ready (abc123)\n", SuccessMarker())) so the test follows marker styling changes; update the other occurrence on the same test (line ~67) similarly.internal/ui/app_test.go (1)
177-177: Prefer shared marker helper in assertion.Using
output.SuccessMarkerText()here avoids brittle literal coupling if the glyph changes later.♻️ Suggested test tweak
- if !strings.Contains(app.lines[0].text, "✔︎") || !strings.Contains(app.lines[0].text, "Done") { + if !strings.Contains(app.lines[0].text, output.SuccessMarkerText()) || !strings.Contains(app.lines[0].text, "Done") { t.Fatalf("expected rendered success message, got: %q", app.lines[0].text) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ui/app_test.go` at line 177, Replace the hard-coded success glyph in the test assertion with the shared helper to avoid brittle coupling: instead of checking for the literal "✔︎" use output.SuccessMarkerText() in the assertion that inspects app.lines[0].text (the same check that also looks for "Done"); update the condition to assert that app.lines[0].text contains output.SuccessMarkerText() and "Done".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/output/plain_format.go`:
- Around line 45-47: FormatEventLine is emitting ANSI-colored markers via
SuccessMarker(), which pollutes non-TTY/plain outputs; change those calls to
SuccessMarkerText() so the function returns ANSI-free marker text (replace
SuccessMarker() with SuccessMarkerText() in FormatEventLine and the other
occurrence around the second branch), and ensure any TTY/UI renderers apply
coloring themselves rather than relying on FormatEventLine to produce colored
output.
In `@internal/output/plain_sink_test.go`:
- Line 62: The test uses hard-coded ANSI escape sequences for the success
marker; replace the raw literal (e.g. "\x1b[38;2;183;201;92m✔︎\x1b[0m") with a
call to SuccessMarker() when building the expected string (for example:
fmt.Sprintf("%s LocalStack ready (abc123)\n", SuccessMarker())) so the test
follows marker styling changes; update the other occurrence on the same test
(line ~67) similarly.
In `@internal/ui/app_test.go`:
- Line 177: Replace the hard-coded success glyph in the test assertion with the
shared helper to avoid brittle coupling: instead of checking for the literal
"✔︎" use output.SuccessMarkerText() in the assertion that inspects
app.lines[0].text (the same check that also looks for "Done"); update the
condition to assert that app.lines[0].text contains output.SuccessMarkerText()
and "Done".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 2219f247-e368-4826-bf7b-d9b2288e81ce
📒 Files selected for processing (7)
internal/output/plain_format.gointernal/output/plain_format_test.gointernal/output/plain_sink_test.gointernal/output/style.gointernal/ui/app_test.gointernal/ui/components/message.gointernal/ui/styles/styles.go
gtsiolis
force-pushed
the
george/des-169
branch
from
fc18360 to
11eab6b
Compare
internal/output/style.go
Outdated
| func SuccessMarker() string { | ||
| return fmt.Sprintf("\x1b[38;2;183;201;92m%s\x1b[0m", SuccessMarkerText()) | ||
| } |
There was a problem hiding this comment.
The problem with having color in the non-TUI output as well is that it won't respect the NO_COLOR environment variable that bubbletea handles.
In general, I don't see the reason for color output in non-TUI mode. It will just make the output unreadable if the target is unable to render those colors.
This is very likely in non-TTY outputs!
There was a problem hiding this comment.
Good point. I didn't intent to add colors in non-TUI output. Made some changes in 5e1991c.
There was a problem hiding this comment.
Could you take another look when you're free, @silv-io?
gtsiolis
force-pushed
the
george/des-169
branch
from
11eab6b to
5e1991c
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/output/plain_format.go (1)
116-153: Use shared marker informatInstanceInfotoo to avoid glyph drift.This file now centralizes success markers in updated paths, but
formatInstanceInfostill hardcodes"✓ ". ReusingSuccessMarkerText()there would keep one canonical source.Proposed refactor
func formatInstanceInfo(e InstanceInfoEvent) string { var sb strings.Builder - sb.WriteString("✓ " + e.EmulatorName + " is running (" + e.Host + ")") + sb.WriteString(SuccessMarkerText() + " " + e.EmulatorName + " is running (" + e.Host + ")") var meta []string if e.Uptime > 0 { meta = append(meta, "UPTIME: "+formatUptime(e.Uptime))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/output/plain_format.go` around lines 116 - 153, formatInstanceInfo currently hardcodes the success glyph ("✓ ") causing marker drift; update the function (formatInstanceInfo) to use the canonical SuccessMarkerText() instead of the literal "✓ " and preserve the existing spacing and string composition around e.EmulatorName and e.Host so the output remains "SuccessMarkerText() + " " + e.EmulatorName + " is running (" + e.Host + ")".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/output/plain_format.go`:
- Around line 116-153: formatInstanceInfo currently hardcodes the success glyph
("✓ ") causing marker drift; update the function (formatInstanceInfo) to use the
canonical SuccessMarkerText() instead of the literal "✓ " and preserve the
existing spacing and string composition around e.EmulatorName and e.Host so the
output remains "SuccessMarkerText() + " " + e.EmulatorName + " is running (" +
e.Host + ")".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 15043f25-2f66-47ea-8885-a32b629c50e1
📒 Files selected for processing (8)
internal/output/plain_format.gointernal/output/plain_format_test.gointernal/output/plain_sink_test.gointernal/output/style.gointernal/ui/app.gointernal/ui/app_test.gointernal/ui/components/message.gointernal/ui/styles/styles.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/output/plain_sink_test.go
- internal/ui/app_test.go
- internal/ui/styles/styles.go
- internal/output/plain_format_test.go
- internal/ui/components/message.go
gtsiolis
force-pushed
the
george/des-169
branch
from
5e1991c to
37f1d1b
Compare
|
Rebased to keep fresh. 🥗 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/output/plain_format.go (1)
152-154: Inconsistent checkmark character informatInstanceInfo.This function uses a hardcoded
"✓ "(U+2713), while the rest of the PR usesSuccessMarkerText()which returns"✔︎"(U+2714 + variation selector). Consider using the shared marker for visual consistency across all success indicators.♻️ Suggested fix
func formatInstanceInfo(e InstanceInfoEvent) string { var sb strings.Builder - sb.WriteString("✓ " + e.EmulatorName + " is running (" + e.Host + ")") + sb.WriteString(SuccessMarkerText() + " " + e.EmulatorName + " is running (" + e.Host + ")")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/output/plain_format.go` around lines 152 - 154, Replace the hardcoded checkmark in formatInstanceInfo with the shared SuccessMarkerText() so the success marker is consistent across the codebase; in the formatInstanceInfo function (which builds the string for InstanceInfoEvent) use SuccessMarkerText() as the prefix instead of "✓ " when writing to the strings.Builder so the output matches other success indicators.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/output/plain_format.go`:
- Around line 152-154: Replace the hardcoded checkmark in formatInstanceInfo
with the shared SuccessMarkerText() so the success marker is consistent across
the codebase; in the formatInstanceInfo function (which builds the string for
InstanceInfoEvent) use SuccessMarkerText() as the prefix instead of "✓ " when
writing to the strings.Builder so the output matches other success indicators.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 55be1436-504e-4662-9be6-c49872e1521f
📒 Files selected for processing (8)
internal/output/plain_format.gointernal/output/plain_format_test.gointernal/output/plain_sink_test.gointernal/output/style.gointernal/ui/app.gointernal/ui/app_test.gointernal/ui/components/message.gointernal/ui/styles/styles.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/output/plain_sink_test.go
- internal/ui/app.go
- internal/output/style.go
gtsiolis
force-pushed
the
george/des-169
branch
2 times, most recently
from
fb804cf to
82363e5
Compare
|
Rebased to keep fresh. 🥗 |
silv-io
left a comment
silv-io
left a comment
There was a problem hiding this comment.
LGTM, just some clean up we'll need I think.
All the color stuff should go to ui styles. Then the output "styles" can become the symbols file instead.
gtsiolis
force-pushed
the
george/des-169
branch
from
82363e5 to
8a425d9
Compare
gtsiolis
force-pushed
the
george/des-169
branch
from
8a425d9 to
3930d13
Compare
|
Rebased to keep fresh. 🥗 |
|
Absolutely, @silv-io! Leaving this to you. 🏀 |
|
Adapted it now, will merge if all tests pass ✔️ |
|
Woohoo! |
2 participants
This will changes success messages rendering from text to a shared checkmark marker. ✔️
This makes success output more compact and visually consistent across renderers. 💄