-
Notifications
You must be signed in to change notification settings - Fork 35
(WIP) [Feature Suggestions UI]: restart implementation for post‑chat list #424
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| import React, { useEffect, useState } from "react"; | ||
| import React, { useEffect, useMemo, useState } from "react"; | ||
| import { quickSuggestions } from "../data/quick-suggestions-data.js"; | ||
|
|
||
| interface QuickSuggestionsProps { | ||
|
|
@@ -12,6 +12,24 @@ interface Suggestion { | |
|
|
||
| function QuickSuggestions({ onSelectSuggestion }: QuickSuggestionsProps) { | ||
| const [randomSuggestions, setRandomSuggestions] = useState<Suggestion[]>([]); | ||
| const [selected, setSelected] = useState<Set<number>>(new Set()); | ||
|
|
||
| const allSelected = useMemo( | ||
| () => | ||
| randomSuggestions.length > 0 && | ||
| selected.size === randomSuggestions.length, | ||
| [randomSuggestions.length, selected.size], | ||
| ); | ||
|
|
||
| const buildCombinedPrompt = (indexes: Set<number>) => { | ||
| const items = Array.from(indexes) | ||
| .sort((a, b) => a - b) | ||
| .map((i) => randomSuggestions[i]) | ||
| .filter(Boolean); | ||
| if (items.length === 0) return ""; | ||
| // Join selected feature prompts in a concise, model-friendly way | ||
| return items.map((s) => `- ${s.label}: ${s.text}`).join("\n"); | ||
| }; | ||
|
|
||
| useEffect(() => { | ||
| const shuffled = [...quickSuggestions].sort(() => 0.5 - Math.random()); | ||
|
|
@@ -23,17 +41,74 @@ function QuickSuggestions({ onSelectSuggestion }: QuickSuggestionsProps) { | |
| <h3 className="mb-4 text-center text-sm font-medium text-gray-600"> | ||
| Create custom vibes from a prompt | ||
| </h3> | ||
| <div className="flex flex-wrap justify-center gap-2"> | ||
| {randomSuggestions.map((suggestion, index) => ( | ||
| <button | ||
| key={index} | ||
| type="button" | ||
| onClick={() => onSelectSuggestion(suggestion.text)} | ||
| className="cursor-pointer rounded-md bg-light-background-01 px-3 py-1.5 text-sm font-medium text-light-primary transition-colors hover:bg-light-decorative-01 dark:bg-dark-background-01 dark:text-dark-primary dark:hover:bg-dark-decorative-01" | ||
| > | ||
| {suggestion.label} | ||
| </button> | ||
| ))} | ||
| {/* Selection toolbar */} | ||
| {randomSuggestions.length > 0 && ( | ||
| <div className="mb-3 flex items-center justify-center gap-4 text-xs text-gray-600"> | ||
| <label className="inline-flex items-center gap-2"> | ||
| <input | ||
| type="checkbox" | ||
| aria-label="Select all suggested features" | ||
| checked={allSelected} | ||
| onChange={(e) => { | ||
| const next = new Set<number>(); | ||
| if (e.currentTarget.checked) { | ||
| for (let i = 0; i < randomSuggestions.length; i++) | ||
| next.add(i); | ||
| } | ||
| setSelected(next); | ||
| const combined = buildCombinedPrompt(next); | ||
| if (combined) onSelectSuggestion(combined); | ||
| }} | ||
| className="h-4 w-4 accent-blue-600" | ||
| /> | ||
| <span>Select all</span> | ||
| </label> | ||
| {selected.size > 0 && ( | ||
| <button | ||
| type="button" | ||
| onClick={() => { | ||
| setSelected(new Set()); | ||
| // Clear the input when clearing selection to avoid confusion | ||
| onSelectSuggestion(""); | ||
| }} | ||
| className="rounded px-2 py-1 text-[11px] text-blue-700 hover:underline dark:text-blue-400" | ||
| > | ||
| Clear selected | ||
| </button> | ||
| )} | ||
| </div> | ||
| )} | ||
|
|
||
| {/* Suggestions list: checkbox for multi-select + pill button for single insert */} | ||
| <div className="flex flex-wrap justify-center gap-3"> | ||
| {randomSuggestions.map((suggestion, index) => { | ||
| const isChecked = selected.has(index); | ||
| return ( | ||
| <div key={index} className="flex items-center gap-2"> | ||
| <input | ||
| type="checkbox" | ||
| aria-label={`Select ${suggestion.label}`} | ||
| checked={isChecked} | ||
| onChange={(e) => { | ||
| const next = new Set(selected); | ||
| if (e.currentTarget.checked) next.add(index); | ||
| else next.delete(index); | ||
| setSelected(next); | ||
| const combined = buildCombinedPrompt(next); | ||
| if (combined) onSelectSuggestion(combined); | ||
| }} | ||
| className="h-4 w-4 accent-blue-600" | ||
| /> | ||
| <button | ||
| type="button" | ||
| onClick={() => onSelectSuggestion(suggestion.text)} | ||
| className="cursor-pointer rounded-md bg-light-background-01 px-3 py-1.5 text-sm font-medium text-light-primary transition-colors hover:bg-light-decorative-01 dark:bg-dark-background-01 dark:text-dark-primary dark:hover:bg-dark-decorative-01" | ||
| > | ||
| {suggestion.label} | ||
| </button> | ||
|
Comment on lines
+102
to
+108
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Clicking a suggestion pill inserts a single suggestion into the input but does not update selection state, which can conflict with any existing multi-selection and produce mismatched UI vs. composed prompt. This dual interaction model (pills insert, checkboxes compose) is confusing and harder to maintain. A simpler, consistent approach is to make the pill click toggle its corresponding selection and rely on the composed prompt for output. SuggestionUnify the behavior by having the pill click toggle its checkbox and then compose the prompt: onClick={() => {
const next = new Set(selected);
if (next.has(index)) next.delete(index);
else next.add(index);
setSelected(next);
onSelectSuggestion(buildCombinedPrompt(next));
}}If you prefer to preserve single-insert behavior when nothing is selected, you could conditionally do single-insert when |
||
| </div> | ||
| ); | ||
| })} | ||
| </div> | ||
| </div> | ||
| ); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -74,3 +74,27 @@ describe("QuickSuggestions", () => { | |
| expect(button).toHaveClass("cursor-pointer"); | ||
| }); | ||
| }); | ||
|
|
||
| it("supports select-all checkbox to auto-fill combined features", () => { | ||
| const onSelectSuggestion = vi.fn(); | ||
| const { container } = render( | ||
| <QuickSuggestions onSelectSuggestion={onSelectSuggestion} />, | ||
| ); | ||
|
|
||
| const selectAll = container.querySelector( | ||
| 'input[aria-label="Select all suggested features"]', | ||
| ) as HTMLInputElement | null; | ||
| expect(selectAll).not.toBeNull(); | ||
|
|
||
| if (selectAll) { | ||
| // Click the select-all checkbox | ||
|
Comment on lines
+84
to
+90
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test can be flaky: SuggestionRewrite the test to await the element. Also avoid the conditional block once it’s awaited. import { render, screen, fireEvent } from "@testing-library/react";
it("supports select-all checkbox to auto-fill combined features", async () => {
const onSelectSuggestion = vi.fn();
render(<QuickSuggestions onSelectSuggestion={onSelectSuggestion} />);
const selectAll = await screen.findByLabelText("Select all suggested features");
fireEvent.click(selectAll);
expect(onSelectSuggestion).toHaveBeenCalled();
const arg = onSelectSuggestion.mock.calls[0][0];
expect(typeof arg).toBe("string");
expect(arg.includes("- ")).toBe(true);
});Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion. |
||
| fireEvent.click(selectAll); | ||
|
|
||
| // Verify callback was invoked with a combined string | ||
| expect(onSelectSuggestion).toHaveBeenCalled(); | ||
| const arg = onSelectSuggestion.mock.calls[0][0]; | ||
| expect(typeof arg).toBe("string"); | ||
| // Combined prompt should contain multiple lines joined with dashes | ||
| expect(arg.includes("- ")).toBe(true); | ||
| } | ||
| }); | ||
|
Comment on lines
+77
to
+100
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There’s no test ensuring the prompt is cleared when all items are deselected (either by unchecking the last item or toggling “Select all” off). Given the behavioral change above, adding a test will prevent regressions and codify expected behavior. SuggestionAdd a test that unselects all and verifies the callback receives an empty string: it("clears composed prompt when selection becomes empty", () => {
const onSelectSuggestion = vi.fn();
const { container } = render(<QuickSuggestions onSelectSuggestion={onSelectSuggestion} />);
const selectAll = container.querySelector('input[aria-label="Select all suggested features"]') as HTMLInputElement;
expect(selectAll).not.toBeNull();
// Select all, then unselect all
fireEvent.click(selectAll);
fireEvent.click(selectAll);
// The latest call should clear
const lastCall = onSelectSuggestion.mock.calls.at(-1);
expect(lastCall?.[0]).toBe("");
});Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unchecking the “Select all” box (or reducing selection to zero) does not clear the prompt, leaving stale content in the input. This violates user expectations introduced by your own inline comment about clearing to avoid confusion and creates inconsistent behavior vs. the "Clear selected" button. You compute the combined prompt but only call
onSelectSuggestionwhen the string is non-empty, so the empty-state isn’t propagated.Suggestion
Change the handler to always propagate the current composed prompt (including empty), so the input clears when the set becomes empty:
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion