feat(app): first-time-visitor home onboarding with chip suggestions - #692
Conversation
📝 WalkthroughWalkthroughAdds HomeSuggestionList UI, typed suggestion state, persisted dismissed IDs, unit and E2E tests, i18n updates, removes rotating placeholder examples, and adds opencode PATH helpers and tests. ChangesHome Suggestion Chips Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Suggested priority: P2 (includes user-path files (packages/app/src/components/home/home-suggestion-list.test.ts, packages/app/src/components/home/home-suggestion-list.tsx, packages/app/src/components/home/home-suggestions-state.test.ts, packages/app/src/components/home/home-suggestions-state.ts, packages/app/src/components/prompt-input.tsx, packages/app/src/components/prompt-input/placeholder.test.ts, packages/app/src/components/prompt-input/placeholder.ts, packages/app/src/components/prompt-input/store-types.ts, packages/app/src/components/session/session-new-view.tsx, packages/app/src/components/settings-general.home-suggestions.test.ts, packages/app/src/components/settings-general.tsx, packages/app/src/context/settings.tsx, packages/app/src/i18n/en.ts, packages/app/src/i18n/zh.ts)).
P1/P0 are reserved for maintainer confirmation. Please relabel manually if this is a release blocker, security issue, data-loss risk, or updater/runtime failure.
There was a problem hiding this comment.
Code Review
This pull request introduces a new onboarding feature consisting of prompt suggestion chips on the home screen for first-time visitors. It includes the implementation of the HomeSuggestionList component, state management for tracking dismissed and seen suggestions, and integration with the settings UI to allow users to restore suggestions. Additionally, it removes the legacy rotating placeholder logic from the prompt input. Feedback is provided regarding an accessibility issue where the per-row dismiss buttons are excluded from the tab order, preventing keyboard-only users from interacting with them.
Perf delta summaryComparator: pass
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/app/e2e/onboarding/home-suggestion-chips.spec.ts (1)
4-6: ⚡ Quick winUse SCREAMING_SNAKE_CASE for test constants.
The coding guidelines specify that constants in test files should use SCREAMING_SNAKE_CASE. As per coding guidelines: "Use SCREAMING_SNAKE_CASE for constants in tests".
♻️ Suggested renaming
-const suggestionListSelector = '[data-component="home-suggestion-list"]' -const rowSelector = '[data-action="home-suggestion-row"]' -const rowDismissSelector = '[data-action="home-suggestion-row-dismiss"]' +const SUGGESTION_LIST_SELECTOR = '[data-component="home-suggestion-list"]' +const ROW_SELECTOR = '[data-action="home-suggestion-row"]' +const ROW_DISMISS_SELECTOR = '[data-action="home-suggestion-row-dismiss"]'Then update all references throughout the file.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/e2e/onboarding/home-suggestion-chips.spec.ts` around lines 4 - 6, Rename the test constants suggestionListSelector, rowSelector, and rowDismissSelector to SCREAMING_SNAKE_CASE (e.g., SUGGESTION_LIST_SELECTOR, ROW_SELECTOR, ROW_DISMISS_SELECTOR) and update every usage in the file (tests, queries, selectors) to reference the new names; ensure the const declarations for suggestionListSelector, rowSelector, and rowDismissSelector are replaced with the new SCREAMING_SNAKE_CASE identifiers and that imports/uses inside functions, assertions, and helper calls are updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/app/src/components/home/home-suggestion-list.tsx`:
- Around line 61-68: prefill currently unconditionally calls prompt.set(...)
which overwrites any user-typed prompt; change prefill to guard against
clobbering in-progress input by first checking the current composer/prompt
content and only calling prompt.set when the composer is empty or contains
exactly the initial empty state (i.e., no user edits); if there is existing user
content, do not call prompt.set or change the cursor, but still call
focusComposerEditor as appropriate. Locate the logic in the prefill function and
use the existing prompt/composer getters (the current prompt state) to decide
whether to invoke prompt.set(...) and requestAnimationFrame(() =>
focusComposerEditor(...)).
In `@packages/app/src/components/session/session-new-view.tsx`:
- Around line 24-28: The suggestion rows are rendered after the composer because
HomeSuggestionList is placed below the composer block; move the
HomeSuggestionList JSX node so it appears before the composer div (i.e., place
<HomeSuggestionList /> above the block that calls props.composer!({
onModeChange: () => {} })), preserving the surrounding Show wrapper and existing
classes/styling on the composer container so the onboarding layout now shows
suggestions above the input.
---
Nitpick comments:
In `@packages/app/e2e/onboarding/home-suggestion-chips.spec.ts`:
- Around line 4-6: Rename the test constants suggestionListSelector,
rowSelector, and rowDismissSelector to SCREAMING_SNAKE_CASE (e.g.,
SUGGESTION_LIST_SELECTOR, ROW_SELECTOR, ROW_DISMISS_SELECTOR) and update every
usage in the file (tests, queries, selectors) to reference the new names; ensure
the const declarations for suggestionListSelector, rowSelector, and
rowDismissSelector are replaced with the new SCREAMING_SNAKE_CASE identifiers
and that imports/uses inside functions, assertions, and helper calls are updated
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fb9aab95-59ef-4286-a60a-0df5a28ce6d6
📒 Files selected for processing (14)
packages/app/e2e/onboarding/home-suggestion-chips.spec.tspackages/app/src/components/home/home-suggestion-list.test.tspackages/app/src/components/home/home-suggestion-list.tsxpackages/app/src/components/home/home-suggestions-state.test.tspackages/app/src/components/home/home-suggestions-state.tspackages/app/src/components/prompt-input.tsxpackages/app/src/components/prompt-input/placeholder.test.tspackages/app/src/components/prompt-input/placeholder.tspackages/app/src/components/prompt-input/store-types.tspackages/app/src/components/session/session-new-view.tsxpackages/app/src/context/settings.tsxpackages/app/src/i18n/en.tspackages/app/src/i18n/zh.tspackages/opencode/test/config/e2e-smoke-tagging.test.ts
💤 Files with no reviewable changes (1)
- packages/app/src/components/prompt-input/store-types.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/app/src/components/home/home-suggestion-list.test.ts (1)
50-55:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThis contract test enforces the wrong prefill behavior.
Line 54 explicitly forbids a
prompt.dirty()guard, which codifies the overwrite bug and blocks the intended fix.💡 Suggested fix
-test("prefill unconditionally replaces composer content (no dirty-guard skip)", () => { - expect(source).not.toMatch(/if \(prompt\.dirty\(\)\)\s*\{[\s\S]{0,200}return/) +test("does not overwrite dirty composer content on chip click", () => { + expect(source).toMatch(/if \(prompt\.dirty\(\)\)\s*\{/) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/src/components/home/home-suggestion-list.test.ts` around lines 50 - 55, The test "prefill unconditionally replaces composer content (no dirty-guard skip)" is asserting that source must not contain a prompt.dirty() guard, which enforces the outdated overwrite behavior; update the test to allow or assert the correct behavior instead of forbidding prompt.dirty(): modify the expectation around the variable source (used in this test) to either remove the negative match against /if \(prompt\.dirty\(\)\)\s*\{[\s\S]{0,200}return/ or replace it with an assertion that verifies the new per-chip currentChipSource/auto-dismiss behavior (e.g., assert presence of the currentChipSource logic or absence of unconditional overwrite), so the test no longer blocks the intended fix involving prompt.dirty() checks.packages/app/src/components/home/home-suggestion-list.tsx (1)
104-108:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent chip clicks from overwriting dirty composer content.
Line 105 always overwrites the editor via
prompt.set(...). This breaks the intended dirty-state behavior (chip click should be a no-op except focus) and can clobber in-progress user text.💡 Suggested fix
const prefill = (chipID: HomeSuggestionChipID, text: string) => { + if (prompt.dirty()) { + requestAnimationFrame(() => { + if (typeof document === "undefined") return + document.querySelector<HTMLElement>(PROMPT_EDITOR_SELECTOR)?.focus() + }) + return + } + prompt.set([{ type: "text", content: text, start: 0, end: text.length }], text.length) setCurrentChipSource(chipID) requestAnimationFrame(() => focusComposerEditor(text.length)) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/src/components/home/home-suggestion-list.tsx` around lines 104 - 108, The prefill function currently always calls prompt.set(...) which overwrites any in-progress editor content; change prefill to first detect the composer's dirty state and only call prompt.set when the composer is clean. Specifically, in prefill (function name), check the existing dirty indicator (e.g., an isComposerDirty / composerDirty flag or prompt.isDirty()/prompt.getText() comparison) and if the composer is dirty, skip prompt.set(...) and only call setCurrentChipSource(chipID) and focusComposerEditor(text.length); otherwise, proceed with prompt.set([...], text.length), setCurrentChipSource(chipID), and requestAnimationFrame(() => focusComposerEditor(text.length)).
🧹 Nitpick comments (2)
packages/app/e2e/onboarding/home-suggestion-chips.spec.ts (2)
5-7: ⚡ Quick winUse SCREAMING_SNAKE_CASE for test constants.
These selectors are constants but are currently camelCase; align naming with the test convention for consistency.
As per coding guidelines: "Use SCREAMING_SNAKE_CASE for constants in tests".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/e2e/onboarding/home-suggestion-chips.spec.ts` around lines 5 - 7, Rename the test constants suggestionListSelector, rowSelector, and rowDismissSelector to SCREAMING_SNAKE_CASE (e.g., SUGGESTION_LIST_SELECTOR, ROW_SELECTOR, ROW_DISMISS_SELECTOR) and update every usage within the test file (onboarding/home-suggestion-chips.spec.ts) to the new names so the selectors remain the same but follow the test convention; ensure no other identifiers are changed and run the tests to verify no reference errors.
206-210: ⚡ Quick winReplace
execCommand("selectAll")withmodKeykeyboard selection.Use the cross-platform shortcut path here instead of
execCommandso selection behavior stays stable in E2E runs.As per coding guidelines: "Use modKey from utils for cross-platform keyboard shortcuts (Meta on Mac, Control on Linux/Windows)".💡 Suggested fix
import { test, expect } from "../fixtures" import { promptSelector } from "../selectors" +import { modKey } from "../utils" @@ - await page.evaluate(() => { - document.execCommand("selectAll") - }) + await page.keyboard.press(`${modKey}+A`) await page.keyboard.press("Backspace")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/e2e/onboarding/home-suggestion-chips.spec.ts` around lines 206 - 210, Replace the non-cross-platform document.execCommand("selectAll") call with a keyboard-based selection using the shared modKey utility: import { modKey } from the test utils, then perform the selection via page.keyboard.down(modKey); await page.keyboard.press("a"); await page.keyboard.up(modKey); and keep the subsequent await page.keyboard.press("Backspace") to clear the composer; remove the document.execCommand call so selection uses the modKey path for cross-platform E2E stability.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/opencode/src/util/env.ts`:
- Around line 31-34: The prependBundledTools function can produce a trailing
path delimiter when currentPath is empty, creating an empty PATH entry; update
prependBundledTools (and its use of bundledToolsDir and path.delimiter) to only
join dir and currentPath with path.delimiter when currentPath is non-empty (or
otherwise return dir alone), ensuring you don't append a delimiter for an empty
currentPath.
---
Duplicate comments:
In `@packages/app/src/components/home/home-suggestion-list.test.ts`:
- Around line 50-55: The test "prefill unconditionally replaces composer content
(no dirty-guard skip)" is asserting that source must not contain a
prompt.dirty() guard, which enforces the outdated overwrite behavior; update the
test to allow or assert the correct behavior instead of forbidding
prompt.dirty(): modify the expectation around the variable source (used in this
test) to either remove the negative match against /if
\(prompt\.dirty\(\)\)\s*\{[\s\S]{0,200}return/ or replace it with an assertion
that verifies the new per-chip currentChipSource/auto-dismiss behavior (e.g.,
assert presence of the currentChipSource logic or absence of unconditional
overwrite), so the test no longer blocks the intended fix involving
prompt.dirty() checks.
In `@packages/app/src/components/home/home-suggestion-list.tsx`:
- Around line 104-108: The prefill function currently always calls
prompt.set(...) which overwrites any in-progress editor content; change prefill
to first detect the composer's dirty state and only call prompt.set when the
composer is clean. Specifically, in prefill (function name), check the existing
dirty indicator (e.g., an isComposerDirty / composerDirty flag or
prompt.isDirty()/prompt.getText() comparison) and if the composer is dirty, skip
prompt.set(...) and only call setCurrentChipSource(chipID) and
focusComposerEditor(text.length); otherwise, proceed with prompt.set([...],
text.length), setCurrentChipSource(chipID), and requestAnimationFrame(() =>
focusComposerEditor(text.length)).
---
Nitpick comments:
In `@packages/app/e2e/onboarding/home-suggestion-chips.spec.ts`:
- Around line 5-7: Rename the test constants suggestionListSelector,
rowSelector, and rowDismissSelector to SCREAMING_SNAKE_CASE (e.g.,
SUGGESTION_LIST_SELECTOR, ROW_SELECTOR, ROW_DISMISS_SELECTOR) and update every
usage within the test file (onboarding/home-suggestion-chips.spec.ts) to the new
names so the selectors remain the same but follow the test convention; ensure no
other identifiers are changed and run the tests to verify no reference errors.
- Around line 206-210: Replace the non-cross-platform
document.execCommand("selectAll") call with a keyboard-based selection using the
shared modKey utility: import { modKey } from the test utils, then perform the
selection via page.keyboard.down(modKey); await page.keyboard.press("a"); await
page.keyboard.up(modKey); and keep the subsequent await
page.keyboard.press("Backspace") to clear the composer; remove the
document.execCommand call so selection uses the modKey path for cross-platform
E2E stability.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 442a6283-c9df-4f97-a36c-22e0376dacdd
📒 Files selected for processing (13)
packages/app/e2e/onboarding/home-suggestion-chips.spec.tspackages/app/src/components/home/home-suggestion-list.test.tspackages/app/src/components/home/home-suggestion-list.tsxpackages/app/src/components/home/home-suggestions-state.test.tspackages/app/src/components/home/home-suggestions-state.tspackages/app/src/i18n/en.tspackages/app/src/i18n/zh.tspackages/opencode/src/pty/index.tspackages/opencode/src/session/prompt.tspackages/opencode/src/session/prompt/pawwork.txtpackages/opencode/src/tool/bash.tspackages/opencode/src/util/env.tspackages/opencode/test/util/env.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/opencode/src/session/prompt/pawwork.txt
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/app/src/components/home/home-suggestions-state.test.ts
- packages/app/src/i18n/zh.ts
- packages/app/src/i18n/en.ts
remove EXAMPLES array, 6.5s setInterval rotation, and random initial index from prompt-input. promptPlaceholder() loses the suggest and example parameters, returning a single static prompt.placeholder.home key for the default branch. 25 prompt.example.* i18n keys and the now- unused prompt.placeholder.normal / prompt.placeholder.simple keys are removed. Part of the first-time-visitor onboarding redesign — see docs/superpowers/specs/2026-05-17-onboarding-design.md
introduce general.homeSuggestionsEnabled (default true) and general.homeSuggestionsDismissed (default empty array) on the persisted settings.v3 store, with withFallback accessors for migration safety. settings → general gains a toggle row with an inline restore button visible only when all three rows are dismissed. include a contract test for the row wiring so the localStorage-based E2E does not silently mask broken UI bindings. Part of the first-time-visitor onboarding redesign.
introduces HOME_SUGGESTION_CHIPS (3 stable chip ids) and resolveVisibleHomeSuggestions() which returns the visible id list given firstTimeVisitor / enabled / dismissed state. pure function so it can be unit-tested without dom or solid runtime.
renders three prompt suggestion rows under the home composer for first-time visitors. clicking a row prefills the composer via usePrompt().set and focuses the editor; per-row X writes the chip id into settings.general.homeSuggestionsDismissed; section X dismisses all three at once. component returns null when not a first-time visitor, when feature is disabled, or when all chips are dismissed. sync.ready gate avoids flashing during initial sync hydration.
wire the new suggestion list as a sibling of the composer container in NewSessionView. visibility, dismissal, and prefill behavior are owned by the component itself; NewSessionView only handles layout placement.
cover 10 paths from the spec: first-time visitor renders 3 rows, clicking prefills + focuses the editor, per-row X persists across reload, section X hides the whole section, placeholder is static (no rotation), returning visitors see no suggestion list, hover reveals per-row X, editing prefilled text preserves the edit on send, settings toggle hides and restores the section, and language switch preserves dismissed state. update the @smoke inventory for the new @smoke titles.
Round 1 fixes after multi-model code review:
- Persist a one-way `homeSuggestionsSeen` flag so returning users who
delete every session do not re-enter onboarding when they revisit home.
- Replace bare `as HomeSuggestionChipID[]` cast with a known-id filter so
stale persisted IDs from a renamed chip do not poison the visibility
contract or block the settings restore action.
- Honor `prompt.dirty()` when a chip click would otherwise overwrite the
user-typed message, appending the suggestion with a separator instead.
- Explicitly `setCursorPosition(editor, text.length)` after `focus()` so
follow-up typing is not browser-selection dependent.
- Switch the rest-state row dismiss button to `pointer-events-none` and
`tabIndex={-1}` so the invisible control cannot receive accidental
clicks or stops in tab order, then restore both on group hover.
- Settings page: gate the restore button on any chip dismissed (not all
three) and replace the hard-coded `>= 3` with
`HOME_SUGGESTION_CHIPS.length` so the section count is not brittle.
- Tighten the E2E placeholder check to a single attribute snapshot
against the actual i18n string and drop the 7s sleep; verify chip text
re-renders on locale switch instead of just row count; exercise the
real Settings switch flow rather than a localStorage poke.
Round 2 crosscheck caught a regression in the previous round's fix: markSeen() was firing from dismissRow, which flips firstTimeVisitor to false and hides the whole section after the first dismiss. The expected UX is that dismissing one row leaves the other two visible; the Settings restore button then becomes a silent no-op because chips never re-render even after the dismissed list clears. Per-row dismiss is fine-grained curation, not an onboarding exit signal. Reserve homeSuggestionsSeen for: hydration with sessions present, chip click (prefill), or section-level dismiss (dismissAll). The persisted comment is updated to spell this out, and three new contract tests pin the per-handler markSeen behavior so a regression cannot land silently again. Also tighten the placeholder E2E to assert against the current locale's i18n value rather than an either-or regex, so a regression in either language fails the test.
Both reviewers in Round 3 converged on the same P1: clicking "Restore" in Settings only cleared the dismissed list, so after a section-level dismiss (which also flips seen=true) the button was a silent no-op. Chips never returned even though the UI promised they would. Restore now resets BOTH state slots, and the gate widens to surface the button whenever either slot is non-empty so users can recover from either path (per-row dismissals or section dismiss). The dead auto-clear branch on switch re-enable also went away: under the seen flag it could never restore visibility on its own, and the dedicated restore button is now the single source of truth for "bring chips back".
…poser Round 4 crosscheck surfaced two remaining issues: - Claude P1: Restore in Settings was still a silent no-op for returning users (sessionCount > 0). Even after clearing both seen and dismissed, firstTimeVisitor stays false because of the sessionCount check. The button promised recovery that could not happen. Settings now consults useSync and hides the restore button once any session exists. - Claude P2 #5: prefill merged user-typed content with the suggestion by joining text parts, which silently dropped file/agent @-mentions. When prompt.dirty() is true, prefill now leaves the composer untouched and only refocuses. No corruption path remains. Also normalize sessionCount to sync.data.session.length (it is Session[], not a record map, confirmed via use-session-blockers.ts:33).
Round 5 P2: prefill flipped seen=true before the dirty-composer no-op check, so clicking a chip while the composer already had user text silently exited onboarding even though nothing happened. Move markSeen after the dirty-branch return so seen only flips when a chip click actually produced a prefill. Locked with an ordering assertion in the contract test. Round 5 was otherwise clean: Codex reported no findings, Claude reported zero P0/P1. This is the iterate-to-zero terminal state per the user directive.
Manual dev:desktop verification surfaced a P0 I missed across five crosscheck rounds: the Settings page renders outside the Sync provider, so calling useSync() in SettingsGeneral throws "Sync context must be used within a context provider" the moment the user opens Settings. This was added in the previous fix as an extra gate that hid the restore button for returning users. The right gate is simpler and does not need sync at all: the dismissed list is non-empty exactly when chips were hidden via either path (per-row or section), because dismissAll already writes all chip ids. For a returning user the createEffect auto-latches seen=true but leaves dismissed empty, so the button stays hidden and we never surface a no-op recovery. Restore still resets BOTH state slots so the section dismiss path actually unwinds. Verified via dev:desktop after the patch: Settings opens without crashing. Lesson saved to memory: visual verification catches provider-boundary bugs that source-substring contract tests cannot see.
…s only Strip everything that the three-question test could not justify: - Section-level X (with header label and dismissAll handler): users who want all chips gone can X each of the three rows in seconds. - Settings master toggle (homeSuggestionsEnabled) and its Restore button: no plausible user crosses two screens to reopen a one-shot onboarding affordance. The toggle was 4 i18n strings, a Settings row, a contract test, and (most painfully) a sync-provider dependency from a page that renders outside it. - homeSuggestionsSeen one-way latch and its createEffect: defends only the edge case of a returning user who deletes every session and then reopens home. The harm in that case is "they see three onboarding rows again" — not worth a persisted flag, two interacting state slots, or the five rounds of crosscheck cascade that the flag triggered. What remains is the minimum that actually carries the design intent: one persisted field (homeSuggestionsDismissed: string[]), one component that renders three rows when sync.ready && sessionCount === 0 && there are undismissed chips, and a per-row X that hides on hover/focus only. Three rows all dismissed naturally collapse the section to nothing. Net: ~150 fewer lines, no provider-boundary bug, contract surface shrinks from 30 assertions to 13.
Three findings from dev:desktop manual verification: - Row width: container max-w was 640px (same as composer), but composer has its own visual padding, so rows looked wider than the input. Tighten to max-w-[520px] so rows sit visually inside the composer's bounding box. - Row hover: the full-width bg-row-hover-overlay highlight felt hollow on short text. Replace with a text-color hover (fg-muted → fg-strong) so the affordance reads as "this is interactive" without the loud band. - Dismiss X: size-4 icon felt heavy. Drop to size-3 inside a 20px square affordance with rounded corners and its own hover bg+color, so the control has clear self-feedback when targeted.
Replace the home-grown hover styling with the DESIGN.md picker-family /
session-row idiom that PawWork uses everywhere else (session row at
L399, picker contract at L415, settings nav row at L595, all share one
hover language). The previous "fg-muted to fg-strong on text only" was
imperceptible at 13px — confirmed live in dev:desktop.
Visual changes:
- Container width 520 to 640, aligned with the composer's outer edge so
rows visually live inside the composer's bounding box rather than
floating above as a separate, narrower strip.
- Row shell: h-30 + radius-sm + px-2 (sitting in DESIGN.md L77 30-system).
- Rest state: text-fg-weak (the quiet onboarding tone).
- Hover: bg-row-hover-overlay (4% black, the PawWork picker-family
standard) + text-fg-strong. This is the hover signal the rest of the
product trains on — list rows should feel the same here.
- Dismiss X: 30x30 ghost icon button + radius-md + 16px icon, hover
uses bg-row-active-overlay (6%) per DESIGN.md L401 ("one tier deeper
than the row to read separately"). 4px negative right margin lets it
visually flush with the row edge.
- focus-within: row gets the same overlay as hover so keyboard users
see the focus target without needing the brand outline ring.
No new tokens, no design deviations — pure reuse of established
patterns. Picked variant B from the side-by-side preview after manual
review.
Composer is max-w-[640px] with 1px border + 16px inner padding, so its text frame starts 17px inside the container. Insetting the row container by 16px on each side (max-w-[608px]) keeps row hover-overlay inside the composer's visible text frame instead of overshooting it. Adds gap-1 (4px) between rows per DESIGN.md L548 list-items rhythm so the three rows breathe instead of reading as a packed menu.
…iceCLI Previously each suggestion row showed the same text it prefilled into the composer, so the chip had no room to be a short hook and the prefill had no room to give the agent task context. Splits chip into a short labelKey (what the user sees in the row) and a longer promptKey (what gets prefilled on click). Replaces the three demo tasks to lean on PawWork's bundled officecli binary, which is the real local-agent differentiator: - folder-organize: organize a folder, propose plan before acting - excel-analysis: surface key data, outliers, trends via officecli - ppt-outline: generate a PPT outline from Word/PDF/Markdown via officecli Each prompt names the task, lists outputs, and points at officecli where relevant, but does not over-prescribe how the agent should ask clarifying questions — the agent will naturally ask for the file path on its own. Labels are written as a substring of their prompts so the existing e2e "row text appears in composer after click" assertion still holds.
The previous dirty-guard treated any non-empty composer as user-owned, which blocked the obvious onboarding flow: click chip A to see it, click chip B to switch. The composer just focused with no visible change, and first-time visitors couldn't tell whether the second click registered. Drop the guard. First-time visitor's home composer has no ownership semantics — chip content is a system suggestion, not user-authored, so freely swapping between chips is the right exploration affordance. The cost of "user typed then misclicked a chip" is a 50-80 char prompt, not real work; the gain is the explore-by-clicking flow finally working. Replaces the "does NOT overwrite user-typed content" e2e test with one that asserts the now-desired behavior: clicking another suggestion replaces the previous prefill.
…scovery Previously chip visibility was a single gate (per-project sessionCount === 0), so switching to a new workspace re-pitched chips users had already engaged with. The fix is to recognize that these chips are capability discovery (each highlighting a distinct officecli use case) rather than a one-shot onboarding tour: each chip should "graduate" on use and stay off, while unused chips can still reappear in fresh workspaces. Adds a currentChipSource signal in HomeSuggestionList that tracks "which chip the composer content came from." Set on chip click, cleared when the composer drains, sticky through user edits. A combined lifecycle effect observes sessionCount + prompt.dirty in a single tick so session-create and composer-empty resolve in a defined order: when a session is created while source is set, that chip is added to homeSuggestionsDismissed globally. The dismissed list semantic widens to "reasons this chip no longer shows" (user-X'd plus auto-graduated). Per-row X behavior unchanged. Adds 3 e2e cases covering the new state machine: chip-used auto-dismiss, switch-then-send only dismisses the last selection, and discard-then-type does not credit any chip. Source-contract unit test for the old dirty guard is replaced with assertions that pin the new tracking shape.
The previous design coupled chip visibility to the current project having zero sessions, which meant switching workspaces caused the entire chip section to disappear for one project and reappear for another — even though the user had already engaged with chips globally. This contradicts the capability-discovery framing: each chip is an independent reminder of one differentiator (officecli on folders, Excel, PPT). It should disappear globally when used or X'd, and otherwise show regardless of where the user is working. Drops sessionCount from the visibility computation. The resolver now only filters against the global dismissed list. sync.ready is kept as a hydration guard so the dismissed list doesn't briefly read as [] mid-load. sessionCount stays in the component but is consumed exclusively by the auto-dismiss lifecycle effect (detect new session → dismiss source chip). Replaces the now-wrong "returning visitor (sessions > 0) sees no suggestion list" e2e with one that asserts the correct semantic: after using one chip and creating a session, the home shows the remaining two chips (not zero).
PawWork ships an AI-friendly Office CLI on PATH, but nothing in the
system prompt or chip text told the model it exists. Two Excel/PPT
home chips quietly leaked the tool name ("用 officecli 读取") into
the prefill prompts to compensate, which both broke the "non-tech
user" framing and only helped when the user clicked the chip — not
when they typed "look at the budget spreadsheet on my desktop".
Move the awareness to where it belongs: a new `# Bundled
capabilities` section in pawwork.txt, placed after `# Tool
collaboration` so it reads as part of PawWork's identity rather
than an external dependency. Drop the implementation-detail
sentences from the zh/en chip prompts now that the awareness lives
in the system prompt.
The new section names officecli, points the model at `officecli
help` for syntax discovery, and locks the trigger to "reading or
modifying a real local Office file" so abstract topics like layout
advice don't accidentally invoke the CLI.
The bash tool prepends PawWork's bundled tools directory (containing officecli) to PATH so the agent can call those CLIs by bare name. The same was missing from two other surfaces a user can reach: - Prompt shell mode (`! <command>` in the composer) — running `! officecli help` in a packaged build still reported "command not found" because shellImpl built env from process.env directly. - The PTY terminal panel — same root cause. Extract `bundledToolsDir()` and `prependBundledTools()` helpers in util/env.ts so the three surfaces share one source of truth, and add unit coverage for the edge cases that matter most: the no-op branch when resourcesPath is missing, the guard against an empty resourcesPath leaking a relative "tools" entry into PATH, and the empty-currentPath case. Shell mode also gets `OFFICECLI_SKIP_UPDATE=1` matching the bash tool — the prompt `!` path is agent-adjacent, not the user's "native" terminal. PTY intentionally does not set that flag so `officecli update` in the terminal panel behaves like any other local shell.
POSIX treats an empty PATH segment (leading, trailing, or doubled colon) as the current directory. When prependBundledTools was called with an empty currentPath it emitted "<dir>:" - PawWork's bundled tools followed by an implicit cwd-shadowing entry. A malicious file in cwd named officecli would then shadow the bundled binary. Return the bundled dir alone instead, and add a regression test.
The dismiss button was hidden at rest via opacity-0 plus pointer-events-none, but it remained in the tab order. Restoring the rest-state guard would have excluded keyboard-only users from dismissing a chip. Reveal on focus-visible instead so Tab brings the button into view and re-enables pointer events, while pointer users still see the chip hover behavior unchanged.
The composer-placeholder test read localStorage["language"] which never exists; the LanguageProvider persists under "pawwork.global.dat:language" (see other e2e tests already using that key). The wrong key always missed, fell back to the test's "zh" default, and asserted the Chinese placeholder against whatever the CI runner's navigator.language detection produced - en-US in GitHub Actions, so the test deterministically failed. Use the same key the provider writes, and fall back to "en" to match detectLocale()'s final return when nothing is stored.
Two cuts to fix perf-probe-baseline regression. The Compare-confirmed run showed +183 to +267ms frame_gap_max on homepage-cold and tool-default-open-heavy-bash (both scenarios start with project.open() landing on home). Hard gate per the team: new features must not regress perf or any regression must be imperceptible. 1. Lazy-import HomeSuggestionList from session-new-view so the module load + reactive setup runs after the home pays its first paint. The chip rows appear within one frame of the import resolving; users never see the gap. 2. Drop prompt.dirty() from the lifecycle effect. It was there to clear currentChipSource when the user emptied the composer, but every keystroke flipped dirty(), making the effect a hot-path subscriber on the composer typing loop. The replacement behavior is simpler and explained in the source comment: chip source is sticky once clicked, so click-then-type-your-own-thing still dismisses the chip. The e2e "discarding chip prefill" test asserted the old reactive behavior and is replaced by a test that pins the new sticky-source semantics.
125ac5b to
c416e5e
Compare
On desktop, sync and settings are independent async hydrations backed by AsyncStorage (see packages/app/src/utils/persist.ts). The previous single guard on sync.ready let the dismissed-list accessor fall through to its withFallback([]) default during the gap before settings finished hydrating, which briefly re-showed chips the user had already dismissed. Require both stores ready before computing visibility. Web is unaffected (both stores are sync there). The unit test now pins the double guard so a refactor can't silently drop it.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/app/e2e/onboarding/home-suggestion-chips.spec.ts (2)
221-223: ⚡ Quick winReplace
execCommand("selectAll")with the cross-platformmodKeyshortcut helper.
execCommandis brittle for E2E input control and bypasses the repo’s keyboard shortcut convention.⌨️ Suggested change
+import { modKey } from "../utils" ... - await page.evaluate(() => { - document.execCommand("selectAll") - }) + await page.keyboard.press(`${modKey}+A`) await page.keyboard.press("Backspace")As per coding guidelines,
Use modKey from utils for cross-platform keyboard shortcuts (Meta on Mac, Control on Linux/Windows).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/e2e/onboarding/home-suggestion-chips.spec.ts` around lines 221 - 223, The test uses document.execCommand("selectAll") inside a page.evaluate call, which is brittle; replace it with the repository's cross-platform keyboard helper by importing modKey from utils and invoking the Playwright keyboard shortcut instead (e.g., use page.keyboard.press with `${modKey}+A` or page.keyboard.down(modKey)/press('a')/keyboard.up(modKey)). Update the line that calls page.evaluate(() => document.execCommand("selectAll")) to use modKey and page.keyboard so the select-all action follows the repo's modKey convention.
5-7: ⚡ Quick winRename selector constants to SCREAMING_SNAKE_CASE.
This file-level constant style is inconsistent with the E2E test convention and makes cross-spec maintenance noisier.
♻️ Suggested rename
-const suggestionListSelector = '[data-component="home-suggestion-list"]' -const rowSelector = '[data-action="home-suggestion-row"]' -const rowDismissSelector = '[data-action="home-suggestion-row-dismiss"]' +const SUGGESTION_LIST_SELECTOR = '[data-component="home-suggestion-list"]' +const ROW_SELECTOR = '[data-action="home-suggestion-row"]' +const ROW_DISMISS_SELECTOR = '[data-action="home-suggestion-row-dismiss"]'As per coding guidelines,
packages/app/e2e/**/*.spec.ts:Use SCREAMING_SNAKE_CASE for constants in tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/e2e/onboarding/home-suggestion-chips.spec.ts` around lines 5 - 7, Rename the file-level selector constants to SCREAMING_SNAKE_CASE and update all usages: change suggestionListSelector -> SUGGESTION_LIST_SELECTOR, rowSelector -> ROW_SELECTOR, and rowDismissSelector -> ROW_DISMISS_SELECTOR in this spec; ensure imports/exports (if any) and every reference inside packages/app/e2e/onboarding/home-suggestion-chips.spec.ts are updated to the new names to keep the test consistent with the E2E convention.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/app/e2e/onboarding/home-suggestion-chips.spec.ts`:
- Around line 183-184: Replace the direct immediate localStorage assertion that
calls readDismissedFromStorage(page) and checks dismissed toContain firstChipID!
with a polling-style assertion using expect.poll to retry until persistence
completes; specifically, wrap the readDismissedFromStorage(page) call inside
expect.poll(...) and assert that the returned value contains firstChipID! (and
apply the same pattern for the other occurrences referenced around lines 200-203
and 229-230), ensuring you reference the readDismissedFromStorage helper, the
dismissed result, and the firstChipID identifier when making the change.
In `@packages/app/src/i18n/en.ts`:
- Line 573: Update the translation value for the key
"home.suggestion.row.dismiss" to use consistent terminology with the feature
(e.g., change "Dismiss this prompt" to "Dismiss this suggestion" or simply
"Dismiss") so it matches other labels that refer to suggestion chips; modify the
value in packages/app/src/i18n/en.ts for the "home.suggestion.row.dismiss" entry
accordingly.
In `@packages/opencode/src/session/prompt.ts`:
- Around line 1212-1219: The code currently always reads/writes the uppercase
PATH which can drop Windows' "Path" entry; update the merge that builds env so
you resolve the existing path key case-insensitively (check shellEnvRecord and
process.env for "PATH" or "Path") and pass that resolved value into
prependBundledTools using the nullish coalescing operator so an explicit empty
plugin value is preserved; apply this change around the env construction that
references shellEnvRecord, currentPath and prependBundledTools (and replicate
the same fix in the analogous spots in pty/index.ts and tool/bash.ts).
---
Nitpick comments:
In `@packages/app/e2e/onboarding/home-suggestion-chips.spec.ts`:
- Around line 221-223: The test uses document.execCommand("selectAll") inside a
page.evaluate call, which is brittle; replace it with the repository's
cross-platform keyboard helper by importing modKey from utils and invoking the
Playwright keyboard shortcut instead (e.g., use page.keyboard.press with
`${modKey}+A` or page.keyboard.down(modKey)/press('a')/keyboard.up(modKey)).
Update the line that calls page.evaluate(() =>
document.execCommand("selectAll")) to use modKey and page.keyboard so the
select-all action follows the repo's modKey convention.
- Around line 5-7: Rename the file-level selector constants to
SCREAMING_SNAKE_CASE and update all usages: change suggestionListSelector ->
SUGGESTION_LIST_SELECTOR, rowSelector -> ROW_SELECTOR, and rowDismissSelector ->
ROW_DISMISS_SELECTOR in this spec; ensure imports/exports (if any) and every
reference inside packages/app/e2e/onboarding/home-suggestion-chips.spec.ts are
updated to the new names to keep the test consistent with the E2E convention.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f13d19b4-f2ec-4ba4-91c0-61cc46189831
📒 Files selected for processing (20)
packages/app/e2e/onboarding/home-suggestion-chips.spec.tspackages/app/src/components/home/home-suggestion-list.test.tspackages/app/src/components/home/home-suggestion-list.tsxpackages/app/src/components/home/home-suggestions-state.test.tspackages/app/src/components/home/home-suggestions-state.tspackages/app/src/components/prompt-input.tsxpackages/app/src/components/prompt-input/placeholder.test.tspackages/app/src/components/prompt-input/placeholder.tspackages/app/src/components/prompt-input/store-types.tspackages/app/src/components/session/session-new-view.tsxpackages/app/src/context/settings.tsxpackages/app/src/i18n/en.tspackages/app/src/i18n/zh.tspackages/opencode/src/pty/index.tspackages/opencode/src/session/prompt.tspackages/opencode/src/session/prompt/pawwork.txtpackages/opencode/src/tool/bash.tspackages/opencode/src/util/env.tspackages/opencode/test/config/e2e-smoke-tagging.test.tspackages/opencode/test/util/env.test.ts
💤 Files with no reviewable changes (1)
- packages/app/src/components/prompt-input/store-types.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- packages/app/src/components/prompt-input/placeholder.test.ts
- packages/opencode/src/session/prompt/pawwork.txt
- packages/app/src/components/home/home-suggestions-state.ts
- packages/opencode/test/config/e2e-smoke-tagging.test.ts
- packages/app/src/components/session/session-new-view.tsx
- packages/app/src/components/home/home-suggestions-state.test.ts
- packages/app/src/context/settings.tsx
- packages/opencode/src/tool/bash.ts
- packages/app/src/i18n/zh.ts
- packages/app/src/components/prompt-input/placeholder.ts
- packages/opencode/src/pty/index.ts
- packages/opencode/src/util/env.ts
- packages/opencode/test/util/env.test.ts
- packages/app/src/components/prompt-input.tsx
The aria-label said "Dismiss this prompt" / "关闭该提示词", but the feature is named "suggestion" everywhere else (home-suggestion-list, home-suggestion-row, homeSuggestionsDismissed, home.suggestion.*). "prompt" collides with the composer terminology (prompt-input, the prompt context). Use the feature noun in both locales.
On Windows process.env exposes the path under "Path" (or any other casing the OS chose), and shell.env plugins are free to emit "Path" or "path". The previous code at three spawn sites read shellEnv.PATH and process.env.PATH directly, so the lookup missed Windows values and fell through to "". The merge then carried the inherited "Path" from the process env into the spread, and we wrote a separate "PATH" on top. The spawned child received both keys with implementation- defined precedence, which could drop the inherited system path. Add stripPathKeys to packages/opencode/src/util/env.ts, alongside the existing envValueCaseInsensitive read helper, and use the pair at every prependBundledTools call site: - packages/opencode/src/tool/bash.ts - packages/opencode/src/session/prompt.ts - packages/opencode/src/pty/index.ts After the change each call resolves currentPath case-insensitively from the available env sources, strips every PATH casing from the merged env, then writes back a single canonical PATH. New unit tests in test/util/env.test.ts pin the strip behavior.
Two nitpick fixes for this file:
- Selector constants now use SCREAMING_SNAKE_CASE, matching the
convention documented for e2e specs and the surrounding files.
- The drain-the-prefill step replaces document.execCommand("selectAll")
with the cross-platform modKey helper from packages/app/e2e/utils.ts;
execCommand is the brittle path and modKey is what every other spec
uses for Meta-on-Mac / Control-elsewhere shortcuts.
No behavior change; renames are local to this file.
Prepare PawWork v2026.5.18 for the stable desktop release. - Bump the desktop package version to 2026.5.18. - Scope the diagnostics unreadable-file retention test to POSIX permission semantics so Windows advisory does not fail on chmod behavior that Windows does not enforce the same way. Verification: - Focused desktop diagnostics test passed locally: 11 pass / 0 fail. - Release typecheck passed locally for packages/desktop-electron. - PR #706 CI passed, including ci, desktop-smoke, e2e-artifacts, CodeQL, dependency-review, label-policy, commit-lint, and title lint. Release notes: - Drafted against the merged range since v2026.5.17: #691, #692, #693, #694, #702, and #703. - Cold-read review completed before merge; wording was tightened to avoid overclaiming diagnostics impact and to keep verification short.
Summary
Replace the rotating 25-prompt placeholder pool and 6.5s interval with a single static composer placeholder ("Type your task, or @ to mention files" / "输入你的任务,或 @ 引用文件"), and add a 3-row suggestion list above the home composer for first-time visitors. Each row prefills the composer on click; rows have a hover-revealed X for per-row dismiss. Dismissing all three rows naturally collapses the section. No section-level dismiss, no Settings toggle, no Restore button, no
seenflag — the minimum that carries the design intent.Why
Spec at
docs/superpowers/specs/2026-05-17-onboarding-design.md(research atdocs/research/2026-05-17-onboarding-direction.md). PawWork is positioned as an open-source local Claude Cowork alternative for non-technical users; the prior placeholder rotation gave new visitors no visible affordance to start work, and the 25-string i18n set was high-friction churn. The new design follows the validated "0 wizard, 3 concrete tasks, show don't tell" pattern: zero modal, zero tour, zero overlay, deterministic placeholder.Initial implementation also shipped a Settings master toggle, a Restore button, and a one-way
homeSuggestionsSeenflag latching across hydration. Five rounds of crosscheck and a single dev:desktop run revealed that complexity was load-bearing for nobody: the Settings page renders outside the Sync provider souseSyncinSettingsGeneralthrew at runtime, and the two-flag interaction generated a P1 cascade at every round. The final commit strips everything that the three-question test cannot justify, leaving one persisted field (homeSuggestionsDismissed: string[]) and one render path.Related Issue
None. Spec-driven follow-up to the home redesign track.
Human Review Status
Pending.
Review Focus
sync.ready && settings.ready()) and the persistedhomeSuggestionsDismissedlist, not by per-project session count. Returning users with sessions still see undismissed chips, and switching workspaces does not re-pitch chips the user has already dismissed.sessionCountis observed only by the auto-dismiss effect, never by the visibility memo.prompt.set(...)— the previous dirty-guard was removed deliberately so "try chip A, then chip B" exploration actually works. The chip lifecycle state machine handles ownership:currentChipSourceis sticky once a chip is clicked, and any subsequent session create dismisses that chip. Clicking another chip overwrites the source; clearing the composer does not reset it. The contract is pinned byhome-suggestion-list.test.tsand by the e2e cases "switching chips before send dismisses only the last selection", "using a chip via send auto-dismisses it", and "clicking a chip then sending any content dismisses it (sticky source)".if (!sync.ready || !settings.ready()) return []. Sync and settings are independent async hydrations on desktop (packages/app/src/utils/persist.tsAsyncStorage branch), so gating onsync.readyalone would let the dismissed-list accessor fall through itswithFallback([])default during the gap, briefly re-showing chips the user has already dismissed. Web is unaffected (both stores are sync there).HomeSuggestionListis mounted vialazy()+<Suspense>fromsession-new-view.tsxso the chip module and its reactive setup do not enter the home cold-paint window. The lifecycle effect tracks onlysessionCount, notprompt.dirty()— subscribing the effect to per-keystroke composer changes was a footgun visible in perf-probe-baseline before the rework.homeSuggestionsEnabled, nohomeSuggestionsSeen, nohome-suggestion-section-dismissdata-action, nohome.suggestion.section.*i18n keys.packages/opencode/test/config/e2e-smoke-tagging.test.tswas updated in alphabetical order. Four @smoke entries cover: 3 rows visible, click prefills composer, per-row X persists across reload, static home placeholder. Seven non-smoke e2e cases pin the chip lifecycle and layout contracts.Risk Notes
settings.v3(homeSuggestionsDismissed: string[]), gated bywithFallback. Existing installs default to empty. No migration step required.prompt.example.*i18n keys plusprompt.placeholder.normal/prompt.placeholder.simple. Added oneprompt.placeholder.homeand fourhome.suggestion.*keys (1 aria-label + 3 chip texts). zh/en parity verified by the existing i18n parity test.prompt.dirty()branch and avoids a per-keystroke effect re-run. Captured in the "clicking a chip then sending any content dismisses it (sticky source)" e2e case.How To Verify
Screenshots or Recordings
Pending: dev:desktop manual verification with screenshots will be attached as a follow-up comment before requesting merge.
Checklist
dev, and my PR title and commit messages use Conventional Commits in EnglishSummary by CodeRabbit
New Features
Changes