Conversation
Astro-Han
added
bug
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new E2E test for shell mode session initialization and refactors the sidebar session indicator by replacing a SolidJS Switch component with a function. A review comment suggests using createMemo for the indicator to prevent unnecessary re-renders and potential UI flickering.
📝 WalkthroughWalkthroughThis pull request adds e2e test coverage for shell mode workflow and refactors session status indicator rendering. The e2e test starts a new session directly from home, executes shell commands, validates output, and ensures no renderer crashes occur. The sidebar component replaces Switch/Match conditional logic with a helper function while maintaining identical behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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-shell.spec.ts`:
- Around line 135-136: The test creates a session via project.shell(cmd) and
assigns it to sessionID outside the withSession fixture but never registers it
for teardown; call project.trackSession(sessionID, directory?) immediately after
the session is created (and if you created a temp directory for the session call
project.trackDirectory(directory)) so the test harness can clean up resources;
locate the session creation around project.shell in prompt-shell.spec.ts and add
the trackSession/trackDirectory calls referencing sessionID and the directory
used.
🪄 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: 22b74016-03ed-4c8f-bce2-ae7d0aa98e41
📒 Files selected for processing (2)
packages/app/e2e/prompt/prompt-shell.spec.tspackages/app/src/pages/layout/sidebar-items.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). (4)
- GitHub Check: unit-windows-desktop
- GitHub Check: unit-windows-opencode-server-tools
- GitHub Check: unit-windows-opencode-session
- GitHub Check: unit-windows-opencode-config-project
🧰 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/e2e/prompt/prompt-shell.spec.tspackages/app/src/pages/layout/sidebar-items.tsx
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-shell.spec.ts
🧠 Learnings (9)
📚 Learning: 2026-04-20T14:36:04.099Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.099Z
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-shell.spec.ts
📚 Learning: 2026-04-22T08:49:44.563Z
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:44.563Z
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-shell.spec.ts
📚 Learning: 2026-04-20T14:36:04.099Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.099Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Test one feature per test file
Applied to files:
packages/app/e2e/prompt/prompt-shell.spec.ts
📚 Learning: 2026-04-20T14:36:04.099Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.099Z
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-shell.spec.ts
📚 Learning: 2026-04-20T14:36:04.099Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.099Z
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-shell.spec.ts
📚 Learning: 2026-04-20T14:36:04.099Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.099Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Test files should be named with the pattern `feature-name.spec.ts`
Applied to files:
packages/app/e2e/prompt/prompt-shell.spec.ts
📚 Learning: 2026-04-20T14:36:04.099Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.099Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Use lowercase, descriptive test names (e.g., 'sidebar can be toggled')
Applied to files:
packages/app/e2e/prompt/prompt-shell.spec.ts
📚 Learning: 2026-04-20T14:36:04.099Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.099Z
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-shell.spec.ts
📚 Learning: 2026-04-20T14:36:04.099Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.099Z
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-shell.spec.ts
🔇 Additional comments (1)
packages/app/src/pages/layout/sidebar-items.tsx (1)
104-127: Indicator refactor is solid and preserves behavior.The new helper keeps status precedence clear and safely avoids the prior
Switchrendering edge case while keepingnullfallback behavior explicit.
1 participant
Summary
Fix the renderer crash when shell mode starts from the new session home. The sidebar session row now renders its status indicator with a plain conditional helper instead of a Solid Switch that can evaluate with no matching branch during the new-session store flush. Added e2e coverage for the home shell path.
Why
PR #174 covered shell rendering inside an existing session, but the home path still crashed after typing
!and submitting a command because creating the session also inserts a sidebar row with a transient empty indicator state. This PR fixes that remaining path.Related Issue
Fixes #182. Follow-up to PR #174.
How To Verify
bun --cwd packages/app test:e2e e2e/prompt/prompt-shell.spec.ts --project=chromium --grep "new session home" bun --cwd packages/app test:e2e e2e/prompt/prompt-shell.spec.ts --project=chromium bun --cwd packages/app typecheck git diff --checkScreenshots or Recordings
Not applicable. This fixes a crash path without changing visible UI.
Checklist
dev, and my PR title and commit messages use Conventional Commits in EnglishSummary by CodeRabbit
Tests
Refactor