Conversation
gtsiolis
requested review from
anisaoshafi,
carole-lavillonniere and
silv-io
as code owners
📝 WalkthroughWalkthroughThe changes reorganize the update prompt config from root level to a new Changes
Sequence DiagramsequenceDiagram
participant User
participant App as App.Update
participant InputPrompt as InputPrompt
participant UI as UI Renderer
User->>App: Send tea.KeyDown
App->>InputPrompt: handleVerticalPromptKey() detects KeyDown
InputPrompt->>InputPrompt: SetSelectedIndex(current + 1)
InputPrompt->>UI: View() renders with filled marker at new index
UI-->>User: Display updated vertical prompt
User->>App: Send tea.KeyEnter
App->>InputPrompt: handleVerticalPromptKey() detects KeyEnter
InputPrompt->>InputPrompt: Get option at selectedIndex
InputPrompt->>App: Send InputResponse with SelectedKey
App->>InputPrompt: Clear and hide prompt
UI-->>User: Prompt disappears, selection confirmed
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 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 docstrings
🧪 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.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/update/notify_test.go (1)
129-142:⚠️ Potential issue | 🟠 MajorExercise the actual skip branch and isolate config writes.
Using
"n"here hitsconfig.DisableUpdatePrompt(), not the per-version skip path the test name describes. Because the test doesn't redirect config home/path first, that branch can also mutate the runner's reallstkconfig. Please set up a temp config likeTestNotifyUpdateSkippedVersion, send"s", and assert thatupdate_skipped_versionwas persisted.Based on learnings, "Viper uses TOML format and searches in order: ./.lstk/config.toml, $HOME/.config/lstk/config.toml, then OS default. When no config exists, create at $HOME/.config/lstk/config.toml..."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/update/notify_test.go` around lines 129 - 142, Rename the test to TestNotifyUpdateSkippedVersion, create a temporary config directory and point the app's config loader there (so writes don't hit the real ~/.config/lstk), use output.SinkFunc to send the "s" response (not "n") to trigger the per-version skip branch in notifyUpdateWithVersion with NotifyOptions{UpdatePrompt: true} and the existing test fetcher/server, then assert the config key "update_skipped_version" was written and equals the current version (read back via the same config loader or Viper instance) and finally restore any env/config state you changed.
🧹 Nitpick comments (3)
internal/ui/components/message.go (1)
17-32: Consider whether the localtextvariable is necessary.The introduction of
text := e.Textand its usage throughout doesn't provide a clear benefit sincee.Textis already a simple, efficient field access. The original code was equally readable and avoided the extra variable. Unless this is preparing for future text transformations (not evident in the current change), reverting to directe.Textusage would keep the code simpler.♻️ Optional simplification
func RenderWrappedMessage(e output.MessageEvent, width int) string { prefixText, prefix := messagePrefix(e) - text := e.Text - if prefixText == "" { style := styles.Message if e.Severity == output.SeveritySecondary { style = styles.SecondaryMessage } - return style.Render(strings.Join(wrap.SoftWrap(text, width), "\n")) + return style.Render(strings.Join(wrap.SoftWrap(e.Text, width), "\n")) } if width <= len([]rune(prefixText))+1 { - return prefix + " " + styles.Message.Render(text) + return prefix + " " + styles.Message.Render(e.Text) } availableWidth := width - len([]rune(prefixText)) - 1 - lines := wrap.SoftWrap(text, availableWidth) + lines := wrap.SoftWrap(e.Text, availableWidth) if len(lines) == 0 { return prefix }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ui/components/message.go` around lines 17 - 32, The local variable text := e.Text is unnecessary; replace usages of text with the struct field e.Text directly (e.g., in calls to wrap.SoftWrap(e.Text, width/availableWidth) and in styles.Message.Render / styles.SecondaryMessage.Render) and remove the redundant text declaration, keeping the existing prefixText, availableWidth, and severity-based style logic intact (functions/identifiers to update: text variable, e.Text, wrap.SoftWrap, styles.Message.Render, styles.SecondaryMessage.Render).internal/ui/app.go (1)
98-117: Avoid deriving prompt behavior from label text.Both the vertical-navigation path and the resolved confirmation line are now keyed off string checks on the first option label (
"["/"Update now"). That hard-couples the UI to the exact literals emitted byinternal/update/notify.go, so a copy tweak or localization will silently break navigation or the post-selection summary. Please make the prompt layout/summary style explicit on the event instead of inferring it from labels.Also applies to: 318-354
internal/ui/components/input_prompt.go (1)
98-107: Make vertical layout explicit instead of inferring from label text.Detecting layout from
[/]in labels is fragile (wording/i18n changes can alter behavior). Prefer an explicit layout flag onInputPromptset by the caller.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ui/components/input_prompt.go` around lines 98 - 107, The layout detection in usesVerticalOptions (which currently inspects input labels for "["/"]") is fragile; add an explicit boolean field (e.g., VerticalLayout or UseVerticalOptions) to the InputPrompt struct and change usesVerticalOptions to read that flag (or add a new function like shouldUseVerticalOptions(prompt InputPrompt, options []output.InputOption) that checks prompt.VerticalLayout first); remove the label-based bracket detection and update all callers that construct InputPrompt to set the new flag where a vertical layout is required so layout is driven explicitly rather than inferred from opt.Label.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/config/config.go`:
- Around line 135-140: The fallback that maps legacy top-level keys to cfg.CLI
fields is never reached because viper.SetDefault made "cli.update_prompt" and
"cli.update_skipped_version" appear as set; update the conditions in
internal/config/config.go to use viper.InConfig("cli.update_prompt") /
viper.InConfig("cli.update_skipped_version") (or check viper.InConfig for the
top-level keys "update_prompt" and "update_skipped_version" as appropriate)
instead of viper.IsSet, and when the top-level legacy keys are present map them
into cfg.CLI.UpdatePrompt and cfg.CLI.UpdateSkippedVersion so legacy config
files are honored.
In `@internal/ui/components/input_prompt.go`:
- Around line 86-88: The selection rendering currently compares each option's
Key to p.options[p.selectedIndex].Key, which can mark multiple rows selected
when keys collide; change the range loop in the rendering code for input_prompt
(the loop that iterates over p.options and calls sb.WriteString with
styles.NimboMid.Render) to use the loop index (e.g., compare i ==
p.selectedIndex) instead of comparing opt.Key, so only the option at
p.selectedIndex is rendered as selected.
In `@internal/update/notify.go`:
- Line 20: The single shared const checkTimeout is too long for the inline
startup path called by NotifyUpdate (used during lstk start --non-interactive)
and causes a 5s stall; split the timeout into two constants (e.g.
startupCheckTimeout with a short duration and fullCheckTimeout with the existing
5s) and update call sites so NotifyUpdate (or the startup-specific call) uses
startupCheckTimeout while the explicit update command/CheckForUpdate uses
fullCheckTimeout; alternatively add a timeout parameter to
NotifyUpdate/CheckForUpdate and pass the short timeout from the non-interactive
startup path and the longer one from the explicit update command.
---
Outside diff comments:
In `@internal/update/notify_test.go`:
- Around line 129-142: Rename the test to TestNotifyUpdateSkippedVersion, create
a temporary config directory and point the app's config loader there (so writes
don't hit the real ~/.config/lstk), use output.SinkFunc to send the "s" response
(not "n") to trigger the per-version skip branch in notifyUpdateWithVersion with
NotifyOptions{UpdatePrompt: true} and the existing test fetcher/server, then
assert the config key "update_skipped_version" was written and equals the
current version (read back via the same config loader or Viper instance) and
finally restore any env/config state you changed.
---
Nitpick comments:
In `@internal/ui/components/input_prompt.go`:
- Around line 98-107: The layout detection in usesVerticalOptions (which
currently inspects input labels for "["/"]") is fragile; add an explicit boolean
field (e.g., VerticalLayout or UseVerticalOptions) to the InputPrompt struct and
change usesVerticalOptions to read that flag (or add a new function like
shouldUseVerticalOptions(prompt InputPrompt, options []output.InputOption) that
checks prompt.VerticalLayout first); remove the label-based bracket detection
and update all callers that construct InputPrompt to set the new flag where a
vertical layout is required so layout is driven explicitly rather than inferred
from opt.Label.
In `@internal/ui/components/message.go`:
- Around line 17-32: The local variable text := e.Text is unnecessary; replace
usages of text with the struct field e.Text directly (e.g., in calls to
wrap.SoftWrap(e.Text, width/availableWidth) and in styles.Message.Render /
styles.SecondaryMessage.Render) and remove the redundant text declaration,
keeping the existing prefixText, availableWidth, and severity-based style logic
intact (functions/identifiers to update: text variable, e.Text, wrap.SoftWrap,
styles.Message.Render, styles.SecondaryMessage.Render).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 6b0fc112-c5e4-4c89-a8b1-1230417ae1ae
📒 Files selected for processing (11)
cmd/root.gointernal/config/config.gointernal/config/default_config.tomlinternal/ui/app.gointernal/ui/app_test.gointernal/ui/components/input_prompt.gointernal/ui/components/message.gointernal/update/github.gointernal/update/notify.gointernal/update/notify_test.gotest/integration/update_test.go
gtsiolis
force-pushed
the
george/des-183
branch
from
83f6741 to
7a4e4e7
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/update/github.go (1)
43-43: Wrap propagated errors with operation context.These paths currently return bare errors, which makes failure source less clear at call sites. Prefer contextual
%wwrapping.Suggested diff
func fetchLatestRelease(ctx context.Context, token string) (*githubRelease, error) { resp, err := githubRequest(ctx, latestReleaseURL, token) if err != nil { - return nil, err + return nil, fmt.Errorf("fetch latest release request failed: %w", err) } defer func() { _ = resp.Body.Close() }() @@ var release githubRelease if err := json.NewDecoder(resp.Body).Decode(&release); err != nil { - return nil, err + return nil, fmt.Errorf("decode latest release response failed: %w", err) } @@ func fetchLatestVersion(ctx context.Context, token string) (string, error) { release, err := fetchLatestRelease(ctx, token) if err != nil { - return "", err + return "", fmt.Errorf("fetch latest version failed: %w", err) } return release.TagName, nil }Also applies to: 53-53, 62-62
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/update/github.go` at line 43, The three bare error propagations in internal/update/github.go (the `return nil, err` occurrences around the block at lines referenced) should be wrapped with contextual errors using fmt.Errorf and the %w verb; update each `return nil, err` to return a wrapped error that describes the operation (e.g., "fetching releases", "parsing release", or "creating request" — choose wording appropriate to the surrounding code) and include the original err via %w so callers get both context and the wrapped error; apply this change to all three occurrences (the returns at the three locations noted) and ensure fmt is imported if not already.internal/config/config.go (1)
135-140: Backward-compat logic is correct.Using
viper.InConfig()properly checks for explicit presence in the loaded config file (not defaults), ensuring legacy top-levelupdate_promptandupdate_skipped_versionkeys are migrated tocfg.CLI.*only when the newcli.*keys aren't explicitly set.However, no existing tests cover this legacy migration path. All current tests use the new
[cli]section format. Adding a unit test for configs with the legacy top-level keys would improve coverage and prevent regressions if this migration logic changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/config.go` around lines 135 - 140, Add a unit test that verifies the legacy top-level keys migrate into the CLI struct when the new cli.* keys are absent: create a test (e.g., TestLoadConfig_LegacyTopLevelKeys) that provides a Viper config containing update_prompt and update_skipped_version but no cli.update_prompt or cli.update_skipped_version, call the config load function used in your code path that reads viper.InConfig, and assert that cfg.CLI.UpdatePrompt and cfg.CLI.UpdateSkippedVersion are populated accordingly; also include a negative case where cli.update_prompt is present to ensure the migration does not override explicit cli.* values.
🤖 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/config/config.go`:
- Around line 135-140: Add a unit test that verifies the legacy top-level keys
migrate into the CLI struct when the new cli.* keys are absent: create a test
(e.g., TestLoadConfig_LegacyTopLevelKeys) that provides a Viper config
containing update_prompt and update_skipped_version but no cli.update_prompt or
cli.update_skipped_version, call the config load function used in your code path
that reads viper.InConfig, and assert that cfg.CLI.UpdatePrompt and
cfg.CLI.UpdateSkippedVersion are populated accordingly; also include a negative
case where cli.update_prompt is present to ensure the migration does not
override explicit cli.* values.
In `@internal/update/github.go`:
- Line 43: The three bare error propagations in internal/update/github.go (the
`return nil, err` occurrences around the block at lines referenced) should be
wrapped with contextual errors using fmt.Errorf and the %w verb; update each
`return nil, err` to return a wrapped error that describes the operation (e.g.,
"fetching releases", "parsing release", or "creating request" — choose wording
appropriate to the surrounding code) and include the original err via %w so
callers get both context and the wrapped error; apply this change to all three
occurrences (the returns at the three locations noted) and ensure fmt is
imported if not already.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 01cec8fe-04f5-476f-ab26-763c1eaafcbf
📒 Files selected for processing (12)
cmd/root.gointernal/config/config.gointernal/config/default_config.tomlinternal/ui/app.gointernal/ui/app_test.gointernal/ui/components/input_prompt.gointernal/ui/components/message.gointernal/update/github.gointernal/update/notify.gointernal/update/notify_test.gotest/integration/start_test.gotest/integration/update_test.go
✅ Files skipped from review due to trivial changes (4)
- internal/config/default_config.toml
- internal/ui/components/message.go
- internal/ui/app.go
- internal/ui/app_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/update/notify_test.go
- internal/ui/components/input_prompt.go
- internal/update/notify.go
anisaoshafi
left a comment
anisaoshafi
left a comment
There was a problem hiding this comment.
The UX looks really nice, thanks George 🤩
I left a few comments
internal/config/config.go
Outdated
| return Set("cli.update_skipped_version", version) | ||
| } | ||
|
|
||
| func GetUpdateSkippedVersion() string { |
| } | ||
|
|
||
| const checkTimeout = 500 * time.Millisecond | ||
| const checkTimeout = 2 * time.Second |
There was a problem hiding this comment.
nit: PR description says the timeout was reduced
internal/ui/app.go
Outdated
| case "n": | ||
| return checkmark + " Won't ask again" |
There was a problem hiding this comment.
seems like this code is unreachable 🤔
internal/config/default_config.toml
Outdated
| # CLI settings | ||
| [cli] | ||
| update_prompt = true | ||
|
|
There was a problem hiding this comment.
nit: not sure we want this here since true is the default value anyways? Showing to users might invite them to disable it? Not sure what's right. If not documented here, we don't really have another place where we document all settings...
| if !viper.InConfig("cli.update_skipped_version") && viper.InConfig("update_skipped_version") { | ||
| cfg.CLI.UpdateSkippedVersion = viper.GetString("update_skipped_version") | ||
| } | ||
| for i := range cfg.Containers { |
There was a problem hiding this comment.
issue: this looks like migration code, is this right?
update_prompt: I think we could ignore the fact that some users previously had this setting one level up in the config. They would just be asked again and in exchange we get rid of some complexity.update_skipped_version: This is introduced in this PR so there is nothing to migrate.
In short: can we remove these lines?
| return a, responseCmd | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
nit: the Update method is already doing a lot and the PR adds another ~20 lines. Can we extract this in a new method, something like:
func (a App) handleVerticalPromptKey(msg tea.KeyMsg) (App, tea.Cmd, bool) {
...
return a, nil, false
}
internal/ui/app.go
Outdated
| } | ||
|
|
||
| func componentsUsesVerticalPrompt(prompt components.InputPrompt, options []output.InputOption) bool { | ||
| return prompt.Visible() && len(options) > 1 && strings.Contains(options[0].Label, "[") |
There was a problem hiding this comment.
issue:
strings.Contains(options[0].Label, "[")
We should not rely on string parsing. Can we add something in the model that indicates that this user input should be displayed vertically? Maybe as simple as:
type UserInputRequestEvent struct {
Prompt string
Options []InputOption
ResponseCh chan<- InputResponse
Vertical bool
}
Not sure if it would help but we might want to add this to CLAUDE.md since it's come back a few times.
| case "n": | ||
| return checkmark + " Won't ask again" | ||
| } | ||
| } |
There was a problem hiding this comment.
issue: this file is generic UI rendering code, it should not know anything about the update logic.
We need to find another solution for this. Maybe a confirmation text can be added on the input options.
gtsiolis
force-pushed
the
george/des-183
branch
from
0dd4a7b to
0b42a8b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/ui/components/input_prompt_test.go (1)
17-59: Consider adding a test case for vertical rendering.All test cases set
vertical: false. While the vertical selection behavior is tested inapp_test.go, adding a test case here forvertical: truewould verify the component's vertical rendering output (e.g., ensuring options render on separate lines with selection indicators).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ui/components/input_prompt_test.go` around lines 17 - 59, Add a new table-driven test case in input_prompt_test.go that sets vertical: true (e.g., name: "vertical renders options on separate lines") using the existing options field (output.InputOption) and populate contains/excludes to assert that each option appears on its own line and that selection indicators/markers are present (for example, check for each label on separate lines and for a selection marker like ">" or "(Y)"). Place the new case alongside the other cases in the test slice so the test harness iterates over it, using the same test helpers/assertions already used in the file to validate the rendered output.
🤖 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/ui/components/input_prompt_test.go`:
- Around line 17-59: Add a new table-driven test case in input_prompt_test.go
that sets vertical: true (e.g., name: "vertical renders options on separate
lines") using the existing options field (output.InputOption) and populate
contains/excludes to assert that each option appears on its own line and that
selection indicators/markers are present (for example, check for each label on
separate lines and for a selection marker like ">" or "(Y)"). Place the new case
alongside the other cases in the test slice so the test harness iterates over
it, using the same test helpers/assertions already used in the file to validate
the rendered output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 76bfc11f-bcae-4f5a-be16-d5a844d58c92
📒 Files selected for processing (13)
cmd/root.gointernal/config/config.gointernal/output/events.gointernal/ui/app.gointernal/ui/app_test.gointernal/ui/components/input_prompt.gointernal/ui/components/input_prompt_test.gointernal/ui/components/message.gointernal/update/github.gointernal/update/notify.gointernal/update/notify_test.gotest/integration/start_test.gotest/integration/update_test.go
✅ Files skipped from review due to trivial changes (3)
- internal/ui/components/message.go
- test/integration/start_test.go
- test/integration/update_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- cmd/root.go
- internal/ui/app_test.go
- internal/update/github.go
- internal/ui/components/input_prompt.go
|
Tried to address all review comments in one commit, haven't tested yet. I will re-request reviews after testing, unless someone wants to pick this up in the meantime. |
3 participants
This PR simplifies the update notification flow and removes the "Never ask again" option.
Users now see a cleaner three-option prompt: Update now [U], Remind me next time [R], Skip this version [S].
The "Skip" option no longer persists - users will be reminded again on next run.
Changes
Update Notification
SkippedVersionfield fromNotifyOptions(no longer persisting skipped versions)PersistSkipVersioncallback fromNotifyOptionsConfig Schema
update_skipped_versionfield fromCLIConfigstructSetUpdateSkippedVersion()andGetUpdateSkippedVersion()functionsupdate_promptandupdate_skipped_versionkeys at root levelupdate_prompt = truefrom default config template (true is already the default via viper)UI Improvements
Verticalfield toUserInputRequestEventto explicitly indicate vertical layoutstrings.Contains(options[0].Label, "[")) with explicit boolean fieldhandleVerticalPromptKey()method fromUpdate()to reduce complexityformatResolvedInput()function