fix(gui): address goal UI review feedback

This commit is contained in:
_Kerman
2026-07-22 22:18:13 +08:00
parent feeea91bf8
commit beec67f187
31 changed files with 325 additions and 220 deletions

View File

@@ -63,8 +63,9 @@ export class Session implements ObservableSnapshot<ConversationSnapshot> {
private lastAgentError: string | null = null
/** Current goal projection; undefined = not yet fetched, null = no goal set. */
private goal: GoalView | null | undefined = undefined
/** Coalesced goal refetch (the open() idiom): live goal-change events share one in-flight get. */
/** Coalesced goal refetch; a trigger received in flight schedules one trailing read. */
private goalFetch: Promise<void> | null = null
private goalFetchPending = false
/** Bumped on every local goal write; a get result older than the latest write is stale and
* dropped (a mutation response that landed mid-fetch is always newer than the get's read). */
private goalWriteRev = 0
@@ -127,17 +128,27 @@ export class Session implements ObservableSnapshot<ConversationSnapshot> {
return result
}
/** Fetch the current goal, coalesced: concurrent triggers share the in-flight get (identity-guarded
* like openPromise — a superseded fetch must not null out the one that replaced it). */
/** Fetch the current goal, coalesced: concurrent triggers share the in-flight get. */
private fetchGoal(): Promise<void> {
if (this.goalFetch !== null) return this.goalFetch
const promise = this.doFetchGoal().finally(() => {
if (this.goalFetch === promise) this.goalFetch = null
})
if (this.goalFetch !== null) {
this.goalFetchPending = true
return this.goalFetch
}
const promise = this.drainGoalFetches().finally(() => { this.goalFetch = null })
this.goalFetch = promise
return promise
}
/** Drain the current read plus one coalesced trailing read for triggers received in flight. */
private async drainGoalFetches(): Promise<void> {
do {
this.goalFetchPending = false
await this.doFetchGoal()
// A goal-change callback can set the flag while doFetchGoal is suspended.
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
} while (this.goalFetchPending)
}
/** The get behind fetchGoal: folds transport failures (fail-soft like loadOlder, logged) and
* drops the result when a mutation response landed mid-flight (write revision moved on). */
private async doFetchGoal(): Promise<void> {
@@ -153,18 +164,13 @@ export class Session implements ObservableSnapshot<ConversationSnapshot> {
}
}
/**
* Create a goal for this session.
* @param objective - the goal's objective text.
* @param maxGoalRounds - optional cap on admitted goal rounds (host default when absent).
* @returns the created goal view; transport failures fold into a failed result, never a rejection.
*/
async createGoal(objective: string, maxGoalRounds?: number): Promise<RpcResult<{ goal: GoalView }>> {
/** Execute a goal mutation and publish its successful projection. */
private async updateGoal(
request: () => Promise<{ result: RpcResult<{ goal: GoalView }> }>,
): Promise<RpcResult<{ goal: GoalView }>> {
let result: RpcResult<{ goal: GoalView }>
try {
result = (await this.api.goals.create({
sessionId: this.sessionId, objective, ...(maxGoalRounds !== undefined ? { maxGoalRounds } : {}),
})).result
result = (await request()).result
} catch (error) {
result = transportError(error)
}
@@ -176,6 +182,18 @@ export class Session implements ObservableSnapshot<ConversationSnapshot> {
return result
}
/**
* Create a goal for this session.
* @param objective - the goal's objective text.
* @param maxGoalRounds - optional cap on admitted goal rounds (host default when absent).
* @returns the created goal view; transport failures fold into a failed result, never a rejection.
*/
async createGoal(objective: string, maxGoalRounds?: number): Promise<RpcResult<{ goal: GoalView }>> {
return this.updateGoal(() => this.api.goals.create({
sessionId: this.sessionId, objective, ...(maxGoalRounds !== undefined ? { maxGoalRounds } : {}),
}))
}
/**
* Edit this session's goal objective or round cap (CAS with the locally held revision).
* @param objective - replacement objective text; absent leaves it unchanged.
@@ -186,23 +204,22 @@ export class Session implements ObservableSnapshot<ConversationSnapshot> {
if (this.goal === null || this.goal === undefined) {
return { ok: false, error: { code: 'internal', message: 'No goal to edit', details: {} } }
}
let result: RpcResult<{ goal: GoalView }>
try {
result = (await this.api.goals.edit({
sessionId: this.sessionId,
ref: { id: this.goal.id, revision: this.goal.revision },
...(objective !== undefined ? { objective } : {}),
...(maxGoalRounds !== undefined ? { maxGoalRounds } : {}),
})).result
} catch (error) {
result = transportError(error)
const goal = this.goal
return this.updateGoal(() => this.api.goals.edit({
sessionId: this.sessionId,
ref: { id: goal.id, revision: goal.revision },
...(objective !== undefined ? { objective } : {}),
...(maxGoalRounds !== undefined ? { maxGoalRounds } : {}),
}))
}
/** Apply a phase transition to the current goal. */
private transitionGoal(operation: 'pause' | 'resume' | 'complete'): Promise<RpcResult<{ goal: GoalView }>> {
if (this.goal === null || this.goal === undefined) {
return Promise.resolve({ ok: false, error: { code: 'internal', message: `No goal to ${operation}`, details: {} } })
}
if (result.ok) {
this.goal = result.value.goal
this.goalWriteRev++
this.notifier.markDirty()
}
return result
const ref = { id: this.goal.id, revision: this.goal.revision }
return this.updateGoal(() => this.api.goals[operation]({ sessionId: this.sessionId, ref }))
}
/**
@@ -210,23 +227,7 @@ export class Session implements ObservableSnapshot<ConversationSnapshot> {
* @returns the updated goal view; fails without a current goal, and transport failures fold into a failed result.
*/
async pauseGoal(): Promise<RpcResult<{ goal: GoalView }>> {
if (this.goal === null || this.goal === undefined) {
return { ok: false, error: { code: 'internal', message: 'No goal to pause', details: {} } }
}
let result: RpcResult<{ goal: GoalView }>
try {
result = (await this.api.goals.pause({
sessionId: this.sessionId, ref: { id: this.goal.id, revision: this.goal.revision },
})).result
} catch (error) {
result = transportError(error)
}
if (result.ok) {
this.goal = result.value.goal
this.goalWriteRev++
this.notifier.markDirty()
}
return result
return this.transitionGoal('pause')
}
/**
@@ -234,23 +235,7 @@ export class Session implements ObservableSnapshot<ConversationSnapshot> {
* @returns the updated goal view; fails without a current goal, and transport failures fold into a failed result.
*/
async resumeGoal(): Promise<RpcResult<{ goal: GoalView }>> {
if (this.goal === null || this.goal === undefined) {
return { ok: false, error: { code: 'internal', message: 'No goal to resume', details: {} } }
}
let result: RpcResult<{ goal: GoalView }>
try {
result = (await this.api.goals.resume({
sessionId: this.sessionId, ref: { id: this.goal.id, revision: this.goal.revision },
})).result
} catch (error) {
result = transportError(error)
}
if (result.ok) {
this.goal = result.value.goal
this.goalWriteRev++
this.notifier.markDirty()
}
return result
return this.transitionGoal('resume')
}
/**
@@ -258,23 +243,7 @@ export class Session implements ObservableSnapshot<ConversationSnapshot> {
* @returns the updated goal view; fails without a current goal, and transport failures fold into a failed result.
*/
async completeGoal(): Promise<RpcResult<{ goal: GoalView }>> {
if (this.goal === null || this.goal === undefined) {
return { ok: false, error: { code: 'internal', message: 'No goal to complete', details: {} } }
}
let result: RpcResult<{ goal: GoalView }>
try {
result = (await this.api.goals.complete({
sessionId: this.sessionId, ref: { id: this.goal.id, revision: this.goal.revision },
})).result
} catch (error) {
result = transportError(error)
}
if (result.ok) {
this.goal = result.value.goal
this.goalWriteRev++
this.notifier.markDirty()
}
return result
return this.transitionGoal('complete')
}
/**

View File

@@ -647,6 +647,12 @@ describe('goal session methods', () => {
expect(session.getSnapshot().goal).toEqual(goal)
})
it('createGoal forwards an explicit round cap', async () => {
const { api, session } = makeSession()
await session.createGoal('bounded', 7)
expect(api.callsOf('goal.create')).toEqual([{ sessionId: SID, objective: 'bounded', maxGoalRounds: 7 }])
})
it('editGoal sends the current ref and updates snapshot', async () => {
const { api, session } = makeSession()
api.onHistory = () => histResponse(plainTurn(0, 0, 'a', 'b'))
@@ -661,6 +667,18 @@ describe('goal session methods', () => {
expect(session.getSnapshot().goal).toEqual(edited)
})
it('editGoal can replace only the round cap', async () => {
const { api, session } = makeSession()
const goal = makeGoal()
api.goals.create = () => Promise.resolve(ok({ goal }))
await session.createGoal('test-goal')
const edited = makeGoal({ revision: 2, maxGoalRounds: 9 })
const edit = vi.fn(() => Promise.resolve(ok({ goal: edited })))
api.goals.edit = edit
await session.editGoal(undefined, 9)
expect(edit).toHaveBeenCalledWith({ sessionId: SID, ref: { id: goal.id, revision: 1 }, maxGoalRounds: 9 })
})
it('editGoal returns an error when no goal exists', async () => {
const { api, session } = makeSession()
api.onHistory = () => histResponse(plainTurn(0, 0, 'a', 'b'))
@@ -670,6 +688,15 @@ describe('goal session methods', () => {
expect((r as { error: { code: string } }).error.code).toBe('internal')
})
it('phase mutations and clear return an error when no goal exists', async () => {
const { api, session } = makeSession()
api.onHistory = () => histResponse(plainTurn(0, 0, 'a', 'b'))
await session.open()
for (const mutate of [() => session.pauseGoal(), () => session.resumeGoal(), () => session.completeGoal(), () => session.clearGoal()]) {
expect((await mutate()).ok).toBe(false)
}
})
it('pauseGoal pauses and updates snapshot', async () => {
const { api, session } = makeSession()
api.onHistory = () => histResponse(plainTurn(0, 0, 'a', 'b'))
@@ -748,6 +775,14 @@ describe('goal session methods', () => {
expect(session.getSnapshot().goal).toEqual(goal)
})
it('keeps the goal unresolved when the eager fetch returns an RPC error', async () => {
const { api, session } = makeSession()
api.onHistory = () => histResponse([])
api.goals.get = () => Promise.resolve(err({ code: 'internal', message: 'unavailable', details: {} }))
await session.open()
expect(session.getSnapshot().goal).toBeUndefined()
})
const goalChangeEvent = (seq: number, operation: string): SessionEvent =>
at(seq, {
type: 'context/message', surfaceOp: 'append',
@@ -773,7 +808,7 @@ describe('goal session methods', () => {
},
})
it('live goal-change meta triggers one coalesced refetch; window replays never refetch', async () => {
it('live goal-change meta coalesces to one in-flight read plus one trailing read; window replays never refetch', async () => {
const { api, session } = makeSession()
// The history window replays goal-change meta (a snapshot change AND a clear tombstone).
api.onHistory = () => histResponse([...plainTurn(0, 0, 'a', 'b'), goalChangeEvent(6, 'create'), goalClearEvent(7)])
@@ -791,12 +826,14 @@ describe('goal session methods', () => {
})
expect(refetches).toBe(0)
// Two live goal events (change + clear tombstone) coalesce into a single refetch.
// Two live goal events (change + clear tombstone) share the in-flight read, then the
// second trigger schedules a trailing read so an independently ordered GET cannot win.
session.handleMuxEnvelope('r1' as never, { type: 'session/event', sessionId: SID, event: goalChangeEvent(9, 'edit') })
session.handleMuxEnvelope('r2' as never, { type: 'session/event', sessionId: SID, event: goalClearEvent(10) })
expect(refetches).toBe(1)
const goal = makeGoal({ revision: 3 })
gate.resolve(ok({ goal }))
await vi.waitFor(() => { expect(refetches).toBe(2) })
await vi.waitFor(() => { expect(session.getSnapshot().goal).toEqual(goal) })
})
@@ -837,6 +874,11 @@ describe('goal session methods', () => {
const paused = await session.pauseGoal()
expect(paused).toMatchObject({ ok: false, error: { code: 'internal', message: 'pause wire down' } })
expect(session.getSnapshot().goal).toEqual(goal) // local state untouched
api.goals.clear = () => Promise.reject(new Error('clear wire down'))
const cleared = await session.clearGoal()
expect(cleared).toMatchObject({ ok: false, error: { code: 'internal', message: 'clear wire down' } })
expect(session.getSnapshot().goal).toEqual(goal)
})
it('a goal.get transport rejection on a live refetch is logged and swallowed', async () => {