Re: [PR] feat(cli): add searchable session picker [maka]

["jackwener (via GitHub)" <gi...@apache.org>]

jackwener commented on code in PR #5027:
URL: https://github.com/apache/maka/pull/5027#discussion_r3959210047


##########
packages/cli/src/pi-tui-pickers.ts:
##########
@@ -802,6 +805,129 @@ function matchesModelChoice(choice: ModelChoice, query: string): boolean {
   return false;
 }
 
+export interface SessionSearchChoice {
+  item: SelectItem;
+  searchText: string;
+}
+
+export interface SessionSearchOverlayInput {
+  locale: UiLocale;
+  choices: readonly SessionSearchChoice[];
+  scopeLabel: string;
+  onSelect: (item: SelectItem) => void;
+  onCancel: () => void;
+  onToggleScope: () => void;
+}
+
+export class SessionSearchOverlay implements Component {
+  private readonly searchEditor: Editor;
+  private readonly copy: TuiPickerCopy;
+  private choices: readonly SessionSearchChoice[];
+  private filtered: readonly SessionSearchChoice[];
+  private list: SelectList;
+  private selectedValue: string | undefined;
+  private scopeLabel: string;
+
+  constructor(
+    private readonly tui: TUI,
+    private readonly input: SessionSearchOverlayInput,
+  ) {
+    this.copy = getTuiPickerCopy(input.locale);
+    this.choices = input.choices;
+    this.filtered = input.choices;
+    this.scopeLabel = input.scopeLabel;
+    this.list = this.buildList();
+    this.searchEditor = new Editor(tui, editorTheme(), { paddingX: 0 });
+    this.searchEditor.onChange = (text) => this.applyQuery(text);
+  }
+
+  updateChoices(choices: readonly SessionSearchChoice[], scopeLabel: string): void {
+    this.choices = choices;
+    this.scopeLabel = scopeLabel;
+    this.applyQuery(this.searchEditor.getText());
+  }
+
+  private buildList(): SelectList {
+    const list = new SelectList(
+      this.filtered.map(({ item }) => item),
+      10,
+      selectListTheme(),
+      {
+        minPrimaryColumnWidth: 20,
+        maxPrimaryColumnWidth: 48,
+      },
+    );
+    const selectedIndex = this.filtered.findIndex(({ item }) => item.value === this.selectedValue);
+    if (selectedIndex >= 0) list.setSelectedIndex(selectedIndex);
+    list.onSelectionChange = (item) => {
+      this.selectedValue = item.value;
+    };
+    list.onSelect = (item) => {
+      this.selectedValue = item.value;
+      this.input.onSelect(item);
+    };
+    list.onCancel = () => this.input.onCancel();
+    return list;
+  }
+
+  private applyQuery(text: string): void {
+    const query = text.trim().toLocaleLowerCase();
+    this.filtered = query
+      ? this.choices.filter((choice) => choice.searchText.includes(query))
+      : this.choices;
+    this.list = this.buildList();
+  }
+
+  invalidate(): void {
+    this.searchEditor.invalidate();
+    this.list.invalidate();
+  }
+
+  handleInput(data: string): void {
+    if (matchesKey(data, Key.escape) || matchesKey(data, Key.ctrl('c'))) {
+      this.input.onCancel();
+      return;
+    }
+    if (matchesKey(data, Key.tab) && !isKeyRelease(data) && !isKeyRepeat(data)) {
+      this.input.onToggleScope();
+      return;
+    }
+    if (matchesKey(data, Key.up) || matchesKey(data, Key.down) || matchesKey(data, Key.enter)) {
+      if (matchesKey(data, Key.enter) && isKeyRepeat(data)) return;
+      this.list.handleInput(data);
+      return;
+    }
+    this.searchEditor.handleInput(data);
+  }
+
+  render(width: number): string[] {
+    const safeWidth = Math.max(1, width);
+    this.searchEditor.focused = true;
+    return [
+      padLine(`Resume Session ${ansi.accent(this.scopeLabel)}`, safeWidth),

Review Comment:
   **[P2] The `Resume Session` allowance was deleted, but the literal was not — it moved into a position the checker does not inspect, so the guard silently lost an entry that still ships.**
   
   `scripts/check-tui-copy.mjs` drops both `'Resume Session'` and `'Tab scope · ↑↓ move · Enter select · Esc close'` from the `pi-tui-runner.ts` allowance list. The second literal really is gone — it became `copy.sessionSearchHint`, localised in all three locales. This one is still here, now as a template literal argument to `padLine(...)`.
   
   **Why the check still passes, with the control that rules out the obvious explanation.** `packages/cli/src/pi-tui-pickers.ts` *is* in the scanned file list, so “the file isn’t checked” is not the answer. Running the checker on this head after changing an already-allowed literal in the same file (`'Full access'` → `'Full accessZZZ'`) gives:
   
   ```
   packages/cli/src/pi-tui-pickers.ts:1080: visible-literal: "Full accessZZZ"
   packages/cli/src/pi-tui-pickers.ts:1: stale-allowance: "visible-literal: Full access"
   ```
   
   So the file is scanned, and this is the same diagnostic pair that was failing before. I then assumed the template-literal syntax was hiding it, since `staticText()` normalises template literals to `'Resume Session ${…}'` and that entry is absent from the pickers allowance list. That is **not** the reason: rewriting the line as a plain `padLine('Resume SessionZZZ', safeWidth)` is equally unreported. The blind spot is the **argument position** — the checker looks at visible sinks like `label:` and `description:` properties, not at arbitrary call arguments.
   
   **Why this is P2 even though nothing the user sees has changed.** The string was hardcoded English before too, so there is no runtime regression. The regression is in the guard: this allowance list is meant to be the complete, reviewable inventory of un-localised visible text, and an entry has now left the inventory while the text remains — invisible to CI and to every future reviewer, in a spot where no future edit will ever flag it. It also leaves this one overlay internally inconsistent: the same diff localises its hint and its empty state, so a `zh` user now reads a Chinese hint underneath an English title.
   
   **Suggested fix**, matching what this PR already does twice: add a `resumeSessionTitle` (and, if you want it complete, the `Current`/`All` scope labels, which are hardcoded in `showSessionList`) to `TUI_COPY_RESOURCES` and render `this.copy.resumeSessionTitle` here. Re-adding an allowance under `pi-tui-pickers.ts` would restore the tracking but keeps the string English, which seems like the wrong direction given the rest of this overlay.
   
   > **Automated review notice:** This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.



##########
packages/cli/src/pi-tui-pickers.ts:
##########
@@ -802,6 +805,129 @@ function matchesModelChoice(choice: ModelChoice, query: string): boolean {
   return false;
 }
 
+export interface SessionSearchChoice {
+  item: SelectItem;
+  searchText: string;
+}
+
+export interface SessionSearchOverlayInput {
+  locale: UiLocale;
+  choices: readonly SessionSearchChoice[];
+  scopeLabel: string;
+  onSelect: (item: SelectItem) => void;
+  onCancel: () => void;
+  onToggleScope: () => void;
+}
+
+export class SessionSearchOverlay implements Component {
+  private readonly searchEditor: Editor;
+  private readonly copy: TuiPickerCopy;
+  private choices: readonly SessionSearchChoice[];
+  private filtered: readonly SessionSearchChoice[];
+  private list: SelectList;
+  private selectedValue: string | undefined;
+  private scopeLabel: string;
+
+  constructor(
+    private readonly tui: TUI,
+    private readonly input: SessionSearchOverlayInput,
+  ) {
+    this.copy = getTuiPickerCopy(input.locale);
+    this.choices = input.choices;
+    this.filtered = input.choices;
+    this.scopeLabel = input.scopeLabel;
+    this.list = this.buildList();
+    this.searchEditor = new Editor(tui, editorTheme(), { paddingX: 0 });
+    this.searchEditor.onChange = (text) => this.applyQuery(text);
+  }
+
+  updateChoices(choices: readonly SessionSearchChoice[], scopeLabel: string): void {
+    this.choices = choices;
+    this.scopeLabel = scopeLabel;
+    this.applyQuery(this.searchEditor.getText());
+  }
+
+  private buildList(): SelectList {
+    const list = new SelectList(
+      this.filtered.map(({ item }) => item),
+      10,
+      selectListTheme(),
+      {
+        minPrimaryColumnWidth: 20,
+        maxPrimaryColumnWidth: 48,

Review Comment:
   **[P3] The Session picker loses its terminal-adaptive column width here; long session names truncate much earlier on wide terminals.**
   
   The list this overlay replaces was built in `pi-tui-runner.ts` with:
   
   ```ts
   maxPrimaryColumnWidth: Math.max(20, terminal.columns - 30)
   ```
   
   It was the only picker in the runner that sized its primary column from the terminal; every other one uses a fixed constant. `SessionSearchOverlay` now hardcodes `48`, which is `ModelSearchOverlay`’s value — plausibly carried over with the surrounding structure rather than chosen. On a 160-column terminal (the width the new test itself uses) the label column goes from ~130 to 48, so session names that were fully readable now truncate.
   
   P3 because nothing is unreachable — the name is still in `searchText`, and search arguably reduces how much a user needs to read from the list. But it is a visible narrowing, and `SessionSearchOverlay` receives `tui`, so the terminal width is available if you want the previous behaviour back.
   
   > **Automated review notice:** This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org