Commit Graph
2 Commits
Author SHA1 Message Date
Brennan Benson c511e51442 fix(mobile): label native-chat tool rows with a clean, expandable input summary (STA-3333) (#12498)
* fix(mobile): label tool rows with a clean summary, expand full input (STA-3333)

Mobile tool rows showed the raw input JSON (`{"file_path":…}`) as the row
label, and the expanded detail just repeated that same truncated string.

- `describeToolInput` labels a row with the target file path, else the
  primary argument (command/cmd/query/pattern/url/description), else the
  bounded JSON preview.
- Codex delivers tool arguments as a JSON string; normalize those into the
  object shape the helpers already understand, so labels, file links,
  run summaries and the expanded detail all work for Codex calls too.
- The expanded detail now renders the fully formatted input, capped at
  MAX_TOOL_RESULT_CHARS like desktop's tool detail (and like the result
  body), and a structured input makes the row expandable.

* fix(mobile): name search rows by their term and keep the filename in path labels (STA-3333)

Review follow-ups to the tool-row summary, all in the shared helper:

- A Grep/Glob row labelled itself with the directory it scanned and dropped
  the pattern entirely, because `toolFilePath` treats `path` as a file target.
  That path is a scan root, so it also rendered a tap-to-open link that asked
  the app to open a folder. `toolFilePath` now ignores the generic `path` key
  for search-shaped input, which lets the pattern win the label and drops the
  bogus link; an explicit `file_path` still wins.

- An overlong path was truncated from the head, cutting off the basename —
  the one part that tells two rows apart. Trim from the front instead, so
  the label reads `…/session/MobileNativeChatMessage.tsx`.

- The primary-argument chain used `??`, so a present-but-blank key selected
  itself and swallowed the keys ranked after it, dropping the label all the
  way back to raw JSON. Take the first key that actually yields a label.

Refs STA-3333.

* fix(mobile): don't offer an expander whose detail repeats the row (STA-3333)

An empty tool input formats back to the row label verbatim, so `{}` and `[]`
advertised an expander and then re-showed the label — the same repeat-the-JSON
problem this change set out to remove. Gate `isStructuredToolInput` on the
collection actually having contents; the lazy detail path is untouched.

Also pins the overlong-path test to the path itself: asserting only length<=80
plus a `…` passed just as well with path labelling deleted.

* fix(mobile): gate the tool detail panel on having detail (STA-3333)

The Tools toggle opens every row at once, bypassing the row's tap guard,
so a row with nothing to expand rendered its own label again underneath
itself — and the tap that would dismiss it is a no-op. Matches desktop.

* fix(mobile): keep a blank tool argument out of the run header (STA-3333)

Skipping a present-but-blank primary key let `briefToolArg` fall through
to the raw JSON preview, so a run header read `Bash {"command":""}` where
it used to read `Bash`. Also state the search-path trade-off honestly:
suppressing the link costs a file-scoped search its tap target.

* fix(mobile): only treat a blank primary key as a missing argument (STA-3333)

The previous guard tested key presence, so a populated but non-string
argument — a mixed argv like ['kill','-9',pid], or a structured query —
dropped out of the run header instead of falling back to the preview.

* test(mobile): pin the tool-row chevron to the detail panel (STA-3333)

The panel gate was covered but the chevron beside it was not: swapping
`showDetail` back to `expanded` on the icon alone left all 909 mobile
tests green, so the affordance lie this branch fixes could return
unnoticed — a down-chevron over no panel, on a row whose tap is guarded
off.

Asserts both icon counts on the fixture that test already renders. The
two halves now die for distinct reasons: the panel gate on the duplicate
label text, the chevron on the icon count.

* test(shared): pin the blank-search-key guard in the tool label (STA-3333)

Dropping `.trim()` from summarizePrimaryToolArg left all 32 tests green,
yet it leaks through isSearchToolInput: a whitespace-only `query` starts
counting as a search term, which suppresses `path`. One character takes
the row's label, its tap-to-open link and its run-header argument at
once, and puts the raw JSON label back — the bug this branch removes.

Asserts all three outputs on that shape. Kills only that mutant; the
isSearchToolInput mutant still dies on the existing search test.

* fix(native-chat): share tool input display semantics (STA-3333)

Build the tool row label, file target, detail eligibility and bounded detail from one normalized input model. Mobile no longer reparses JSON-string input across independent helpers or repeats an already-complete plain label, and desktop now uses the same clean row summary instead of retaining raw JSON.\n\nKeep full detail formatting lazy for collapsed rows and share the 4000-character detail cap across both renderers. Tests pin desktop adoption, mobile disclosure parity, one-pass JSON parsing and the shared bound.
2026-08-04 19:34:59 -07:00
64181fdd42 feat(native-chat): native chat view across mobile, desktop, and web (#5824)
* feat(native-chat): add native chat view across mobile

* fix(native-chat): address review findings and CodeRabbit threads

Correctness:
- Restore an independent initial readSession seed and surface initial-drain
  errors as snapshot frames so the chat view can never strand on 'loading'
- Pair mobile tool results to calls by ordinal FIFO (parallel calls no longer
  misgraft results); clear a pending ask only when its own call resolves
- Show a new streaming reply immediately (same-turn suppression, not length)
- Delegate mobile noise filtering to the shared harness-injected classifier
- Admit soft-leaving mobile clients in beginMobileInputFloor (parity with
  mobileTookFloor) so grace-window writes aren't dropped
- Self-heal a stale 'working' status once this turn's reply lands
- Catch RPC rejections in mobile file-open helpers; guard sanitizeToolInput
  key collisions; settle web/runtime transports on unrecognized first frames
  and forward snapshot errors

Perf:
- Throttle the mobile streaming bubble (50ms) so per-part status frames stop
  re-parsing the whole accumulated markdown
- Short-circuit markdown path detection on dot-less or oversized runs
  (quadratic backtracking guard)

UX/minor:
- Wire hold-mode dictation through the native chat composer
- Allow scoped-package (@) paths in file-path detection
- Move caret after mid-text autocomplete insertion; index-prefixed ask option
  keys; single scroll-to-end effect; bounded wait + toast when image attach
  races a resubscribe; count-based pending reconciliation; cache-hit search
  cancels stale debounce; chat-tab toggle wins over in-flight preference load
- Share shouldStepNativeChatAskAnswer between desktop and mobile; import
  block guards/source priority from shared instead of local copies
- Defensive non-positive transcript limits; test strengthening (TTL expiry,
  post-unsubscribe stale frame, lease readiness, filtered console.error)

* refactor(native-chat): share desktop/mobile chat logic in src/shared

Extract the parity-mirrored native-chat modules into shared implementations
both surfaces re-export: ask parsing (registry, parseAskFromStatus,
extractPendingAsk, formatAskAnswer), answer stepping offsets/scheduler, diff
detection/parsing, harness-noise filtering, tool fold/pair/split, and tool
summaries. Removes the hand-synced copies and their stale Metro comments.

Divergence reconciliations take the safer side of each: diffs truncate at
120 lines/32KB everywhere (desktop previously unbounded), tool-run summaries
cap at 3 parts with bounded-depth previews, nameless tool calls are skipped,
and basenames split on both separators.

Also: settle and kill every sibling quick-open pass when one reaches
maxResults (main rg/git and relay git; relay rg already did) so a capped
search cannot leave a scan walking a huge tree; fold window-bounding into
the shared merger's applyAppend; localize the web 'Pair a host' snapshot
error.

* fix(native-chat): address CodeRabbit follow-ups on shared modules

- Attachment lease gate re-checks connection/target/tab after the bounded
  wait, so a tab/host switch or disconnect mid-wait can't send into a stale
  terminal; a moved-away target drops silently like the pre-wait guard and
  only an unrecovered lease surfaces the toast. Adds hook tests.
- extractPendingAsk parses transcript tool-calls through the same
  registered-parser + canonical-shape fallback as live status, so a custom
  question tool that rendered live survives reconnect/replay.
- Direct unit tests for the shared ask parser (FIFO ordering, fallback,
  malformed payloads) and tool-summary bounded preview (depth/collection
  caps, circular refs, basename/command branches).

* fix(native-chat): treat initialLimit 0 as a valid empty window

Both engine guards used truthiness, so an explicit zero limit skipped the
bounded tail reader and fell back to an unbounded incremental read. Latent
only (every caller clamps positive), hardened for consistency with the
tail reader's non-positive-limit handling.

* fix(mobile): native-chat composer lock UX + send-failure feedback

- Distinguish input-lock reasons: transport 'disconnected' shows Reconnecting…
  instead of mislabeling a reconnect as locked-by-another-client
- Guard the composer lock behind a 600ms hold so connState blips / lease
  hand-offs don't flicker the placeholder; unlock stays instant
- Surface a rejected send inline above the composer (a bottom toast hides
  behind the keyboard); auto-dismisses after 4s
- waiting-session hint invites the first message instead of implying the
  agent is still starting

* test(mobile): sync answer-send pacing test to the 500ms advance buffer

Missed in merge 8fe3c391c, which carried main's NATIVE_CHAT_ADVANCE_BUFFER_MS
300->500 (#8568) into the shared stepping module that mobile derives from.

* fix(mobile): restore terminal stream after chat cold start

* fix(native-chat): harden retries, optimistic sends, and file scans

* fix(mobile): deliver AskUserQuestion answers by option number (STA-1860)

Port #8840's fix to the mobile native chat: the Ask card now tracks
per-question option INDICES (+ free text) and the answer-send hook drives
Claude's arrow-navigate selector with buildAskAnswerKeys keystroke groups —
option numbers, next-tab arrows, Enter — paced one selector step apart, instead
of pasting label text that the selector ignores (which silently committed the
default option). Non-Claude agents keep the pasted-label path via the
selection-based formatAskAnswer.

Backcompat: keystrokes are built client-side and written through the EXISTING
terminal.send passthrough with enter:false — the same contract the permission
card already uses — so an older desktop runtime (SSH/relay included) replays
them verbatim; no RPC/contract change in either update order. Free text is
newline-sanitized because terminal.send has no paste framing.

Drops the now-unused formatCompleteAskAnswer from the shared module.

* fix native chat send and runtime races

* fix mobile native chat formatting

* fix(native-chat): mobile empty state matches desktop copy

Mobile showed a single generic line ('Send a message to get started') where
desktop shows a titled two-line empty state naming the agent ('Start a chat with
Claude' + 'Ask Claude to inspect code, explain output, or make a change.'). Align
them from one source of truth so they can't drift again:

- Extract the agent-type label map + formatAgentTypeLabel to
  src/shared/agent-type-label.ts (desktop re-exports; mobile imports).
- Add src/shared/native-chat-empty-state.ts with the canonical English copy;
  desktop uses it as its i18n fallbacks (localization unchanged — en/es/ja/ko/zh
  keys still win), mobile substitutes the agent label and renders it directly
  (mobile ships English only).
- Mobile: render title + subtitle for waiting-session AND ready-but-empty (both
  are 'start a chat'), error copy for errors; keep the loading spinner.

Live-verified on the iOS sim against a pn-dev of this branch. typecheck node/web
+ mobile tsc clean; 30 mobile + 428 desktop/shared native-chat tests green.

* style: oxfmt the empty-state parity test (line wrap)

---------

Co-authored-by: Brennan Benson <brennanbenson@Brennans-MacBook-Pro.local>
Co-authored-by: Brennan Benson <79079362+brennanb2025@users.noreply.github.com>
2026-07-16 13:26:15 -07:00