(WIP) [Feature Suggestions UI]: restart implementation for post‑chat list #424
Conversation
…o-fill into input\n\n- Allow selecting one or many suggested features\n- Add "Select all" and "Clear selected" controls\n- Auto-compose selected features into the prompt area\n\nRefs #423
✅ Deploy Preview for fireproof-ai-builder ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
- Clearing behavior is inconsistent: unchecking all items (via item toggles or turning off “Select all”) leaves stale prompt text because empty combined values are not propagated to
onSelectSuggestion. - The pill click path bypasses selection state, causing potential mismatch between visible selections and the prompt contents; unify interactions to reduce confusion.
- The “Select all” checkbox lacks an indeterminate state, reducing clarity for partial selections.
- Test coverage does not verify clearing behavior when selections go to zero, risking regressions.
Additional notes (3)
-
Style |
vibes.diy/pkg/app/components/QuickSuggestions.tsx:49-63
The “Select all” checkbox does not reflect a partial selection state. Settingindeterminateimproves clarity and accessibility for mixed selections. -
Maintainability |
vibes.diy/pkg/app/components/QuickSuggestions.tsx:87-87
Using the array index as both the Reactkeyand the selection identifier couples UI state to render order. If the list ever changes (e.g., new data, reordering), selection can point at the wrong items. Consider moving to a stable ID (e.g.,labelor a dedicatedid) for keys and for theselectedset. -
Maintainability |
vibes.diy/pkg/app/components/QuickSuggestions.tsx:52-61
The selection update logic (computenext, set state, compute combined string, call callback) is duplicated in both the master checkbox and each item’s checkbox. This repetition increases drift risk and makes behavioral changes (like clearing on empty) easy to miss in one path.
Summary of changes
- Introduced multi-select behavior in QuickSuggestions with a selection toolbar.
- Added a “Select all” checkbox and “Clear selected” button.
- Implemented
buildCombinedPromptto compose selected suggestions into a single string (dash-prefixed lines). - Updated UI to render a checkbox alongside each suggestion pill; clicking a pill still inserts a single suggestion.
- Added a test verifying select-all triggers combined prompt composition.
| 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); | ||
| }} |
There was a problem hiding this comment.
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 onSelectSuggestion when 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:
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);
onSelectSuggestion(combined);
}}Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion
| <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> |
There was a problem hiding this comment.
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.
Suggestion
Unify 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 selected.size === 0 and toggle otherwise.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion
|
|
||
| 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 | ||
| 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); | ||
| } | ||
| }); |
There was a problem hiding this comment.
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.
Suggestion
Add 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
| 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 |
There was a problem hiding this comment.
This test can be flaky: randomSuggestions is set inside useEffect, so the select-all checkbox won’t be present on the initial render. Querying the DOM immediately with querySelector risks a null result depending on timing. Prefer waiting for the element by label using Testing Library’s async queries.
Suggestion
Rewrite 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.
Resolves #423
Context
What’s in this PR
Next steps (follow-ups)
How to test (dev)
Notes