mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-23 16:36:23 +00:00
fix(ui-tui): stacked dialog gets input before the grid under it
Review on #20379, finding 3 (Medium). /grid-test's `d` opens a dialog on top without clearing the grid, but the grid's input branch ran FIRST — so Esc/q/Enter mutated the hidden grid (close/unzoom/promote) instead of closing the visible dialog, contradicting its "Esc/q/Enter close" hint. Input routing now follows visual stacking: the dialog/grid dispatch is extracted into handleStackedModalInput() with the dialog branch first, the hook consumes through it, and tests drive the real dispatch against the overlay store — each advertised close key closes only the dialog (grid byte-identical), grid keys don't leak through while the dialog is up, and the same keys route to the grid again after it closes.
This commit is contained in:
parent
e6cc5612e6
commit
c25e08aa2b
4 changed files with 243 additions and 154 deletions
|
|
@ -1,6 +1,6 @@
|
|||
import { describe, expect, it } from 'vitest'
|
||||
|
||||
import { invalidateBootBackground, seedBootEnvironment, type BootTheme } from '../lib/themeBoot.js'
|
||||
import { type BootTheme, invalidateBootBackground, seedBootEnvironment } from '../lib/themeBoot.js'
|
||||
import { defaultTheme } from '../theme.js'
|
||||
|
||||
// Review on #20379 (finding 2): the boot cache seeds the previous session's
|
||||
|
|
|
|||
|
|
@ -4,12 +4,12 @@ import { $uiState, resetUiState } from '../app/uiStore.js'
|
|||
import {
|
||||
applyDisplay,
|
||||
hydrateFullConfig,
|
||||
type McpRevState,
|
||||
normalizeBusyInputMode,
|
||||
normalizeIndicatorStyle,
|
||||
normalizeMouseTracking,
|
||||
normalizeStatusBar,
|
||||
syncMcpReload,
|
||||
type McpRevState
|
||||
syncMcpReload
|
||||
} from '../app/useConfigSync.js'
|
||||
|
||||
describe('applyDisplay', () => {
|
||||
|
|
@ -449,11 +449,13 @@ describe('syncMcpReload (revision-aware ack)', () => {
|
|||
|
||||
it('does not stack requests while one is in flight', async () => {
|
||||
let resolveFirst!: (v: unknown) => void
|
||||
|
||||
const gw = {
|
||||
request: vi.fn(() => new Promise(res => (resolveFirst = res))),
|
||||
on: vi.fn(),
|
||||
off: vi.fn()
|
||||
} as any
|
||||
|
||||
const state = freshState()
|
||||
|
||||
const first = syncMcpReload(gw, 's1', 'rev-b', state)
|
||||
|
|
|
|||
|
|
@ -1,10 +1,12 @@
|
|||
import { describe, expect, it, vi } from 'vitest'
|
||||
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
|
||||
import type { GridTestState } from '../app/interfaces.js'
|
||||
import { getOverlayState, patchOverlayState, resetOverlayState } from '../app/overlayStore.js'
|
||||
import {
|
||||
applyVoiceRecordResponse,
|
||||
dismissSensitivePrompt,
|
||||
handleIdleHotkeyExit,
|
||||
handleStackedModalInput,
|
||||
shouldAllowIdleHotkeyExit,
|
||||
shouldFallThroughForScroll
|
||||
} from '../app/useInputHandlers.js'
|
||||
|
|
@ -144,3 +146,96 @@ describe('dismissSensitivePrompt', () => {
|
|||
await pending
|
||||
})
|
||||
})
|
||||
|
||||
// Review on #20379 (finding 3): a dialog stacked over /grid-test was
|
||||
// visually modal but did not receive input — the grid branch ran first, so
|
||||
// every advertised close key (Esc/q/Enter) mutated the hidden grid instead
|
||||
// of closing the visible dialog. Input routing must follow visual stacking.
|
||||
describe('handleStackedModalInput — dialog over grid-test', () => {
|
||||
const baseModalKey = {
|
||||
ctrl: false,
|
||||
downArrow: false,
|
||||
escape: false,
|
||||
leftArrow: false,
|
||||
return: false,
|
||||
rightArrow: false,
|
||||
upArrow: false
|
||||
}
|
||||
|
||||
const grid: GridTestState = {
|
||||
activeCol: 1,
|
||||
activeRow: 1,
|
||||
areas: false,
|
||||
cols: 4,
|
||||
gap: null,
|
||||
nested: false,
|
||||
paddingX: null,
|
||||
rows: 3,
|
||||
streamFocus: 0,
|
||||
streamMain: 0,
|
||||
streams: false,
|
||||
zoomed: false
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
resetOverlayState()
|
||||
patchOverlayState({ gridTest: { ...grid } })
|
||||
})
|
||||
|
||||
const openDialogViaD = () => {
|
||||
expect(handleStackedModalInput(getOverlayState(), baseModalKey, 'd')).toBe(true)
|
||||
expect(getOverlayState().dialog).not.toBeNull()
|
||||
expect(getOverlayState().gridTest).not.toBeNull()
|
||||
}
|
||||
|
||||
it.each([
|
||||
['Esc', { ...baseModalKey, escape: true }, ''],
|
||||
['q', baseModalKey, 'q'],
|
||||
['Enter', { ...baseModalKey, return: true }, ''],
|
||||
['Ctrl+C', { ...baseModalKey, ctrl: true }, 'c']
|
||||
])('%s closes only the dialog, leaving the grid untouched', (_label, key, ch) => {
|
||||
openDialogViaD()
|
||||
|
||||
const gridBefore = getOverlayState().gridTest
|
||||
|
||||
expect(handleStackedModalInput(getOverlayState(), key, ch)).toBe(true)
|
||||
expect(getOverlayState().dialog).toBeNull()
|
||||
// The grid must be byte-identical: not closed, not zoomed, not reset.
|
||||
expect(getOverlayState().gridTest).toBe(gridBefore)
|
||||
})
|
||||
|
||||
it('after the dialog closes, the same keys route to the grid again', () => {
|
||||
openDialogViaD()
|
||||
handleStackedModalInput(getOverlayState(), { ...baseModalKey, escape: true }, '')
|
||||
expect(getOverlayState().dialog).toBeNull()
|
||||
|
||||
// Esc now closes the grid — the dialog no longer shields it.
|
||||
expect(handleStackedModalInput(getOverlayState(), { ...baseModalKey, escape: true }, '')).toBe(true)
|
||||
expect(getOverlayState().gridTest).toBeNull()
|
||||
})
|
||||
|
||||
it('the dialog swallows grid keys entirely while open (no leak-through)', () => {
|
||||
openDialogViaD()
|
||||
|
||||
const gridBefore = getOverlayState().gridTest
|
||||
|
||||
// 'a' toggles areas mode when the grid has focus — it must not now.
|
||||
expect(handleStackedModalInput(getOverlayState(), baseModalKey, 'a')).toBe(true)
|
||||
expect(getOverlayState().gridTest).toBe(gridBefore)
|
||||
expect(getOverlayState().dialog).not.toBeNull()
|
||||
})
|
||||
|
||||
it('stacking works from streams mode too', () => {
|
||||
patchOverlayState({ gridTest: { ...grid, streams: true } })
|
||||
openDialogViaD()
|
||||
|
||||
expect(handleStackedModalInput(getOverlayState(), { ...baseModalKey, return: true }, '')).toBe(true)
|
||||
expect(getOverlayState().dialog).toBeNull()
|
||||
expect(getOverlayState().gridTest?.streams).toBe(true)
|
||||
})
|
||||
|
||||
it('reports unconsumed when neither modal is up', () => {
|
||||
resetOverlayState()
|
||||
expect(handleStackedModalInput(getOverlayState(), baseModalKey, 'x')).toBe(false)
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -146,6 +146,145 @@ const keepGridCursorInBounds = (grid: GridTestState): GridTestState => ({
|
|||
activeRow: clamp(grid.activeRow, 0, grid.rows - 1)
|
||||
})
|
||||
|
||||
interface StackedModalKey {
|
||||
ctrl: boolean
|
||||
downArrow: boolean
|
||||
escape: boolean
|
||||
leftArrow: boolean
|
||||
return: boolean
|
||||
rightArrow: boolean
|
||||
upArrow: boolean
|
||||
}
|
||||
|
||||
/**
|
||||
* Input routing for the stacked demo modals (dialog over grid-test).
|
||||
* Exported so tests can drive the real dispatch against the overlay store.
|
||||
*
|
||||
* ORDER IS THE CONTRACT: input routing follows visual stacking. /grid-test's
|
||||
* `d` opens a dialog ON TOP of the grid without clearing it — the dialog
|
||||
* branch must run first, or Esc/q/Enter mutate the hidden grid instead of
|
||||
* closing the visible dialog, contradicting its "Esc/q/Enter close" hint
|
||||
* (review on #20379, finding 3).
|
||||
*
|
||||
* Returns true when a modal consumed the key (callers stop routing).
|
||||
*/
|
||||
export function handleStackedModalInput(
|
||||
overlay: Pick<OverlayState, 'dialog' | 'gridTest'>,
|
||||
key: StackedModalKey,
|
||||
ch: string
|
||||
): boolean {
|
||||
if (overlay.dialog) {
|
||||
if (key.escape || isCtrl(key, ch, 'c') || ch === 'q' || key.return) {
|
||||
patchOverlayState({ dialog: null })
|
||||
}
|
||||
|
||||
return true
|
||||
}
|
||||
|
||||
if (!overlay.gridTest) {
|
||||
return false
|
||||
}
|
||||
|
||||
const updateGrid = (fn: (grid: GridTestState) => GridTestState) =>
|
||||
patchOverlayState(prev => (prev.gridTest ? { ...prev, gridTest: keepGridCursorInBounds(fn(prev.gridTest)) } : prev))
|
||||
|
||||
const openDemoDialog = () =>
|
||||
patchOverlayState({
|
||||
dialog: {
|
||||
body: ['Dialog overlaid on top of /grid-test.', '', 'Backdrop dims the grid behind.'].join('\n'),
|
||||
hint: 'Esc/q/Enter close',
|
||||
title: 'Overlay primitive',
|
||||
zone: 'center'
|
||||
}
|
||||
})
|
||||
|
||||
const resetGrid = () =>
|
||||
updateGrid(grid => ({
|
||||
...grid,
|
||||
activeCol: 0,
|
||||
activeRow: 0,
|
||||
areas: false,
|
||||
cols: 4,
|
||||
gap: null,
|
||||
nested: false,
|
||||
paddingX: null,
|
||||
rows: 3,
|
||||
streamFocus: 0,
|
||||
streamMain: 0,
|
||||
streams: false,
|
||||
zoomed: false
|
||||
}))
|
||||
|
||||
if (isCtrl(key, ch, 'c')) {
|
||||
patchOverlayState({ gridTest: null })
|
||||
|
||||
return true
|
||||
}
|
||||
|
||||
// Streams mode swallows the grid-shape keys: focus cycles across the
|
||||
// live panels and Enter promotes the focused one to the 2x2 slot.
|
||||
if (overlay.gridTest.streams) {
|
||||
if (key.escape || ch === 'q' || ch === 's') {
|
||||
updateGrid(grid => ({ ...grid, streams: false }))
|
||||
} else if (key.return) {
|
||||
updateGrid(grid => ({ ...grid, streamMain: grid.streamFocus }))
|
||||
} else if (ch === 'd') {
|
||||
openDemoDialog()
|
||||
} else if (ch === 'r') {
|
||||
resetGrid()
|
||||
} else if (key.leftArrow || key.upArrow || ch === 'h' || ch === 'k') {
|
||||
updateGrid(grid => ({
|
||||
...grid,
|
||||
streamFocus: (grid.streamFocus + GRID_STREAM_COUNT - 1) % GRID_STREAM_COUNT
|
||||
}))
|
||||
} else if (key.rightArrow || key.downArrow || ch === 'l' || ch === 'j') {
|
||||
updateGrid(grid => ({ ...grid, streamFocus: (grid.streamFocus + 1) % GRID_STREAM_COUNT }))
|
||||
}
|
||||
|
||||
return true
|
||||
}
|
||||
|
||||
if (overlay.gridTest.zoomed && (key.escape || ch === 'q')) {
|
||||
updateGrid(grid => ({ ...grid, zoomed: false }))
|
||||
} else if (key.escape || ch === 'q') {
|
||||
patchOverlayState({ gridTest: null })
|
||||
} else if (key.return) {
|
||||
updateGrid(grid => ({ ...grid, nested: true, zoomed: true }))
|
||||
} else if (ch === 'n') {
|
||||
updateGrid(grid => ({ ...grid, nested: !grid.nested }))
|
||||
} else if (ch === 'a') {
|
||||
updateGrid(grid => ({ ...grid, areas: !grid.areas, streams: false }))
|
||||
} else if (ch === 's') {
|
||||
updateGrid(grid => ({ ...grid, areas: false, streams: true }))
|
||||
} else if (ch === 'g') {
|
||||
updateGrid(grid => ({ ...grid, gap: cycleAutoNumber(grid.gap, 3) }))
|
||||
} else if (ch === 'p') {
|
||||
updateGrid(grid => ({ ...grid, paddingX: cycleAutoNumber(grid.paddingX, 2) }))
|
||||
} else if (ch === 'd') {
|
||||
openDemoDialog()
|
||||
} else if (ch === 'r') {
|
||||
resetGrid()
|
||||
} else if (ch === '+' || ch === '=') {
|
||||
updateGrid(grid => ({ ...grid, cols: clamp(grid.cols + 1, 1, GRID_TEST_MAX_SIZE) }))
|
||||
} else if (ch === '-' || ch === '_') {
|
||||
updateGrid(grid => ({ ...grid, cols: clamp(grid.cols - 1, 1, GRID_TEST_MAX_SIZE) }))
|
||||
} else if (ch === ']') {
|
||||
updateGrid(grid => ({ ...grid, rows: clamp(grid.rows + 1, 1, GRID_TEST_MAX_SIZE) }))
|
||||
} else if (ch === '[') {
|
||||
updateGrid(grid => ({ ...grid, rows: clamp(grid.rows - 1, 1, GRID_TEST_MAX_SIZE) }))
|
||||
} else if (key.leftArrow || ch === 'h') {
|
||||
updateGrid(grid => ({ ...grid, activeCol: clamp(grid.activeCol - 1, 0, grid.cols - 1) }))
|
||||
} else if (key.rightArrow || ch === 'l') {
|
||||
updateGrid(grid => ({ ...grid, activeCol: clamp(grid.activeCol + 1, 0, grid.cols - 1) }))
|
||||
} else if (key.upArrow || ch === 'k') {
|
||||
updateGrid(grid => ({ ...grid, activeRow: clamp(grid.activeRow - 1, 0, grid.rows - 1) }))
|
||||
} else if (key.downArrow || ch === 'j') {
|
||||
updateGrid(grid => ({ ...grid, activeRow: clamp(grid.activeRow + 1, 0, grid.rows - 1) }))
|
||||
}
|
||||
|
||||
return true
|
||||
}
|
||||
|
||||
export function useInputHandlers(ctx: InputHandlerContext): InputHandlerResult {
|
||||
const { actions, composer, gateway, terminal, voice, wheelStep } = ctx
|
||||
const { actions: cActions, refs: cRefs, state: cState } = composer
|
||||
|
|
@ -428,156 +567,9 @@ export function useInputHandlers(ctx: InputHandlerContext): InputHandlerResult {
|
|||
return
|
||||
}
|
||||
|
||||
if (overlay.gridTest) {
|
||||
const updateGrid = (fn: (grid: GridTestState) => GridTestState) =>
|
||||
patchOverlayState(prev =>
|
||||
prev.gridTest ? { ...prev, gridTest: keepGridCursorInBounds(fn(prev.gridTest)) } : prev
|
||||
)
|
||||
|
||||
const openDemoDialog = () =>
|
||||
patchOverlayState({
|
||||
dialog: {
|
||||
body: ['Dialog overlaid on top of /grid-test.', '', 'Backdrop dims the grid behind.'].join('\n'),
|
||||
hint: 'Esc/q/Enter close',
|
||||
title: 'Overlay primitive',
|
||||
zone: 'center'
|
||||
}
|
||||
})
|
||||
|
||||
const resetGrid = () =>
|
||||
updateGrid(grid => ({
|
||||
...grid,
|
||||
activeCol: 0,
|
||||
activeRow: 0,
|
||||
areas: false,
|
||||
cols: 4,
|
||||
gap: null,
|
||||
nested: false,
|
||||
paddingX: null,
|
||||
rows: 3,
|
||||
streamFocus: 0,
|
||||
streamMain: 0,
|
||||
streams: false,
|
||||
zoomed: false
|
||||
}))
|
||||
|
||||
if (isCtrl(key, ch, 'c')) {
|
||||
return patchOverlayState({ gridTest: null })
|
||||
}
|
||||
|
||||
// Streams mode swallows the grid-shape keys: focus cycles across the
|
||||
// live panels and Enter promotes the focused one to the 2x2 slot.
|
||||
if (overlay.gridTest.streams) {
|
||||
if (key.escape || ch === 'q' || ch === 's') {
|
||||
return updateGrid(grid => ({ ...grid, streams: false }))
|
||||
}
|
||||
|
||||
if (key.return) {
|
||||
return updateGrid(grid => ({ ...grid, streamMain: grid.streamFocus }))
|
||||
}
|
||||
|
||||
if (ch === 'd') {
|
||||
return openDemoDialog()
|
||||
}
|
||||
|
||||
if (ch === 'r') {
|
||||
return resetGrid()
|
||||
}
|
||||
|
||||
if (key.leftArrow || key.upArrow || ch === 'h' || ch === 'k') {
|
||||
return updateGrid(grid => ({
|
||||
...grid,
|
||||
streamFocus: (grid.streamFocus + GRID_STREAM_COUNT - 1) % GRID_STREAM_COUNT
|
||||
}))
|
||||
}
|
||||
|
||||
if (key.rightArrow || key.downArrow || ch === 'l' || ch === 'j') {
|
||||
return updateGrid(grid => ({ ...grid, streamFocus: (grid.streamFocus + 1) % GRID_STREAM_COUNT }))
|
||||
}
|
||||
|
||||
return
|
||||
}
|
||||
|
||||
if (overlay.gridTest.zoomed && (key.escape || ch === 'q')) {
|
||||
return updateGrid(grid => ({ ...grid, zoomed: false }))
|
||||
}
|
||||
|
||||
if (key.escape || ch === 'q') {
|
||||
return patchOverlayState({ gridTest: null })
|
||||
}
|
||||
|
||||
if (key.return) {
|
||||
return updateGrid(grid => ({ ...grid, nested: true, zoomed: true }))
|
||||
}
|
||||
|
||||
if (ch === 'n') {
|
||||
return updateGrid(grid => ({ ...grid, nested: !grid.nested }))
|
||||
}
|
||||
|
||||
if (ch === 'a') {
|
||||
return updateGrid(grid => ({ ...grid, areas: !grid.areas, streams: false }))
|
||||
}
|
||||
|
||||
if (ch === 's') {
|
||||
return updateGrid(grid => ({ ...grid, areas: false, streams: true }))
|
||||
}
|
||||
|
||||
if (ch === 'g') {
|
||||
return updateGrid(grid => ({ ...grid, gap: cycleAutoNumber(grid.gap, 3) }))
|
||||
}
|
||||
|
||||
if (ch === 'p') {
|
||||
return updateGrid(grid => ({ ...grid, paddingX: cycleAutoNumber(grid.paddingX, 2) }))
|
||||
}
|
||||
|
||||
if (ch === 'd') {
|
||||
return openDemoDialog()
|
||||
}
|
||||
|
||||
if (ch === 'r') {
|
||||
return resetGrid()
|
||||
}
|
||||
|
||||
if (ch === '+' || ch === '=') {
|
||||
return updateGrid(grid => ({ ...grid, cols: clamp(grid.cols + 1, 1, GRID_TEST_MAX_SIZE) }))
|
||||
}
|
||||
|
||||
if (ch === '-' || ch === '_') {
|
||||
return updateGrid(grid => ({ ...grid, cols: clamp(grid.cols - 1, 1, GRID_TEST_MAX_SIZE) }))
|
||||
}
|
||||
|
||||
if (ch === ']') {
|
||||
return updateGrid(grid => ({ ...grid, rows: clamp(grid.rows + 1, 1, GRID_TEST_MAX_SIZE) }))
|
||||
}
|
||||
|
||||
if (ch === '[') {
|
||||
return updateGrid(grid => ({ ...grid, rows: clamp(grid.rows - 1, 1, GRID_TEST_MAX_SIZE) }))
|
||||
}
|
||||
|
||||
if (key.leftArrow || ch === 'h') {
|
||||
return updateGrid(grid => ({ ...grid, activeCol: clamp(grid.activeCol - 1, 0, grid.cols - 1) }))
|
||||
}
|
||||
|
||||
if (key.rightArrow || ch === 'l') {
|
||||
return updateGrid(grid => ({ ...grid, activeCol: clamp(grid.activeCol + 1, 0, grid.cols - 1) }))
|
||||
}
|
||||
|
||||
if (key.upArrow || ch === 'k') {
|
||||
return updateGrid(grid => ({ ...grid, activeRow: clamp(grid.activeRow - 1, 0, grid.rows - 1) }))
|
||||
}
|
||||
|
||||
if (key.downArrow || ch === 'j') {
|
||||
return updateGrid(grid => ({ ...grid, activeRow: clamp(grid.activeRow + 1, 0, grid.rows - 1) }))
|
||||
}
|
||||
|
||||
return
|
||||
}
|
||||
|
||||
if (overlay.dialog) {
|
||||
if (key.escape || isCtrl(key, ch, 'c') || ch === 'q' || key.return) {
|
||||
return patchOverlayState({ dialog: null })
|
||||
}
|
||||
|
||||
// Stacked demo modals (dialog over grid-test): shared routing where
|
||||
// the topmost visual layer consumes input first.
|
||||
if (handleStackedModalInput(overlay, key, ch)) {
|
||||
return
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue