fix(app): apply home model selector choices#206
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughCentralizes model identity checks in E2E tests, refactors session lifecycle handling, adds coverage for home model selection and keyboard behavior, and changes a popover's focus-out handler to ignore focus-out events caused by a pointer-down inside the popover. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
Comment |
There was a problem hiding this comment.
Code Review
This pull request updates E2E tests for prompt focus and model selection, including a new test case to verify that the selected model is correctly applied to new sessions. In the UI components, the model selector popover was modified to prevent closing on outside focus. However, feedback suggests that this unconditional prevention may hinder keyboard navigation and accessibility, recommending a more specific check instead of a global preventDefault.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/app/e2e/prompt/prompt-footer-focus.spec.ts`:
- Around line 133-140: The test currently reads backend messages once after
calling project.prompt and can race persistence; change the direct call to
project.sdk.session.messages and immediate assert to use expect.poll to
repeatedly fetch project.sdk.session.messages({ sessionID, limit: 50 }) until a
message with info.role === "user" and info.model matching { providerID:
next.providerID, modelID: next.modelID } appears, then perform the assertion;
keep using the same sessionID from project.prompt and reference sessionID,
project.prompt, project.sdk.session.messages and expect.poll when implementing
the retry/polling logic.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0684855b-b037-4aa3-b6f7-0dfdebc22303
📒 Files selected for processing (2)
packages/app/e2e/prompt/prompt-footer-focus.spec.tspackages/app/src/components/dialog-select-model.tsx
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: unit-windows-desktop
- GitHub Check: unit-windows-opencode-session
- GitHub Check: unit-windows-opencode-config-project
- GitHub Check: unit-windows-app
- GitHub Check: unit-windows-opencode-server-tools
- GitHub Check: typecheck
- GitHub Check: unit-opencode
- GitHub Check: unit-app
- GitHub Check: unit-desktop
- GitHub Check: smoke-macos-arm64
- GitHub Check: e2e-artifacts
- GitHub Check: analyze-js-ts
🧰 Additional context used
📓 Path-based instructions (2)
packages/app/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (packages/app/AGENTS.md)
Always prefer
createStoreover multiplecreateSignalcalls in SolidJS
Files:
packages/app/src/components/dialog-select-model.tsxpackages/app/e2e/prompt/prompt-footer-focus.spec.ts
packages/app/e2e/**/*.spec.ts
📄 CodeRabbit inference engine (packages/app/e2e/AGENTS.md)
packages/app/e2e/**/*.spec.ts: Import test utilities from../fixturesinstead of@playwright/test
Test files should be named with the patternfeature-name.spec.ts
Use lowercase, descriptive test names (e.g., 'sidebar can be toggled')
Use camelCase for variable names in tests
Use SCREAMING_SNAKE_CASE for constants in tests
Use fixture-managed cleanup withwithSession(sdk, title, callback)for temporary sessions instead of callingsdk.session.delete(...)directly
Prefer theprojectfixture for tests that need a dedicated project with LLM mocking
Usedata-component,data-action, or semantic roles for selectors instead of CSS class names or IDs
UsemodKeyfrom utils for cross-platform keyboard shortcuts (Meta on Mac, Control on Linux/Windows)
In terminal tests, type through the browser usingrunTerminal()andwaitTerminalReady()instead of writing to the PTY through the SDK
Never use wall-clock waits likepage.waitForTimeout(...)to make a test pass
Wait on observable state withexpect(...),expect.poll(...), or existing helpers instead of assuming work is finished after an action
Use locator assertions liketoBeVisible(),toHaveCount(0), andtoHaveAttribute(...)for normal UI state verification
Useexpect.poll(...)for probing mock or backend state rather than transient DOM visibility
Prefer fluent helpers and drivers when they make intent obvious and reduce locator-heavy noise in tests
Use direct locators when the interaction is simple and a helper would not add clarity
When validating routing, assert against canonical or resolved workspace slugs using shared helpers from../actionsto account for Windows canonicalization
Test one feature per test file
Callproject.trackSession(sessionID, directory?)andproject.trackDirectory(directory)for any resources created outside the fixture so teardown can clean them up
Files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
🧠 Learnings (15)
📚 Learning: 2026-04-23T07:23:23.849Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 180
File: packages/app/src/components/session/session-new-view.tsx:13-18
Timestamp: 2026-04-23T07:23:23.849Z
Learning: In pawwork (Astro-Han/pawwork), prefer using `createStore` instead of multiple `createSignal` calls only when the signals represent **coupled** object state that is updated together (i.e., there is at least one shared batch-update site where the state is changed in the same transaction). If the state fields are **independent** and are mutated by separate handlers (e.g., one handler updates only `selectedSkill` while another updates only `mode`), keep them as individual `createSignal` calls—using `createStore` for truly independent fields adds boilerplate without behavioral benefit.
Applied to files:
packages/app/src/components/dialog-select-model.tsxpackages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-23T15:10:21.635Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 191
File: packages/app/src/components/session/pawwork-skill-meta.ts:38-39
Timestamp: 2026-04-23T15:10:21.635Z
Learning: This repo configures Tailwind v4 with `--color-*: initial`, which effectively breaks standard Tailwind palette utilities (e.g., `text-violet-500` can resolve to no CSS variable and render as a no-op/black). For brand/accent colors that are not backed by semantic design tokens, use inline styles with the exact hex value (e.g., `style={{ color: '#8B5FBF' }}` / `homeIconStyle: { color: '#8B5FBF' }`) and add a short comment explaining that Tailwind palette utilities won’t work due to the `--color-*: initial` setup. Do not suggest replacing these inline hex colors with Tailwind palette classes anywhere in this repo.
Applied to files:
packages/app/src/components/dialog-select-model.tsx
📚 Learning: 2026-04-23T15:25:27.182Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 193
File: packages/app/e2e/sidebar/sidebar-leading-slot.spec.ts:5-55
Timestamp: 2026-04-23T15:25:27.182Z
Learning: In Astro-Han/pawwork E2E tests (e.g., *.spec.ts under packages/app/e2e), reaching a real "running" session state is not achievable with the bare `sdk` fixture. Use the `project` fixture (to bootstrap the model) and orchestrate the transition with `llm.wait(1)`; even if you set `agent: "build"` and a `system` prompt via `sdk.session.promptAsync`, the current test infrastructure does not trigger an actual LLM call, so it won’t simulate "running" cheaply. Review any attempt to mock/force "running" using only `sdk` as likely ineffective unless it also uses `project` + `llm.wait(1)`.
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-24T00:02:50.599Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 203
File: packages/app/e2e/sidebar/sidebar-session-links.spec.ts:34-55
Timestamp: 2026-04-24T00:02:50.599Z
Learning: For Astro-Han/pawwork E2E tests under packages/app/e2e/**/*.spec.ts, do not call project.trackDirectory() or project.trackSession() before project.open() has run. The project fixture throws until open() initializes internal state. Use this ordering pattern: (1) call project.trackSession(sessionID) inside the beforeGoto callback (where state is already available), (2) call project.trackDirectory(directory) and any cross-workspace tracking like project.trackSession(id, directory) immediately after project.open() returns, and (3) if you create any resources before open() that cannot yet be tracked via the fixture, ensure you clean them up explicitly in finally blocks (e.g., cleanupSession / cleanupTestProject).
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Use fixture-managed cleanup with `withSession(sdk, title, callback)` for temporary sessions instead of calling `sdk.session.delete(...)` directly
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Prefer the `project` fixture for tests that need a dedicated project with LLM mocking
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Prefer fluent helpers and drivers when they make intent obvious and reduce locator-heavy noise in tests
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Call `project.trackSession(sessionID, directory?)` and `project.trackDirectory(directory)` for any resources created outside the fixture so teardown can clean them up
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Use `expect.poll(...)` for probing mock or backend state rather than transient DOM visibility
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : In terminal tests, type through the browser using `runTerminal()` and `waitTerminalReady()` instead of writing to the PTY through the SDK
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Import test utilities from `../fixtures` instead of `playwright/test`
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Test one feature per test file
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Use locator assertions like `toBeVisible()`, `toHaveCount(0)`, and `toHaveAttribute(...)` for normal UI state verification
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Use direct locators when the interaction is simple and a helper would not add clarity
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-20T14:35:49.291Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/AGENTS.md:0-0
Timestamp: 2026-04-20T14:35:49.291Z
Learning: Use `agent-browser` for web automation following the core workflow: (1) `agent-browser open <url>` to navigate, (2) `agent-browser snapshot -i` to get interactive elements with refs, (3) interact using refs with `agent-browser click e1` / `fill e2 "text"`, (4) re-snapshot after page changes
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
🔇 Additional comments (3)
packages/app/src/components/dialog-select-model.tsx (1)
151-153: Good fix for internal model-item click flow.This keeps pointer-outside/escape dismissal intact while preventing focus-outside from closing the popover before item selection completes.
packages/app/e2e/prompt/prompt-footer-focus.spec.ts (2)
32-33: Nice helper extraction for model identity comparisons.Centralizing model key formatting removes duplicate string construction and keeps comparisons consistent.
62-87: Session setup refactor is solid.Using
project.open()+withSession(...)makes lifecycle management explicit and keeps the agent-focus test deterministic.
e5172b6 to
589267f
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/app/e2e/prompt/prompt-footer-focus.spec.ts`:
- Around line 64-66: The test double-registers the temporary session:
withSession(...) already owns and cleans the session in its finally block, so
remove the extra ownership to avoid duplicate teardown errors by deleting the
project.trackSession(session.id) call inside the withSession callback (leave
project.gotoSession(session.id) intact); alternatively, if you need to record
the session without taking ownership, replace project.trackSession(...) with a
non-owning recorder API if one exists, but do not register the same session for
cleanup twice.
- Around line 133-145: The test creates a real session via project.prompt() and
assigns it to sessionID but never registers it for teardown; after obtaining
sessionID from project.prompt (the variable named sessionID used in the poll),
call project.trackSession(sessionID, <optionalDirectoryIfCreated>) to register
the session with the project fixture (and call project.trackDirectory(directory)
if the prompt created any filesystem directory) so the fixture can clean up on
teardown; update the block that declares sessionID to immediately call
project.trackSession(sessionID) before using sessionID in
project.sdk.session.messages.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: edca81e5-8e33-4bb1-942a-6a727103e07c
📒 Files selected for processing (2)
packages/app/e2e/prompt/prompt-footer-focus.spec.tspackages/app/src/components/dialog-select-model.tsx
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: unit-windows-opencode-config-project
- GitHub Check: unit-windows-app
- GitHub Check: unit-windows-opencode-server-tools
- GitHub Check: unit-windows-opencode-session
- GitHub Check: typecheck
- GitHub Check: unit-app
- GitHub Check: unit-opencode
- GitHub Check: unit-desktop
- GitHub Check: smoke-macos-arm64
- GitHub Check: analyze-js-ts
- GitHub Check: e2e-artifacts
🧰 Additional context used
📓 Path-based instructions (2)
packages/app/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (packages/app/AGENTS.md)
Always prefer
createStoreover multiplecreateSignalcalls in SolidJS
Files:
packages/app/src/components/dialog-select-model.tsxpackages/app/e2e/prompt/prompt-footer-focus.spec.ts
packages/app/e2e/**/*.spec.ts
📄 CodeRabbit inference engine (packages/app/e2e/AGENTS.md)
packages/app/e2e/**/*.spec.ts: Import test utilities from../fixturesinstead of@playwright/test
Test files should be named with the patternfeature-name.spec.ts
Use lowercase, descriptive test names (e.g., 'sidebar can be toggled')
Use camelCase for variable names in tests
Use SCREAMING_SNAKE_CASE for constants in tests
Use fixture-managed cleanup withwithSession(sdk, title, callback)for temporary sessions instead of callingsdk.session.delete(...)directly
Prefer theprojectfixture for tests that need a dedicated project with LLM mocking
Usedata-component,data-action, or semantic roles for selectors instead of CSS class names or IDs
UsemodKeyfrom utils for cross-platform keyboard shortcuts (Meta on Mac, Control on Linux/Windows)
In terminal tests, type through the browser usingrunTerminal()andwaitTerminalReady()instead of writing to the PTY through the SDK
Never use wall-clock waits likepage.waitForTimeout(...)to make a test pass
Wait on observable state withexpect(...),expect.poll(...), or existing helpers instead of assuming work is finished after an action
Use locator assertions liketoBeVisible(),toHaveCount(0), andtoHaveAttribute(...)for normal UI state verification
Useexpect.poll(...)for probing mock or backend state rather than transient DOM visibility
Prefer fluent helpers and drivers when they make intent obvious and reduce locator-heavy noise in tests
Use direct locators when the interaction is simple and a helper would not add clarity
When validating routing, assert against canonical or resolved workspace slugs using shared helpers from../actionsto account for Windows canonicalization
Test one feature per test file
Callproject.trackSession(sessionID, directory?)andproject.trackDirectory(directory)for any resources created outside the fixture so teardown can clean them up
Files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
🧠 Learnings (16)
📚 Learning: 2026-04-23T15:26:07.250Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 193
File: packages/app/src/pages/layout/sidebar-items.tsx:102-107
Timestamp: 2026-04-23T15:26:07.250Z
Learning: In Astro-Han/pawwork (`packages/app/src/pages/layout/sidebar-items.tsx`), the `indicator()` function in `SessionRow` intentionally renders `props.leadingSlot` (the pin button) only as a fallback when no status indicator (running/permission/error/unseen) is active. When a higher-priority status wins the slot, the pin button is removed from the DOM — this is a deliberate design choice for the merged leading slot (`#150`). The keyboard unpin path is preserved via: (1) focusing the row anchor triggers `group-focus-within` which reveals the dots menu trigger, then Tab → Enter → "Unpin Session"; (2) the context menu (right-click / Shift+F10) exposes "Unpin Session". The "always render + CSS overlay" approach was considered but rejected due to z-index/pointer-events complexity; residual `...` slot behavior is tracked in `#192`. Do NOT flag the absence of the pin button from the DOM when a status is active as an accessibility regression.
Applied to files:
packages/app/src/components/dialog-select-model.tsx
📚 Learning: 2026-04-23T07:23:23.849Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 180
File: packages/app/src/components/session/session-new-view.tsx:13-18
Timestamp: 2026-04-23T07:23:23.849Z
Learning: In pawwork (Astro-Han/pawwork), prefer using `createStore` instead of multiple `createSignal` calls only when the signals represent **coupled** object state that is updated together (i.e., there is at least one shared batch-update site where the state is changed in the same transaction). If the state fields are **independent** and are mutated by separate handlers (e.g., one handler updates only `selectedSkill` while another updates only `mode`), keep them as individual `createSignal` calls—using `createStore` for truly independent fields adds boilerplate without behavioral benefit.
Applied to files:
packages/app/src/components/dialog-select-model.tsxpackages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-23T15:10:21.635Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 191
File: packages/app/src/components/session/pawwork-skill-meta.ts:38-39
Timestamp: 2026-04-23T15:10:21.635Z
Learning: This repo configures Tailwind v4 with `--color-*: initial`, which effectively breaks standard Tailwind palette utilities (e.g., `text-violet-500` can resolve to no CSS variable and render as a no-op/black). For brand/accent colors that are not backed by semantic design tokens, use inline styles with the exact hex value (e.g., `style={{ color: '#8B5FBF' }}` / `homeIconStyle: { color: '#8B5FBF' }`) and add a short comment explaining that Tailwind palette utilities won’t work due to the `--color-*: initial` setup. Do not suggest replacing these inline hex colors with Tailwind palette classes anywhere in this repo.
Applied to files:
packages/app/src/components/dialog-select-model.tsx
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Prefer fluent helpers and drivers when they make intent obvious and reduce locator-heavy noise in tests
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-24T00:02:50.599Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 203
File: packages/app/e2e/sidebar/sidebar-session-links.spec.ts:34-55
Timestamp: 2026-04-24T00:02:50.599Z
Learning: For Astro-Han/pawwork E2E tests under packages/app/e2e/**/*.spec.ts, do not call project.trackDirectory() or project.trackSession() before project.open() has run. The project fixture throws until open() initializes internal state. Use this ordering pattern: (1) call project.trackSession(sessionID) inside the beforeGoto callback (where state is already available), (2) call project.trackDirectory(directory) and any cross-workspace tracking like project.trackSession(id, directory) immediately after project.open() returns, and (3) if you create any resources before open() that cannot yet be tracked via the fixture, ensure you clean them up explicitly in finally blocks (e.g., cleanupSession / cleanupTestProject).
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Use fixture-managed cleanup with `withSession(sdk, title, callback)` for temporary sessions instead of calling `sdk.session.delete(...)` directly
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : In terminal tests, type through the browser using `runTerminal()` and `waitTerminalReady()` instead of writing to the PTY through the SDK
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-23T15:25:27.182Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 193
File: packages/app/e2e/sidebar/sidebar-leading-slot.spec.ts:5-55
Timestamp: 2026-04-23T15:25:27.182Z
Learning: In Astro-Han/pawwork E2E tests (e.g., *.spec.ts under packages/app/e2e), reaching a real "running" session state is not achievable with the bare `sdk` fixture. Use the `project` fixture (to bootstrap the model) and orchestrate the transition with `llm.wait(1)`; even if you set `agent: "build"` and a `system` prompt via `sdk.session.promptAsync`, the current test infrastructure does not trigger an actual LLM call, so it won’t simulate "running" cheaply. Review any attempt to mock/force "running" using only `sdk` as likely ineffective unless it also uses `project` + `llm.wait(1)`.
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Prefer the `project` fixture for tests that need a dedicated project with LLM mocking
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Use `expect.poll(...)` for probing mock or backend state rather than transient DOM visibility
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Use locator assertions like `toBeVisible()`, `toHaveCount(0)`, and `toHaveAttribute(...)` for normal UI state verification
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Use direct locators when the interaction is simple and a helper would not add clarity
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Use `data-component`, `data-action`, or semantic roles for selectors instead of CSS class names or IDs
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Wait on observable state with `expect(...)`, `expect.poll(...)`, or existing helpers instead of assuming work is finished after an action
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Import test utilities from `../fixtures` instead of `playwright/test`
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-20T14:35:49.291Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/AGENTS.md:0-0
Timestamp: 2026-04-20T14:35:49.291Z
Learning: Use `agent-browser` for web automation following the core workflow: (1) `agent-browser open <url>` to navigate, (2) `agent-browser snapshot -i` to get interactive elements with refs, (3) interact using refs with `agent-browser click e1` / `fill e2 "text"`, (4) re-snapshot after page changes
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
🔇 Additional comments (1)
packages/app/src/components/dialog-select-model.tsx (1)
113-131: Nice regression guard for internal pointer selection.Preventing the cancelable focus-out only when it follows a pointer-down inside the popover keeps list-item clicks/selects alive without weakening Escape or outside-pointer dismissal.
Also applies to: 170-172
589267f to
d303e88
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/app/e2e/prompt/prompt-footer-focus.spec.ts`:
- Around line 61-65: The test creates a session via
project.sdk.session.create(...) but never registers it for teardown, risking
leaked sessions; after creating the session and confirming session.id, call
project.trackSession(session.id) (and project.trackDirectory(...) if you created
any directories) so the fixture teardown can clean the resource; update the test
around the session creation in prompt-footer-focus.spec.ts to call
project.trackSession(session.id) immediately after obtaining session.id.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c7e84f8f-18ca-4a55-8330-72fff5144bcc
📒 Files selected for processing (2)
packages/app/e2e/prompt/prompt-footer-focus.spec.tspackages/app/src/components/dialog-select-model.tsx
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: unit-windows-app
- GitHub Check: unit-windows-desktop
- GitHub Check: unit-windows-opencode-session
- GitHub Check: unit-windows-opencode-server-tools
- GitHub Check: unit-windows-opencode-config-project
- GitHub Check: unit-app
- GitHub Check: typecheck
- GitHub Check: unit-opencode
- GitHub Check: unit-desktop
- GitHub Check: smoke-macos-arm64
- GitHub Check: analyze-js-ts
- GitHub Check: e2e-artifacts
🧰 Additional context used
📓 Path-based instructions (2)
packages/app/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (packages/app/AGENTS.md)
Always prefer
createStoreover multiplecreateSignalcalls in SolidJS
Files:
packages/app/src/components/dialog-select-model.tsxpackages/app/e2e/prompt/prompt-footer-focus.spec.ts
packages/app/e2e/**/*.spec.ts
📄 CodeRabbit inference engine (packages/app/e2e/AGENTS.md)
packages/app/e2e/**/*.spec.ts: Import test utilities from../fixturesinstead of@playwright/test
Test files should be named with the patternfeature-name.spec.ts
Use lowercase, descriptive test names (e.g., 'sidebar can be toggled')
Use camelCase for variable names in tests
Use SCREAMING_SNAKE_CASE for constants in tests
Use fixture-managed cleanup withwithSession(sdk, title, callback)for temporary sessions instead of callingsdk.session.delete(...)directly
Prefer theprojectfixture for tests that need a dedicated project with LLM mocking
Usedata-component,data-action, or semantic roles for selectors instead of CSS class names or IDs
UsemodKeyfrom utils for cross-platform keyboard shortcuts (Meta on Mac, Control on Linux/Windows)
In terminal tests, type through the browser usingrunTerminal()andwaitTerminalReady()instead of writing to the PTY through the SDK
Never use wall-clock waits likepage.waitForTimeout(...)to make a test pass
Wait on observable state withexpect(...),expect.poll(...), or existing helpers instead of assuming work is finished after an action
Use locator assertions liketoBeVisible(),toHaveCount(0), andtoHaveAttribute(...)for normal UI state verification
Useexpect.poll(...)for probing mock or backend state rather than transient DOM visibility
Prefer fluent helpers and drivers when they make intent obvious and reduce locator-heavy noise in tests
Use direct locators when the interaction is simple and a helper would not add clarity
When validating routing, assert against canonical or resolved workspace slugs using shared helpers from../actionsto account for Windows canonicalization
Test one feature per test file
Callproject.trackSession(sessionID, directory?)andproject.trackDirectory(directory)for any resources created outside the fixture so teardown can clean them up
Files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
🧠 Learnings (22)
📚 Learning: 2026-04-23T15:26:07.250Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 193
File: packages/app/src/pages/layout/sidebar-items.tsx:102-107
Timestamp: 2026-04-23T15:26:07.250Z
Learning: In Astro-Han/pawwork (`packages/app/src/pages/layout/sidebar-items.tsx`), the `indicator()` function in `SessionRow` intentionally renders `props.leadingSlot` (the pin button) only as a fallback when no status indicator (running/permission/error/unseen) is active. When a higher-priority status wins the slot, the pin button is removed from the DOM — this is a deliberate design choice for the merged leading slot (`#150`). The keyboard unpin path is preserved via: (1) focusing the row anchor triggers `group-focus-within` which reveals the dots menu trigger, then Tab → Enter → "Unpin Session"; (2) the context menu (right-click / Shift+F10) exposes "Unpin Session". The "always render + CSS overlay" approach was considered but rejected due to z-index/pointer-events complexity; residual `...` slot behavior is tracked in `#192`. Do NOT flag the absence of the pin button from the DOM when a status is active as an accessibility regression.
Applied to files:
packages/app/src/components/dialog-select-model.tsx
📚 Learning: 2026-04-23T07:23:23.849Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 180
File: packages/app/src/components/session/session-new-view.tsx:13-18
Timestamp: 2026-04-23T07:23:23.849Z
Learning: In pawwork (Astro-Han/pawwork), prefer using `createStore` instead of multiple `createSignal` calls only when the signals represent **coupled** object state that is updated together (i.e., there is at least one shared batch-update site where the state is changed in the same transaction). If the state fields are **independent** and are mutated by separate handlers (e.g., one handler updates only `selectedSkill` while another updates only `mode`), keep them as individual `createSignal` calls—using `createStore` for truly independent fields adds boilerplate without behavioral benefit.
Applied to files:
packages/app/src/components/dialog-select-model.tsxpackages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-23T15:10:21.635Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 191
File: packages/app/src/components/session/pawwork-skill-meta.ts:38-39
Timestamp: 2026-04-23T15:10:21.635Z
Learning: This repo configures Tailwind v4 with `--color-*: initial`, which effectively breaks standard Tailwind palette utilities (e.g., `text-violet-500` can resolve to no CSS variable and render as a no-op/black). For brand/accent colors that are not backed by semantic design tokens, use inline styles with the exact hex value (e.g., `style={{ color: '#8B5FBF' }}` / `homeIconStyle: { color: '#8B5FBF' }`) and add a short comment explaining that Tailwind palette utilities won’t work due to the `--color-*: initial` setup. Do not suggest replacing these inline hex colors with Tailwind palette classes anywhere in this repo.
Applied to files:
packages/app/src/components/dialog-select-model.tsx
📚 Learning: 2026-04-24T03:51:54.050Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 206
File: packages/app/e2e/prompt/prompt-footer-focus.spec.ts:131-143
Timestamp: 2026-04-24T03:51:54.050Z
Learning: In Astro-Han/pawwork E2E tests under packages/app/e2e, do not manually call `project.trackSession(sessionID)` when you obtain a `sessionID` via `project.prompt(text)`. The `project.prompt()` implementation already registers `trackSession(next.sessionID, active.directory)` automatically after the prompt submission is observed and the active session is resolved, so calling `project.trackSession(sessionID)` again will create duplicate session ownership/teardown handling.
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Prefer fluent helpers and drivers when they make intent obvious and reduce locator-heavy noise in tests
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : In terminal tests, type through the browser using `runTerminal()` and `waitTerminalReady()` instead of writing to the PTY through the SDK
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Use `expect.poll(...)` for probing mock or backend state rather than transient DOM visibility
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Use `modKey` from utils for cross-platform keyboard shortcuts (Meta on Mac, Control on Linux/Windows)
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Use fixture-managed cleanup with `withSession(sdk, title, callback)` for temporary sessions instead of calling `sdk.session.delete(...)` directly
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Prefer the `project` fixture for tests that need a dedicated project with LLM mocking
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Use locator assertions like `toBeVisible()`, `toHaveCount(0)`, and `toHaveAttribute(...)` for normal UI state verification
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Use direct locators when the interaction is simple and a helper would not add clarity
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Use `data-component`, `data-action`, or semantic roles for selectors instead of CSS class names or IDs
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-23T15:25:27.182Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 193
File: packages/app/e2e/sidebar/sidebar-leading-slot.spec.ts:5-55
Timestamp: 2026-04-23T15:25:27.182Z
Learning: In Astro-Han/pawwork E2E tests (e.g., *.spec.ts under packages/app/e2e), reaching a real "running" session state is not achievable with the bare `sdk` fixture. Use the `project` fixture (to bootstrap the model) and orchestrate the transition with `llm.wait(1)`; even if you set `agent: "build"` and a `system` prompt via `sdk.session.promptAsync`, the current test infrastructure does not trigger an actual LLM call, so it won’t simulate "running" cheaply. Review any attempt to mock/force "running" using only `sdk` as likely ineffective unless it also uses `project` + `llm.wait(1)`.
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Wait on observable state with `expect(...)`, `expect.poll(...)`, or existing helpers instead of assuming work is finished after an action
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-24T00:02:50.599Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 203
File: packages/app/e2e/sidebar/sidebar-session-links.spec.ts:34-55
Timestamp: 2026-04-24T00:02:50.599Z
Learning: For Astro-Han/pawwork E2E tests under packages/app/e2e/**/*.spec.ts, do not call project.trackDirectory() or project.trackSession() before project.open() has run. The project fixture throws until open() initializes internal state. Use this ordering pattern: (1) call project.trackSession(sessionID) inside the beforeGoto callback (where state is already available), (2) call project.trackDirectory(directory) and any cross-workspace tracking like project.trackSession(id, directory) immediately after project.open() returns, and (3) if you create any resources before open() that cannot yet be tracked via the fixture, ensure you clean them up explicitly in finally blocks (e.g., cleanupSession / cleanupTestProject).
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-22T08:49:47.800Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 126
File: packages/desktop-electron/src/main/index-sidecar-source.test.ts:3-11
Timestamp: 2026-04-22T08:49:47.800Z
Learning: In `packages/desktop-electron/src/main/index-sidecar-source.test.ts` (Astro-Han/pawwork), the test intentionally uses `expect(source).toContain` / `expect(source).not.toContain` string matching against the raw `index.ts` source text as a lightweight sidecar contract guard. The maintainer has explicitly chosen not to introduce an AST parser (e.g., `babel/parser` or acorn) for this purpose. Do not flag these string-based assertions as fragile or suggest converting them to AST-based matching.
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-22T09:32:54.556Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 126
File: packages/opencode/test/provider/provider.test.ts:64-85
Timestamp: 2026-04-22T09:32:54.556Z
Learning: In `packages/opencode/test/provider/provider.test.ts`, the file intentionally uses AppRuntime-based async helpers (`run`, `list`, `getProvider`, etc.) rather than `testEffect(...)` for all tests. Converting individual tests to `testEffect` while leaving the rest on the async pattern would create internal inconsistency. A full harness migration of this file is the right approach if the pattern needs to change, but that should be a separate PR.
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-20T17:03:40.214Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 73
File: packages/opencode/src/cli/cmd/tui/context/sync.tsx:486-489
Timestamp: 2026-04-20T17:03:40.214Z
Learning: In Astro-Han/pawwork (`packages/opencode/src/cli/cmd/tui/context/sync.tsx`), `sync.ready` returning `true` when `process.env.OPENCODE_FAST_BOOT` is set is intentional. The plugin-facing data properties `state.config` (initialized to `{}`) and `state.provider` (initialized to `[]`) expose safe-empty defaults, so they are safe to access before bootstrap completes. Do not flag these as needing null-guards or conditional patterns to match `vcs` — the difference is intentional because `vcs` starts as `undefined` while the others have initialized defaults. Changing this would alter the plugin API contract without a concrete failing case.
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Call `project.trackSession(sessionID, directory?)` and `project.trackDirectory(directory)` for any resources created outside the fixture so teardown can clean them up
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-20T14:36:31.032Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.032Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : Use the `tmpdir` function from `fixture/fixture.ts` to create temporary directories for tests with automatic cleanup. Use `await using` syntax to ensure automatic cleanup when the variable goes out of scope.
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-20T14:35:49.291Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/AGENTS.md:0-0
Timestamp: 2026-04-20T14:35:49.291Z
Learning: Use `agent-browser` for web automation following the core workflow: (1) `agent-browser open <url>` to navigate, (2) `agent-browser snapshot -i` to get interactive elements with refs, (3) interact using refs with `agent-browser click e1` / `fill e2 "text"`, (4) re-snapshot after page changes
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
🔇 Additional comments (3)
packages/app/src/components/dialog-select-model.tsx (1)
113-131: Good fix for pointer-inside focus-out race.This change correctly prevents premature popover dismissal during internal item clicks while preserving outside/Escape dismissal paths.
Also applies to: 170-172
packages/app/e2e/prompt/prompt-footer-focus.spec.ts (2)
31-32: Nice consolidation of model identity comparisons.Using
modelKey(...)removes duplicated key construction and reduces mismatch risk across tests.Also applies to: 94-99
129-143: Backend assertion strategy looks solid.Polling
project.sdk.session.messages(...)until user-message model metadata matches avoids persistence races and stabilizes this regression test.
d303e88 to
315525e
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/app/e2e/prompt/prompt-footer-focus.spec.ts`:
- Around line 101-107: Tests duplicate the "open model selector, choose a model,
and poll until the probe reflects the new model" flow; extract this into a
reusable helper (e.g., selectDifferentModel or selectModel) that accepts page
and a target key and encapsulates the locator and wait logic. Implement the
helper to locate `${promptModelSelector} [data-action="prompt-model"]` and click
it, then find `[data-slot="list-item"][data-key="${providerID}:${modelID}"]`
(use the passed target's providerID/modelID or a combined nextKey), click the
item (force if needed), and await expect.poll(() => probe(page).then(v =>
modelKey(v?.model)), { timeout: 10_000 }).toBe(nextKey); replace the duplicated
blocks in tests (including occurrences around
promptModelSelector/probe/modelKey/next.providerID/next.modelID/nextKey) with
calls to this helper to reduce locator duplication and clarify intent.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 964b2819-7c52-4bdf-9a44-ad3e2e0b4174
📒 Files selected for processing (2)
packages/app/e2e/prompt/prompt-footer-focus.spec.tspackages/app/src/components/dialog-select-model.tsx
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: unit-windows-opencode-server-tools
- GitHub Check: unit-windows-desktop
- GitHub Check: unit-windows-opencode-session
- GitHub Check: unit-windows-app
- GitHub Check: unit-windows-opencode-config-project
- GitHub Check: unit-desktop
- GitHub Check: typecheck
- GitHub Check: unit-opencode
- GitHub Check: unit-app
- GitHub Check: smoke-macos-arm64
- GitHub Check: e2e-artifacts
- GitHub Check: analyze-js-ts
🧰 Additional context used
📓 Path-based instructions (2)
packages/app/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (packages/app/AGENTS.md)
Always prefer
createStoreover multiplecreateSignalcalls in SolidJS
Files:
packages/app/src/components/dialog-select-model.tsxpackages/app/e2e/prompt/prompt-footer-focus.spec.ts
packages/app/e2e/**/*.spec.ts
📄 CodeRabbit inference engine (packages/app/e2e/AGENTS.md)
packages/app/e2e/**/*.spec.ts: Import test utilities from../fixturesinstead of@playwright/test
Test files should be named with the patternfeature-name.spec.ts
Use lowercase, descriptive test names (e.g., 'sidebar can be toggled')
Use camelCase for variable names in tests
Use SCREAMING_SNAKE_CASE for constants in tests
Use fixture-managed cleanup withwithSession(sdk, title, callback)for temporary sessions instead of callingsdk.session.delete(...)directly
Prefer theprojectfixture for tests that need a dedicated project with LLM mocking
Usedata-component,data-action, or semantic roles for selectors instead of CSS class names or IDs
UsemodKeyfrom utils for cross-platform keyboard shortcuts (Meta on Mac, Control on Linux/Windows)
In terminal tests, type through the browser usingrunTerminal()andwaitTerminalReady()instead of writing to the PTY through the SDK
Never use wall-clock waits likepage.waitForTimeout(...)to make a test pass
Wait on observable state withexpect(...),expect.poll(...), or existing helpers instead of assuming work is finished after an action
Use locator assertions liketoBeVisible(),toHaveCount(0), andtoHaveAttribute(...)for normal UI state verification
Useexpect.poll(...)for probing mock or backend state rather than transient DOM visibility
Prefer fluent helpers and drivers when they make intent obvious and reduce locator-heavy noise in tests
Use direct locators when the interaction is simple and a helper would not add clarity
When validating routing, assert against canonical or resolved workspace slugs using shared helpers from../actionsto account for Windows canonicalization
Test one feature per test file
Callproject.trackSession(sessionID, directory?)andproject.trackDirectory(directory)for any resources created outside the fixture so teardown can clean them up
Files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
🧠 Learnings (23)
📚 Learning: 2026-04-23T15:26:07.250Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 193
File: packages/app/src/pages/layout/sidebar-items.tsx:102-107
Timestamp: 2026-04-23T15:26:07.250Z
Learning: In Astro-Han/pawwork (`packages/app/src/pages/layout/sidebar-items.tsx`), the `indicator()` function in `SessionRow` intentionally renders `props.leadingSlot` (the pin button) only as a fallback when no status indicator (running/permission/error/unseen) is active. When a higher-priority status wins the slot, the pin button is removed from the DOM — this is a deliberate design choice for the merged leading slot (`#150`). The keyboard unpin path is preserved via: (1) focusing the row anchor triggers `group-focus-within` which reveals the dots menu trigger, then Tab → Enter → "Unpin Session"; (2) the context menu (right-click / Shift+F10) exposes "Unpin Session". The "always render + CSS overlay" approach was considered but rejected due to z-index/pointer-events complexity; residual `...` slot behavior is tracked in `#192`. Do NOT flag the absence of the pin button from the DOM when a status is active as an accessibility regression.
Applied to files:
packages/app/src/components/dialog-select-model.tsx
📚 Learning: 2026-04-23T07:23:23.849Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 180
File: packages/app/src/components/session/session-new-view.tsx:13-18
Timestamp: 2026-04-23T07:23:23.849Z
Learning: In pawwork (Astro-Han/pawwork), prefer using `createStore` instead of multiple `createSignal` calls only when the signals represent **coupled** object state that is updated together (i.e., there is at least one shared batch-update site where the state is changed in the same transaction). If the state fields are **independent** and are mutated by separate handlers (e.g., one handler updates only `selectedSkill` while another updates only `mode`), keep them as individual `createSignal` calls—using `createStore` for truly independent fields adds boilerplate without behavioral benefit.
Applied to files:
packages/app/src/components/dialog-select-model.tsxpackages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-23T15:10:21.635Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 191
File: packages/app/src/components/session/pawwork-skill-meta.ts:38-39
Timestamp: 2026-04-23T15:10:21.635Z
Learning: This repo configures Tailwind v4 with `--color-*: initial`, which effectively breaks standard Tailwind palette utilities (e.g., `text-violet-500` can resolve to no CSS variable and render as a no-op/black). For brand/accent colors that are not backed by semantic design tokens, use inline styles with the exact hex value (e.g., `style={{ color: '#8B5FBF' }}` / `homeIconStyle: { color: '#8B5FBF' }`) and add a short comment explaining that Tailwind palette utilities won’t work due to the `--color-*: initial` setup. Do not suggest replacing these inline hex colors with Tailwind palette classes anywhere in this repo.
Applied to files:
packages/app/src/components/dialog-select-model.tsx
📚 Learning: 2026-04-24T03:51:54.050Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 206
File: packages/app/e2e/prompt/prompt-footer-focus.spec.ts:131-143
Timestamp: 2026-04-24T03:51:54.050Z
Learning: In Astro-Han/pawwork E2E tests under packages/app/e2e, do not manually call `project.trackSession(sessionID)` when you obtain a `sessionID` via `project.prompt(text)`. The `project.prompt()` implementation already registers `trackSession(next.sessionID, active.directory)` automatically after the prompt submission is observed and the active session is resolved, so calling `project.trackSession(sessionID)` again will create duplicate session ownership/teardown handling.
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Prefer fluent helpers and drivers when they make intent obvious and reduce locator-heavy noise in tests
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Use `expect.poll(...)` for probing mock or backend state rather than transient DOM visibility
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Use locator assertions like `toBeVisible()`, `toHaveCount(0)`, and `toHaveAttribute(...)` for normal UI state verification
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : In terminal tests, type through the browser using `runTerminal()` and `waitTerminalReady()` instead of writing to the PTY through the SDK
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Use direct locators when the interaction is simple and a helper would not add clarity
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Use fixture-managed cleanup with `withSession(sdk, title, callback)` for temporary sessions instead of calling `sdk.session.delete(...)` directly
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Use `modKey` from utils for cross-platform keyboard shortcuts (Meta on Mac, Control on Linux/Windows)
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Use `data-component`, `data-action`, or semantic roles for selectors instead of CSS class names or IDs
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Prefer the `project` fixture for tests that need a dedicated project with LLM mocking
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-23T15:25:27.182Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 193
File: packages/app/e2e/sidebar/sidebar-leading-slot.spec.ts:5-55
Timestamp: 2026-04-23T15:25:27.182Z
Learning: In Astro-Han/pawwork E2E tests (e.g., *.spec.ts under packages/app/e2e), reaching a real "running" session state is not achievable with the bare `sdk` fixture. Use the `project` fixture (to bootstrap the model) and orchestrate the transition with `llm.wait(1)`; even if you set `agent: "build"` and a `system` prompt via `sdk.session.promptAsync`, the current test infrastructure does not trigger an actual LLM call, so it won’t simulate "running" cheaply. Review any attempt to mock/force "running" using only `sdk` as likely ineffective unless it also uses `project` + `llm.wait(1)`.
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Wait on observable state with `expect(...)`, `expect.poll(...)`, or existing helpers instead of assuming work is finished after an action
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-24T00:02:50.599Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 203
File: packages/app/e2e/sidebar/sidebar-session-links.spec.ts:34-55
Timestamp: 2026-04-24T00:02:50.599Z
Learning: For Astro-Han/pawwork E2E tests under packages/app/e2e/**/*.spec.ts, do not call project.trackDirectory() or project.trackSession() before project.open() has run. The project fixture throws until open() initializes internal state. Use this ordering pattern: (1) call project.trackSession(sessionID) inside the beforeGoto callback (where state is already available), (2) call project.trackDirectory(directory) and any cross-workspace tracking like project.trackSession(id, directory) immediately after project.open() returns, and (3) if you create any resources before open() that cannot yet be tracked via the fixture, ensure you clean them up explicitly in finally blocks (e.g., cleanupSession / cleanupTestProject).
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-22T08:49:47.800Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 126
File: packages/desktop-electron/src/main/index-sidecar-source.test.ts:3-11
Timestamp: 2026-04-22T08:49:47.800Z
Learning: In `packages/desktop-electron/src/main/index-sidecar-source.test.ts` (Astro-Han/pawwork), the test intentionally uses `expect(source).toContain` / `expect(source).not.toContain` string matching against the raw `index.ts` source text as a lightweight sidecar contract guard. The maintainer has explicitly chosen not to introduce an AST parser (e.g., `babel/parser` or acorn) for this purpose. Do not flag these string-based assertions as fragile or suggest converting them to AST-based matching.
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-22T09:32:54.556Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 126
File: packages/opencode/test/provider/provider.test.ts:64-85
Timestamp: 2026-04-22T09:32:54.556Z
Learning: In `packages/opencode/test/provider/provider.test.ts`, the file intentionally uses AppRuntime-based async helpers (`run`, `list`, `getProvider`, etc.) rather than `testEffect(...)` for all tests. Converting individual tests to `testEffect` while leaving the rest on the async pattern would create internal inconsistency. A full harness migration of this file is the right approach if the pattern needs to change, but that should be a separate PR.
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-20T17:03:40.214Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 73
File: packages/opencode/src/cli/cmd/tui/context/sync.tsx:486-489
Timestamp: 2026-04-20T17:03:40.214Z
Learning: In Astro-Han/pawwork (`packages/opencode/src/cli/cmd/tui/context/sync.tsx`), `sync.ready` returning `true` when `process.env.OPENCODE_FAST_BOOT` is set is intentional. The plugin-facing data properties `state.config` (initialized to `{}`) and `state.provider` (initialized to `[]`) expose safe-empty defaults, so they are safe to access before bootstrap completes. Do not flag these as needing null-guards or conditional patterns to match `vcs` — the difference is intentional because `vcs` starts as `undefined` while the others have initialized defaults. Changing this would alter the plugin API contract without a concrete failing case.
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Call `project.trackSession(sessionID, directory?)` and `project.trackDirectory(directory)` for any resources created outside the fixture so teardown can clean them up
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-23T08:51:04.230Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 186
File: packages/opencode/test/plugin/workspace-adaptor.test.ts:139-144
Timestamp: 2026-04-23T08:51:04.230Z
Learning: In Astro-Han/pawwork (`packages/opencode/src/util/filesystem.ts`), the `Filesystem` utility does NOT expose a `remove` or `unlink` helper. The established repository pattern for auth.json teardown in tests (e.g. `provider.test.ts`, `amazon-bedrock.test.ts`, `workspace-adaptor.test.ts`) is to combine `Filesystem.write` with `node:fs/promises unlink`. Do not flag this mixed usage as inconsistent — it is the correct and intentional pattern.
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-20T14:36:31.032Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.032Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : Use the `tmpdir` function from `fixture/fixture.ts` to create temporary directories for tests with automatic cleanup. Use `await using` syntax to ensure automatic cleanup when the variable goes out of scope.
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
📚 Learning: 2026-04-20T14:35:49.291Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/AGENTS.md:0-0
Timestamp: 2026-04-20T14:35:49.291Z
Learning: Use `agent-browser` for web automation following the core workflow: (1) `agent-browser open <url>` to navigate, (2) `agent-browser snapshot -i` to get interactive elements with refs, (3) interact using refs with `agent-browser click e1` / `fill e2 "text"`, (4) re-snapshot after page changes
Applied to files:
packages/app/e2e/prompt/prompt-footer-focus.spec.ts
🔇 Additional comments (5)
packages/app/src/components/dialog-select-model.tsx (1)
113-131: Good fix for internal-click dismissal race.This guard cleanly prevents focus-outside from closing the popover during internal pointer interaction while preserving outside/Escape dismissal paths.
Also applies to: 170-172
packages/app/e2e/prompt/prompt-footer-focus.spec.ts (4)
31-33: Nice centralization of model identity comparison.Using
modelKey(...)removes duplicated key formatting logic and keeps assertions consistent.
61-66: Session setup/ownership flow is correct here.Opening the project, creating the session, and explicitly tracking it before navigation is the right cleanup-safe sequence.
95-100: Strong regression coverage for model application path.The key-based model change check plus
expect.poll(...)on persisted message metadata directly validates the#205failure mode and avoids race-prone assertions.Also applies to: 107-107, 114-144
162-174: Keyboard-focus dismissal behavior is well-covered.This test usefully protects the focus-leave close behavior introduced alongside the popover focus-out changes.
315525e to
6fa3156
Compare
7 tasks
5 tasks
7 tasks
13 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ENOTEMPTYcleanup races do not fail an otherwise successful smoke run.Why
The home model selector was closing on
focus outsideduring the pointer sequence. The list item received pointer down, then the popover unmounted before pointer up and click, soList.onSelectnever calledmodel.set. This meant the UI still submitted prompts with the old model.The fix keeps pointer outside and Escape dismissal behavior, but prevents Kobalte's cancelable focus outside event from closing the popover during internal item clicks.
The failed
desktop-smokerun completed the report-problem smoke assertions successfully, then failed while deleting the temporary home directory withENOTEMPTY. The cleanup now uses Node's built-in recursive remove retry options, which targets the failing cleanup layer without changing the smoke behavior under test.Related Issue
Closes #205
How To Verify
bun --cwd packages/app test:e2e e2e/prompt/prompt-footer-focus.spec.ts --project=chromium --grep "model select returns focus|home selected model" bun --cwd packages/app test:e2e e2e/prompt/prompt-footer-focus.spec.ts --project=chromium bun --cwd packages/app typecheck bun --cwd packages/app test:unit src/components/prompt-input/submit.test.ts src/context/models.test.ts node --check packages/desktop-electron/scripts/report-problem-smoke.mjs bun --cwd packages/desktop-electron build PAWWORK_FEEDBACK_FORM_URL=https://example.com/pawwork-feedback bun --cwd packages/desktop-electron smoke:reportNote: I also checked
e2e/session/session-model-persistence.spec.tsbecause it is adjacent, but did not include it as a passing verifier. It currently has an older helper mismatch around reading the home footer and agent controls, separate from this click-path fix.Screenshots or Recordings
No screenshot attached. There is no visual appearance change. The behavior is covered by the E2E checks above.
Checklist
dev, and my PR title and commit messages use Conventional Commits in EnglishSummary by CodeRabbit
Bug Fixes
Testing & Validation