Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -39,34 +39,30 @@ function resolveToolRenderer(toolName: unknown): Component | undefined {
return toolRenderers[String(toolName || '').toLowerCase()]
}

// 组内专用渲染器的展开状态:执行中默认展开,结束后默认收起,手动切换后以手动为准
// 组内专用渲染器的展开状态:默认收起(执行中也是),手动切换后以手动为准
const manualToolExpanded = ref<Dict<boolean>>({})

function toolKey(toolItem: ChatContent, index: number) {
return String(toolItem.id ?? index)
}

function isToolExpanded(toolItem: ChatContent, index: number) {
return manualToolExpanded.value[toolKey(toolItem, index)] ?? toolItem.status === 'RUNNING'
return manualToolExpanded.value[toolKey(toolItem, index)] ?? false
}

function toggleTool(toolItem: ChatContent, index: number) {
manualToolExpanded.value[toolKey(toolItem, index)] = !isToolExpanded(toolItem, index)
}

// 汇总执行状态;分组和工具的手动展开状态优先于默认状态。
// 汇总执行状态;分组默认收起,手动切换后以手动为准。
const groupStatus = computed(() => {
const childStatuses = toolItems.value.map((toolItem) => toolItem.status)
if (childStatuses.includes('RUNNING')) return 'running'
if (childStatuses.some((childStatus) => childStatus === 'FAIL' || childStatus === 'FAILURE')) return 'error'
return childStatuses.length ? 'success' : ''
})

const manualExpanded = ref<boolean>()
const isExpanded = computed({
get: () => manualExpanded.value ?? groupStatus.value === 'running',
set: (value: boolean) => (manualExpanded.value = value),
})
const isExpanded = ref(false)
</script>

<template>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
<script setup lang="ts">
import { computed, ref } from 'vue'
import { computed, ref, useTemplateRef } from 'vue'
import type { ChatContent } from '@/api/types'
import ContentList from '@/conversation-panel/components/content-list/index.vue'
import { useBottomFollow } from '@/conversation-panel/core/bottom-follow'

/**
* 通用分组:content.name 为标题,content.status 可选(不传不显示状态),
* content.children 为任意内容项(可再嵌套 GROUP),交由 ContentList 渲染。
* 执行中或无状态时默认展开,结束后默认收起,手动切换后以手动为准;收起时展示最新一条子项在做什么。
* 默认收起(执行中也是),手动切换后以手动为准。
*/
defineOptions({ name: 'ConversationGroupContent' })

Expand All @@ -31,11 +32,10 @@ const statusType = computed(() => {

const isFailed = computed(() => props.content.status === 'FAIL' || props.content.status === 'FAILURE')

const manualExpanded = ref<boolean>()
const isExpanded = computed({
get: () => manualExpanded.value ?? (!props.content.status || props.content.status === 'RUNNING'),
set: (value: boolean) => (manualExpanded.value = value),
})
const isExpanded = ref(false)

// 分组内容限高滚动,高度与新建文件的差异区一致;子项流式更新时跟随到底部
useBottomFollow(useTemplateRef<HTMLElement>('groupScrollRef'))

function tail(text: string) {
const trimmed = text.trim()
Expand Down Expand Up @@ -82,7 +82,7 @@ const currentActivity = computed(() => {
<el-tag v-if="isFailed" size="small" :type="statusType">{{ statusText }}</el-tag>
</template>

<div v-if="isExpanded && childContents.length" class="mt-1 border-l-2 border-N900/15 pl-3">
<div v-if="isExpanded && childContents.length" ref="groupScrollRef" class="mt-1 max-h-[400px] overflow-y-auto border-l-2 border-N900/15 pl-3">
<ContentList :content-list="childContents" />
</div>
</MkCollapse>

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are my observations on this diff:

Potential Issues

  1. Behavioral regression (intentional but worth validating):

    • The default state changed from "expanded when RUNNING or no status" to always collapsed.
    • If any UX flow, test, or screenshot relies on the old auto-expand behavior for running groups, this will break. Confirm product agrees.
  2. useBottomFollow is called unconditionally with a template ref:

    • useTemplateRef<HTMLElement>('groupScrollRef') resolves only after mount, and only when the element exists. Since the target div is inside v-if="isExpanded && childContents.length", the ref will be null whenever the group is collapsed or has no children.
    • Verify that useBottomFollow handles a null/initially-undefined ref gracefully (attaching watchers lazily, re-resolving when the ref later becomes non-null after expansion). If it only grabs ref.value once on setup or onMounted, following won't work after the first expand/collapse cycle.
    • Also confirm it re-hooks when the element is unmounted/remounted by the v-if toggle.
  3. currentActivity / collapse header now shows even when collapsed:

    • The comment previously said "collapsed shows latest child activity." Now that default is collapsed, ensure the currentActivity computed is actually rendered in the header (this diff doesn't show the header, so verify elsewhere).
  4. Removed dynamic getter means content.status changes no longer affect expansion:

    • Previously a transition from RUNNING → DONE would auto-collapse. Now it stays as user left it. That's presumably desired ("manual wins"), just note it.

Optimization / Cleanliness

  1. max-h-[400px] hardcoded: Consider extracting to a shared constant / CSS variable alongside the "new file diff region" height mentioned in the comment, so the two stay in sync.

  2. overflow-y-auto + border styling: With border-l-2 pl-3, the scrollbar will sit at the far right of the div including padding. Minor, but if scrollbar aesthetics matter, consider inner wrapper.

  3. Accessibility: A scrollable container with fixed max-height should ideally have tabindex="0" (and possibly role/aria-label) so keyboard users can scroll it. Same could apply to the follow-scroll region — decide whether full a11y is required.

  4. Ref naming: groupScrollRef used via string key in useTemplateRef — fine in Vue 3.5+. Ensure the project's Vue version supports useTemplateRef (it was introduced in 3.5). If the codebase targets <3.5, this import will fail.

  5. Test coverage: Add/adjust tests verifying:

    • Default collapsed regardless of status.
    • Manual toggle persists and is unaffected by status change.
    • Bottom-follow triggers on child content updates while expanded.
  6. Import ordering: useBottomFollow is imported from core, placed after the component import — acceptable, but keep the project's lint grouping consistent.

Summary

The main risk is useBottomFollow receiving a ref that is frequently null due to the v-if. Please confirm its implementation handles lazy/conditional elements, and confirm the Vue version supports useTemplateRef. Otherwise the diff is straightforward and the behavior change looks intentional.

Expand Down
22 changes: 8 additions & 14 deletions ui/src/conversation-panel/components/content/items/reasoning.vue
Original file line number Diff line number Diff line change
@@ -1,21 +1,15 @@
<script setup lang="ts">
import { ref, watch } from 'vue'
import { ref, useTemplateRef } from 'vue'
import { useBottomFollow } from '@/conversation-panel/core/bottom-follow'

defineOptions({ name: 'ConversationReasoningContent' })

const props = defineProps<{ content: { content?: string | null; status?: string } }>()
const isExpanded = ref(true)
let prevStatus = props.content.status
defineProps<{ content: { content?: string | null; status?: string } }>()
// 默认收起,思考中也只显示状态文案,手动展开后查看内容
const isExpanded = ref(false)

watch(
() => props.content.status,
(status) => {
if (prevStatus === 'RUNNING' && status !== 'RUNNING') {
isExpanded.value = false
}
prevStatus = status
},
)
// 思考内容限高滚动,高度与新建文件的差异区一致;流式输出时跟随到底部
useBottomFollow(useTemplateRef<HTMLElement>('reasoningScrollRef'))
</script>

<template>
Expand All @@ -35,7 +29,7 @@ watch(
{{ content.status === 'RUNNING' ? '思考中' : '思考完成' }}
</span>
</template>
<div class="text-N500! whitespace-pre-wrap break-words">
<div ref="reasoningScrollRef" class="max-h-[400px] overflow-y-auto text-N500! whitespace-pre-wrap break-words">
{{ content.content }}
</div>
</MkCollapse>

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's my review of the diff:

Potential Issues

  1. isExpanded no longer controlled by lifecycle — possible UX regression (unverifiable from this diff). Previously, content auto-collapsed when status transitioned from RUNNING to done. Now it defaults to false and never changes programmatically. If the MkCollapse is still bound to isExpanded, fine—but confirm nothing else relies on the old auto-collapse behavior (e.g., tests, snapshots, or parent expectations). Also note: the commit message references "与新建文件的差异区一致" and "流式输出时跟随到底部", but there's no visible logic change that ensures the follow only happens while RUNNING. If useBottomFollow auto-scrolls unconditionally, it could jump the viewport unexpectedly after completion.

  2. ref given to v-model not shown in diff. With isExpanded = ref(false), make sure the <MkCollapse> binding (presumably v-model="isExpanded") remains intact throughout the template — it's not included in this diff hunk.

  3. Auto-collapse removal loses user-state preservation semantics. Previously expanded content collapsed on completion; now it starts collapsed. Verify product intent — "思考中也只显示状态文案" suggests content is hidden during streaming even though new text is arriving. This is a deliberate change, but users who expand mid-stream rely on useBottomFollow to keep up; ensure it handles the case where the scroll container appears/disappears with isExpanded.

  4. max-h-[400px] magic number duplicates layout knowledge. The comment says it matches the "新建文件的差异区" height. Consider extracting to a shared CSS variable/token so both places stay in sync.

  5. useTemplateRef<HTMLElement>('reasoningScrollRef') is evaluated at setup time. That's fine if the composable watches the ref reactively (it should, per the Vue 3.5 pattern). Confirm useBottomFollow accepts a Readonly<ShallowRef> and reacts to the element being toggled via isExpanded — otherwise following won't engage after expansion.

  6. defineProps result unused (no props const). Fine under vue/consistent-define-props-style rules, but double-check other tooling doesn't require a named variable. Otherwise lint-clean.

  7. whitespace-pre-wrap + overflow-y-auto: long single tokens can still overflow horizontally unless break-words (overflow-wrap: break-word) applies; it's present, good. But add overflow-x-hidden or pr-* padding to avoid a scrollbar overlaying content vertically.

  8. Accessibility: scroll container has no focusability/tabindex. Keyboard users can't scroll the reasoning content without focusing an ancestor. Consider tabindex="0" or a role="region" with a label.

Minor / Style

  • Typo in naming: text-N500! looks like a custom utility; ensure ! suffix convention matches the project (Tailwind v4?).
  • Composable name useBottomFollow(...) immediately invoked — fine, but consider calling it with the return value assigned if you later need pause/resume on user scroll.
  • Removing the watch eliminates reliance on prevStatus, which was fragile (didn't handle initial RUNNING → undefined etc.). Good simplification overall.

Optimization Suggestions

  • Bind height via a shared token: class="max-h-[var(--reasoning-max-height,400px)]".
  • Add shrink-0/min-h-0 as appropriate inside the collapse body so flex layouts don't distort the scroll area.
  • Ensure useBottomFollow respects user scrolling (only auto-follow when near bottom), otherwise streaming will fight manual scroll-up.
  • Keep a test/snapshot covering the "expand during RUNNING" path to lock in the new intended behavior.

Summary: No outright bugs in the shown hunks, but verify (a) the MkCollapse binding still wires isExpanded, (b) useBottomFollow gates following to streaming/near-bottom state, and (c) the behavioral change (start collapsed, no auto-collapse) is intentional and documented/tested.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ const props = defineProps<{ content: ChatContent }>()

// 工具执行状态与展开状态。
const loading = computed(() => props.content.status === 'RUNNING')
const isExpanded = ref(props.content.status === 'RUNNING')
const isExpanded = ref(false)

// 工具执行完成自动收起
watch(loading, (current, previous) => {
Expand Down
34 changes: 34 additions & 0 deletions ui/src/conversation-panel/core/bottom-follow.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import { onBeforeUnmount, watch, type Ref } from 'vue'
import { Scroll } from './scroll'

/**
* 限高滚动容器的底部跟随:容器挂载后定位到底部,内容变化时仅在用户仍位于底部附近才跟随。
* 容器随展开 / 收起挂载和卸载,跟随实例与内容监听随之创建和释放。
*/
export function useBottomFollow(containerRef: Ref<HTMLElement | null | undefined>) {
let scroll: Scroll | undefined
let observer: MutationObserver | undefined

function release() {
observer?.disconnect()
observer = undefined
scroll?.destroy()
scroll = undefined
}

watch(
containerRef,
(container) => {
release()
if (!container) return
const follow = new Scroll(container)
scroll = follow
follow.forceBottom()
observer = new MutationObserver(() => follow.scrollBottom())
observer.observe(container, { childList: true, subtree: true, characterData: true })
},
{ flush: 'post', immediate: true },
)

onBeforeUnmount(release)
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Potential Issues

  1. No "user near bottom" detection — The docblock promises "only follow when the user is still near the bottom," but MutationObserver always calls follow.scrollBottom(). This will yank the scroll position even if the user scrolled up to read something. You likely need a check like if (isNearBottom(container)).

  2. scrollBottom() on every mutation without throttling — Rapid DOM mutations (e.g., streaming text, character-by-character updates) trigger many synchronous layout reads/writes per frame, causing jank. Consider debouncing with requestAnimationFrame or checking inside the callback.

  3. Feedback loop risk — If scrollBottom() itself mutates children or attributes that fall under the observer's scope (childList, subtree, characterData), it can re-trigger the observer and create an infinite loop. Ensure scrolling doesn't cause DOM changes, or disconnect/guard during the scroll call.

  4. Missing attributes: true — Content expansion often occurs via CSS class/style changes (e.g., collapsed vs. expanded) without any child/character mutation. If your container grows this way, the observer won't fire. Consider adding attributes: true (but beware of noise).

  5. No unmount cleanup for a mid-flight debounce — If you add debouncing, ensure pending timers/frames are cancelled in release(); otherwise callbacks can run after the element is detached.

  6. scroll?.destroy() naming asymmetry — forceBottom/scrollBottom are imperative-scroll operations; destroy implies lifecycle. Verify Scroll.destroy() actually removes listeners and isn't just a no-op, since it's the only teardown available here.

  7. flush: 'post' correctness — Good choice so the container exists in the DOM before measuring. However, immediate: true with flush: 'post' may still run before the browser has laid out (fonts, images). forceBottom() could compute an incorrect height on first mount; consider deferring initial scroll to the next requestAnimationFrame.

  8. Return value absent — The composable returns nothing. Callers can't force a manual follow (e.g., "jump to latest" button). Consider exposing { scrollToBottom, release }.

  9. Ref<HTMLElement | null | undefined> — Undefined and null are both handled identically; typically Ref<HTMLElement | null> suffices. Minor.

  10. SSR safety — MutationObserver isn't guarded. If the module ever runs server-side, instantiation will throw. Guard with typeof MutationObserver !== 'undefined' or import from vue (some setups polyfill this; otherwise add a check).

Optimization Suggestions

import { onBeforeUnmount, watch, type Ref } from 'vue'
import { Scroll } from './scroll'

export function useBottomFollow(containerRef: Ref<HTMLElement | null>) {
  let scroll: Scroll | undefined
  let observer: MutationObserver | undefined
  let rafId = 0

  const isNearBottom = (el: HTMLElement) =>
    el.scrollHeight - el.scrollTop - el.clientHeight < 32

  function release() {
    cancelAnimationFrame(rafId)
    rafId = 0
    observer?.disconnect()
    observer = undefined
    scroll?.destroy()
    scroll = undefined
  }

  watch(
    containerRef,
    (container) => {
      release()
      if (!container || typeof MutationObserver === 'undefined') return

      const follow = new Scroll(container)
      scroll = follow
      requestAnimationFrame(() => follow.forceBottom())

      observer = new MutationObserver(() => {
        if (!isNearBottom(container)) return
        cancelAnimationFrame(rafId)
        rafId = requestAnimationFrame(() => follow.scrollBottom())
      })
      observer.observe(container, { childList: true, subtree: true, characterData: true })
    },
    { flush: 'post', immediate: true },
  )

  onBeforeUnmount(release)

  return { release }
}

Summary

The main functional gap is the missing "near bottom" guard — the current implementation defeats its own stated behavior. After that, throttling via rAF and guarding against observer feedback loops are the highest-value fixes.

Loading