Skip to content

feat(tui): pills for known basin locations in create form#496

Merged
cristi- merged 3 commits into
mainfrom
cc/tui-location-pills
Jun 10, 2026
Merged

feat(tui): pills for known basin locations in create form#496
cristi- merged 3 commits into
mainfrom
cc/tui-location-pills

Conversation

@cristi-

@cristi- cristi- commented May 27, 2026

Copy link
Copy Markdown
Member

Fetches list_locations lazily on first CreateBasin open and renders the result as pills ([Default] [aws:us-east-1] … [Custom…]). Falls back to today's freeform text input if the fetch fails or returns empty.

@cristi-
cristi- marked this pull request as ready for review May 28, 2026 14:30
@greptile-apps

greptile-apps Bot commented May 28, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

Adds lazy list_locations fetching and renders the result as pill selectors ([Default] [aws:us-east-1] … [Custom…]) in the CreateBasin dialog, falling back to freeform text input when the fetch fails or returns empty.

  • app.rs: loads locations on first CreateBasin open, stores them on App, handles Left/Right to cycle pills, and dispatches a new LocationsLoaded event.
  • ui.rs: renders the pill row and conditionally shows a text-input row beneath it when "Custom…" is active; contains duplicated pill-index logic that also lives in app.rs.
  • event.rs / ci.yml: adds the new event variant and skips location API tests in CI.

Confidence Score: 3/5

The Custom pill — the core interactive feature introduced by this PR — is unreachable via keyboard navigation when known locations are loaded.

set_location_for_pill clears location on the Custom slot, but an empty string maps back to Default in location_pill_idx(), so the Custom pill can never appear highlighted and the custom text-input row never renders.

cli/src/tui/app.rs — specifically set_location_for_pill and its interaction with the location_pill_idx closure

Important Files Changed

Filename Overview
cli/src/tui/app.rs Adds location loading, pill-navigation keyboard handling, and set_location_for_pill; the helper incorrectly clears location for the Custom pill, making that pill unselectable via keyboard.
cli/src/tui/ui.rs Renders location pills with a conditional custom text-input row; duplicates the pill-index computation that also lives in app.rs.
cli/src/tui/event.rs Adds the LocationsLoaded event variant; straightforward and correct.
.github/workflows/ci.yml Skips location-related tests in Go and Python CI runs, consistent with the pattern used for other credential-dependent tests.

Sequence Diagram

sequenceDiagram
    participant U as User
    participant App
    participant API as s2 API

    U->>App: press 'c' (CreateBasin)
    App->>App: locations.is_none()?
    App->>API: list_locations()
    API-->>App: LocationsLoaded(Ok(locs))
    App->>App: "self.locations = Some(locs)"

    U->>App: navigate to field 1 (Location)
    App->>App: render pills [Default][aws:us-east-1][Custom…]

    U->>App: press Right
    App->>App: "new_idx = 1"
    App->>App: "set_location_for_pill sets location = known_locs[0]"

    U->>App: press Right again (toward Custom)
    App->>App: "new_idx = custom_idx"
    App->>App: set_location_for_pill clears location (bug)
    App->>App: location_pill_idx returns 0 (Default) instead of custom_idx

    U->>App: press Enter to submit
    App->>API: create_basin with empty location
Loading
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
cli/src/tui/app.rs:1022-1027
**Custom pill unreachable via keyboard navigation**

`set_location_for_pill` calls `location.clear()` when `pill_idx == custom_idx`, but `location_pill_idx()` maps an empty string back to index 0 (Default), not to `custom_idx`. After pressing Right to reach "Custom…", the app's state is identical to having Default selected, so the pill row renders with Default highlighted and the custom text-input row never appears. Because `location_pill_idx()` immediately evaluates to 0, the `editable` guard in the Enter handler also rejects entry to edit mode for the Custom slot.

The `location` field alone cannot encode three distinct states (Default = `""`, known location = its name, Custom-but-empty = ???). A separate flag or sub-enum for the current pill selection would resolve the ambiguity without changing the submitted value semantics.

### Issue 2 of 2
cli/src/tui/ui.rs:4620-4631
**Duplicated pill-index computation**

The mapping from `location` string → active pill index is computed independently here (in `draw_input_dialog`) and in the `location_pill_idx` closure inside `handle_input` in `app.rs`. If the logic ever diverges (e.g., a new sentinel value is added), the rendered highlight and the keyboard navigation cursor will disagree silently. Extracting this into a small shared helper — or passing a pre-computed index through `InputMode` — would keep the two sites in sync.

Reviews (1): Last reviewed commit: "maybe" | Re-trigger Greptile

Comment thread cli/src/tui/app.rs Outdated
Comment on lines +1022 to +1027
if pill_idx == 0 || pill_idx == custom_idx {
location.clear();
} else if let Some(name) = names.get(pill_idx - 1) {
location.clear();
location.push_str(name);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Custom pill unreachable via keyboard navigation

set_location_for_pill calls location.clear() when pill_idx == custom_idx, but location_pill_idx() maps an empty string back to index 0 (Default), not to custom_idx. After pressing Right to reach "Custom…", the app's state is identical to having Default selected, so the pill row renders with Default highlighted and the custom text-input row never appears. Because location_pill_idx() immediately evaluates to 0, the editable guard in the Enter handler also rejects entry to edit mode for the Custom slot.

The location field alone cannot encode three distinct states (Default = "", known location = its name, Custom-but-empty = ???). A separate flag or sub-enum for the current pill selection would resolve the ambiguity without changing the submitted value semantics.

Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/src/tui/app.rs
Line: 1022-1027

Comment:
**Custom pill unreachable via keyboard navigation**

`set_location_for_pill` calls `location.clear()` when `pill_idx == custom_idx`, but `location_pill_idx()` maps an empty string back to index 0 (Default), not to `custom_idx`. After pressing Right to reach "Custom…", the app's state is identical to having Default selected, so the pill row renders with Default highlighted and the custom text-input row never appears. Because `location_pill_idx()` immediately evaluates to 0, the `editable` guard in the Enter handler also rejects entry to edit mode for the Custom slot.

The `location` field alone cannot encode three distinct states (Default = `""`, known location = its name, Custom-but-empty = ???). A separate flag or sub-enum for the current pill selection would resolve the ambiguity without changing the submitted value semantics.

How can I resolve this? If you propose a fix, please make it concise.

Comment thread cli/src/tui/ui.rs Outdated
Comment on lines +4620 to +4631
} else {
let custom_pill_idx = known_locs.len() + 1;
let pill_idx = if location.is_empty() {
0
} else if let Some(i) = known_locs
.iter()
.position(|l| l.name.as_ref() == location.as_str())
{
i + 1
} else {
custom_pill_idx
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Duplicated pill-index computation

The mapping from location string → active pill index is computed independently here (in draw_input_dialog) and in the location_pill_idx closure inside handle_input in app.rs. If the logic ever diverges (e.g., a new sentinel value is added), the rendered highlight and the keyboard navigation cursor will disagree silently. Extracting this into a small shared helper — or passing a pre-computed index through InputMode — would keep the two sites in sync.

Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/src/tui/ui.rs
Line: 4620-4631

Comment:
**Duplicated pill-index computation**

The mapping from `location` string → active pill index is computed independently here (in `draw_input_dialog`) and in the `location_pill_idx` closure inside `handle_input` in `app.rs`. If the logic ever diverges (e.g., a new sentinel value is added), the rendered highlight and the keyboard navigation cursor will disagree silently. Extracting this into a small shared helper — or passing a pre-computed index through `InputMode` — would keep the two sites in sync.

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

cristi- and others added 3 commits June 10, 2026 19:02
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@cristi-
cristi- force-pushed the cc/tui-location-pills branch from 3cbd983 to 4f27a7f Compare June 10, 2026 16:05
@cristi-
cristi- merged commit 2181e8d into main Jun 10, 2026
17 checks passed
@cristi-
cristi- deleted the cc/tui-location-pills branch June 10, 2026 16:21
@release-pleaze release-pleaze Bot mentioned this pull request Jun 10, 2026
cristi- pushed a commit that referenced this pull request Jun 10, 2026
## 🤖 New release

* `s2-lite`: 0.36.2 -> 0.36.3 (✓ API compatible changes)
* `s2-cli`: 0.36.2 -> 0.36.3

<details><summary><i><b>Changelog</b></i></summary><p>

## `s2-lite`

<blockquote>

## [0.36.3] - 2026-06-10

### Bug Fixes

- Add proxy-friendly SSE headers
([#500](#500))
- Add proxy-friendly S2S headers
([#503](#503))
- Read AWS_SESSION_TOKEN for static S3 credentials
([#507](#507))
- Improve error handling and observability
([#506](#506))
- Use saturating_add for seq_num in SSE resume to prevent overflow
([#527](#527))
- Apply S3 custom endpoint for AWS SDK/IAM credentials
([#512](#512))

### Refactor

- Extract shared KV key ser/deser helpers for StreamId and BasinName
([#505](#505))

<!-- generated by git-cliff -->
</blockquote>

## `s2-cli`

<blockquote>

## [0.36.3] - 2026-06-10

### Features

- Pills for known basin locations in create form
([#496](#496))

### Bug Fixes

- Check S2_ACCESS_TOKEN case-insensitively in token source detection
([#508](#508))
- Improve error handling and observability
([#506](#506))
- Restrict TUI location input to LocationName-valid characters
([#510](#510))
- Propagate config parse errors instead of silently overwriting
([#516](#516))
- Include access_token_not_found in auth error detection
([#511](#511))
- Compute token source before SDK init for better error messages
([#513](#513))

<!-- generated by git-cliff -->
</blockquote>


</p></details>

---
This PR was generated with
[release-plz](https://github.com/release-plz/release-plz/).

Co-authored-by: release-pleaze[bot] <262023388+release-pleaze[bot]@users.noreply.github.com>
@release-pleaze release-pleaze Bot mentioned this pull request Jun 10, 2026
infiniteregrets added a commit that referenced this pull request Jun 27, 2026
…ions load (#607)

Closes #537.

## Problem

In the TUI **Create basin** form, if you type a custom location *before*
the async locations list finishes loading, you can't re-edit it
afterwards — Enter is silently ignored even though the form still shows
an editable-looking text input.

The field had two diverging sources of truth:
- The **UI** decides whether to show the custom text input via
`location_pill_idx`, which resolves any unknown non-empty location to
the `Custom…` pill — so it renders an editable input.
- The **Enter guard** only allowed editing when
`known_location_names.is_empty() || custom_location_active`.
`custom_location_active` is set only by Left/Right pill navigation,
never when you type directly. So once locations load (list non-empty)
and the flag was never set, the guard is `false`.

Result: input looks editable, Enter does nothing. The only workaround —
navigating to the `Custom…` pill — clears the typed text (data loss).
This was a regression from #496, which added the Enter guard.

## Fix

Derive editability from the **same** `location_pill_idx` logic the UI
uses, via a shared `location_field_editable` helper, so the guard and
the rendered state can't diverge.

I chose this over the issue's suggested "sync the flag on edit-exit": at
edit-exit time the locations may not be loaded yet, so you can't
reliably tell whether the typed value is a known or custom location.
Deriving editability lazily at Enter-press time (when the list *is*
loaded) matches the UI exactly and removes the divergence at the root.

Scope note: TUI-only; the plain CLI `--location` flag was never
affected.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants