Skip to content

feat: Data sources for conversation support - #7840

Merged
shaohuzhang1 merged 1 commit into
v3from
pr@v3@feat_knowledge_source
Nov 11, 2026
Merged

shaohuzhang1 merged 1 commit into
v3from
pr@v3@feat_knowledge_source

Conversation

@shaohuzhang1

Copy link
Copy Markdown
Contributor

feat: Data sources for conversation support

}

return { isFullscreen, bind, unbind, toggle }
}

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 is a review of the provided code, highlighting potential issues, irregularities, and optimization suggestions.

1. Critical Issues & Bugs

A. ResizeObserver Loop Risk

Issue: In the resize observer, you recalculate renderedWidth inside drawPages. However, changing the canvas width/height or appending elements can slightly alter the container's scroll dimensions, potentially triggering the ResizeObserver again.
Fix: Ensure the check Math.floor(entry.contentRect.width) === renderedWidth is robust. In drawPages, update renderedWidth before any DOM mutations that affect layout (though replaceChildren happens later, the logic flow needs care). More importantly, ensure the debounce timeout (200ms) is sufficient to absorb multiple resize events during one interaction.

B. Memory Leak in releaseDocument

Issue: loadingTask?.destroy() returns a Promise. If pdfDocument.getPage() or page.render() is hanging internally, destroy() might not clean up immediately. While await handles this, if releaseDocument throws (e.g., unexpected null), subsequent cleanup might be skipped.
Suggestion: Wrap await task?.destroy() in a try-catch block within releaseDocument to prevent breaking the cleanup chain.

C. Race Condition in redraw vs. render

Issue: The redraw function increments token and calls drawPages(current, false). If render is currently active (progressive loading), redraw will also increment token. While the token check prevents old data from being used, calling drawPages while a progressive render is ongoing might lead to partial renders mixed with non-progressive logic if not careful. However, since token increments, the previous loop exits. The main risk is performance: resizing during initial load cancels the load and restarts it non-progressively (or vice versa depending on timing), potentially wasting work.
Optimization: Consider debouncing all draw actions or preventing resize-triggered redraws during the initial progressive load unless strictly necessary for correct sizing.

D. scrollRatio Calculation Timing

Issue: In drawPages(..., false):

const scrollRatio = scroller && scroller.scrollHeight > 0 ? scroller.scrollTop / scroller.scrollHeight : 0
target.replaceChildren(...canvases)
if (scroller) scroller.scrollTop = scrollRatio * scroller.scrollHeight

Problem: scroller.scrollHeight reads the DOM synchronously after replaceChildren. Since canvases have fixed CSS dimensions (style.width/height), scrollHeight should reflect the new state immediately. This is likely correct, but browser reflow might delay it. Using scroller.scrollTop = Math.round(scrollRatio * scroller.scrollHeight) ensures integer pixel alignment, avoiding sub-pixel jitter.

2. Optimization Suggestions

A. Canvas Context Initialization

Current: page.render({ canvas, ... }) pdf.js sets up the context internally.
Suggestion: For better performance on high-DPI screens, explicitly setting alpha: false or premultipliedAlpha: false when creating the context (if you were doing it manually) helps. However, pdf.js abstracts this. No direct change needed, but ensure outputScale calculation avoids excessive memory usage. window.devicePixelRatio || 1 is fine, but consider capping it at 2 or 3 to prevent massive canvas allocations on high-end devices (4K/Retina + Browser Zoom).

B. isPdfDocument Logic

Current: Checks file extension.
Suggestion: File extensions are unreliable (user-controlled). Ensure backend validation exists. In frontend, this is acceptable for UI hints, but never trust it for security.

C. Debounce Timer Management

Current:

resizeTimer = window.setTimeout(redraw, 200)

Suggestion: Clear previous timer correctly (done), but also ensure resizeTimer is cleared on destroy (done). One minor improvement: Use requestAnimationFrame inside the timeout if further smoothing is needed, though 200ms debounce is usually sufficient for PDFs.

D. loadPdfJs Caching

Current: Correctly caches the promise.
Suggestion: If the worker URL changes dynamically (unlikely in static builds), this cache would break. Ensure build tools (vite/webpack) handle the ?url import correctly.

3. Code Style & Best Practices

A. Error Handling in createFullscreenToggle

Current: Silently swallows errors in toggle.
Improvement: Log the error or expose a reactive error ref similar to the renderer, so users can see why fullscreen failed (e.g., iframe restrictions).

B. Type Safety for worker.default

Current:

pdfJs.GlobalWorkerOptions.workerSrc = worker.default

Note: import('...?url') typically exports a default string. If types are incorrect, cast carefully:

const workerUrl = (worker as any).default as string;

C. Unused Variable renderTasks Filtering

In finally:

renderTasks = renderTasks.filter((renderTask) => renderTask !== task)

This is O(n) on every page finish. If a document has 100 pages, this runs 100 times against a shrinking array. It’s negligible for <500 pages but inefficient. A Set<RenderTask> would be more appropriate, but manual removal is fine for typical PDF sizes. Alternatively, just set task.cancel() logic to rely solely on the global token, and stop tracking individual task lists if cancellation happens via token invalidation mostly. However, explicit cancellation (task.cancel()) stops rendering early, saving CPU/GPU. So keeping this is good for responsiveness.

D. Vue Reactivity

container and scroller are plain variables, not ref. This is correct because they hold DOM nodes which shouldn't be deeply reactive. Accessing them via closure is efficient.

4. Modified Code Snippet (Key Improvements)

Here are targeted fixes for the identified issues:

// ... imports ...

/**
 * PDF 渲染器优化版说明:
 * 1. Cap outputScale to prevent memory explosions on high-DPI/Zoomed environments.
 * 2. Robust cleanup for releaseDocument.
 * 3. Integer rounding for scroll positions to prevent jitter.
 */
export function createPdfRenderer() {
  // ... existing refs ...
  
  let renderedWidth = 0
  let resizeTimer: number | undefined
  
  // ... existing helpers ...

  const releaseDocument = async () => {
    cancelRenderTasks()
    container?.replaceChildren()
    const task = loadingTask
    loadingTask = null
    pdfDocument = null
    
    // Ensure destroy doesn't halt execution if it throws/resolves late
    if (task) {
      try {
        await task.destroy()
      } catch (e) {
        console.warn("Error destroying pdf task", e)
      }
    }
  }

  async function drawPages(current: number, progressive: boolean) {
    if (!pdfDocument || !container) return
    cancelRenderTasks()
    const target = container
    
    // IMPORTANT: Only clear children for progressive. 
    // Non-progressive draws to offscreen canvases first.
    if (progressive) target.replaceChildren() 
    
    // Calculate width safely
    const containerWidth = Math.floor(target.clientWidth)
    if (containerWidth <= 0) return
    
    renderedWidth = containerWidth
    
    // Cap scale to avoid huge canvases (e.g. zoom 500% on 4k screen)
    const dpr = window.devicePixelRatio || 1
    const outputScale = Math.min(dpr > 1 ? dpr : 1, 3) 

    const canvases: HTMLCanvasElement[] = []

    for (let pageNumber = 1; pageNumber <= pdfDocument.numPages; pageNumber++) {
      // Check token before heavy work
      if (current !== token) return 

      const page = await pdfDocument!.getPage(pageNumber)
      if (current !== token) return // Re-check after await
      
      const baseViewport = page.getViewport({ scale: 1 })
      
      // Avoid division by zero
      const scale = renderedWidth > 0 ? renderedWidth / baseViewport.width : 1
      const viewport = page.getViewport({ scale: scale })

      const canvas = document.createElement('canvas')
      const w = Math.floor(viewport.width * outputScale)
      const h = Math.floor(viewport.height * outputScale)
      
      // Sanity check canvas size
      if (w === 0 || h === 0) continue

      canvas.width = w
      canvas.height = h
      canvas.style.width = `${Math.floor(viewport.width)}px`
      canvas.style.height = `${Math.floor(viewport.height)}px`
      
      if (progressive) target.appendChild(canvas)
      else canvases.push(canvas)

      const task = page.render({
        canvas,
        viewport: viewport,
        transform: outputScale === 1 ? undefined : [outputScale, 0, 0, outputScale, 0, 0],
      })
      
      renderTasks.push(task)
      
      try {
        await task.promise
      } catch (renderError) {
        if (isRenderCancelled(renderError)) {
          // Clean up removed canvas if progressive
          if (progressive && canvas.parentNode === target) target.removeChild(canvas)
          return
        }
        throw renderError
      } finally {
        renderTasks = renderTasks.filter(t => t !== task)
      }
    }

    // If non-progressive, replace all at once
    if (!progressive && current === token && canvases.length > 0) {
       // Preserve scroll position
       const scrollY = scroller?.scrollTop || 0
       const targetHeight = target.scrollHeight
       const totalContentHeight = canvases.reduce((acc, c) => acc + parseInt(c.style.height), 0)
       
       target.replaceChildren(...canvases)
       
       if (scroller) {
         // Recalculate ratio based on NEW content height if possible, 
         // or keep simple ratio if old/new heights are similar.
         // Using old scrollY vs old targetHeight can drift if aspect ratios vary significantly per page.
         // Safer: Try to maintain the "visual" position relative to specific pages if needed, 
         // but for general fit, scaling scrollY by (newTotalHeight / oldTargetHeight) is an approximation.
         
         // Simple approach often works best for identical documents:
         const newHeight = target.scrollHeight
         if (targetHeight > 0 && newHeight > 0) {
            scroller.scrollTop = Math.round((scrollY / targetHeight) * newHeight)
         }
       }
    }
  }

  // ... rest of the code remains similar, ensure ResizeObserver uses the capped logic implicitly through drawPages recalculating width ...

  const resizeObserver = new ResizeObserver(([entry]) => {
    if (!entry || !pdfDocument) return
    
    const newWidth = Math.floor(entry.contentRect.width)
    // Strict comparison to avoid unnecessary redraws on 1px fluctuations
    if (newWidth === renderedWidth || newWidth === 0) return

    window.clearTimeout(resizeTimer)
    resizeTimer = window.setTimeout(() => {
      if (newWidth === Math.floor(container?.clientWidth || 0)) {
         redraw()
      }
    }, 200)
  })
  
  // ...
}

Summary of Key Changes Recommended:

  1. Safety: Added checks for clientWidth <= 0 and canvas dimension validity.
  2. Performance: Capped devicePixelRatio to prevent memory crashes.
  3. Scroll Accuracy: Improved scroll restoration logic to account for new content height after replacement.
  4. Robustness: Wrapped destroy in try-catch to ensure cleanup completes.
  5. Responsiveness: Added re-checking of token immediately after awaiting getPage and render promise, ensuring stale loops exit faster.

return { paragraphs, previewDocument, sourceParagraphs, count, setParagraphs, openDocument, closePreview }
}

export type KnowledgeSourceStore = ReturnType<typeof createKnowledgeSourceStore>

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 is the code review, potential issues, and optimization suggestions for the provided Vue 3 composable.

1. Potential Issues & Bugs

A. previewDocument Type Mismatch (Critical)

  • Issue: The previewDocument ref is typed as PreviewDocument | null, but in openDocument, you are creating a new object: { name: documentName, url: documentUrl }.
  • Risk: If PreviewDocument type definition (from ./document-preview/index) does not exactly match { name: string; url: string }, TypeScript might complain, or more importantly, if PreviewDocument expects additional fields or specific structures, passing this partial object could break the preview component. It also creates a new reactive wrapper implicitly, which is fine here, but ensure the types align strictly.
  • Suggestion: Ensure PreviewDocument interface matches { name: string; url: string } or create a specific type for this local state if PreviewDocument is broader.

B. similarity Sorting Stability

  • Issue: In sourceParagraphs, you sort by similarity. If multiple paragraphs have the same similarity score or if similarity is undefined/null (treated as 0), their relative order depends on the underlying JavaScript engine's sort stability (which is stable in modern engines) and the original array order.
  • Suggestion: This is likely acceptable, but consider adding a secondary sort key (e.g., by index or ID) to guarantee deterministic display order when similarities are equal.

C. toSafeWebUrl Regex Limitation

  • Issue: The regex /^https?:\/\//i.test(url.trim()) checks for protocol but does not validate the domain part against XSS vectors like http://javascript:... (though browsers block this usually) or other protocols that might slip through if the input is malformed but starts with http://.
  • Suggestion: Consider using the native URL constructor for validation if you need stricter safety, though the current check is generally sufficient for filtering out javascript: and data: URIs.

D. getFileUrl Side Effects/Statelessness

  • Issue: getFileUrl is called inside computed properties (toSourceParagraph -> getDocumentTarget). If getFileUrl relies on external mutable state (e.g., auth tokens changing via global ref) without triggering reactivity properly, the computed value might be stale.
  • Suggestion: Ensure getFileUrl is a pure function based on its arguments. If it depends on reactive auth state, ensure that state is tracked. If it's just concatenating strings, it's safe.

2. Code Quality & Readability

A. Magic Strings/Numbers

  • Issue: '图片向量' : '文本向量' are hardcoded strings.
  • Suggestion: Move these to constants or i18n keys if internationalization is required later, even if currently Chinese-only, to maintain consistency with other UI text handling.

B. parseMeta Robustness

  • Issue: JSON.parse(meta) can throw non-syntax errors too (e.g., memory limits on huge strings, though rare).
  • Suggestion: The current catch {} is broad. Log a warning in development mode to help diagnose bad data from the backend.

C. Naming Conventions

  • Consistency: You use meta variable for both parsed meta and raw meta in different contexts. In getDocumentTarget, meta is clearly the parsed object. In parseMeta, it's the raw input. This is clear locally.
  • Variable naming: left/right in sort comparator is good practice over a/b.

3. Optimization Suggestions

A. Avoid Unnecessary Array Copies in Computed Property

  • Current: [...paragraphs.value].sort(...)
  • Issue: ref arrays are not deeply reactive, but spreading them to mutate/sort avoids mutating the source paragraphs.value array directly. This is correct for immutability. However, if paragraphs.value is large, sorting every time sourceParagraphs is accessed (if not properly cached by Vue's lazy evaluation) could be costly.
  • Note: Vue's computed is cached and only recomputes if dependencies change. Since paragraphs.value is a ref accessed as .value, changes to the array (via setParagraphs replacing the whole ref) will trigger recompute. Partial updates to the array (splice/push) might not trigger if not done reactively. Given setParagraphs replaces the entire ref, this is efficient. No change needed unless you update the array incrementally without replacing the ref.

B. Early Return in getImageUrl

  • Current: Logic is readable.
  • Suggestion: Break down the complex condition in getImageUrl for better readability:
    function getImageUrl(paragraph: ChatParagraphSource): string {
      const fileId = paragraph.document_resource_type === DOCUMENT_RESOURCE_TYPE.IMAGE 
        ? paragraph.document_image_id 
        : paragraph.hit_asset?.file_id;
    
      if (!fileId) return '';
    
      // Check if content already embeds the image URL to avoid duplication
      if (paragraph.content?.includes(`/oss/file/${fileId}`)) return '';
    
      return getFileUrl(fileId);
    }
    This separates the "Get File ID" logic from "Check Content" and "Get URL" logic, making it easier to test and understand.

C. toSourceParagraph Efficiency

  • Optimization: You call getFileUrl(meta.source_file_id) inside getDocumentTarget. If meta.source_file_id is falsy, getFileUrl still executes (depending on implementation). Ensure getFileUrl handles undefined gracefully or check before calling if calculation is heavy. Given it likely just builds a path, it's probably fine.

4. Revised Code Snippet (Suggestions Applied)

import { computed, ref } from 'vue'
import { DOCUMENT_RESOURCE_TYPE } from '@/api/enums'
import type { ChatParagraphSource, ChatParagraphSourceMeta } from '@/api/types'
import { downloadByURL, getFileUrl } from '@/utils/common'
import { MsgInfo } from '@/utils/message'
import { isPdfDocument, type PreviewDocument } from './document-preview/index'

/**
 * 来源文档的打开方式:知识库文件中的 PDF 在知识来源内用 pdf.js 预览,其他文件直接下载,均不交给浏览器原生打开;
 * 网页来源在新窗口打开原网址;无可用地址时提示原因。
 */
export const SOURCE_DOCUMENT_ACTION = {
  PREVIEW: 'PREVIEW',
  DOWNLOAD: 'DOWNLOAD',
  LINK: 'LINK',
  UNAVAILABLE: 'UNAVAILABLE',
} as const

export type SourceDocumentAction = (typeof SOURCE_DOCUMENT_ACTION)[keyof typeof SOURCE_DOCUMENT_ACTION]

/** 知识来源面板展示的分段,字段已按展示需要整理。 */
export interface KnowledgeSourceParagraph {
  key: string
  order: number
  title: string
  content: string
  similarity: string
  vectorLabel: string
  imageUrl: string
  documentName: string
  documentAction: SourceDocumentAction
  /** 来源文档地址,UNAVAILABLE 时为空字符串 */
  documentUrl: string
  allowDownload: boolean
  knowledgeName: string
  knowledgeType?: string
}

const VECTOR_LABEL_IMAGE = '图片向量'
const VECTOR_LABEL_TEXT = '文本向量'
const DEFAULT_TITLE = '-'

// 历史快照中 meta 可能是 JSON 字符串,解析失败按无元数据处理
function parseMeta(meta: ChatParagraphSource['meta']): ChatParagraphSourceMeta {
  if (!meta) return {}
  if (typeof meta !== 'string') return meta
  try {
    return JSON.parse(meta) as ChatParagraphSourceMeta
  } catch (error) {
    console.warn('Failed to parse meta:', error)
    return {}
  }
}

// 图片文档或命中图片资产时展示召回图片;正文已内嵌同一图片时不重复展示
function getImageUrl(paragraph: ChatParagraphSource): string {
  const fileId =
    (paragraph.document_resource_type === DOCUMENT_RESOURCE_TYPE.IMAGE ? paragraph.document_image_id : undefined) || paragraph.hit_asset?.file_id

  if (!fileId) return ''

  // 如果内容中已经包含了该图片的 URL,则不再单独展示缩略图
  if (paragraph.content?.includes(`/oss/file/${fileId}`)) return ''

  return getFileUrl(fileId)
}

// 网页来源只接受 http(s) 地址,避免 javascript: 等协议
const toSafeWebUrl = (url?: string): string => {
  if (!url) return ''
  const trimmedUrl = url.trim()
  return /^https?:\/\//i.test(trimmedUrl) ? trimmedUrl : ''
}

interface DocumentTarget {
  action: SourceDocumentAction
  url: string
}

function getDocumentTarget(documentName: string, meta: ChatParagraphSourceMeta): DocumentTarget {
  // 优先尝试文件预览/下载
  if (meta.source_file_id) {
    const fileUrl = getFileUrl(meta.source_file_id)
    if (fileUrl) {
      const action = isPdfDocument(documentName) ? SOURCE_DOCUMENT_ACTION.PREVIEW : SOURCE_DOCUMENT_ACTION.DOWNLOAD
      return { action, url: fileUrl }
    }
  }

  // 其次尝试网页链接
  const webUrl = toSafeWebUrl(meta.source_url)
  if (webUrl) return { action: SOURCE_DOCUMENT_ACTION.LINK, url: webUrl }

  return { action: SOURCE_DOCUMENT_ACTION.UNAVAILABLE, url: '' }
}

function toSourceParagraph(paragraph: ChatParagraphSource, index: number): KnowledgeSourceParagraph {
  const meta = parseMeta(paragraph.meta)
  const documentName = paragraph.document_name?.trim() || ''
  const documentTarget = getDocumentTarget(documentName, meta)
  
  const isImageResource = paragraph.document_resource_type === DOCUMENT_RESOURCE_TYPE.IMAGE || !!paragraph.hit_asset

  return {
    key: paragraph.id || String(index),
    order: index + 1,
    title: paragraph.title || DEFAULT_TITLE,
    content: paragraph.content || '',
    similarity: typeof paragraph.similarity === 'number' ? paragraph.similarity.toFixed(3) : '',
    vectorLabel: isImageResource ? VECTOR_LABEL_IMAGE : VECTOR_LABEL_TEXT,
    imageUrl: getImageUrl(paragraph),
    documentName,
    documentAction: documentTarget.action,
    documentUrl: documentTarget.url,
    allowDownload: meta.allow_download !== false, // Explicitly check for false, allowing undefined/default to true? Check business logic. Usually !== false means undefined is treated as true.
    knowledgeName: paragraph.knowledge_name || DEFAULT_TITLE,
    knowledgeType: paragraph.knowledge_type,
  }
}

/**
 * 知识来源面板 store:当前回答引用的分段(按相似度从高到低整理为展示数据)及正在预览的来源文档。
 * 由右侧栏 store 创建并组合,右侧开合与面板切换由右侧栏 store 维护。
 */
export function createKnowledgeSourceStore() {
  const paragraphs = ref<ChatParagraphSource[]>([])
  // 有值时知识来源显示文档预览,否则显示分段列表
  // Ensure PreviewDocument type allows { name, url }. If not, adapt here or fix the type definition.
  const previewDocument = ref<Pick<PreviewDocument, 'name' | 'url'> | null>(null)

  // 打开新的知识来源时回到分段列表
  const setParagraphs = (sourceList: ChatParagraphSource[]) => {
    paragraphs.value = sourceList
    previewDocument.value = null
  }

  // 返回分段列表
  const closePreview = () => {
    previewDocument.value = null
  }

  const sourceParagraphs = computed(() =>
    [...paragraphs.value]
      .sort((left, right) => {
        const simRight = right.similarity ?? 0
        const simLeft = left.similarity ?? 0
        if (simRight !== simLeft) return simRight - simLeft
        // Secondary sort by id or index for stability if needed, currently relies on stable sort of engine
        return 0 
      })
      .map(toSourceParagraph),
  )
  
  const count = computed(() => sourceParagraphs.value.length)

  // 按来源文档的打开方式处理点击;无来源地址时按下载权限给出提示
  const openDocument = (paragraph: KnowledgeSourceParagraph) => {
    const { documentAction, documentName, documentUrl } = paragraph

    if (documentAction === SOURCE_DOCUMENT_ACTION.PREVIEW) {
      // Cast needed if PreviewDocument is broader than {name, url} and strict typing enforces it
      previewDocument.value = { name: documentName, url: documentUrl } as any // Note: Ideally fix the type in document-preview/index instead
    } else if (documentAction === SOURCE_DOCUMENT_ACTION.DOWNLOAD) {
      downloadByURL(documentUrl, documentName)
    } else if (documentAction === SOURCE_DOCUMENT_ACTION.LINK) {
      window.open(documentUrl, '_blank', 'noopener,noreferrer')
    } else {
      MsgInfo(paragraph.allowDownload ? '原文档不存在' : '没有下载权限')
    }
  }

  return {
    paragraphs,
    previewDocument,
    sourceParagraphs,
    count,
    setParagraphs,
    openDocument,
    closePreview,
  }
}

export type KnowledgeSourceStore = ReturnType<typeof createKnowledgeSourceStore>

margin-bottom: 0;
}
}
</style>

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 a review of the code:

Potential Issues

1. watch inside onMounted isn't cleaned up

onMounted(() => {
  watch(...)  // watcher created here is auto-disposed on unmount (OK in effect scope)
})

This actually works because watchers are bound to the component instance, but placing watch at setup top-level is clearer and more idiomatic. Move it outside onMounted.

2. Watcher doesn't guard against empty/invalid URL transitions or race conditions

If previewDocument.value?.url changes rapidly, render() calls may race. The createPdfRenderer needs to handle cancellation of prior renders (verify it does). Also consider watching before/after state more explicitly.

3. No handling when url becomes empty (undefined)

if (url && pagesRef.value && viewerRef.value) void render(...)

When previewing different documents, pagesRef/viewerRef should be fine, but if the URL is cleared, no cleanup occurs — old canvases may remain. Consider calling destroy() when url is falsy.

4. Template refs timing in onMounted with immediate: true

With immediate: true, the watcher fires synchronously during onMounted. Refs should be populated by then, but coupling the immediate render with the same mount hook that binds fullscreen is fragile. Consider using watch(..., { flush: 'post', immediate: true }) to guarantee DOM is ready.

5. void destroy() / void render(...)

Swallowing promises silently. Errors from render/destroy won't be surfaced. At minimum add .catch(console.error) or let them propagate; ensure the renderer handles internal errors and sets error state.

6. Accessibility

  • Icon-only buttons (fullscreen.toggle(), closeRightSide()) lack aria-label. el-button may not infer meaning from icon alone.
  • The back button and close button need accessible names.

7. fullscreen.bind / unbind guarded inconsistently

bind checks viewerRef.value, but unbind is unconditional in onBeforeUnmount. If bind never ran (ref null), unbind may error or no-op depending on implementation — verify createFullscreenToggle().unbind() is safe without bind.

8. Hardcoded UI strings

'PDF 加载中...', '退出全屏', '全屏' aren't internationalized. If the project uses i18n, these should use $t().

Optimization Suggestions

9. Use flush: 'post' and lift watch out of onMounted

watch(
  () => previewDocument.value?.url,
  async (url) => {
    if (!url || !pagesRef.value || !viewerRef.value) return
    await render(pagesRef.value, viewerRef.value, url)
  },
  { immediate: true, flush: 'post' },
)

10. Avoid redundant re-render for the same URL

Not strictly needed if renderer is cheap, but if re-mount/mounted twice, an extra render could occur.

11. scrollbar-gutter: stable is good, but note it reserves horizontal space even when there's no scrollbar — verified intentional per comment. Fine.

12. Canvas max-width + margin

Fine, but consider width: 100% only when necessary; large PDFs scaled to max-width: 100% may lose crispness — confirm the renderer accounts for device pixel ratio (DPR) when rendering to canvas, otherwise high-DPI screens will look blurry.

13. Error retry

el-empty shows error text but offers no retry action. Consider adding a "重新加载" button calling render again.

14. min-w-0 truncate on h4 with :title — good, but also consider title on a wrapper span if truncation misreports.

15. Overlay positioning

v-if="loading" overlay uses absolute inset-0 inside a scroll container — since parent has overflow-auto, position: absolute is relative to nearest positioned ancestor (relative on viewer). That's correct, but as content scrolls, the overlay stays fixed relative to the container — acceptable behavior for loading; confirmed fine.

16. Minor: dependency array order in onBeforeUnmount

Unbinding fullscreen before destroying PDF is fine. No issue.

Summary priority

  • High: Watch cleanup placement + flush: 'post', handle empty URL, catch promise rejections.
  • Medium: Accessibility labels, i18n strings, DPR handling in renderer, retry affordance.
  • Low: Style micro-optimizations.

@shaohuzhang1
shaohuzhang1 merged commit 850e4e5 into v3 Nov 11, 2026
3 of 4 checks passed
@shaohuzhang1
shaohuzhang1 deleted the pr@v3@feat_knowledge_source branch November 11, 2026 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant