mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-24 16:54:43 +00:00
fix(tui): fix mount-collapse and live-progress gating for MoA reference panels
Maintainer review (hermes-sweeper) on this PR found the fix was incomplete: two paths still hid the MoA reference panel under thinking: hidden. 1. thinking.tsx: the mount useState correctly seeds openThinking from (visible.thinking === 'expanded' || reasoningAlwaysVisible), but the re-sync effect on [visible] fires after the FIRST render too, not just later updates, and lacks the reasoningAlwaysVisible OR — so it immediately collapsed a just-opened MoA panel right after mount. Skip only the effect's very first run (a ref flag); every later visible change still re-syncs without the override, preserving the documented no-OR-at-effect-time contract (manual collapse sticks). 2. useMainApp.ts: showProgressArea's streamSegments predicate gated thinking content on thinkingPanelVisible alone, so an MoA reference segment (segment.isMoaReference, same flag messageLine.tsx's shouldShowThinkingTrail already honors per #64657) never kept the live progress area up when thinking was hidden — StreamingAssistant then returned early before MessageLine was ever reached. Added the same override. Added tests/thinkingMoaReferenceVisibility.test.tsx: mounts ToolTrail with reasoningAlwaysVisible + sections.thinking: hidden, awaits queued effects, and asserts the chevron is still open (▾, not ▸) once they settle. Validation: npx vitest run src/__tests__/thinkingMoaReferenceVisibility.test.tsx -> 1 passed Fail-before: reverting only the thinking.tsx ref-guard reproduces the exact regression -- the same test's frame capture shows the panel open on first paint then collapsing to ▸ once the effect fires, and the 'not.toContain(▸)' assertion fails as expected. npx vitest run (full ui-tui suite): 1115 passed, 8 failed -- all 8 pre-existing and unrelated (terminalSetup/terminalParity/editor resolution env-path tests), confirmed by running them in isolation with the same result regardless of this diff. npx tsc --noEmit: clean. npx eslint src/components/thinking.tsx src/app/useMainApp.ts: clean.
This commit is contained in:
parent
07a732c2e5
commit
d7fbd13997
3 changed files with 85 additions and 5 deletions
61
ui-tui/src/__tests__/thinkingMoaReferenceVisibility.test.tsx
Normal file
61
ui-tui/src/__tests__/thinkingMoaReferenceVisibility.test.tsx
Normal file
|
|
@ -0,0 +1,61 @@
|
|||
import { PassThrough } from 'stream'
|
||||
|
||||
import { renderSync } from '@hermes/ink'
|
||||
import React from 'react'
|
||||
import { describe, expect, it } from 'vitest'
|
||||
|
||||
import { ToolTrail } from '../components/thinking.js'
|
||||
import { stripAnsi } from '../lib/text.js'
|
||||
import { DEFAULT_THEME } from '../theme.js'
|
||||
|
||||
describe('ToolTrail — MoA reference panel visibility (#64701)', () => {
|
||||
it('stays expanded after mount effects settle when reasoningAlwaysVisible is set, even under sections.thinking: hidden', async () => {
|
||||
const stdout = new PassThrough()
|
||||
const stdin = new PassThrough()
|
||||
const stderr = new PassThrough()
|
||||
let output = ''
|
||||
|
||||
Object.assign(stdout, { columns: 60, isTTY: false, rows: 20 })
|
||||
Object.assign(stdin, { isTTY: false })
|
||||
Object.assign(stderr, { isTTY: false })
|
||||
stdout.on('data', chunk => {
|
||||
output += chunk.toString()
|
||||
})
|
||||
|
||||
const instance = renderSync(
|
||||
<ToolTrail
|
||||
reasoning="Reference model output that must stay visible on first paint."
|
||||
reasoningAlwaysVisible
|
||||
sections={{ thinking: 'hidden' }}
|
||||
t={DEFAULT_THEME}
|
||||
/>,
|
||||
{
|
||||
patchConsole: false,
|
||||
stderr: stderr as NodeJS.WriteStream,
|
||||
stdin: stdin as NodeJS.ReadStream,
|
||||
stdout: stdout as NodeJS.WriteStream
|
||||
}
|
||||
)
|
||||
|
||||
// Let queued passive effects (and any re-render they trigger) flush
|
||||
// before reading the frame — the #64701 bug is specifically that the
|
||||
// re-sync effect fires AFTER the first paint and clobbers the
|
||||
// reasoningAlwaysVisible mount value, so asserting on the pre-effect
|
||||
// frame alone would miss the regression entirely.
|
||||
await new Promise(resolve => setImmediate(resolve))
|
||||
await new Promise(resolve => setImmediate(resolve))
|
||||
|
||||
const frame = stripAnsi(output)
|
||||
|
||||
instance.unmount()
|
||||
instance.cleanup()
|
||||
|
||||
// Open chevron (▾) means the panel is still expanded once effects have
|
||||
// settled, as the reasoningAlwaysVisible-seeded useState value intends.
|
||||
// A collapsed (▸) render here means the re-sync effect fired on mount
|
||||
// and clobbered it — the exact #64701 regression.
|
||||
expect(frame).toContain('▾ ')
|
||||
expect(frame).toContain('Thinking')
|
||||
expect(frame).not.toContain('▸ ')
|
||||
})
|
||||
})
|
||||
|
|
@ -1057,16 +1057,21 @@ export function useMainApp(gw: GatewayClient) {
|
|||
state.streamSegments.some(segment => {
|
||||
const hasThinking = Boolean(segment.thinking?.trim())
|
||||
const hasTrailTools = Boolean(segment.tools?.length)
|
||||
// A MoA reference segment (segment.isMoaReference) is the
|
||||
// user-facing mixture-of-agents process the user opted into, not
|
||||
// private model reasoning — it must keep the live progress area
|
||||
// (and therefore StreamingAssistant) up even when the thinking
|
||||
// panel is hidden, matching shouldShowThinkingTrail's settled-
|
||||
// transcript override in messageLine.tsx (#64657/#64701).
|
||||
const thinkingVisible = thinkingPanelVisible || Boolean(segment.isMoaReference)
|
||||
|
||||
if (segment.kind === 'trail' && !segment.text) {
|
||||
return (
|
||||
(thinkingPanelVisible && hasThinking) || ((toolsPanelVisible || activityPanelVisible) && hasTrailTools)
|
||||
)
|
||||
return (thinkingVisible && hasThinking) || ((toolsPanelVisible || activityPanelVisible) && hasTrailTools)
|
||||
}
|
||||
|
||||
return (
|
||||
Boolean(segment.text?.trim()) ||
|
||||
(thinkingPanelVisible && hasThinking) ||
|
||||
(thinkingVisible && hasThinking) ||
|
||||
((toolsPanelVisible || activityPanelVisible) && hasTrailTools)
|
||||
)
|
||||
}) ||
|
||||
|
|
|
|||
|
|
@ -1,5 +1,5 @@
|
|||
import { Box, NoSelect, Text } from '@hermes/ink'
|
||||
import { memo, type ReactNode, useEffect, useMemo, useState } from 'react'
|
||||
import { memo, type ReactNode, useEffect, useMemo, useRef, useState } from 'react'
|
||||
import spinners, { type BrailleSpinnerName } from 'unicode-animations'
|
||||
|
||||
import { THINKING_COT_MAX } from '../config/limits.js'
|
||||
|
|
@ -751,7 +751,21 @@ export const ToolTrail = memo(function ToolTrail({
|
|||
return () => clearInterval(id)
|
||||
}, [openTools, tools.length, visible.tools])
|
||||
|
||||
// Effects run after the FIRST render too, not just on later updates — so
|
||||
// this re-sync was clobbering the reasoningAlwaysVisible mount value above
|
||||
// right after mount, collapsing a just-opened MoA reference panel under
|
||||
// `thinking: hidden` before the user ever saw it (#64701). Skip only the
|
||||
// very first run; every subsequent `visible` change (the case this effect
|
||||
// exists for) still re-syncs without the override, so a manual collapse
|
||||
// still sticks per the no-OR-at-effect-time rule above.
|
||||
const skippedInitialSync = useRef(false)
|
||||
useEffect(() => {
|
||||
if (!skippedInitialSync.current) {
|
||||
skippedInitialSync.current = true
|
||||
|
||||
return
|
||||
}
|
||||
|
||||
setOpenThinking(visible.thinking === 'expanded')
|
||||
setOpenTools(visible.tools === 'expanded')
|
||||
setOpenSubagents(visible.subagents === 'expanded')
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue