feat: add image preview #444
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds an image preview lightbox to the custom-image module: a singleton ImagePreviewModal (wheel zoom, close via click/Escape, scale tooltip), re-exports, toolbar preview button, double-click handler, CSS, and a small demo template change to include an image element in the editor container. Changes
Sequence DiagramsequenceDiagram
actor User
participant Editor as Editor Element
participant Spec as CustomImageSpec
participant ModalFactory as getImagePreviewModal()
participant Modal as ImagePreviewModal
participant DOM as Browser DOM/CSS
User->>Editor: double-click image
Editor->>Spec: dblclick event
Spec->>Spec: extract image src
Spec->>ModalFactory: getImagePreviewModal()
ModalFactory-->>Spec: ImagePreviewModal (singleton)
Spec->>Modal: show(imageSrc)
Modal->>DOM: create overlay & modal nodes
Modal->>DOM: render image, apply animations
User->>Modal: wheel / click / press Escape
Modal->>Modal: update scale or hide()
Modal->>DOM: update transform / remove nodes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@packages/fluent-editor/src/modules/custom-image/preview/preview-modal.ts`:
- Around line 266-277: The singleton globalPreviewModal can point to a destroyed
ImagePreviewModal, so update the code to clear that reference when the instance
is torn down: either set globalPreviewModal = null inside
ImagePreviewModal.destroy() (ensure destroy() is the method name used) or add
and export a resetImagePreviewModal() function that sets globalPreviewModal to
null and call that from destroy; keep getImagePreviewModal() as-is so it will
create a new ImagePreviewModal when globalPreviewModal is null.
- Around line 113-120: The document-level keydown and wheel listeners added in
the constructor/initialization (the anonymous keydown handler that calls
this.hide() and the wheel handler that calls this.onMouseWheel) are never
removed; store both listeners as bound instance methods or properties (e.g.,
this._onKeyDown and this._onWheel using .bind(this) or arrow wrappers) when
attaching them and then remove them in destroy() via
document.removeEventListener('keydown', this._onKeyDown) and
document.removeEventListener('wheel', this._onWheel) (matching options for the
wheel listener), ensuring destroy() cleans up these handlers to prevent leaks
and duplicate handlers on recreation.
In `@packages/fluent-editor/src/modules/custom-image/specs/custom-image-spec.ts`:
- Around line 137-145: The onImageDoubleClick handler currently calls
getAttribute on event.target (typed EventTarget) without ensuring it's an
Element and without verifying it's an image; update onImageDoubleClick to first
guard that event.target is an Element (e.g., using instanceof Element) and that
it is an image (check tagName/nodeName === 'IMG' or use matches('img') or
instanceof HTMLImageElement), then safely read src or data-image and, if
present, call getImagePreviewModal().show(imageSrc); also handle null/undefined
target to avoid runtime errors.
🧹 Nitpick comments (2)
packages/fluent-editor/src/modules/custom-image/preview/preview.css (1)
40-70: Consider consolidating identical keyframes.
fadeInandimageShowkeyframes are functionally identical (both animate opacity from 0 to 1). While keeping them separate provides semantic clarity, you could consolidate them if you want to reduce duplication.packages/fluent-editor/src/modules/custom-image/preview/preview-modal.ts (1)
134-148: Wheel event handler intercepts scrolling globally when modal is open.When the modal is visible,
event.preventDefault()blocks all wheel scrolling on the page. This is intentional for zoom, but consider limiting the scope to events over the modal/overlay elements rather than all wheel events.♻️ Suggested approach
Attach the wheel listener to the overlay or modal element instead of
document, so normal page scrolling outside the modal (if any visible area exists) isn't blocked:- document.addEventListener('wheel', (e) => this.onMouseWheel(e), { passive: false }) + this.overlay.addEventListener('wheel', (e) => this.onMouseWheel(e), { passive: false }) + this.modal.addEventListener('wheel', (e) => this.onMouseWheel(e), { passive: false })This also simplifies cleanup since the listeners are removed when DOM elements are removed.
| document.addEventListener('keydown', (e) => { | ||
| if (e.key === 'Escape') { | ||
| this.hide() | ||
| } | ||
| }) | ||
|
|
||
| // 绑定滚轮缩放事件 | ||
| document.addEventListener('wheel', (e) => this.onMouseWheel(e), { passive: false }) |
There was a problem hiding this comment.
Global event listeners are not cleaned up in destroy().
The keydown and wheel event listeners are added to document but never removed when destroy() is called. This causes memory leaks and ghost event handlers if the modal is destroyed and recreated.
🔧 Proposed fix - store handler references and remove in destroy()
export class ImagePreviewModal {
private modal: HTMLElement | null = null
private overlay: HTMLElement | null = null
private previewImage: HTMLImageElement | null = null
private scaleTooltip: HTMLElement | null = null
private currentScale: number = 1
private minScale: number = 0.5
private maxScale: number = 3
private scaleStep: number = 0.1
private tooltipHideTimer: number | null = null
+ private keydownHandler: ((e: KeyboardEvent) => void) | null = null
+ private wheelHandler: ((e: WheelEvent) => void) | null = null
// In initModal():
- document.addEventListener('keydown', (e) => {
- if (e.key === 'Escape') {
- this.hide()
- }
- })
- document.addEventListener('wheel', (e) => this.onMouseWheel(e), { passive: false })
+ this.keydownHandler = (e: KeyboardEvent) => {
+ if (e.key === 'Escape') {
+ this.hide()
+ }
+ }
+ this.wheelHandler = (e: WheelEvent) => this.onMouseWheel(e)
+ document.addEventListener('keydown', this.keydownHandler)
+ document.addEventListener('wheel', this.wheelHandler, { passive: false })
// In destroy():
+ if (this.keydownHandler) {
+ document.removeEventListener('keydown', this.keydownHandler)
+ }
+ if (this.wheelHandler) {
+ document.removeEventListener('wheel', this.wheelHandler)
+ }🤖 Prompt for AI Agents
In `@packages/fluent-editor/src/modules/custom-image/preview/preview-modal.ts`
around lines 113 - 120, The document-level keydown and wheel listeners added in
the constructor/initialization (the anonymous keydown handler that calls
this.hide() and the wheel handler that calls this.onMouseWheel) are never
removed; store both listeners as bound instance methods or properties (e.g.,
this._onKeyDown and this._onWheel using .bind(this) or arrow wrappers) when
attaching them and then remove them in destroy() via
document.removeEventListener('keydown', this._onKeyDown) and
document.removeEventListener('wheel', this._onWheel) (matching options for the
wheel listener), ensuring destroy() cleans up these handlers to prevent leaks
and duplicate handlers on recreation.
| // 全局单例实例 | ||
| let globalPreviewModal: ImagePreviewModal | null = null | ||
|
|
||
| /** | ||
| * 获取或创建全局预览模态框实例 | ||
| */ | ||
| export function getImagePreviewModal(): ImagePreviewModal { | ||
| if (!globalPreviewModal) { | ||
| globalPreviewModal = new ImagePreviewModal() | ||
| } | ||
| return globalPreviewModal | ||
| } |
There was a problem hiding this comment.
Singleton instance not cleared after destroy().
If destroy() is called on the singleton, globalPreviewModal still holds a reference to the destroyed (unusable) instance. Subsequent calls to getImagePreviewModal() will return the destroyed modal.
🔧 Proposed fix
Either expose a function to reset the singleton, or clear it in destroy():
+ export function destroyImagePreviewModal(): void {
+ if (globalPreviewModal) {
+ globalPreviewModal.destroy()
+ globalPreviewModal = null
+ }
+ }📝 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.
| // 全局单例实例 | |
| let globalPreviewModal: ImagePreviewModal | null = null | |
| /** | |
| * 获取或创建全局预览模态框实例 | |
| */ | |
| export function getImagePreviewModal(): ImagePreviewModal { | |
| if (!globalPreviewModal) { | |
| globalPreviewModal = new ImagePreviewModal() | |
| } | |
| return globalPreviewModal | |
| } | |
| // 全局单例实例 | |
| let globalPreviewModal: ImagePreviewModal | null = null | |
| /** | |
| * 获取或创建全局预览模态框实例 | |
| */ | |
| export function getImagePreviewModal(): ImagePreviewModal { | |
| if (!globalPreviewModal) { | |
| globalPreviewModal = new ImagePreviewModal() | |
| } | |
| return globalPreviewModal | |
| } | |
| export function destroyImagePreviewModal(): void { | |
| if (globalPreviewModal) { | |
| globalPreviewModal.destroy() | |
| globalPreviewModal = null | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@packages/fluent-editor/src/modules/custom-image/preview/preview-modal.ts`
around lines 266 - 277, The singleton globalPreviewModal can point to a
destroyed ImagePreviewModal, so update the code to clear that reference when the
instance is torn down: either set globalPreviewModal = null inside
ImagePreviewModal.destroy() (ensure destroy() is the method name used) or add
and export a resetImagePreviewModal() function that sets globalPreviewModal to
null and call that from destroy; keep getImagePreviewModal() as-is so it will
create a new ImagePreviewModal when globalPreviewModal is null.
| onImageDoubleClick = (event: MouseEvent) => { | ||
| const target = event.target | ||
| const imageSrc = target.getAttribute('src') || target.getAttribute('data-image') | ||
|
|
||
| if (imageSrc) { | ||
| const modal = getImagePreviewModal() | ||
| modal.show(imageSrc) | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing type guard for event target.
The handler attempts to call getAttribute() on event.target which is typed as EventTarget, but getAttribute only exists on Element. Additionally, there's no check to ensure the double-clicked element is actually an image.
🔧 Proposed fix
onImageDoubleClick = (event: MouseEvent) => {
const target = event.target
+ if (!(target instanceof HTMLImageElement)) {
+ return
+ }
- const imageSrc = target.getAttribute('src') || target.getAttribute('data-image')
+ const imageSrc = target.src || target.getAttribute('data-image')
if (imageSrc) {
const modal = getImagePreviewModal()
modal.show(imageSrc)
}
}📝 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.
| onImageDoubleClick = (event: MouseEvent) => { | |
| const target = event.target | |
| const imageSrc = target.getAttribute('src') || target.getAttribute('data-image') | |
| if (imageSrc) { | |
| const modal = getImagePreviewModal() | |
| modal.show(imageSrc) | |
| } | |
| } | |
| onImageDoubleClick = (event: MouseEvent) => { | |
| const target = event.target | |
| if (!(target instanceof HTMLImageElement)) { | |
| return | |
| } | |
| const imageSrc = target.src || target.getAttribute('data-image') | |
| if (imageSrc) { | |
| const modal = getImagePreviewModal() | |
| modal.show(imageSrc) | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@packages/fluent-editor/src/modules/custom-image/specs/custom-image-spec.ts`
around lines 137 - 145, The onImageDoubleClick handler currently calls
getAttribute on event.target (typed EventTarget) without ensuring it's an
Element and without verifying it's an image; update onImageDoubleClick to first
guard that event.target is an Element (e.g., using instanceof Element) and that
it is an image (check tagName/nodeName === 'IMG' or use matches('img') or
instanceof HTMLImageElement), then safely read src or data-image and, if
present, call getImagePreviewModal().show(imageSrc); also handle null/undefined
target to avoid runtime errors.
PR
效果如下：
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.