Skip to content

feat(cli): improve export format completion navigation - #3701

Merged
wenshao merged 8 commits into
QwenLM:mainfrom
shenyankm:sheny/optimize-export-ux
May 5, 2026
Merged

feat(cli): improve export format completion navigation#3701
wenshao merged 8 commits into
QwenLM:mainfrom
shenyankm:sheny/optimize-export-ux

Conversation

@shenyankm

@shenyankm shenyankm commented Apr 28, 2026

Copy link
Copy Markdown
Contributor

Summary

  • What changed: Improved the interactive /export command completion flow so arrow-key navigation can insert and cycle through export formats (html, md, json, jsonl) directly in the input. The isPerfectMatch + navigated + Enter autocomplete path now applies to all slash commands with sub-commands (e.g., /memory, /agents), not just /export.
  • Why it changed: Selecting an export format from the completion UI should be faster and more discoverable for keyboard-driven CLI users.
  • Reviewer focus: Please verify the /export completion behavior, especially that pressing Enter on plain /export still preserves the existing default behavior when the user has not navigated suggestions. Also verify that /memory + Down + Enter autocompletes the selected sub-command.

Validation

  • Commands run:
    git diff --check
    cd packages/cli
    npx vitest run src/ui/components/InputPrompt.test.tsx
    cd ../..
    npm run dev
  • Prompts / inputs used:
    • Type /export
    • Press Down once
    • Press Down again
    • Press Enter
    • Type /export
    • Press Enter without navigating suggestions
  • Expected result:
    • /export shows html, md, json, and jsonl suggestions.
    • Down-arrow fills /export md, then /export json on the next Down press (no trailing space).
    • Suggestions remain visible while cycling through export formats.
    • Enter submits the selected export command.
    • Enter on plain /export without suggestion navigation keeps the existing default behavior.
  • Observed result:
    • Windows manual validation with npm run dev matched the expected interactive behavior.
    • InputPrompt.test.tsx passed: 122 passed, 2 skipped.
    • git diff --check completed with no whitespace errors.
    • The pre-commit hook also completed Prettier and ESLint checks for the staged files.
  • Quickest reviewer verification path:
    1. Run npm run dev.
    2. Type /export.
    3. Press Down to verify the input becomes /export md.
    4. Press Down again to verify it cycles to /export json.
    5. Press Enter to verify the selected export command is submitted.
    6. Run cd packages/cli && npx vitest run src/ui/components/InputPrompt.test.tsx.
  • Evidence (output, logs, screenshots, video, JSON, before/after, etc.):

Scope / Risk

  • Main risk or tradeoff: This adds /export-specific completion handling inside InputPrompt, so the main risk is accidentally affecting generic slash-command completion or history navigation.
  • The isPerfectMatch + navigated + Enter autocomplete path now fires for any slash command with sub-commands, not just /export. This is intentional: when the user has navigated suggestions via arrow keys, pressing Enter should autocomplete the selected suggestion rather than submit the raw input. A non-/export regression test covers this path.
  • Not covered / not validated: Manual CLI validation was only performed on Windows; macOS and Linux manual validation were not performed.
  • Breaking changes / migration notes: None. This does not change export formats or export file generation logic.

Testing Matrix

macOS Windows Linux
npm run N/A Pass N/A
npx N/A Pass N/A
Docker N/A N/A N/A
Podman N/A N/A N/A
Seatbelt N/A N/A N/A

Testing matrix notes:

  • Windows npm run path was tested manually with npm run dev.
  • Windows npx path was tested with cd packages/cli && npx vitest run src/ui/components/InputPrompt.test.tsx.
  • Docker, Podman, and Seatbelt are not relevant to this focused interactive CLI completion change.
  • macOS and Linux manual validation were not performed.

Linked Issues / Bugs

Closes #3700

@tanzhenxin tanzhenxin added the type/feature-request New feature or enhancement request label Apr 28, 2026
Comment thread packages/cli/src/ui/components/InputPrompt.tsx Outdated
Comment thread packages/cli/src/ui/components/InputPrompt.tsx Outdated
Comment thread packages/cli/src/ui/components/InputPrompt.tsx Outdated
Comment thread packages/cli/src/ui/components/InputPrompt.tsx
Comment thread packages/cli/src/ui/components/InputPrompt.tsx Outdated
Comment thread packages/cli/src/ui/components/InputPrompt.tsx Outdated
Comment thread packages/cli/src/ui/components/InputPrompt.tsx Outdated
Comment thread packages/cli/src/ui/components/InputPrompt.tsx Outdated
Comment thread packages/cli/src/ui/components/InputPrompt.tsx
Critical:
- Guard phase-2 cycling by checking buffer text starts with "/export "
  so a manually edited buffer is never clobbered by stale nav state (C1)
- Derive export format suggestions from slashCommands.subCommands to
  keep a single source of truth with the command registry (C2)
- Reset completionSelectionWasNavigatedRef on showSuggestions rising
  edge instead of on every suggestions change to avoid a race where
  an already-navigated selection is forgotten before Enter (C3)
- Add regression tests for isPerfectMatch + navigated + Enter,
  including the positive path and a control case (C4)

Suggestions:
- Prefix-guard getExportFormatFromInput to skip regex on non-/export
  input (S1)
- Drop trailing space from setExportCompletionInput output so buffer
  text is no longer implicitly coupled to the cycling heuristic (S2)
- Document the two-phase state machine (one-shot fill + cycling) (S3)
- Accept Tab as an additional cycling key alongside Up/Down (S4)
- Remove the unconditional ref reset at the tail of handleInput;
  correctness is now guaranteed by the buffer-text guard (C1) and
  the showSuggestions edge-triggered useEffect (C3) (S5)
@shenyankm

Copy link
Copy Markdown
Contributor Author

Review feedback addressed

Thanks @wenshao for the thorough review. All 4 Critical and 5 Suggestion items have been addressed in 5685e1b4a. Each individual review comment now has an inline reply linking to this commit; a summary follows below.

Critical

# Issue Resolution
C1 Phase-2 cycling could clobber a manually edited buffer because the selection ref was only cleared on submit / Escape / Ctrl+C. Added a buffer-text prefix guard around the cycling branch: cycling only runs when the buffer still looks like /export ….
C2 The hard-coded format list created a three-way coupling with the command registry. The suggestion list and cycling order are now both derived from slashCommands.subCommands; the registry is the single source of truth.
C3 The navigated flag was reset on every suggestions array change, introducing a race where an already-navigated perfect-match selection could be forgotten before Enter. The reset now fires on the rising/falling edge of showSuggestions, not on every suggestions reference change.
C4 The isPerfectMatch + navigated + Enter path had no test coverage. Added three regression tests: the positive path, a control case (perfect match + Enter without prior navigation), and a C1 regression for manual edit after export fill.

Suggestions

# Issue Resolution
S1 The format regex ran on every keystroke, even for unrelated input. Added a cheap startsWith('/export') prefix guard.
S2 The filled buffer had a trailing space, implicitly coupling the cycling heuristic to the slash parser's whitespace handling. Dropped the trailing space; the parser still accepts the value and the cycling heuristic no longer depends on it.
S3 The implicit two-phase state machine (one-shot fill → cycling) lacked documentation. Added a block comment explaining phase-1 vs. phase-2 and why phase-2 uses its own ref.
S4 Tab had no effect on the fill popup after the initial fill. Tab now cycles to the next format, consistent with Up/Down.
S5 The unconditional ref reset at the tail of handleInput collapsed every unhandled key (including Home/End/Ctrl+A) to a state-clearing edge. Removed the unconditional reset; correctness is now maintained by the C1 buffer-text guard and the C3 edge-triggered effect.

Verification

Unit tests (packages/cli): 116 passed, 2 skipped — includes the three new regression tests for C1/C4.

Manual E2E (dev CLI):

# Scenario Expected Result
1 /export → Down (cycle) buffer cycles through html → md → json → jsonl, no trailing space
2 /export md → Ctrl+U → type /help → Down buffer stays /help (cycling guard blocks overwrite)
3 /export md → Home → End → Down cycling continues to /export json (cursor-only keys don't reset state)
4 /export md → Tab cycles to the next format
5 /export → Down (→ md) → Up (→ html) → Enter submits /export html via the autocomplete path
6 /export md → Enter export runs end-to-end

Notes

  • Commit 5685e1b4a is a single follow-up on top of the original PR — not squashed — so the review trail stays visible. Happy to squash on merge if preferred.
  • git rebase upstream/main is pending; will rebase before requesting re-review if the branch falls further behind.

@shenyankm
shenyankm requested a review from wenshao May 4, 2026 00:19

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

测试覆盖缺口

以下代码路径缺少测试覆盖:

  • Phase 1 上箭头hasExportFormatSuggestions 代码块中的 isCompletionUpKey 路径(包括从索引 0 回绕到 lastIndex)无测试。所有导出测试仅使用下箭头 (\u001B[B)。
  • Phase 2 上箭头 — 循环测试仅发送下箭头。上箭头回绕逻辑无测试覆盖。
  • Phase 2 Tab 键isCompletionTabKey 分支完全无测试覆盖。

建议添加对应测试用例以覆盖这些路径。


— deepseek-v4-pro via Qwen Code /review

Comment thread packages/cli/src/ui/components/InputPrompt.tsx Outdated
Comment thread packages/cli/src/ui/components/InputPrompt.tsx
- Phase 2 cycling guard: replace startsWith('/export ') with strict
  getExportFormatFromInput() to prevent overwriting inputs with extra
  arguments (e.g. '/export html --verbose').
- ACCEPT_SUGGESTION in Phase 1 popup: add hasExportFormatSuggestions
  branch so Tab/Enter seeds exportCompletionSelectionIndexRef,
  allowing Phase 2 cycling to continue from the selected format
  (consistent with Up/Down arrow behavior).
- Add 4 regression tests: Phase 1 Up wrap, Phase 2 Up wrap, Tab
  seed + Phase 2 Tab cycle, guard prevents overwriting extra args.

Ref: PR QwenLM#3701 second-round review by wenshao
@shenyankm
shenyankm requested a review from wenshao May 4, 2026 06:22
Comment thread packages/cli/src/ui/components/InputPrompt.tsx Outdated
Comment thread packages/cli/src/ui/components/InputPrompt.tsx Outdated
Comment thread packages/cli/src/ui/components/InputPrompt.tsx Outdated
Comment thread packages/cli/src/ui/components/InputPrompt.tsx Outdated
Comment thread packages/cli/src/ui/components/InputPrompt.tsx Outdated
…rt completion

- S6: use dynamic exportFormatSuggestions.findIndex() for highlight index
  instead of static EXPORT_FORMAT_COMPLETIONS.indexOf()
- S7: derive Phase 2 cycling current index from buffer text via
  getExportFormatFromInput + indexOf, with defensive ref fallback
- S8: extract getNextExportCompletionIndex as module-level pure function;
  cache exportCycleFormats via useMemo to avoid per-keystroke .map()
- S9/S10: add tests for ESC and Ctrl+C reset of export cycling state
@shenyankm

Copy link
Copy Markdown
Contributor Author

Third-round review feedback addressed

Thanks @wenshao for the additional review. All 5 Suggestion items from the third round have been addressed in 17c2b7d48.

Summary

# Issue Resolution
S6 selectedExportFormatIndex used static EXPORT_FORMAT_COMPLETIONS.indexOf() Now uses exportFormatSuggestions.findIndex() for consistency with the rendered list
S7 Phase 2 cycling used stale ref as base index after manual buffer edits Now derives current index from getExportFormatFromInput(buffer.text) + exportCycleFormats.indexOf(), with defensive ref fallback
S8 exportCycleFormats .map() on every keystroke + closure recreation getNextExportCompletionIndex is now a module-level pure function; exportCycleFormats is cached via useMemo
S9 ESC reset of exportCompletionSelectionIndexRef lacked test coverage Added test: enter Phase 2, press ESC, verify Down no longer cycles
S10 Ctrl+C reset lacked test coverage Added test: enter Phase 2, press Ctrl+C, type unrelated command, verify Down does not overwrite

Verification

Unit tests (packages/cli): 122 passed, 2 skipped — includes the two new ESC/Ctrl+C tests and all existing export completion tests.

Typecheck: all workspaces pass (npm run typecheck).

Notes

  • Commit 17c2b7d48 is on top of c42f017ff.
  • Happy to squash the three fix commits on merge.

Comment thread packages/cli/src/ui/components/InputPrompt.tsx Outdated
Comment thread packages/cli/src/ui/components/InputPrompt.tsx Outdated
Comment thread packages/cli/src/ui/components/InputPrompt.tsx Outdated
@shenyankm
shenyankm requested a review from wenshao May 5, 2026 00:29

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Overview

Adds a two-phase state machine so /export + arrow/Tab cycles through html/md/json/jsonl and keeps the suggestion panel persistent. ~210 production lines + ~670 test lines, with 11 explicit regression tests for prior review rounds.

Strengths

  • Test coverage is unusually thorough — almost every edge case I'd flag already has a regression test (ESC, Ctrl+C, manual edit, superset matching, missing-format fallthrough, Tab seeding Phase 2, Up-arrow wrap, trailing whitespace, extra-args guard).
  • Format list is derived from slashCommands.subCommands so adding a new export sub-command picks up cycling automatically; the static fallback is only used during early renders.
  • getExportFormatFromInput is a strict whitespace-trimmed parser that correctly prevents Phase 2 from clobbering inputs like /export html --verbose.
  • Documentation comments explain why (invariants like "the two phases are mutually exclusive", "do NOT widen hasExportFormatSuggestions") rather than just what — unusually good for this codebase.

Issues raised inline

  1. Test typos (must fix)\u001B[B] (stray ]) at three places in the test file.
  2. Perfect-match Enter behavior change is broader than /export — affects any slash command with sub-commands; not documented in the PR body.
  3. PR description / code mismatch — description says /export md (trailing space), code writes /export md (no trailing space).
  4. Ctrl+U missing from the ref-reset set.
  5. UX asymmetry — users who type /export <fmt> directly don't get cycling because the ref is only ever seeded by Phase-1 setExportCompletionInput.

Smaller notes (non-blocking)

  • EXPORT_FORMAT_COMPLETIONS (line 78) duplicates the slashCommands source of truth as a static fallback; if a format is ever removed from exportCommand.ts but not here, the fallback will silently re-introduce it during early renders.
  • getExportFormatFromInput is exported but no consumer outside this file uses it — if it's only for testability, prefer keeping it module-private.
  • The "should not clobber manually edited buffer" test depends on useTextBuffer actually applying \u0015 (Ctrl+U). An expect(buffer.text).toBe('') between the Ctrl+U and /help writes would pin the intermediate state so a future hook change doesn't make the test pass for the wrong reason.

Risk

  • Correctness: medium-low — well-tested for /export, but the broader perfect-match Enter change deserves explicit validation against /memory, /agents, etc.
  • Regression: low for /export; low-medium for other slash commands with sub-commands.
  • Performance: negligible.

Comment thread packages/cli/src/ui/components/InputPrompt.test.tsx Outdated
Comment thread packages/cli/src/ui/components/InputPrompt.test.tsx Outdated
Comment thread packages/cli/src/ui/components/InputPrompt.test.tsx Outdated
Comment thread packages/cli/src/ui/components/InputPrompt.tsx Outdated
Comment thread packages/cli/src/ui/components/InputPrompt.tsx
Comment thread packages/cli/src/ui/components/InputPrompt.tsx Outdated
Comment thread packages/cli/src/ui/components/InputPrompt.tsx Outdated
- S1: Strengthen EXPORT_FORMAT_COMPLETIONS fallback comment with
  an IMPORTANT sync warning for format removals
- S2: De-export getExportFormatFromInput (no external consumers)
- S3: Add intermediate buffer-clear assertion after Ctrl+U in test
  to pin state and prevent false positives from future hook changes

@shenyankm shenyankm left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

All issues and smaller notes from this review have been addressed in cd40dae:

Issues resolved:

  1. Test typos — ANSI escape sequence stray ] fixed across 3 locations.
  2. Perfect-match Enter doc — PR body updated to document that the isPerfectMatch + navigated + Enter path applies to all slash commands with sub-commands.
  3. PR description trailing space — All /export md references corrected to /export md (no trailing space).
  4. Ctrl+U ref-reset — Added exportCompletionSelectionIndexRef.current = null in Ctrl+U handler (L1370-1371).
  5. UX asymmetry — Added useEffect that seeds exportCompletionSelectionIndexRef from getExportFormatFromInput(buffer.text) so manually typing /export <fmt> also enters Phase 2 cycling.

Smaller notes addressed:

  • S1: Strengthened the EXPORT_FORMAT_COMPLETIONS fallback comment with an IMPORTANT sync warning for format removals.
  • S2: De-exported getExportFormatFromInput (confirmed zero external consumers).
  • S3: Added intermediate expect(...).not.toContain('/export') assertion after Ctrl+U to pin buffer-clear state.

Tests: 127 passed, 2 skipped. TypeScript: no new errors in changed files.

@shenyankm
shenyankm requested a review from wenshao May 5, 2026 04:33
@wenshao

wenshao commented May 5, 2026

Copy link
Copy Markdown
Collaborator

Overview

This PR introduces a two-phase completion state machine for the /export command:

  • Phase 1: User types /export, format list pops up (html/md/json/jsonl); pressing Down or Tab fills the first format.
  • Phase 2: After the popup closes and the buffer becomes /export <fmt>, Up/Down/Tab keep cycling through formats; the suggestion panel stays visible.

It also incidentally changes shared logic: when isPerfectMatch + user has navigated + Enter, autocomplete the active suggestion instead of submitting the raw buffer. This rule applies to all slash commands with sub-commands (/memory, /agents, etc.).


Strengths

  • Test coverage is unusually thorough: 16+ new test cases regress every issue raised across rounds (ESC, Ctrl+C, Ctrl+U, manual edit, superset matching, missing-format fallthrough, Tab seeding Phase 2, Up/Down wrap, trailing whitespace, --verbose extra-args guard).
  • Format list derived from slashCommands.subCommands (InputPrompt.tsx:103-122); exportCommand.ts is the SSOT, and adding a new format automatically enables cycling.
  • getExportFormatFromInput uses strict trim + slice + explicit space check, correctly rejecting /export html --verbose from Phase-2 cycling (InputPrompt.tsx:37-56).
  • Comments explain Why, not What — invariants like "the two phases are mutually exclusive, do NOT widen hasExportFormatSuggestions" are unusually well-documented for this repo.

Issues to discuss

1. Architectural coupling: ~310 lines of /export-specific logic in a generic input component

InputPrompt.tsx is the shared input component for every input scenario. This PR adds:

  • Module-level constants: EXPORT_COMMAND_INPUT, EXPORT_FORMAT_COMPLETIONS
  • Module-level functions: getExportFormatFromInput, getNextExportCompletionIndex
  • Component-level refs: exportCompletionSelectionIndexRef, completionSelectionWasNavigatedRef, prevShowSuggestionsRef
  • Component-level useMemo / useEffect: exportFormatSuggestions, exportCycleFormats, seeding effect, navigated-flag reset effect
  • A large block in handleInput: hasExportFormatSuggestions, getExportIndexForActiveSuggestion, setExportCompletionInput, acceptActiveCompletionSuggestion, Phase-2 guard
  • 5 displayed* derived props in render

Suggestion: Extract into useExportCompletion(buffer, slashCommands) returning { shouldShowFormatSuggestions, displayedSuggestions, displayedActiveIndex, handleArrowOrTab, handlePerfectMatchEnter, resetCyclingState }. InputPrompt would only call the hook and dispatch — no specialized state. Benefits:

  • The hook can be unit-tested in isolation, without rendering the whole InputPrompt.
  • Future cycling for /agents, /memory could reuse the hook instead of duplicating the state machine.
  • InputPrompt.tsx would grow ~50 net lines (hook call + two dispatch points) instead of ~310.

Non-blocking, but worth considering as long-term maintenance cost.

2. exportCompletionSelectionIndexRef's value is never read — it's used purely as a boolean sentinel

Tracing every usage:

  • Writes: setExportCompletionInput (writes index), seeding effect (writes index)
  • Reads: shouldKeepExportFormatSuggestions checks !== null, Phase-2 guard checks !== nullboth only test for null

The actual currentIndex during cycling is recomputed from exportCycleFormats.indexOf(parsedFormat) (InputPrompt.tsx:286); the ref's numeric value is never read.

Suggestion: Rename to exportCyclingActiveRef = useRef(false). Clearer semantics; one less "why is the index stored but unused" question.

3. The navigated-flag reset effect's if/else if is redundant

InputPrompt.tsx:157-166:

useEffect(() => {
  if (!completion.showSuggestions) {
    completionSelectionWasNavigatedRef.current = false;
  } else if (!prevShowSuggestionsRef.current) {
    completionSelectionWasNavigatedRef.current = false;
  }
  prevShowSuggestionsRef.current = completion.showSuggestions;
}, [completion.showSuggestions]);

Deps only include completion.showSuggestions, so the effect only fires on true↔false transitions:

  • false → true: !showSuggestions is false, !prev (prev was false) is true → reset
  • true → false: !showSuggestions is true → reset

Both paths reset. Can simplify to:

useEffect(() => {
  completionSelectionWasNavigatedRef.current = false;
}, [completion.showSuggestions]);

prevShowSuggestionsRef can be deleted entirely. Pure simplification.

4. EXPORT_FORMAT_COMPLETIONS static fallback's actual benefit is unclear

InputPrompt.tsx:24 and 117-121. The comment says "for early renders before slashCommands wired up". But slashCommands is a required field in InputPromptProps, supplied by the parent at construction time; all production callers already pass the full command list.

Problem: The fallback duplicates the source of truth in exportCommand.ts. If someone removes jsonl but forgets to sync this constant, the fallback silently re-introduces it. The IMPORTANT-tagged comment can warn but can't prevent drift.

Suggestion: Drop the fallback. Let exportFormatSuggestions return an empty array when slashCommands is unwired; hasExportFormatSuggestions naturally won't activate. If the author can identify a real code path where this branch fires (some early render scenario where slashCommands isn't ready yet), then it's worth keeping.

5. The perfect-match Enter behavior change reaches further than the PR body suggests (please add a test)

The PR body acknowledges this rule applies to all slash commands with sub-commands. But there's an edge case without coverage:

Scenario: User types /memory, presses Down once (flag → true), presses Backspace to /memor, types y back to /memory, presses Enter.

  • Key question: while at /memor, does the popup hide? If yes → reset effect clears the flag, Enter submits as expected. If no (the popup still shows "memory" as a partial match) → flag stays true, and Enter would autocomplete the first sub-command instead of submitting /memory.

This depends on useCommandCompletion's showSuggestions behavior during partial matches. If the popup persists across edits, the flag becomes "sticky" and produces unexpected Enter behavior.

Suggestion: Add a test simulating "navigate → backspace → retype → Enter" to verify the popup-visibility transitions clear state correctly. If stickiness reproduces, also clear the flag on buffer.text changes.

6. Local closures inside handleInput rebuild on every keystroke

acceptActiveCompletionSuggestion, setExportCompletionInput, getExportIndexForActiveSuggestion, hasExportFormatSuggestions are all local values/functions inside the handleInput useCallback.

Performance impact is negligible (closures are cheap), but it makes handleInput even bigger. Combined with #1, extracting to the hook eliminates this naturally.

7. Four parallel ternaries in render can be unified

InputPrompt.tsx:473-484. displayedSuggestions / displayedActiveSuggestionIndex / displayedSuggestionsScrollOffset / displayedSuggestionsLoading are four independent ternaries gated on the same shouldKeepExportFormatSuggestions.

Combine into a single displayProps object ternary:

const suggestionDisplayProps = shouldKeepExportFormatSuggestions
  ? { suggestions: exportFormatSuggestions, activeIndex: selectedExportFormatIndex, isLoading: false, scrollOffset: 0 }
  : { suggestions: activeCompletion.suggestions, activeIndex: activeCompletion.activeSuggestionIndex, isLoading: activeCompletion.isLoadingSuggestions, scrollOffset: activeCompletion.visibleStartIndex };

JSX becomes <SuggestionsDisplay {...suggestionDisplayProps} ... />. Pure cleanup.


Risk Assessment

Dimension Rating Notes
Correctness Medium-Low /export path is well-tested; the perfect-match Enter change for /memory, /agents edge cases (see #5) needs more validation.
Regression risk Low Phase-2 guard uses strict format parsing; external inputs (/help, /export html --verbose) are not silently overwritten.
Performance Negligible Seeding effect runs startsWith + slice on every buffer.text change, O(1).
Maintenance cost Medium-High Single file +366 lines of specialized logic, 3 refs, 2 effects, 1 useMemo — #1 hook extraction would significantly lower this.

Recommended merge strategy

Overall the PR is functionally correct and well-tested, and can be merged. But I strongly recommend a follow-up to extract the export-completion logic into a useExportCompletion hook — otherwise future similar UX work for /agents / /memory will be tempted to copy this two-phase state machine.

Blocking item: #5 (navigate → backspace → retype → Enter) — please add a test to confirm behavior.

Non-blocking cleanups: #2, #3, #4, #7 — pure cleanup, can ship together in follow-up.


中文版本(点击展开)

概览

这个 PR 给 /export 命令引入了一个两阶段补全状态机

  • Phase 1:用户输入 /export,弹出格式列表(html/md/json/jsonl),按 Down 或 Tab 自动填入第一个格式。
  • Phase 2:弹窗关闭后,buffer 变成 /export <fmt>,继续按 Up/Down/Tab 在格式间循环切换;建议面板持续可见。

同时顺带修改了通用逻辑:当 isPerfectMatch + 用户已经按过箭头 + Enter 时,自动补全选中项而不是提交原始 buffer。这条规则适用于所有含 sub-command 的 slash 命令(/memory/agents 等)。


优点

  • 测试覆盖罕见地完整:16+ 个新测试点逐个回归 review 提出的问题(ESC、Ctrl+C、Ctrl+U、手动编辑、superset 匹配、缺失格式 fallthrough、Tab 种入 Phase 2、上下箭头回绕、尾部空格、--verbose 额外参数防误覆盖)。
  • 格式列表从 slashCommands.subCommands 派生InputPrompt.tsx:103-122),exportCommand.ts 是 SSOT;新增格式自动启用循环。
  • getExportFormatFromInput 用严格的 trim + 切片 + 显式空格判断,正确把 /export html --verbose 排除在 Phase-2 循环外(InputPrompt.tsx:37-56)。
  • 注释解释 Why 而不是 What:例如"两个 phase 是互斥的,不要扩大 hasExportFormatSuggestions"——这种 invariant 注释在该仓中不常见。

待商榷的问题

1. 架构耦合:把 ~310 行 /export 特化逻辑塞进通用输入组件

InputPrompt.tsx 是所有输入场景共享的组件。本次新增的代码里:

  • 模块级常量:EXPORT_COMMAND_INPUTEXPORT_FORMAT_COMPLETIONS
  • 模块级函数:getExportFormatFromInputgetNextExportCompletionIndex
  • 组件内 ref:exportCompletionSelectionIndexRefcompletionSelectionWasNavigatedRefprevShowSuggestionsRef
  • 组件内 useMemo / useEffect:exportFormatSuggestionsexportCycleFormats、seeding effect、navigated-flag reset effect
  • handleInput 内大段:hasExportFormatSuggestionsgetExportIndexForActiveSuggestionsetExportCompletionInputacceptActiveCompletionSuggestion、Phase-2 guard
  • 渲染层:shouldKeepExportFormatSuggestions 等 5 个 displayed* 派生 props

建议:抽成 useExportCompletion(buffer, slashCommands) 自定义 hook,返回 { shouldShowFormatSuggestions, displayedSuggestions, displayedActiveIndex, handleArrowOrTab, handlePerfectMatchEnter, resetCyclingState }InputPrompt 只调用 hook、不持有特化状态。这样:

  • 该 hook 可以独立单测,不需要渲染整个 InputPrompt
  • 未来 /agents/memory 想做类似的循环,能复用 hook 而不是再复制一份。
  • InputPrompt.tsx 净增 ~50 行(hook 调用 + 两处 dispatch),而不是 ~310 行。

非阻塞,但作为长期维护成本值得考虑。

2. exportCompletionSelectionIndexRef 的值从未被读取,只当布尔哨兵用

通读所有用法:

  • 设值:setExportCompletionInput(写 index)、seeding effect(写 index)
  • 读值:shouldKeepExportFormatSuggestions!== null,Phase-2 guard 用 !== null两处都只判 null

而循环时的 currentIndex 是从 exportCycleFormats.indexOf(parsedFormat) 现算的(InputPrompt.tsx:286),不读 ref 里的数字

建议:把 ref 改成 exportCyclingActiveRef = useRef(false),语义更清楚。能少一处"为什么存了 index 又不用"的认知负担。

3. navigated-flag reset effect 的 if/else if 是冗余的

InputPrompt.tsx:157-166

useEffect(() => {
  if (!completion.showSuggestions) {
    completionSelectionWasNavigatedRef.current = false;
  } else if (!prevShowSuggestionsRef.current) {
    completionSelectionWasNavigatedRef.current = false;
  }
  prevShowSuggestionsRef.current = completion.showSuggestions;
}, [completion.showSuggestions]);

deps 只有 completion.showSuggestions,所以 effect 只在 true↔false 转换时才会跑:

  • false → true:!showSuggestions 假,!prev(prev 是 false)真 → reset
  • true → false:!showSuggestions 真 → reset

两条路径都是 reset。可以直接:

useEffect(() => {
  completionSelectionWasNavigatedRef.current = false;
}, [completion.showSuggestions]);

prevShowSuggestionsRef 整个可以删掉。纯简化。

4. EXPORT_FORMAT_COMPLETIONS 静态 fallback 的实际收益不明

InputPrompt.tsx:24117-121:注释说"用于早期 render——slashCommands 还没接入时"。但 slashCommandsInputPromptProps 必填字段,由父组件构造时传入;所有 production 调用都已经带着完整的命令列表。

问题:fallback 跟 exportCommand.ts 里的 subCommands 列表是双重维护——如果有人删掉 jsonl 但忘了同步这里,fallback 会"复活"它。注释里加了大写 IMPORTANT 提醒,但提醒不能阻止漂移。

建议:直接去掉 fallback,让 exportFormatSuggestions 在 slashCommands 没接入时返回空数组;hasExportFormatSuggestions 自然不触发。如果作者能给出一个真实的早期 render 场景(哪条 code path 会在 slashCommands 还没准备好时进入这段循环逻辑),再保留也不迟。

5. perfect-match Enter 行为变更影响范围比 PR 描述更广(建议补测试)

PR body 已经说明这条变更适用于所有有 subCommands 的 slash 命令。但有一个边缘场景测试没覆盖:

场景:用户输入 /memory,按 Down 一次(ref 翻 true),按 Backspace 退到 /memor,再输入 y 回到 /memory,按 Enter。

  • 关键问题:在 /memor 阶段,popup 是否消失?若消失 → ref 被 reset effect 清掉,行为正常提交;若仍显示 "memory" 这一条建议(不消失) → ref 还是 true,Enter 会自动补全成第一个 sub-command 而不是提交 /memory

这取决于 useCommandCompletion 在部分匹配时的 showSuggestions 行为。如果 popup 在跨编辑过程中持续可见,ref 会"粘住",导致 Enter 行为出乎意料。

建议:加一个测试模拟"navigate → backspace → 重新输入 → Enter",验证 popup visibility 翻转能正确清状态;如果能复现"粘住",则要在 buffer.text 变化时也加一道清理。

6. handleInput 内的局部闭包每次按键都会重建

acceptActiveCompletionSuggestionsetExportCompletionInputgetExportIndexForActiveSuggestionhasExportFormatSuggestions 都是 handleInput useCallback 内部的局部值/函数。

性能影响微乎其微(闭包很便宜),但可读性上让 handleInput 进一步膨胀。配合第 1 条建议,抽到 hook 后这部分自然也就出去了。

7. 渲染层 4 个并列 ternary 可聚合

InputPrompt.tsx:473-484displayedSuggestions / displayedActiveSuggestionIndex / displayedSuggestionsScrollOffset / displayedSuggestionsLoading 4 个独立 ternary 共用同一个 shouldKeepExportFormatSuggestions gate。

可以聚成单个 displayProps 对象的 ternary:

const suggestionDisplayProps = shouldKeepExportFormatSuggestions
  ? { suggestions: exportFormatSuggestions, activeIndex: selectedExportFormatIndex, isLoading: false, scrollOffset: 0 }
  : { suggestions: activeCompletion.suggestions, activeIndex: activeCompletion.activeSuggestionIndex, isLoading: activeCompletion.isLoadingSuggestions, scrollOffset: activeCompletion.visibleStartIndex };

JSX 里 <SuggestionsDisplay {...suggestionDisplayProps} ... />。纯结构清理。


风险评估

维度 评级 说明
正确性 中-低 /export 路径测试充分;perfect-match Enter 改动对 /memory/agents 的边缘场景(见第 5 条)需要再验一次。
回归风险 Phase-2 guard 用严格的格式解析,外部输入(/help/export html --verbose)不会被误覆盖。
性能 可忽略 seeding effect 每次 buffer.text 变化跑一次 startsWith + slice,O(1)。
维护成本 中-高 单一文件 +366 行特化逻辑、3 个 ref、2 个 effect、1 个 useMemo——第 1 条的 hook 抽离能显著降低这部分。

建议合并策略

总体上 PR 的功能正确、测试到位,可以合并;但强烈建议在 follow-up 里把 export 完成逻辑抽到 useExportCompletion hook——否则未来给 /agents / /memory 做类似 UX 时会有"复制一遍这套两阶段状态机"的诱惑。

阻塞项:第 5 条(navigate → backspace → 重新输入 → Enter)建议补一个测试,确认行为符合预期。

非阻塞优化:第 2、3、4、7 条都是纯清理,可以一起放进 follow-up。

Address all feedback from PR QwenLM#3701 review comment:
- Extract ~310 lines of /export state machine from InputPrompt into
  dedicated useExportCompletion hook
- Replace exportCompletionSelectionIndexRef (number|null) with
  cyclingActiveRef (boolean) since index was never read
- Simplify navigated-flag lifecycle: reset on buffer.text changes
  instead of popup visibility transitions; add navigatedTextRef
  snapshot to prevent sticky autocomplete after buffer edits
- Remove static EXPORT_FORMAT_COMPLETIONS fallback; derive entirely
  from slashCommands.subCommands
- Aggregate 4 parallel ternaries into single suggestionDisplayProps
- Add regression test: navigate + backspace + retype + Enter
  should submit raw buffer, not autocomplete
- Remove redundant navigatedRef reset in ESC handler (already
  covered by exportCompletion.reset())
@shenyankm

Copy link
Copy Markdown
Contributor Author

All seven items from this round's review have been addressed in commit 4cb640a on the sheny/optimize-export-ux branch.

Items resolved

#1 — Hook extraction
The export state machine has been extracted from InputPrompt.tsx into useExportCompletion.ts hook. InputPrompt now only calls the hook and dispatches — zero specialized state remains.

#2 — Boolean ref rename
exportCompletionSelectionIndexRef<number|null> replaced with cyclingActiveRef<boolean>. The cycling index is derived from getExportFormatFromInput(buffer.text, exportCycleFormats) at keystroke time.

#3 — Simplified navigated-flag lifecycle ✅ (improved)
Instead of tracking popup visibility transitions (prevShowSuggestionsRef), the navigated flag now resets on every buffer.text change. This eliminates the redundant if/else if and directly addresses the root cause of #5.

#4 — Removed static fallback
EXPORT_FORMAT_COMPLETIONS deleted. When slashCommands lacks /export, exportFormatSuggestions returns [].

#5 — Regression test ✅ (blocking item)
Added test: "should submit directly on Enter after arrow-navigate + backspace + retype to perfect match". A navigatedTextRef snapshot guard additionally prevents stale navigation state after buffer edits.

#6 — Closure rebuilding
Resolved naturally by hook extraction. setExportCompletionInput, getExportIndexForActiveSuggestion are now stable useCallback refs within the hook.

#7 — Ternary aggregation
Four parallel ternaries aggregated into single suggestionDisplayProps object with ?? fallback to generic completion state.

Bonus fix

Removed redundant exportCompletion.navigatedRef.current = false in ESC handler (already covered by exportCompletion.reset()).

Validation

  • Tests: 128 passed, 2 skipped
  • TypeScript: zero new errors in changed files
  • Pre-commit: Prettier + ESLint passed

Ready for re-review.

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

No issues found. LGTM! ✅ — gpt-5.5 via Qwen Code /review

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] useExportCompletion hook 缺少独立的单元测试文件。所有覆盖均来自 InputPrompt 集成测试(17+ 用例),但纯函数 getExportFormatFromInputgetNextExportCompletionIndex 的边界情况、ref 逻辑及 useEffect 播种行为均未做直接测试。建议创建 useExportCompletion.test.ts 覆盖至少以下场景:

  • getExportFormatFromInput 边界(空输入、无空格、无格式名、非法格式名)
  • getNextExportCompletionIndex 边界(空列表、越界索引、单元素列表)
  • 播种 effect:验证手动输入 /export <fmt>cyclingActiveRef 为 true
  • reset() 清除所有 ref
  • suggestionDisplayProps 在非 cycling 状态时为 null

— deepseek-v4-pro via Qwen Code /review

Comment thread packages/cli/src/ui/hooks/useExportCompletion.ts Outdated
Comment thread packages/cli/src/ui/hooks/useExportCompletion.ts
Comment thread packages/cli/src/ui/components/InputPrompt.tsx Outdated
@shenyankm

Copy link
Copy Markdown
Contributor Author

Addressed the latest review feedback in 0b3d834d6.

Changes made:

  • Added focused useExportCompletion.test.ts coverage for helper boundaries, reset/ref behavior, user-edit seeding, suggestion props, and return-object stability.
  • Memoized useExportCompletion display props and returned object.
  • Prevented arbitrary/history/programmatic /export <fmt> buffer text from arming phase-2 cycling.
  • Gave command search and reverse search suggestions priority over export suggestions.

Validation run:

  • cd packages/cli && npx vitest run src/ui/hooks/useExportCompletion.test.ts
  • cd packages/cli && npx vitest run src/ui/components/InputPrompt.test.tsx
  • cd packages/cli && npx eslint src/ui/hooks/useExportCompletion.ts src/ui/hooks/useExportCompletion.test.ts src/ui/components/InputPrompt.tsx src/ui/components/InputPrompt.test.tsx
  • npm run typecheck
  • npm run build
  • git diff --check

@shenyankm
shenyankm requested a review from wenshao May 5, 2026 15:14

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

新一轮反馈都已落地:useExportCompletion.test.ts 的 20 个用例覆盖了纯函数边界和 hook 行为;markNextTextChangeAsUserInput 握手避免了 history/programmatic setText 误触发 phase-2;reverse/command-search 已优先于 export 面板;返回对象和 display props 都做了 memoize。本地试合并到当前 main 后 150 passed / 1 skipped,CI 三平台全绿。

@wenshao
wenshao merged commit 89cb326 into QwenLM:main May 5, 2026
13 checks passed
@shenyankm
shenyankm deleted the sheny/optimize-export-ux branch May 6, 2026 03:39
DragonnZhang pushed a commit that referenced this pull request May 8, 2026
* feat(cli): improve export format completion navigation

* fix(cli): address PR #3701 review feedback on /export completion

Critical:
- Guard phase-2 cycling by checking buffer text starts with "/export "
  so a manually edited buffer is never clobbered by stale nav state (C1)
- Derive export format suggestions from slashCommands.subCommands to
  keep a single source of truth with the command registry (C2)
- Reset completionSelectionWasNavigatedRef on showSuggestions rising
  edge instead of on every suggestions change to avoid a race where
  an already-navigated selection is forgotten before Enter (C3)
- Add regression tests for isPerfectMatch + navigated + Enter,
  including the positive path and a control case (C4)

Suggestions:
- Prefix-guard getExportFormatFromInput to skip regex on non-/export
  input (S1)
- Drop trailing space from setExportCompletionInput output so buffer
  text is no longer implicitly coupled to the cycling heuristic (S2)
- Document the two-phase state machine (one-shot fill + cycling) (S3)
- Accept Tab as an additional cycling key alongside Up/Down (S4)
- Remove the unconditional ref reset at the tail of handleInput;
  correctness is now guaranteed by the buffer-text guard (C1) and
  the showSuggestions edge-triggered useEffect (C3) (S5)

* fix(cli): tighten export completion cycling guard and unify Tab behavior

- Phase 2 cycling guard: replace startsWith('/export ') with strict
  getExportFormatFromInput() to prevent overwriting inputs with extra
  arguments (e.g. '/export html --verbose').
- ACCEPT_SUGGESTION in Phase 1 popup: add hasExportFormatSuggestions
  branch so Tab/Enter seeds exportCompletionSelectionIndexRef,
  allowing Phase 2 cycling to continue from the selected format
  (consistent with Up/Down arrow behavior).
- Add 4 regression tests: Phase 1 Up wrap, Phase 2 Up wrap, Tab
  seed + Phase 2 Tab cycle, guard prevents overwriting extra args.

Ref: PR #3701 second-round review by wenshao

* fix(cli): address PR #3701 third-round review feedback on /export completion

- S6: use dynamic exportFormatSuggestions.findIndex() for highlight index
  instead of static EXPORT_FORMAT_COMPLETIONS.indexOf()
- S7: derive Phase 2 cycling current index from buffer text via
  getExportFormatFromInput + indexOf, with defensive ref fallback
- S8: extract getNextExportCompletionIndex as module-level pure function;
  cache exportCycleFormats via useMemo to avoid per-keystroke .map()
- S9/S10: add tests for ESC and Ctrl+C reset of export cycling state

* fix(cli): tighten /export prefix guard, add superset matching fallthrough, and improve documentation

* fix(cli): address review #4224860127 - smaller notes optimization

- S1: Strengthen EXPORT_FORMAT_COMPLETIONS fallback comment with
  an IMPORTANT sync warning for format removals
- S2: De-export getExportFormatFromInput (no external consumers)
- S3: Add intermediate buffer-clear assertion after Ctrl+U in test
  to pin state and prevent false positives from future hook changes

* refactor(cli): extract export completion into useExportCompletion hook

Address all feedback from PR #3701 review comment:
- Extract ~310 lines of /export state machine from InputPrompt into
  dedicated useExportCompletion hook
- Replace exportCompletionSelectionIndexRef (number|null) with
  cyclingActiveRef (boolean) since index was never read
- Simplify navigated-flag lifecycle: reset on buffer.text changes
  instead of popup visibility transitions; add navigatedTextRef
  snapshot to prevent sticky autocomplete after buffer edits
- Remove static EXPORT_FORMAT_COMPLETIONS fallback; derive entirely
  from slashCommands.subCommands
- Aggregate 4 parallel ternaries into single suggestionDisplayProps
- Add regression test: navigate + backspace + retype + Enter
  should submit raw buffer, not autocomplete
- Remove redundant navigatedRef reset in ESC handler (already
  covered by exportCompletion.reset())

* fix(cli): guard export completion state
xaelistic pushed a commit to xaelistic/qwen-code that referenced this pull request Jun 7, 2026
Co-authored-by: Jacob Richman <jacob314@gmail.com>
xaelistic pushed a commit to xaelistic/qwen-code that referenced this pull request Jun 7, 2026
* feat(cli): improve export format completion navigation

* fix(cli): address PR QwenLM#3701 review feedback on /export completion

Critical:
- Guard phase-2 cycling by checking buffer text starts with "/export "
  so a manually edited buffer is never clobbered by stale nav state (C1)
- Derive export format suggestions from slashCommands.subCommands to
  keep a single source of truth with the command registry (C2)
- Reset completionSelectionWasNavigatedRef on showSuggestions rising
  edge instead of on every suggestions change to avoid a race where
  an already-navigated selection is forgotten before Enter (C3)
- Add regression tests for isPerfectMatch + navigated + Enter,
  including the positive path and a control case (C4)

Suggestions:
- Prefix-guard getExportFormatFromInput to skip regex on non-/export
  input (S1)
- Drop trailing space from setExportCompletionInput output so buffer
  text is no longer implicitly coupled to the cycling heuristic (S2)
- Document the two-phase state machine (one-shot fill + cycling) (S3)
- Accept Tab as an additional cycling key alongside Up/Down (S4)
- Remove the unconditional ref reset at the tail of handleInput;
  correctness is now guaranteed by the buffer-text guard (C1) and
  the showSuggestions edge-triggered useEffect (C3) (S5)

* fix(cli): tighten export completion cycling guard and unify Tab behavior

- Phase 2 cycling guard: replace startsWith('/export ') with strict
  getExportFormatFromInput() to prevent overwriting inputs with extra
  arguments (e.g. '/export html --verbose').
- ACCEPT_SUGGESTION in Phase 1 popup: add hasExportFormatSuggestions
  branch so Tab/Enter seeds exportCompletionSelectionIndexRef,
  allowing Phase 2 cycling to continue from the selected format
  (consistent with Up/Down arrow behavior).
- Add 4 regression tests: Phase 1 Up wrap, Phase 2 Up wrap, Tab
  seed + Phase 2 Tab cycle, guard prevents overwriting extra args.

Ref: PR QwenLM#3701 second-round review by wenshao

* fix(cli): address PR QwenLM#3701 third-round review feedback on /export completion

- S6: use dynamic exportFormatSuggestions.findIndex() for highlight index
  instead of static EXPORT_FORMAT_COMPLETIONS.indexOf()
- S7: derive Phase 2 cycling current index from buffer text via
  getExportFormatFromInput + indexOf, with defensive ref fallback
- S8: extract getNextExportCompletionIndex as module-level pure function;
  cache exportCycleFormats via useMemo to avoid per-keystroke .map()
- S9/S10: add tests for ESC and Ctrl+C reset of export cycling state

* fix(cli): tighten /export prefix guard, add superset matching fallthrough, and improve documentation

* fix(cli): address review #4224860127 - smaller notes optimization

- S1: Strengthen EXPORT_FORMAT_COMPLETIONS fallback comment with
  an IMPORTANT sync warning for format removals
- S2: De-export getExportFormatFromInput (no external consumers)
- S3: Add intermediate buffer-clear assertion after Ctrl+U in test
  to pin state and prevent false positives from future hook changes

* refactor(cli): extract export completion into useExportCompletion hook

Address all feedback from PR QwenLM#3701 review comment:
- Extract ~310 lines of /export state machine from InputPrompt into
  dedicated useExportCompletion hook
- Replace exportCompletionSelectionIndexRef (number|null) with
  cyclingActiveRef (boolean) since index was never read
- Simplify navigated-flag lifecycle: reset on buffer.text changes
  instead of popup visibility transitions; add navigatedTextRef
  snapshot to prevent sticky autocomplete after buffer edits
- Remove static EXPORT_FORMAT_COMPLETIONS fallback; derive entirely
  from slashCommands.subCommands
- Aggregate 4 parallel ternaries into single suggestionDisplayProps
- Add regression test: navigate + backspace + retype + Enter
  should submit raw buffer, not autocomplete
- Remove redundant navigatedRef reset in ESC handler (already
  covered by exportCompletion.reset())

* fix(cli): guard export completion state
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type/feature-request New feature or enhancement request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve /export format selection with keyboard navigation

3 participants