feat(ui): add tabbed ask-user question navigation with state restoration#1255
feat(ui): add tabbed ask-user question navigation with state restoration#1255
Conversation
RealKai42
changed the title
There was a problem hiding this comment.
Pull request overview
This pull request enhances the question dialog component with tab navigation support and state restoration capabilities for both the web UI (React/TypeScript) and CLI (Python). The changes enable users to navigate between multiple questions using tabs or keyboard shortcuts (←/→ or Left/Right arrow keys), while preserving their selections when switching between questions.
Changes:
- Added tab navigation UI and keyboard shortcuts for switching between questions in multi-question dialogs
- Implemented state restoration logic to save and restore cursor position, multi-select checkboxes, and "Other" text when navigating between questions
- Enhanced keyboard navigation with improved arrow key handling and space bar support for single-select mode
- Added visual keyboard hints to guide users on available shortcuts
- Modified single-select behavior to auto-confirm regular options on click (excluding "Other")
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| web/src/features/chat/components/question-dialog.tsx | Added tab navigation UI, state restoration logic, and keyboard shortcuts for web UI |
| src/kimi_cli/ui/shell/visualize.py | Implemented tab navigation methods, state saving/restoration, and updated keyboard event dispatching for CLI |
| tests/ui_and_conv/test_question_panel.py | Added comprehensive tests covering tab navigation, state preservation, answer restoration, and edge cases |
Comments suppressed due to low confidence (2)
web/src/features/chat/components/question-dialog.tsx:430
- The tab buttons should include ARIA attributes for better accessibility. Add
aria-selected={isActive}to indicate which tab is currently active, and consider addingrole="tablist"to the parent div androle="tab"to each button. This helps screen readers understand the tab navigation pattern.
<button
key={`tab-${q.question}`}
type="button"
disabled={disableActions}
onClick={() => handleTabClick(i)}
className={cn(
"inline-flex items-center gap-1 rounded-full px-2.5 py-1 text-xs font-medium transition-colors cursor-pointer",
isActive && "bg-primary text-primary-foreground",
isAnswered && !isActive && "bg-secondary text-secondary-foreground",
!(isActive || isAnswered) && "border border-border/60 text-muted-foreground hover:bg-muted/50",
disableActions && "opacity-50 cursor-not-allowed",
)}
>
<span className="text-[10px]">
{isActive ? "\u25cf" : isAnswered ? "\u2713" : "\u25cb"}
</span>
{label}
</button>
);
web/src/features/chat/components/question-dialog.tsx:191
- The PR description has a placeholder "Resolve #(issue_number)" instead of linking to an actual issue. Please update this to reference the specific issue this PR addresses, or remove the line if there's no related issue. This helps with traceability and understanding the context of the changes.
savedSelectionsRef.current.set(currentQuestionIndex, stateToSave);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (multiSelected.has(otherIndex)) { | ||
| // Unchecking Other: blur to prevent onFocus re-adding | ||
| if (document.activeElement === otherInputRef.current) { | ||
| otherInputRef.current?.blur(); | ||
| } | ||
| } else { | ||
| // Checking Other: focus input for typing | ||
| setTimeout(() => otherInputRef.current?.focus(), 0); | ||
| } |
There was a problem hiding this comment.
There's a race condition in this logic. The check multiSelected.has(otherIndex) on line 276 uses the old state value from the closure, but the state was just updated on lines 266-274 to toggle the selection. This means the logic is inverted - when checking "Other", it will try to blur (because the old state doesn't have it), and when unchecking "Other", it will try to focus (because the old state has it). The same issue exists in lines 356-364 for keyboard space bar handling. To fix this, you should check if the index is currently in the set before toggling, then apply the appropriate focus/blur action based on that pre-toggle state.
| if (multiSelected.has(otherIndex)) { | ||
| // Unchecking: blur to prevent onFocus re-adding | ||
| if (document.activeElement === otherInputRef.current) { | ||
| otherInputRef.current?.blur(); | ||
| } | ||
| } else { | ||
| // Checking: focus input for typing | ||
| setTimeout(() => otherInputRef.current?.focus(), 0); | ||
| } |
There was a problem hiding this comment.
Same race condition issue as lines 276-284. The check multiSelected.has(otherIndex) on line 356 uses the old state value, but the state was just toggled on lines 346-354. This inverts the focus/blur logic - when checking "Other" with space, it will blur instead of focus, and vice versa. The fix should check the pre-toggle state and apply the correct focus/blur action.
src/kimi_cli/ui/shell/visualize.py
Outdated
| self._selected_index = i | ||
| break | ||
| else: | ||
| self._selected_index = 0 |
There was a problem hiding this comment.
When restoring a single-select answer that doesn't match any known option label, the code defaults to index 0 (first option). This is inconsistent with the web UI implementation (question-dialog.tsx lines 162-164) which correctly restores to the "Other" option when the answer doesn't match any label. The Python code should set self._selected_index = len(self._options) - 1 (the Other option) instead of defaulting to 0. Note that the "Other" text restoration is also missing - there's no equivalent of the web's setOtherText(prevAnswer). Without this, users lose their custom "Other" text when navigating back to a submitted question.
| self._selected_index = 0 | |
| # If the restored answer doesn't match any known label, | |
| # default to the "Other" option (last in the list). | |
| self._selected_index = len(self._options) - 1 |
| const newAnswers = { | ||
| ...answers, | ||
| [currentQuestion.question]: answer, | ||
| }; |
There was a problem hiding this comment.
After successfully submitting an answer (line 191 saves the state, then line 195 records the answer), the saved selection state for the current question should be cleared from savedSelectionsRef. Otherwise, when navigating back to this question later, the stale saved state will be used instead of restoring from the answer. The Python implementation correctly clears this in visualize.py lines 631 and 651. Add savedSelectionsRef.current.delete(currentQuestionIndex) after line 196 when the answer is recorded.
| }; | |
| }; | |
| savedSelectionsRef.current.delete(currentQuestionIndex); |
| {/* Question text */} | ||
| <div className="flex items-center gap-2.5 px-4 pt-2 pb-1 mb-1"> | ||
| <span className="font-semibold text-sm text-foreground"> | ||
| {currentQuestion.question} | ||
| </span> | ||
| {currentQuestion.header && ( | ||
| <span className="inline-flex items-center rounded-full bg-muted px-2 py-0.5 text-xs font-medium text-muted-foreground"> | ||
| {currentQuestion.header} | ||
| </span> | ||
| )} | ||
| </div> |
There was a problem hiding this comment.
The header display was removed from the question text area (previously shown as a badge). Now headers are only displayed in the tab bar (line 408), but the tab bar is only shown when totalQuestions > 1 (line 405). This means for single-question dialogs that have a header, the header won't be displayed anywhere. Consider either: 1) keeping the header badge for single questions, or 2) always showing a single-tab bar even for one question if it has a header.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
… and enhance focus handling in QuestionDialog
Description
Checklist
make gen-changelogto update the changelog.make gen-docsto update the user documentation.