feat(chat): render inline question tags from the agent in chat#5406
feat(chat): render inline question tags from the agent in chat#5406emir-karabeg wants to merge 3 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview
Also exports Reviewed by Cursor Bugbot for commit 8a9be60. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8a9be60. Configure here.
| type='button' | ||
| variant='ghost' | ||
| onClick={() => goToStep(step + 1)} | ||
| disabled={isLast || answers[step] === null} |
There was a problem hiding this comment.
Inert multi-step hides later prompts
Medium Severity
When QuestionDisplay is inert (onSelect omitted on non-latest messages), the forward stepper stays disabled because answers[step] is always null, so multi-step <question> tags only show the first prompt. Inert <options> tags still list every choice, so historical batches lose questions 2+.
Reviewed by Cursor Bugbot for commit 8a9be60. Configure here.
There was a problem hiding this comment.
Valid — fixed in a6890d3. Inert renders now leave the forward chevron enabled so all prompts in a historical batch stay browsable; interactive renders still gate forward movement on the current question being answered.
| data, | ||
| next.map((a) => a ?? '') | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Earlier edits keep stale answers
Medium Severity
In a multi-step batch, changing an answer on an earlier step updates only that index in answers. Later steps keep prior selections, and the user can step forward via the chevrons and submit from the last question without revisiting them, so formatQuestionAnswerMessage may combine a new early answer with outdated later ones.
Reviewed by Cursor Bugbot for commit 8a9be60. Configure here.
There was a problem hiding this comment.
Intended behavior. Back-navigation is a review/edit affordance: changing an earlier answer shouldn't discard later answers the user already made deliberately (they stay visibly highlighted when stepping through, standard wizard semantics). Submit only fires by explicitly answering the last question, so nothing is sent without the user's final action.
Greptile SummaryThis PR adds a new
Confidence Score: 4/5Safe to merge; the feature is well-isolated, properly tested, and the inert-mode behavior for old messages is correctly handled through the existing disabled-prop pattern. The implementation is clean and the 17 new tests cover the important parsing and formatting paths. Two small issues were found: an implicit undefined return in formatQuestionAnswerMessage for answers[0] (guarded by callers today but fragile to reuse), and stepper chevrons that rely on derived state rather than an explicit disabled guard to stay inert on older messages. question.tsx — the formatQuestionAnswerMessage return and the stepper chevron disabled conditions both warrant a second look before the function is reused elsewhere. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Agent as Agent (stream)
participant Parser as parseSpecialTags
participant ST as SpecialTags renderer
participant QD as QuestionDisplay
participant CB as onOptionSelect callback
Agent->>Parser: chunk containing question tag
Parser->>Parser: parseQuestionTagBody → QuestionItem[]
Parser-->>ST: "segment { type: 'question', data: QuestionItem[] }"
ST->>QD: "render(data, onSelect=onOptionSelect)"
Note over QD: phase='active', step=0, answers=[null,...]
loop For each step 0..N-1
QD->>QD: user picks option / enters free text
QD->>QD: "setAnswers[step]=answer → goToStep(step+1)"
end
QD->>QD: last step answered → setPhase('answered')
QD->>CB: onSelect(formatQuestionAnswerMessage(questions, answers))
CB-->>Agent: user message submitted
Note over QD: renders Q/A recap (inert)
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Agent as Agent (stream)
participant Parser as parseSpecialTags
participant ST as SpecialTags renderer
participant QD as QuestionDisplay
participant CB as onOptionSelect callback
Agent->>Parser: chunk containing question tag
Parser->>Parser: parseQuestionTagBody → QuestionItem[]
Parser-->>ST: "segment { type: 'question', data: QuestionItem[] }"
ST->>QD: "render(data, onSelect=onOptionSelect)"
Note over QD: phase='active', step=0, answers=[null,...]
loop For each step 0..N-1
QD->>QD: user picks option / enters free text
QD->>QD: "setAnswers[step]=answer → goToStep(step+1)"
end
QD->>QD: last step answered → setPhase('answered')
QD->>CB: onSelect(formatQuestionAnswerMessage(questions, answers))
CB-->>Agent: user message submitted
Note over QD: renders Q/A recap (inert)
Reviews (1): Last reviewed commit: "fix(chat): let inert multi-step question..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| export function formatQuestionAnswerMessage(questions: QuestionItem[], answers: string[]): string { | ||
| if (questions.length === 1) return answers[0] | ||
| return questions.map((q, i) => `${q.prompt} — ${answers[i]}`).join('\n') | ||
| } |
There was a problem hiding this comment.
answers[0] returns undefined when the caller passes an empty array, but the declared return type is string. TypeScript won't catch this without noUncheckedIndexedAccess. Adding an explicit guard closes the gap and makes the intent self-documenting.
| export function formatQuestionAnswerMessage(questions: QuestionItem[], answers: string[]): string { | |
| if (questions.length === 1) return answers[0] | |
| return questions.map((q, i) => `${q.prompt} — ${answers[i]}`).join('\n') | |
| } | |
| export function formatQuestionAnswerMessage(questions: QuestionItem[], answers: string[]): string { | |
| if (questions.length === 1) return answers[0] ?? '' | |
| return questions.map((q, i) => `${q.prompt} — ${answers[i] ?? ''}`).join('\n') | |
| } |
There was a problem hiding this comment.
Applied in 85e8dec — the only caller passes a parallel non-empty array, but the guard is cheap and makes the exported helper safe standalone.
| <div className='flex items-center gap-2'> | ||
| <Button | ||
| type='button' | ||
| variant='ghost' | ||
| onClick={() => goToStep(step - 1)} | ||
| disabled={step === 0} | ||
| className={cn( | ||
| ICON_BUTTON_CLASSES, | ||
| 'before:absolute before:inset-[-8px] before:content-[""] disabled:opacity-50' | ||
| )} | ||
| > | ||
| <ChevronLeft className='h-[9px] w-[7px] text-[var(--text-icon)]' /> | ||
| <span className='sr-only'>Previous question</span> | ||
| </Button> | ||
| <span className='whitespace-nowrap text-[var(--text-muted)] text-sm tabular-nums'> | ||
| {step + 1} of {data.length} | ||
| </span> | ||
| <Button | ||
| type='button' | ||
| variant='ghost' | ||
| onClick={() => goToStep(step + 1)} | ||
| // Inert renders (older messages) browse freely; interactive ones | ||
| // gate forward movement on the current question being answered. | ||
| disabled={isLast || (!disabled && answers[step] === null)} | ||
| className={cn( | ||
| ICON_BUTTON_CLASSES, | ||
| 'before:absolute before:inset-[-8px] before:content-[""] disabled:opacity-50' | ||
| )} | ||
| > | ||
| <ChevronRight className='h-[9px] w-[7px] text-[var(--text-icon)]' /> |
There was a problem hiding this comment.
disabled prop
The back/forward chevrons only check step === 0 and isLast || answers[step] === null — they never check disabled. In inert mode (older messages, onSelect undefined) this works out because answers is all-null so the forward button is always disabled and step starts at 0 so back is also disabled, but the dependency is implicit. If the initialization logic ever changes (e.g. answers pre-populated from a persisted state), the stepper would become interactive on messages that should be read-only. Passing disabled={disabled || step === 0} and disabled={disabled || isLast || answers[step] === null} makes the invariant explicit.
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!
There was a problem hiding this comment.
Intentional, and made explicit in a6890d3: in inert mode the stepper deliberately stays browsable so historical multi-step batches can show all their prompts (Bugbot flagged the opposite problem — inert renders trapping users on question 1). Stepping is pure local navigation; answering is what's disabled. The forward chevron now reads disabled={isLast || (!disabled && answers[step] === null)} with a comment stating the invariant.
1 participant
Summary
<question>special tag to the chat stream:single_select,confirm, andtextquestions render as an inline div with the user input's chrome (rounded-2xl, --border-1)‹ 1 of 3 ›stepper and back-navigation before submitsingle_selectalways appends a "Something else" free-text row,textrenders only the inputPrompt — Answerper line) through the existing option-select path and collapses the div to a question/answer recapChevronLeft/ChevronRightto@sim/emcnicons (rotations of the existingChevronDowncaret)questiontags from copy-to-plaintextType of Change
Testing
17 new unit tests for the tag parser (object/array bodies, invalid schemas, streaming suppression) and the answer-message formatter; drove all states in the running app (light + dark). Lint and strict API validation pass.
Checklist