Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughA comprehensive Attachments component for Vue 3 was introduced, including all source code, composables, type definitions, LESS styles, and detailed documentation. Multiple interactive demos and sidebar navigation were added to the documentation. The component supports drag-and-drop uploads, file previews, custom actions, status messages, integration with other UI elements, and upload retry capabilities. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Attachments
participant FileCard
participant FullscreenOverlay
participant ImagePreview
participant ParentComponent
User->>Attachments: Drag-and-drop or select files
Attachments->>Attachments: Add files, generate metadata
Attachments->>FileCard: Render each file as card
FileCard-->>User: Show file info, preview, status, actions
User->>FileCard: Click preview/download/retry/remove
FileCard->>Attachments: Emit preview/download/retry/remove event
Attachments->>ParentComponent: Emit event (file-preview, file-remove, etc.)
User->>Attachments: Triggers preview for image
Attachments->>ImagePreview: Show modal with image, zoom/download controls
User->>Attachments: Drops files in fullscreen mode
Attachments->>FullscreenOverlay: Show overlay during drag
Poem
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
SonyLeo
force-pushed
the
component/attachments
branch
from
bdac60f to
180884c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (21)
packages/components/src/index.ts (1)
4-5: Tree-shakability hintImporting the plugin default (
Attachments) rather than the raw SFC means the file carries its owninstalland potentially side-effects.
If you only need the component object, consider importing from./attachments/index.vueto keep the bundle tree-shakable:-import Attachments from './attachments' +import Attachments from './attachments/index.vue'Not blocking, but worth noting for library size optimisation.
packages/components/src/attachments/index.ts (2)
4-8: Avoid mutating the component’snameat runtimeRe-assigning
Attachments.nameafter import mutates the SFC instance, which is immutable in Vue <4.0 edge cases and elides treeshaking.Prefer defining the name inside the
.vuefile:<script lang="ts"> export default defineComponent({ name: 'TrAttachments', ... }) </script>Then you can drop line 4 entirely.
6-8:installshould returnappfor chainingMinor DX tweak: returning the passed-in
appallows chained installs.-const install = function <T>(app: App<T>) { - app.component(Attachments.name!, Attachments) -} +const install = function <T>(app: App<T>) { + app.component(Attachments.name!, Attachments) + return app +}packages/components/src/sender/index.less (1)
130-159: Scrollbar styling – add non-WebKit fallback
::-webkit-scrollbarrules are ignored by Firefox/Edge (non-Blink). You already setscrollbar-width, but the thumb colour fallback is missing when not hovered.+@supports (scrollbar-color: auto) { + & { + scrollbar-width: thin; + scrollbar-color: rgba(0,0,0,.12) transparent; + } +}This keeps visual parity across browsers.
packages/components/src/attachments/composables/useIconType.ts (1)
13-16:DefaultIconOptionsis declared but never usedThe interface isn’t referenced anywhere in this composable or the surrounding codebase.
Drop it (or wire it up) to avoid dead code and unnecessary exports.-export interface DefaultIconOptions { - size?: number - color?: string -}docs/demos/attachments/basic.vue (2)
10-17: Prefer templaterefover manualgetElementByIdYou already hold a Vue ref for the container. Use
ref="containerRef"on the component and drop the manual DOM query; this avoids an extra render tick and keeps the code idiomatic.-<tr-attachments - id="containerRef" +<tr-attachments + ref="containerRef" v-model:items="files" :drag="{ mode: dragMode, - target: dragMode === 'container' ? containerRef : undefined, + target: dragMode === 'container' ? containerRef : undefined, }"-const containerRef = ref<HTMLElement | null>(null) - -onMounted(() => { - containerRef.value = document.getElementById('containerRef') -}) +const containerRef = ref<HTMLElement | null>(null)
31-33: Type mismatch between declared ref and actual element
containerRefpoints to a Vue component instance, not anHTMLElement. Change the type toComponentPublicInstance | null(or simply drop the generic) to avoid TS noise.docs/demos/attachments/file-card-list.vue (1)
40-46: InconsistentsizeunitsMost sizes are specified in raw bytes (e.g.,
200), but this entry multiplies by1024, suggesting kilobytes. Standardise the unit (prefer bytes) so the demo doesn’t display misleading values.- size: 200 * 1024, + size: 200,docs/demos/attachments/file-image-preview.vue (1)
122-157: Clear the retry timer on component unmount
setTimeoutmay still mutate state after the component is destroyed, leading to warnings or leaks.
Store the timer ID and clear it inonBeforeUnmount.- setTimeout(() => { + const t = setTimeout(() => { ... }, 1500) + + // 清理定时器 + onBeforeUnmount(() => clearTimeout(t))packages/components/src/attachments/components/ImagePreview.vue (2)
35-40: Avoid repeatedquerySelector& direct DOM mutation
updateZoom()queries the DOM on every zoom action and mutates inline styles, which:
- Couples logic to markup/class names.
- Incurs extra DOM look-ups.
- Bypasses Vue’s reactive system.
Prefer a template ref:
-const zoomLevel = ref(1) -const zoomStep = 0.1 +const zoomLevel = ref(1) +const zoomStep = 0.1 +const imgRef = ref<HTMLImageElement>() // 更新图片缩放 function updateZoom() { - const img = document.querySelector('.tr-image-preview__image') as HTMLElement - if (img) { - img.style.transform = `scale(${zoomLevel.value})` - } + if (imgRef.value) { + imgRef.value.style.transform = `scale(${zoomLevel.value})` + } }and in template:
-<img :src="imageUrl" alt="预览图片" class="tr-image-preview__image" /> +<img + ref="imgRef" + :src="imageUrl" + alt="预览图片" + class="tr-image-preview__image" />
42-46: Reset zoom when dialog closesClosing the preview does not reset
zoomLevel; reopening a previously zoomed image opens at the last scale.
After emittingclose, reset the internal state:function close() { emit('close') + resetZoom() }packages/components/src/attachments/components/FullscreenOverlay.vue (1)
62-63: Consider responsive design for overlay layout.The fixed margin (10px) and fixed width (253px) for the description may not work well on all screen sizes, especially mobile devices.
.tr-fullscreen-overlay { position: fixed; top: 0; left: 0; right: 0; bottom: 0; backdrop-filter: blur(16px); background: rgba(255, 255, 255, 0.72); display: flex; align-items: center; justify-content: center; - margin: 10px; + margin: min(10px, 2vw); border: 2px dashed #1976d2; border-radius: 8px; animation: pulse-border 1.5s infinite; &__description { - width: 253px; + max-width: 253px; + width: 100%; height: 40px;Also applies to: 101-102
packages/components/src/attachments/index.less (2)
38-38: Use English for code comments.For consistency and international collaboration, use English for all code comments.
- // 不悬停时隐藏滚动条 + // Hide scrollbar when not hovering
101-106: Apply scrollbar styles consistently.The vertical scroll mode should use the
.scrollbar-stylemixin like the horizontal scroll mode does.&--scrollY { max-height: var(--tr-attachments-list-max-height); overflow-y: auto; padding-right: var(--tr-attachments-list-padding-right); - .scrollbar-style; + .scrollbar-style();packages/components/src/attachments/index.type.ts (1)
43-43: Consider using a more specific type for fileIcons.The generic
Componenttype from Vue could be more specific to ensure type safety.-import { CSSProperties, Component } from 'vue' +import { CSSProperties, Component, DefineComponent } from 'vue' // In the interface: - fileIcons?: Record<FileType, Component> + fileIcons?: Record<FileType, DefineComponent>packages/components/src/attachments/components/FileCard.vue (1)
12-12: Specify the ESLint rule being disabled.Instead of disabling all ESLint rules, specify which rule needs to be disabled.
- // eslint-disable-next-line + // eslint-disable-next-line @typescript-eslint/no-explicit-any fileIcons?: Record<FileType, any>packages/components/src/attachments/index.vue (1)
141-141: Specify the ESLint rule being disabled.Be specific about which ESLint rule is being disabled.
-// eslint-disable-next-line +// eslint-disable-next-line @typescript-eslint/no-explicit-any function handleAction(payload: any) {docs/src/components/attachments.md (4)
11-11: Consistent Heading Levels
Avoid skipping heading levels. Change "#### 基本示例" to "### 基本示例" to follow from "## 代码示例".-#### 基本示例 +### 基本示例
24-24: Use a Proper Subheading Instead of Bold
Replace bold text with a markdown heading for better structure.-**文件状态消息类型** +#### 文件状态消息类型
26-31: Fix Table Column Count
Ensure the first table has exactly five columns by removing the extra trailing pipe in each row.@@ -26,6 +26,6 @@ -| 状态类型 | 属性配置 | 说明 | 相关属性 | 交互/备注 | -|-------------------|---------------------|-----------------------------------|--------------------------|--------------------------------------------------------------------------| -| 信息状态 | `statusType="info"` | 默认状态，显示文件类型和大小 | - | 无特殊交互 | | -| 状态消息 | `statusType="message"` | 显示预设状态消息文本 | `file.messageType`<br>`file.status` | 需配合 `file.messageType` 定义消息类型，`file.status` 可扩展交互功能 | -| 自定义操作 | `statusType="operate"` | 显示自定义操作按钮 | `customActions` (必填) | 需通过数组提供按钮配置 | -| 默认状态 | 不设置或默认配置 | 仅显示 `file.status` 的文本内容 | `file.status` | 纯文本展示无交互 | +| 状态类型 | 属性配置 | 说明 | 相关属性 | 交互/备注 | +|-------------------|---------------------|-----------------------------------|--------------------------|----------| +| 信息状态 | `statusType="info"` | 默认状态，显示文件类型和大小 | - | 无特殊交互 | +| 状态消息 | `statusType="message"` | 显示预设状态消息文本 | `file.messageType`<br>`file.status` | 需配合 `file.messageType` 定义消息类型，`file.status` 可扩展交互功能 | +| 自定义操作 | `statusType="operate"` | 显示自定义操作按钮 | `customActions` (必填) | 需通过数组提供按钮配置 | +| 默认状态 | 不设置或默认配置 | 仅显示 `file.status` 的文本内容 | `file.status` | 纯文本展示无交互 |
33-40: Surround Tables with Blank Lines & Fix Columns
Add blank lines before and after the second table for readability and remove trailing pipes in each row. Convert the inline bold to a proper heading if needed.@@ -32,4 +32,7 @@ -**状态消息类型 (file.messageType 可选值):** -| 消息类型 | 触发条件 | 典型场景 | 特殊交互 | -|-------------------|---------------------------------------|---------------------|----------------------------------| -| error | `file.messageType="error"`<br>且 `file.status="error"` | 上传失败 | 自动显示重试按钮 | -| warning | `file.messageType="warning"` | 文件校验警告 | 仅显示警示图标和文本 | -| success | `file.messageType="success"` | 传输成功 | | -| info | `file.messageType="info"` | 常规提示信息 | 基础文本展示 | -| uploading | `file.messageType="uploading"` | 上传中 | 可配合进度条使用 | +### 状态消息类型 (file.messageType 可选值) + +| 消息类型 | 触发条件 | 典型场景 | 特殊交互 | +|-------------------|---------------------------------------|---------------------|----------| +| error | `file.messageType="error"`<br>且 `file.status="error"` | 上传失败 | 自动显示重试按钮 | +| warning | `file.messageType="warning"` | 文件校验警告 | 仅显示警示图标和文本 | +| success | `file.messageType="success"` | 传输成功 | 无 | +| info | `file.messageType="info"` | 常规提示信息 | 基础文本展示 | +| uploading | `file.messageType="uploading"` | 上传中 | 可配合进度条使用 |
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (10)
packages/svgs/src/assets/file-excel.svgis excluded by!**/*.svgpackages/svgs/src/assets/file-folder.svgis excluded by!**/*.svgpackages/svgs/src/assets/file-image.svgis excluded by!**/*.svgpackages/svgs/src/assets/file-none.svgis excluded by!**/*.svgpackages/svgs/src/assets/file-other.svgis excluded by!**/*.svgpackages/svgs/src/assets/file-pdf.svgis excluded by!**/*.svgpackages/svgs/src/assets/file-ppt.svgis excluded by!**/*.svgpackages/svgs/src/assets/file-word.svgis excluded by!**/*.svgpackages/svgs/src/assets/upload-failed.svgis excluded by!**/*.svgpackages/svgs/src/assets/upload-loading.svgis excluded by!**/*.svg
📒 Files selected for processing (21)
docs/.vitepress/config.mts(1 hunks)docs/demos/attachments/basic.vue(1 hunks)docs/demos/attachments/file-card-list.vue(1 hunks)docs/demos/attachments/file-card-status.vue(1 hunks)docs/demos/attachments/file-image-preview.vue(1 hunks)docs/demos/attachments/integration.vue(1 hunks)docs/src/components/attachments.md(1 hunks)packages/components/src/attachments/components/FileCard.vue(1 hunks)packages/components/src/attachments/components/FullscreenOverlay.vue(1 hunks)packages/components/src/attachments/components/ImagePreview.vue(1 hunks)packages/components/src/attachments/composables/useDragDrop.ts(1 hunks)packages/components/src/attachments/composables/useFileType.ts(1 hunks)packages/components/src/attachments/composables/useIconType.ts(1 hunks)packages/components/src/attachments/index.less(1 hunks)packages/components/src/attachments/index.ts(1 hunks)packages/components/src/attachments/index.type.ts(1 hunks)packages/components/src/attachments/index.vue(1 hunks)packages/components/src/attachments/vars.less(1 hunks)packages/components/src/index.ts(3 hunks)packages/components/src/sender/index.less(3 hunks)packages/components/src/sender/vars.less(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
packages/components/src/attachments/index.ts (1)
packages/components/src/index.ts (3)
Attachments(66-66)Attachments(67-67)install(57-62)
packages/components/src/attachments/composables/useIconType.ts (1)
packages/components/src/attachments/index.type.ts (1)
FileType(3-3)
packages/components/src/attachments/composables/useFileType.ts (1)
packages/components/src/attachments/index.type.ts (1)
FileType(3-3)
packages/components/src/attachments/composables/useDragDrop.ts (1)
packages/components/src/attachments/index.type.ts (2)
AttachmentsProps(33-59)DragConfig(22-31)
🪛 markdownlint-cli2 (0.17.2)
docs/src/components/attachments.md
11-11: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
24-24: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
28-28: Table column count
Expected: 5; Actual: 6; Too many cells, extra data will be missing
(MD056, table-column-count)
34-34: Tables should be surrounded by blank lines
null
(MD058, blanks-around-tables)
🔇 Additional comments (10)
packages/components/src/sender/vars.less (1)
108-112: Confirm downstream usage of the new 164 px header height
--tr-sender-header-max-heightwas raised from 120 px → 164 px.
Please verify that:
- Any hard-coded calculations (e.g. transitions, marquee animations) that rely on the old 120 px value are updated.
- The parent container still has enough vertical space on mobile breakpoints.
A quick
rg --fixed-strings "--tr-sender-header-max-height" -g '*.less'should reveal whether the old value is inlined elsewhere.docs/.vitepress/config.mts (1)
69-70: Ensure the target markdown file exists & matches the routeThe sidebar now links to
'attachments', sodocs/src/components/attachments.md(or/components/attachments/index.md) must exist, otherwise VitePress will 404.#!/bin/bash fd attachments.md docs/srcRun the above to double-check the path.
packages/components/src/index.ts (3)
33-34: Type export path check
export * from './attachments/index.type'presumes that file exists and compiles under TS strict mode.
Please runtsc -p packages/components/tsconfig.json --noEmitto be sure no circular refs are introduced.
36-38: Component double registration is avoided – goodIncluding
Attachmentsin thecomponentsarray is safe because its owninstallis not invoked here; only the object is used. Nice separation.
66-68: Alias export reads wellExporting both
AttachmentsandTrAttachmentskeeps API consistency with the rest of the library.packages/components/src/sender/index.less (1)
115-116:position: relativeaddition is correctThis prevents the absolutely-positioned
.tiny-sender__actions-slotfrom escaping the row.packages/components/src/attachments/composables/useIconType.ts (1)
33-42: Provide a deterministic fallback for unknown/unsupported file types
fileTypehas a default of'other', but callers may still pass an out-of-range string via a type cast or plain JS.
Returningundefinedhere will break the rendering chain.- return computed(() => { - // 优先使用自定义图标 - if (customIcons?.[fileType]) { - return customIcons[fileType] - } - - // 否则使用默认图标 - return DefaultIcons?.[fileType] - }) + return computed(() => { + // 优先使用自定义图标 + if (customIcons?.[fileType]) { + return customIcons[fileType] + } + + // 否则使用默认图标，回退到 'other' + return DefaultIcons[fileType] ?? DefaultIcons.other + })packages/components/src/attachments/vars.less (1)
3-109: Centralised tokens look goodThe variable names are consistently prefixed (
--tr-attachments-*) and grouped logically.
No issues spotted. 👍packages/components/src/attachments/composables/useFileType.ts (1)
86-91: Revoke object URLs to prevent memory leaks
createPreviewUrlcreates a blob URL but the lifecycle management is external.
Provide a helper to revoke when the attachment is removed, or at least document this requirement so consumers don’t leak memory.packages/components/src/attachments/index.vue (1)
254-254: Component name consistency in template.Ensure the component name matches the import after fixing the import statement.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/demos/attachments/integration.vue (1)
52-61: Nested ternary hurts readabilityThe four-level ternary for status text is hard to scan and maintain.
Consider extracting a small helper:const statusLabel = (f: Attachment) => f.messageType === 'success' ? '✓ 上传成功' : f.messageType === 'error' ? '✗ 上传失败' : f.messageType === 'uploading' ? '⏳ 上传中' : f.status || '未知状态'and call
statusLabel(file)in the template.
Same behavior, clearer intent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/demos/attachments/file-card-status.vue(1 hunks)docs/demos/attachments/integration.vue(1 hunks)packages/components/src/attachments/components/FileCard.vue(1 hunks)packages/components/src/attachments/components/FullscreenOverlay.vue(1 hunks)packages/components/src/attachments/composables/useDragDrop.ts(1 hunks)packages/components/src/attachments/index.type.ts(1 hunks)packages/components/src/attachments/index.vue(1 hunks)packages/components/src/sender/index.less(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/components/src/attachments/components/FileCard.vue
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/components/src/sender/index.less
- docs/demos/attachments/file-card-status.vue
- packages/components/src/attachments/components/FullscreenOverlay.vue
- packages/components/src/attachments/composables/useDragDrop.ts
- packages/components/src/attachments/index.type.ts
- packages/components/src/attachments/index.vue
🔇 Additional comments (1)
docs/demos/attachments/integration.vue (1)
49-52: Incorrect ref access causes runtime error
singleAttachmentsRefis a Vueref, therefore the actual component instance is held on the.valueproperty.
UsingsingleAttachmentsRef?.formatFileSize(...)will compile but crash at runtime becauseformatFileSizeisundefined.-<span class="file-size" v-if="file.size">{{ singleAttachmentsRef?.formatFileSize(file.size) }}</span> +<span class="file-size" v-if="file.size">{{ singleAttachmentsRef.value?.formatFileSize(file.size) }}</span>Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
packages/components/src/attachments/components/FileCard.vue (1)
126-126: Internationalize hardcoded text.The component still contains hardcoded Chinese text that should be externalized for i18n support.
Also applies to: 222-233
packages/components/src/attachments/index.vue (2)
10-10: Fix component import name case mismatch.The import name doesn't match the actual file name, which will cause a runtime error.
-import FullScreenOverlay from './components/FullscreenOverlay.vue' +import FullscreenOverlay from './components/FullscreenOverlay.vue'Also update the usage in the template at line 291:
- <FullScreenOverlay + <FullscreenOverlay
250-251: Internationalize empty state text.The empty state contains hard-coded Chinese text that should support internationalization.
+const props = withDefaults(defineProps<AttachmentsProps & { + emptyStateText?: { + title?: string + hint?: string + } +}>(), { + // ... existing defaults + emptyStateText: () => ({ + title: '暂无文件', + hint: '将文件拖拽到此处，或点击上传' + }) +}) - <div class="tr-attachments__empty-text">暂无文件</div> - <div class="tr-attachments__empty-hint">将文件拖拽到此处，或点击上传</div> + <div class="tr-attachments__empty-text">{{ emptyStateText.title }}</div> + <div class="tr-attachments__empty-hint">{{ emptyStateText.hint }}</div>Also applies to: 285-286
🧹 Nitpick comments (6)
packages/components/src/attachments/composables/useImagePreview.ts (1)
4-7: Add input validation for defensive programming.The function should validate inputs to prevent runtime errors.
export function useImagePreview(fileList: Ref<Attachment[]>, emit: AttachmentsEmits) { + if (!fileList || !emit) { + throw new Error('useImagePreview requires fileList and emit parameters') + } + const isPreviewVisible = ref(false) const previewImages = ref<Attachment[]>([]) const previewCurrentIndex = ref(0)packages/components/src/attachments/composables/useUploadIntegration.ts (1)
82-82: Optimize reactive array updates.Multiple places use array spread to trigger reactivity, which creates unnecessary array copies.
Consider using a single reactive update pattern or Vue 3's
triggerReffor manual reactivity triggering when performance is critical. For now, the current approach is acceptable but could be optimized in the future if performance becomes an issue.Also applies to: 112-112, 139-139, 244-244
packages/components/src/attachments/components/ImagePreview.vue (2)
70-70: Icon usage appears to be swapped.The left navigation uses
IconArrowRightand right navigation usesIconArrowLeft, which is counterintuitive even with the CSS transform.Consider using the icons in their semantically correct positions:
- <IconArrowRight /> + <IconArrowLeft />and
- <IconArrowLeft /> + <IconArrowRight />Then adjust the CSS transforms accordingly.
Also applies to: 82-82
74-74: Add error handling for missing image URLs.The image source directly uses
currentImage?.previewUrlwithout fallback handling.- <img :src="currentImage?.previewUrl" :alt="currentImage?.name" class="tr-image-preview__image" /> + <img + :src="currentImage?.previewUrl || ''" + :alt="currentImage?.name || 'Image preview'" + class="tr-image-preview__image" + @error="handleImageError" + />Add an error handler to show a placeholder or emit an error event.
packages/components/src/attachments/components/FileCard.vue (2)
86-99: Simplify custom action handler logic.The conditional logic in
handleCustomActionis complex and could be clearer.const handleCustomAction = (action: ActionButton) => { if (action.handler) { action.handler(props.file) } - // 对预览和下载操作进行特殊处理 - if (action.type === 'preview' && isImage.value) { - handlePreview() - } else if (action.type === 'download' && isImage.value) { - downloadFile() - } else { - emit('action', { action, file: props.file }) - } + // Special handling for image preview and download + const isImageAction = isImage.value && ['preview', 'download'].includes(action.type) + + if (isImageAction) { + if (action.type === 'preview') handlePreview() + if (action.type === 'download') downloadFile() + } else { + emit('action', { action, file: props.file }) + } }
164-164: Remove unnecessary null in click handler.The ternary operator returning
nullis unnecessary in event handlers.- @click.stop="isImage && showPreview ? handlePreview() : null" + @click.stop="isImage && showPreview && handlePreview()"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/svgs/src/assets/close.svgis excluded by!**/*.svg
📒 Files selected for processing (14)
docs/demos/attachments/file-card-status.vue(1 hunks)docs/demos/attachments/file-image-preview.vue(1 hunks)packages/components/package.json(1 hunks)packages/components/src/attachments/components/AttachmentList.vue(1 hunks)packages/components/src/attachments/components/FileCard.vue(1 hunks)packages/components/src/attachments/components/FullscreenOverlay.vue(1 hunks)packages/components/src/attachments/components/ImagePreview.vue(1 hunks)packages/components/src/attachments/composables/useDragDrop.ts(1 hunks)packages/components/src/attachments/composables/useImagePreview.ts(1 hunks)packages/components/src/attachments/composables/useUploadIntegration.ts(1 hunks)packages/components/src/attachments/index.less(1 hunks)packages/components/src/attachments/index.type.ts(1 hunks)packages/components/src/attachments/index.vue(1 hunks)packages/components/src/attachments/vars.less(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/components/package.json
🚧 Files skipped from review as they are similar to previous changes (6)
- docs/demos/attachments/file-image-preview.vue
- docs/demos/attachments/file-card-status.vue
- packages/components/src/attachments/index.less
- packages/components/src/attachments/vars.less
- packages/components/src/attachments/components/FullscreenOverlay.vue
- packages/components/src/attachments/composables/useDragDrop.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/components/src/attachments/composables/useImagePreview.ts (1)
packages/components/src/attachments/index.type.ts (2)
Attachment(25-32)AttachmentsEmits(116-124)
🔇 Additional comments (3)
packages/components/src/attachments/composables/useImagePreview.ts (1)
11-14: Avoid direct mutation of reactive arrays.Using
spliceon the reactive array might not trigger reactivity properly in Vue 3. Consider creating a new array instead.- const fileIndex = fileList.value.findIndex((item) => item.uid === file.uid) - if (fileIndex !== -1 && file.previewUrl) { - fileList.value.splice(fileIndex, 1, file) - } + if (file.previewUrl) { + fileList.value = fileList.value.map((item) => + item.uid === file.uid ? file : item + ) + }Likely an incorrect or invalid review comment.
packages/components/src/attachments/components/AttachmentList.vue (1)
12-37: Clean implementation of the attachment list component.The component properly handles rendering file cards with appropriate event delegation and prop passing. The conditional rendering and class binding based on overflow type is well implemented.
packages/components/src/attachments/index.type.ts (1)
1-158: Well-structured type definitions!The type definitions are comprehensive, properly organized, and follow TypeScript best practices. Good use of utility types like
Pickand proper extension of interfaces.
| if (!uploadConfig.value) { | ||
| // 如果没有配置上传，只是添加到文件列表 | ||
| const attachments = files.map((file) => ({ | ||
| uid: `${Date.now()}-${Math.random()}`, |
There was a problem hiding this comment.
Improve UID generation for better uniqueness.
The current UID generation using Date.now() and Math.random() might produce collisions when processing multiple files quickly.
Consider using a more robust ID generation method:
- uid: `${Date.now()}-${Math.random()}`,
+ uid: `${Date.now()}-${Math.random().toString(36).substr(2, 9)}-${file.name}`,Or better yet, use a proper UUID library:
import { v4 as uuidv4 } from 'uuid'
// ...
uid: uuidv4(),Also applies to: 69-69
🤖 Prompt for AI Agents
In packages/components/src/attachments/composables/useUploadIntegration.ts at
lines 38 and 69, the UID generation using Date.now() and Math.random() is prone
to collisions. Replace this with a more robust method by importing a UUID
library such as 'uuid' and use uuidv4() to generate unique IDs for the uid
fields to ensure better uniqueness and avoid collisions.
| reject(new Error('Upload failed')) | ||
| } | ||
|
|
||
| xhr.open('POST', props.action!) |
There was a problem hiding this comment.
Validate action prop before using non-null assertion.
Using props.action! assumes the prop exists, but this should be validated.
+ if (!props.action) {
+ throw new Error('Action URL is required for default upload')
+ }
xhr.open('POST', props.action!)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| xhr.open('POST', props.action!) | |
| if (!props.action) { | |
| throw new Error('Action URL is required for default upload') | |
| } | |
| xhr.open('POST', props.action!) |
🤖 Prompt for AI Agents
In packages/components/src/attachments/composables/useUploadIntegration.ts at
line 215, the code uses a non-null assertion on props.action without validating
its existence. To fix this, add a check to ensure props.action is defined before
calling xhr.open. If props.action is undefined, handle the error appropriately
or prevent the function from proceeding.
| if ( | ||
| response && | ||
| typeof response === 'object' && | ||
| response !== null && | ||
| 'url' in response && | ||
| typeof response.url === 'string' | ||
| ) { | ||
| internalFileList.value[fileIndex].previewUrl = response.url | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Strengthen type checking for upload response.
The current type checking uses a simple 'url' in response check which could be unsafe.
- if (
- response &&
- typeof response === 'object' &&
- response !== null &&
- 'url' in response &&
- typeof response.url === 'string'
- ) {
- internalFileList.value[fileIndex].previewUrl = response.url
- }
+ if (response && typeof response === 'object' && response !== null) {
+ const typedResponse = response as Record<string, unknown>
+ if (typeof typedResponse.url === 'string' && typedResponse.url) {
+ internalFileList.value[fileIndex].previewUrl = typedResponse.url
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ( | |
| response && | |
| typeof response === 'object' && | |
| response !== null && | |
| 'url' in response && | |
| typeof response.url === 'string' | |
| ) { | |
| internalFileList.value[fileIndex].previewUrl = response.url | |
| } | |
| if (response && typeof response === 'object' && response !== null) { | |
| const typedResponse = response as Record<string, unknown> | |
| if (typeof typedResponse.url === 'string' && typedResponse.url) { | |
| internalFileList.value[fileIndex].previewUrl = typedResponse.url | |
| } | |
| } |
🤖 Prompt for AI Agents
In packages/components/src/attachments/composables/useUploadIntegration.ts
between lines 128 and 136, the type checking for the upload response is weak
because it only checks if 'url' exists in the response object. Improve this by
using a more robust type guard or validation method to ensure the response is of
the expected type with a valid string 'url' property before assigning it to
internalFileList.value[fileIndex].previewUrl.
| <template> | ||
| <div class="tr-image-preview" v-if="visible" @click.self="close"> | ||
| <button class="tr-image-preview__close" @click="close"><IconClose /></button> | ||
|
|
||
| <div class="tr-image-preview__main"> | ||
| <button | ||
| class="tr-image-preview__nav tr-image-preview__nav--left" | ||
| @click.stop="prevImage" | ||
| :disabled="localCurrentIndex === 0" | ||
| > | ||
| <IconArrowRight /> | ||
| </button> | ||
|
|
||
| <div class="tr-image-preview__content"> | ||
| <img :src="currentImage?.previewUrl" :alt="currentImage?.name" class="tr-image-preview__image" /> | ||
| </div> | ||
|
|
||
| <button | ||
| class="tr-image-preview__nav tr-image-preview__nav--right" | ||
| @click.stop="nextImage" | ||
| :disabled="localCurrentIndex === images.length - 1" | ||
| > | ||
| <IconArrowLeft /> | ||
| </button> | ||
| </div> | ||
|
|
||
| <div class="tr-image-preview__footer"> | ||
| <div class="tr-image-preview__thumbnails"> | ||
| <div | ||
| v-for="(image, index) in images" | ||
| :key="image.uid" | ||
| class="tr-image-preview__thumbnail" | ||
| :class="{ 'tr-image-preview__thumbnail--active': index === localCurrentIndex }" | ||
| @click="selectImage(index)" | ||
| > | ||
| <img :src="image.previewUrl" :alt="image.name" /> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </template> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add keyboard navigation and accessibility features.
The image preview lacks keyboard navigation (arrow keys, escape) and proper ARIA labels.
Add keyboard event handling in the script:
const handleKeydown = (event: KeyboardEvent) => {
if (!props.visible) return
switch (event.key) {
case 'Escape':
close()
break
case 'ArrowLeft':
prevImage()
break
case 'ArrowRight':
nextImage()
break
}
}
onMounted(() => {
window.addEventListener('keydown', handleKeydown)
})
onUnmounted(() => {
window.removeEventListener('keydown', handleKeydown)
})And add ARIA labels to the template:
- <button class="tr-image-preview__close" @click="close"><IconClose /></button>
+ <button class="tr-image-preview__close" @click="close" aria-label="Close preview"><IconClose /></button>🤖 Prompt for AI Agents
In packages/components/src/attachments/components/ImagePreview.vue between lines
60 and 100, add keyboard event handling in the script section to support Escape
for closing, ArrowLeft for previous image, and ArrowRight for next image by
defining a handleKeydown function and registering it on window keydown events on
mount and removing it on unmount. Also, enhance accessibility by adding
appropriate ARIA labels and roles to the template elements, such as labeling the
close button and navigation buttons, and ensuring the image container has a
meaningful role for screen readers.
* fix: submit shortcut triggers newline * fix: no valid content in the input box will not be sent. * fix: remove abundant template prop * fix: review suggestion * fix: review suggestion
…e Assistant demo to use SuggestionPopover and SuggestionPills (opentiny#141)
SonyLeo
force-pushed
the
component/attachments
branch
from
7603146 to
dd9b961
Compare
3 participants
【注】 #118 为前置提交，为单行模式下的文件上传提供入口触发函数
附件组件
Summary by CodeRabbit
New Features
Style
Documentation