fix(e2e): adapt Playwright specs + Vitest setup to chat-as-home shell (#3751)#3754
Merged
Conversation
…3751) The root two-panel layout folded Home into the unified chat surface: /home now redirects to /chat, the former Home greeting/markers moved into the chat 'new window' hero (ChatNewWindowHero), and the new-thread control reads 'New Conversation'. Update the E2E suite accordingly: - helpers/core-rpc: wait for the settled #/chat after sign-in (not the transient #/home redirect frame) so callers navigating immediately after auth aren't clobbered by the late /home -> /chat redirect. - Replace stale home markers ('Ask your assistant anything', 'Your device is connected') with structural chat-surface markers ('New Conversation', 'Threads', home-card hero). - navigation: expect /home to land on /chat. - chat-management: anchor new-thread on its testid (label changed New -> New Conversation). - chat-conversation-history: assert on response text (the bubble class is a single 'bg-stone-200/80' token a plain .bg-stone-200 selector can't match). - user-journey-full-task: leave the chat surface via /human (Home == chat now).
The full Vitest suite runs under v8 coverage instrumentation on a single worker, which makes individual renders markedly slower than an isolated, un-instrumented file run. Testing Library's default 1000ms async-utility timeout was too tight there, so findBy*/waitFor assertions in render-heavy, poll-driven suites (e.g. the workflow orchestration stop/resume tests, which wait on a 2s poll interval) flaked intermittently in CI's Frontend Coverage job — and which test tripped first was non-deterministic (TaskSourcesPanel in CI, IntelligenceOrchestrationTab locally). Raising the global async-util budget removes that class of false timeouts without masking real failures: waitFor/findBy still resolve the instant their condition is met; this only widens the ceiling before they give up, well within the 30s testTimeout.
Two follow-ups after exercising the chat-as-home surface end to end:
- chat-management-functional: the inline thread-rename ('Edit thread title')
control was removed from the conversation header by tinyhumansai#3751 (the
updateThreadTitle action/store tests/i18n remain, but the header button is
gone). Per product decision, narrow the test to the still-present thread
delete flow and drop the rename steps + the now-redundant 'Show sidebar'
click (the chat thread sidebar is visible by default on /chat).
- user-journey-full-task: the chat surface lands on its 'new window' hero
after navigating away rather than re-opening the last thread, so re-select
the thread row before asserting the persisted message is visible.
Pre-push hook reflow of the lines changed in the preceding commits (collapsed short expressions, import ordering in setup.ts). No behavior change.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe pull request aligns the test suite to a unified chat surface where ChangesUnified chat surface test alignment
Inline thread title editing
MCP servers tab test reliability
Workflow test resilience
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Two follow-ups requested on top of the chat-as-home E2E adaptation: - Restore the inline thread-rename control dropped from the conversation header by tinyhumansai#3751. Re-add the editingTitle state, handleStartEditTitle / handleCommitTitle handlers (dispatching the still-present updateThreadTitle thunk), and the header title + pencil-edit affordance in Conversations.tsx. Cover it with two deterministic Conversations unit tests (commit on Enter, cancel on Escape) and re-enable the rename assertion in the chat-management-functional E2E (asserting the unique renamed title surfaces, since chat-as-home auto-select can move the header's selected thread). - Fix the lane-2 mcp-tab-flow flake: the empty/no-results assertions used a broad `text=/no.*servers|no.*results/i` locator that, under the new root two-panel shell, also matched the wrapping shell container -> Playwright strict-mode violation. Add stable data-testids (mcp-installed-empty, mcp-catalog-empty) to the empty-state divs and target those instead. Verified locally: mcp-tab-flow + chat-management-functional E2E (18 passed), Conversations + McpServersTab unit tests green.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ed4e88a56
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@app/src/pages/Conversations.tsx`:
- Around line 2530-2541: The thread-title edit button with
data-analytics-id="chat-header-edit-thread-title" is currently only visible on
hover due to the opacity-0 class and group-hover/title:opacity-100 conditional
styling, making it invisible to keyboard users who tab to focus it. Add a
focus-visible state to the className that makes the button visible when it
receives keyboard focus, ensuring keyboard users can see the control they are
interacting with. Use Tailwind's focus-visible utility to apply appropriate
opacity or visibility styling when the button is focused via keyboard.
🪄 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: CHILL
Plan: Pro
Run ID: 468f396a-c4e8-4391-97e8-f40377f51bcf
📒 Files selected for processing (6)
app/src/components/channels/mcp/McpServersTab.test.tsxapp/src/components/channels/mcp/McpServersTab.tsxapp/src/pages/Conversations.tsxapp/src/pages/__tests__/Conversations.attachments.test.tsxapp/test/playwright/specs/chat-management-functional.spec.tsapp/test/playwright/specs/mcp-tab-flow.spec.ts
Address reviewer findings on the rename restoration: - Make the header edit-title button keyboard-discoverable: reveal it on group-focus-within and focus-visible (with a focus ring), not hover only. - Add namespaced [chat] debug diagnostics around the rename start/commit/ success/failure path (per AGENTS.md verbose-logging rule), logging the title length rather than the title text to avoid PII.
The event bus is a process-wide singleton, so other tests running in parallel (e.g. ops_install publishing reason "install"/"uninstall") leak WorkflowsChanged events into this test's receiver. It asserted on the first WorkflowsChanged event seen, which intermittently was a sibling test's "uninstall" -> 'left: "uninstall", right: "create"' failure in Rust Core Coverage. Match only our own "create" reason and skip the rest.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2779120b5a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Per reviewer (P2): chat-as-home may already have a non-null selectedThreadId (auto-created empty thread) before the New Conversation click, so waiting only for non-null could return a stale id and let the rename/delete spec pass without exercising the new-thread flow. Capture the prior id and wait for the selection to advance to the new thread.
The Coverage Gate (diff-cover >= 80%) flagged Conversations.tsx at 68.8% on changed lines: the rename handler's skip branches (unchanged/blank title), the persistence failure (.catch) path, and the onBlur/onMouseDown handlers were unexercised. Add unit tests for unchanged-commit, blank-commit, blur ignore-guard via mousedown-open, and a rejected updateTitle — every changed rename line in Conversations.tsx is now covered.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0fac92a3d5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Per reviewer (P2): for CJK/IME input, the Enter that confirms a composition candidate would also hit the title-edit handler and commit the rename early. Apply the same isImeCompositionKeyEvent guard the composer already uses, and cover it with a unit test (Enter with keyCode 229 must not persist).
oxoxDev
approved these changes
Jun 18, 2026
oxoxDev
left a comment
oxoxDev
left a comment
Collaborator
There was a problem hiding this comment.
Walkthrough: Adapts ~16 Playwright specs + Vitest setup to the chat-as-home shell (#3751: /home→/chat), restores the inline thread-rename control in the chat header, adds MCP empty-state testids, raises the RTL async-util timeout, and de-flakes a Rust event-bus test. Verified against head b8d2971.
Verdict: LGTM — approve. Clean, well-scoped, all 19 checks green. Prior CodeRabbit/Codex findings (diagnostics, keyboard-focus reveal, IME guard, new-thread wait) are all addressed in current code.
Verified good
- Single header rename block (
Conversations.tsx:2517) — no duplication; IME-guarded Enter,focus-visible/group-focus-withinreveal + focus ring for keyboard users, blur-on-open guarded byignoreNextTitleBlurRef. - Rename diagnostics log title length, not text (no PII leak) — good.
- 7 non-vacuous unit tests (commit/cancel/IME/unchanged/blank/open-blur/failure) + MCP testid assertions.
- Rust fix (
ops_create.rs:714): correct race fix for the process-wide event-bus singleton — match guardif reason == "create"+ drain unrelated events +Laggedhandling, without weakening thesawassertion. Not a suppression. asyncUtilTimeout: 5000only widens the give-up ceiling under v8-coverage single-worker runs; doesn't mask real failures. Reasonable.
Nitpicks (non-blocking)
chat-conversation-history.spec.ts:140&user-journeynow assert on raw response text instead of the bubble selector — slightly looser (text could match elsewhere on the page), but.first()+ unique canary mitigates. Fine.- Several auth specs broaden to
^#/(home|chat)while others pin^#/chat. Mixed strictness is intentional (completeAuthCallbacknow settles on#/chat); consider tightening the(home|chat)ones in a follow-up once the redirect is fully bedded in.
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
2 participants
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
/homeredirects to/chat, the former Home greeting moved into the chat "new window" hero, and stale markers/labels updated across ~14 specs.#/chatafter sign-in (not the transient#/homeredirect frame), avoiding post-login navigation races.updateThreadTitlethunk, store tests and i18n keys had survived, so the UI removal was unintentional). Covered by new Conversations unit tests + the chat-management E2E.mcp-tab-flowflake: empty/no-results assertions used a broadtext=/no.*servers|no.*results/ilocator that also matched the new root-shell wrapper (Playwright strict-mode violation). Added stable testids to the empty-state divs.Problem
After the v0.57.43 build (#3751 + release), CI was red:
findBy/waitFor1s timeouts under v8 coverage instrumentation (non-deterministic; pre-dated feat(shell): root two-panel layout — unified sidebar + chat-as-home #3751).#/homelanding / removed Home markers / old "New" thread label;mcp-tab-flow(lane 2) tripped strict mode on a broad empty-state locator now that the root shell wraps the panel.Solution
completeAuthCallbackpolls for settled^#/chat.[data-walkthrough="home-card"])./homeexpected to land on/chat.bg-stone-200/80is one token a.bg-stone-200selector can't match).configure({ asyncUtilTimeout: 5000 })— widens async-wait ceiling without masking real failures.editingTitlestate,handleStartEditTitle/handleCommitTitle, and the header title + pencil-edit affordance; 2 new unit tests (commit on Enter, cancel on Escape).mcp-installed-empty/mcp-catalog-emptytestids; specs + unit test target them.Submission Checklist
pnpm test:coveragegreen locally (6072 passed); new production lines (rename handlers/JSX, mcp testids) covered by the added/extended unit tests.Closes #NNN— N/A: no tracking issue.Impact
data-testids to MCP empty states. Remainder is E2E/Vitest test maintenance.pnpm test:coveragegreen; targeted Playwright runs of all adapted specs green (incl. mcp-tab-flow + chat-management 18 passed).Related
AI Authored PR Metadata
Linear Issue
Commit & Branch
Validation Run
pnpm --filter openhuman-app format:checkpnpm typecheckpnpm test:coverage; Playwright mcp-tab-flow + chat-management-functional (18 passed); Conversations + McpServersTab unit testsValidation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
Parity Contract
/homestill resolves (→/chat); thread delete + message persistence still verified.Duplicate / Superseded PR Handling
Summary by CodeRabbit
New Features
Tests
#/chat) and/homeredirect behavior.data-testidempty-state selectors (including MCP).