mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-31 19:16:29 +00:00
fix(desktop): front the workspace on ⌘N when the selection is already null
New session (⌘N, the sidebar New session row, and the per-worktree "+") all funnel into `startFreshSessionDraft`, which sets the stored-session selection to null. Fronting the workspace pane was never done by that action — it happened as a side effect of `$selectedStoredSessionId.listen`. Nanostores `.listen` only notifies on an actual value CHANGE, so: selected = <id> -> set null -> changed -> workspace fronted (fine) selected = null -> set null -> no change -> listener never runs (dead) With main already parked on a blank draft and a session tile fronted, every subsequent new-session gesture created the session but never revealed it, so it looked like nothing happened at all. Reproduced deterministically against the running app: 4/4 silent no-ops in that state, versus a correct reveal when the selection did change. Extract the listener body as `homeSelectionToWorkspace` and add `homeFreshDraftToWorkspace`, called explicitly from `startFreshSessionDraft`. A fresh draft is a primary navigation, so it applies the homing policy directly instead of depending on a change notification. Homing is idempotent, so the listener firing as well is harmless. The explicit path deliberately does NOT consume the boot-restore one-shot: that flag is armed for a specific pending resume, and swallowing it here would let a cold start clobber the persisted active tab (the ⌘R bug the flag exists to prevent). Covered by its own test. Unit tests verified RED without the fix and GREEN with it. Also adds an E2E covering the two-worktree "+" scenario from the report. That path turns out to be healthy — the spec passes with and without the fix, and it is labelled as coverage rather than a regression test in its header. It is kept because it pins the fiddly worktree-lane fixture (projects registered via the folder-open flow, since a desktop session does not adopt its launch cwd as a workspace) and would catch a future change that collapses the two lanes into one session.
This commit is contained in:
parent
8defb9fd60
commit
0f70aab579
4 changed files with 261 additions and 2 deletions
200
apps/desktop/e2e/worktree-new-session-tabs.spec.ts
Normal file
200
apps/desktop/e2e/worktree-new-session-tabs.spec.ts
Normal file
|
|
@ -0,0 +1,200 @@
|
|||
/**
|
||||
* Worktree A's "+" and worktree B's "+" must each open their OWN new session.
|
||||
*
|
||||
* Motivated by a report that, with a session open per worktree in separate
|
||||
* tabs, "+" on lane A opens a new session but "+" on lane B then does nothing.
|
||||
*
|
||||
* SCOPE — this drives the real two-lane scenario end to end, but it is
|
||||
* COVERAGE, not a regression test: it passes both with and without the
|
||||
* `homeFreshDraftToWorkspace` fix in store/session-states.ts (verified by
|
||||
* neutering that function and re-running). In other words the per-lane "+"
|
||||
* path is healthy on this build — two clicks reliably yield two distinct
|
||||
* stacked sessions. Keep it so a future change that collapses them is caught,
|
||||
* and so the (fiddly) worktree-lane fixture below stays exercised.
|
||||
*
|
||||
* Fixture notes (these were the hard part, don't regress them):
|
||||
* - A desktop session deliberately does NOT adopt its launch directory as a
|
||||
* workspace (`_LAUNCH_CWD_NOT_A_WORKSPACE` in tui_gateway/server.py), so
|
||||
* `terminal.cwd` alone never stamps a session cwd and no lane can derive.
|
||||
* The worktrees are registered as real projects via the folder-open flow
|
||||
* (⌘O / Ctrl+O) with Electron's dialog stubbed.
|
||||
* - Worktree lanes are session-derived: a lane appears once a PERSISTED turn
|
||||
* has a cwd inside it, so each seed waits for the assistant reply.
|
||||
* - `repo_scan_roots` is pinned to the sandbox so the host's real repos can't
|
||||
* leak into the sidebar.
|
||||
* - The `__HERMES_*` window hooks are DEV-only and absent from the packaged
|
||||
* build these tests launch; use the `data-tree-tab` DOM hook instead.
|
||||
*/
|
||||
import { execFileSync } from 'node:child_process'
|
||||
import * as fs from 'node:fs'
|
||||
import * as path from 'node:path'
|
||||
|
||||
import { test, expect } from './test'
|
||||
|
||||
import {
|
||||
buildAppEnv,
|
||||
createSandbox,
|
||||
launchDesktop,
|
||||
writeEnvFile,
|
||||
writeMockProviderConfig,
|
||||
waitForAppReady,
|
||||
type MockBackendFixture,
|
||||
} from './fixtures'
|
||||
import { startMockServer } from './mock-server'
|
||||
|
||||
const WORKTREE_A = 'e2e-tree-a'
|
||||
const WORKTREE_B = 'e2e-tree-b'
|
||||
|
||||
function git(cwd: string, ...args: string[]): string {
|
||||
return execFileSync('git', args, { cwd, encoding: 'utf8' })
|
||||
}
|
||||
|
||||
function createRepoWithWorktrees(root: string): { repo: string; treeA: string; treeB: string } {
|
||||
const repo = path.join(root, 'repo')
|
||||
|
||||
fs.mkdirSync(repo, { recursive: true })
|
||||
git(repo, 'init', '--initial-branch=main')
|
||||
git(repo, 'config', 'user.email', 'e2e@example.com')
|
||||
git(repo, 'config', 'user.name', 'Hermes E2E')
|
||||
fs.writeFileSync(path.join(repo, 'README.md'), '# E2E repo\n', 'utf8')
|
||||
git(repo, 'add', 'README.md')
|
||||
git(repo, 'commit', '-m', 'initial')
|
||||
|
||||
const treeA = path.join(root, WORKTREE_A)
|
||||
const treeB = path.join(root, WORKTREE_B)
|
||||
|
||||
git(repo, 'worktree', 'add', '-b', WORKTREE_A, treeA)
|
||||
git(repo, 'worktree', 'add', '-b', WORKTREE_B, treeB)
|
||||
|
||||
return { repo, treeA, treeB }
|
||||
}
|
||||
|
||||
let fixture: MockBackendFixture | null = null
|
||||
let trees: { repo: string; treeA: string; treeB: string } | null = null
|
||||
|
||||
test.beforeAll(async () => {
|
||||
const sandbox = createSandbox('worktree-new-session-tabs')
|
||||
trees = createRepoWithWorktrees(sandbox.root)
|
||||
const mock = await startMockServer()
|
||||
|
||||
writeMockProviderConfig(sandbox.hermesHome, mock.url)
|
||||
fs.appendFileSync(
|
||||
path.join(sandbox.hermesHome, 'config.yaml'),
|
||||
`\nterminal:\n cwd: ${trees.treeA}\n` +
|
||||
`desktop:\n repo_scan_enabled: true\n repo_scan_roots:\n - ${sandbox.root}\n`,
|
||||
'utf8',
|
||||
)
|
||||
writeEnvFile(sandbox.hermesHome)
|
||||
|
||||
const { app, page } = await launchDesktop(buildAppEnv(sandbox))
|
||||
|
||||
fixture = {
|
||||
app,
|
||||
page,
|
||||
mock,
|
||||
mockUrl: mock.url,
|
||||
sandbox,
|
||||
cleanup: async () => {
|
||||
await app.close().catch(() => undefined)
|
||||
await mock.close()
|
||||
sandbox.cleanup()
|
||||
},
|
||||
}
|
||||
|
||||
await waitForAppReady(fixture, 120_000)
|
||||
})
|
||||
|
||||
test.afterAll(async () => {
|
||||
await fixture?.cleanup()
|
||||
fixture = null
|
||||
trees = null
|
||||
})
|
||||
|
||||
test('"+" on each worktree lane opens its own distinct new session', async ({}, testInfo) => {
|
||||
const { app, page } = fixture!
|
||||
const { treeA, treeB } = trees!
|
||||
|
||||
/** Session-tile tabs in the strip (production DOM hook). */
|
||||
const tabIds = () =>
|
||||
page.evaluate(() =>
|
||||
[...document.querySelectorAll('[data-tree-tab]')]
|
||||
.map(el => el.getAttribute('data-tree-tab') ?? '')
|
||||
.filter(id => id.startsWith('session-tile:')),
|
||||
)
|
||||
|
||||
// The VISIBLE composer — a hidden tab's composer is still in the DOM.
|
||||
const composer = () => page.locator('[contenteditable="true"]:visible').first()
|
||||
|
||||
/** Send a prompt and wait for the reply, so the turn PERSISTS (which is what
|
||||
* gives the session a cwd and makes its worktree lane appear). */
|
||||
const seedSession = async (prompt: string) => {
|
||||
const input = composer()
|
||||
await input.click()
|
||||
await input.type(prompt, { delay: 2 })
|
||||
await page.keyboard.press('Enter')
|
||||
await page.waitForFunction(
|
||||
text =>
|
||||
[...document.querySelectorAll('[data-slot="aui_thread-viewport"]')].some(
|
||||
el => (el as HTMLElement).offsetParent !== null && (el.textContent ?? '').includes(text),
|
||||
),
|
||||
prompt,
|
||||
{ timeout: 20_000 },
|
||||
)
|
||||
await page.waitForFunction(
|
||||
() =>
|
||||
[...document.querySelectorAll('[data-slot="aui_assistant-message-root"]')].some(
|
||||
el => (el.textContent ?? '').includes('mock inference server'),
|
||||
),
|
||||
undefined,
|
||||
{ timeout: 30_000 },
|
||||
)
|
||||
}
|
||||
|
||||
// ── Register both worktrees as projects via the real folder-open flow ────
|
||||
await app.evaluate(async ({ dialog }, paths) => {
|
||||
let i = 0
|
||||
;(dialog as any).showOpenDialog = async () => ({ canceled: false, filePaths: [paths[i++]] })
|
||||
}, [treeA, treeB])
|
||||
|
||||
// Ctrl+O = workspace.openFolder → open folder as a project + fresh session.
|
||||
await page.keyboard.press('Control+O')
|
||||
await page.waitForTimeout(4000)
|
||||
await seedSession('worktree A seed session')
|
||||
|
||||
await page.keyboard.press('Control+O')
|
||||
await page.waitForTimeout(4000)
|
||||
await seedSession('worktree B seed session')
|
||||
|
||||
// ── Both lanes must now be present in the projects view ─────────────────
|
||||
const showProjects = page.getByRole('button', { name: 'Show projects' }).first()
|
||||
|
||||
if (await showProjects.isVisible().catch(() => false)) {
|
||||
await showProjects.click()
|
||||
}
|
||||
|
||||
const plusA = page.locator(`[aria-label="New session in ${WORKTREE_A}"]`).first()
|
||||
const plusB = page.locator(`[aria-label="New session in ${WORKTREE_B}"]`).first()
|
||||
|
||||
await expect(plusA).toBeAttached({ timeout: 30_000 })
|
||||
await expect(plusB).toBeAttached({ timeout: 30_000 })
|
||||
|
||||
const before = await tabIds()
|
||||
await page.screenshot({ path: testInfo.outputPath('01-two-worktree-sessions.png') })
|
||||
|
||||
// ── "+" on lane A ────────────────────────────────────────────────────────
|
||||
await plusA.click({ force: true })
|
||||
await expect.poll(async () => (await tabIds()).length, { timeout: 20_000 }).toBeGreaterThan(before.length)
|
||||
|
||||
const afterA = await tabIds()
|
||||
await page.screenshot({ path: testInfo.outputPath('02-after-plus-worktree-a.png') })
|
||||
|
||||
// ── "+" on lane B: MUST open ANOTHER new session, not reuse A's draft ────
|
||||
await plusB.click({ force: true })
|
||||
await expect.poll(async () => (await tabIds()).length, { timeout: 20_000 }).toBeGreaterThan(afterA.length)
|
||||
|
||||
const afterB = await tabIds()
|
||||
await page.screenshot({ path: testInfo.outputPath('03-after-plus-worktree-b.png') })
|
||||
|
||||
// Every open session must be distinct — no id appearing twice.
|
||||
expect(new Set(afterB).size).toBe(afterB.length)
|
||||
})
|
||||
|
|
@ -60,6 +60,7 @@ import {
|
|||
$sessionTiles,
|
||||
closeSessionTile,
|
||||
dropSessionState,
|
||||
homeFreshDraftToWorkspace,
|
||||
openSessionTile,
|
||||
patchSessionTile,
|
||||
publishSessionState,
|
||||
|
|
@ -343,6 +344,12 @@ export function useSessionActions({
|
|||
setCurrentBranch('')
|
||||
// Never clear the composer here — ChatBar's per-thread draft swap owns it.
|
||||
setFreshDraftReady(true)
|
||||
// A fresh draft is a PRIMARY navigation, so it must front the workspace
|
||||
// even when the selection did not change value. Setting null over an
|
||||
// already-null selection notifies no listener, which left ⌘N / the New
|
||||
// session row / the worktree button silently dead while a session tile
|
||||
// was fronted. Homing is idempotent, so the listener firing too is fine.
|
||||
homeFreshDraftToWorkspace()
|
||||
},
|
||||
[activeSessionIdRef, busyRef, navigate, onFreshDraftRouteIntent, resetViewSync, selectedStoredSessionIdRef]
|
||||
)
|
||||
|
|
|
|||
|
|
@ -8,6 +8,7 @@ import type { SessionTile } from '@/store/session-states'
|
|||
import {
|
||||
blankDraftTile,
|
||||
focusedSessionNeedsRoute,
|
||||
homeFreshDraftToWorkspace,
|
||||
markSelectionRestore,
|
||||
orderTilesByTree,
|
||||
selectionHomesToWorkspace
|
||||
|
|
@ -82,6 +83,35 @@ describe('boot-restore selection homing (⌘R tab persistence)', () => {
|
|||
$selectedStoredSessionId.set('nav-2')
|
||||
expect(activePane()).toBe('workspace')
|
||||
})
|
||||
|
||||
// A fresh draft over an ALREADY-null selection notifies no listener (nanostores
|
||||
// only fires on a value change), so ⌘N / the New session row / the worktree
|
||||
// button all looked dead while a session tile was fronted.
|
||||
it('a fresh draft fronts the workspace even when the selection does not change', () => {
|
||||
$selectedStoredSessionId.set(null)
|
||||
$layoutTree.set(mainGroup())
|
||||
|
||||
// Setting null over null notifies nothing — the tile stays fronted.
|
||||
$selectedStoredSessionId.set(null)
|
||||
expect(activePane()).toBe(tilePane('t'))
|
||||
|
||||
// The explicit new-chat homing call is what actually reveals the draft.
|
||||
homeFreshDraftToWorkspace()
|
||||
expect(activePane()).toBe('workspace')
|
||||
})
|
||||
|
||||
it('a fresh draft does NOT consume the boot-restore one-shot', () => {
|
||||
$layoutTree.set(mainGroup())
|
||||
markSelectionRestore()
|
||||
|
||||
// An explicit fresh-draft home must leave the armed flag for the boot
|
||||
// resume it belongs to, or a cold start clobbers the persisted active tab.
|
||||
homeFreshDraftToWorkspace()
|
||||
expect(activePane()).toBe(tilePane('t'))
|
||||
|
||||
$selectedStoredSessionId.set('boot-2')
|
||||
expect(activePane()).toBe(tilePane('t'))
|
||||
})
|
||||
})
|
||||
|
||||
describe('focusedSessionNeedsRoute', () => {
|
||||
|
|
|
|||
|
|
@ -786,7 +786,15 @@ export function markSelectionRestore() {
|
|||
|
||||
// Homing also FRONTS the workspace tab: the resumed chat loads in the workspace
|
||||
// pane, so a zone parked on a tile tab must switch back or the click looks dead.
|
||||
$selectedStoredSessionId.listen(selected => {
|
||||
//
|
||||
// Exported so an explicit primary navigation can apply the SAME policy without
|
||||
// depending on a selection CHANGE. `.listen` only notifies when the value
|
||||
// actually differs, so starting a fresh draft while the selection is already
|
||||
// null — main is on a blank new chat and the user is looking at a session tile —
|
||||
// never fired this, and ⌘N / the New session row / the worktree button all
|
||||
// looked completely dead. Idempotent, so the listener and an explicit call can
|
||||
// both run for one navigation.
|
||||
export function homeSelectionToWorkspace(selected: null | string): void {
|
||||
const restoring = selectionRestoreInFlight
|
||||
selectionRestoreInFlight = false
|
||||
|
||||
|
|
@ -796,7 +804,21 @@ $selectedStoredSessionId.listen(selected => {
|
|||
|
||||
noteActiveTreeGroup(null)
|
||||
revealTreePane('workspace')
|
||||
})
|
||||
}
|
||||
|
||||
/** Front the workspace for an explicit primary navigation whose selection may
|
||||
* not have CHANGED (a fresh draft over an already-null selection). Unlike the
|
||||
* listener path this never consumes the boot-restore one-shot: that flag is
|
||||
* armed for a specific upcoming resume, and swallowing it here would let a
|
||||
* cold start clobber the persisted active tab (the ⌘R bug the flag prevents). */
|
||||
export function homeFreshDraftToWorkspace(): void {
|
||||
if (!selectionRestoreInFlight && selectionHomesToWorkspace(null, $sessionTiles.get())) {
|
||||
noteActiveTreeGroup(null)
|
||||
revealTreePane('workspace')
|
||||
}
|
||||
}
|
||||
|
||||
$selectedStoredSessionId.listen(selected => homeSelectionToWorkspace(selected))
|
||||
|
||||
// Dev hook for automation (mirrors __HERMES_LAYOUT_TREE__).
|
||||
if (import.meta.env.DEV && typeof window !== 'undefined') {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue