fix: node view renders #8559
fix: node view renders #8559
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds centralized, typed attribute enums and attribute fields across multiple editor extensions, replaces string literal attribute access with enum-based keys (including React keys), and refactors custom-image error/restoration flow while adding an empty alt attribute for images. Changes
Sequence Diagram(s)mermaid Img->>NV: onError event Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 2
🤖 Fix all issues with AI agents
In `@packages/editor/src/core/extensions/custom-image/components/block.tsx`:
- Line 222: The JSX is always rendering an id because of the `?? ""` fallback,
causing duplicate DOM ids when `node.attrs[ECustomImageAttributeNames.ID]` is
missing; update the rendering logic around
`getImageBlockId(node.attrs[ECustomImageAttributeNames.ID] ?? "")` to only pass
an id prop when the attribute is present and non-empty (or ensure a unique id is
generated/assigned beforehand). Locate the usage of `getImageBlockId` in the
component (the JSX id prop) and change it to conditional rendering (omit the id
prop when `node.attrs[ECustomImageAttributeNames.ID]` is falsy) so duplicate
`editor-image-block-` ids are no longer produced.
- Around line 229-235: The interactive image container with role="button" (the
element using containerRef and onKeyDown) is missing an accessible name and
doesn't prevent default browser behavior for Space/Enter; update the element to
provide an accessible name (e.g., add aria-label or aria-labelledby) and modify
the onKeyDown handler (the function that checks e.key === "Enter" || e.key === "
") to call e.preventDefault() before invoking handleImageMouseDown so Space
doesn't scroll the page and the control is announced by screen readers.
🧹 Nitpick comments (6)
packages/editor/src/core/extensions/callout/types.ts (1)
3-12: Prefer erasable attribute-name constants overenum.Project guidance prefers erasable syntax; adding new
enummembers entrenches the pattern. Consider moving to aconstobject + union type and updating usages over time. Based on learnings, ...♻️ Example direction (requires follow-up updates across callout usage)
-export enum ECalloutAttributeNames { - ID = "id", - ICON_COLOR = "data-icon-color", - ICON_NAME = "data-icon-name", - EMOJI_UNICODE = "data-emoji-unicode", - EMOJI_URL = "data-emoji-url", - LOGO_IN_USE = "data-logo-in-use", - BACKGROUND = "data-background", - BLOCK_TYPE = "data-block-type", -} +export const CALLOUT_ATTRIBUTE_NAMES = { + ID: "id", + ICON_COLOR: "data-icon-color", + ICON_NAME: "data-icon-name", + EMOJI_UNICODE: "data-emoji-unicode", + EMOJI_URL: "data-emoji-url", + LOGO_IN_USE: "data-logo-in-use", + BACKGROUND: "data-background", + BLOCK_TYPE: "data-block-type", +} as const; +export type TCalloutAttributeName = + typeof CALLOUT_ATTRIBUTE_NAMES[keyof typeof CALLOUT_ATTRIBUTE_NAMES];Also applies to: 24-30
packages/editor/src/core/extensions/code/types.ts (1)
1-9: Prefer erasable const objects over enums for attribute names.The repo guidance discourages enums in TS. Consider a
constobject + union type to avoid runtime enum emission and keep types erasable.♻️ Proposed refactor
-export enum ECodeBlockAttributeNames { - ID = "id", - LANGUAGE = "language", -} +export const CODE_BLOCK_ATTRIBUTE_NAMES = { + ID: "id", + LANGUAGE: "language", +} as const; + +export type TCodeBlockAttributeName = + (typeof CODE_BLOCK_ATTRIBUTE_NAMES)[keyof typeof CODE_BLOCK_ATTRIBUTE_NAMES]; export type TCodeBlockAttributes = { - [ECodeBlockAttributeNames.ID]: string | null; - [ECodeBlockAttributeNames.LANGUAGE]: string | null; + [CODE_BLOCK_ATTRIBUTE_NAMES.ID]: string | null; + [CODE_BLOCK_ATTRIBUTE_NAMES.LANGUAGE]: string | null; };Based on learnings, avoid enums if the project prefers erasable syntax.
packages/editor/src/core/extensions/work-item-embed/types.ts (1)
1-15: Prefer erasable const object + mapped type overenum.This repo’s TS guidance favors erasable syntax; the enum will emit runtime code. Consider replacing it with a
constobject and a mapped type to keep types-only. Based on learnings and as per coding guidelines, avoid enums.♻️ Proposed refactor (erasable types)
-export enum EWorkItemEmbedAttributeNames { - ID = "id", - ENTITY_IDENTIFIER = "entity_identifier", - PROJECT_IDENTIFIER = "project_identifier", - WORKSPACE_IDENTIFIER = "workspace_identifier", - ENTITY_NAME = "entity_name", -} - -export type TWorkItemEmbedAttributes = { - [EWorkItemEmbedAttributeNames.ID]: string | undefined; - [EWorkItemEmbedAttributeNames.ENTITY_IDENTIFIER]: string | undefined; - [EWorkItemEmbedAttributeNames.PROJECT_IDENTIFIER]: string | undefined; - [EWorkItemEmbedAttributeNames.WORKSPACE_IDENTIFIER]: string | undefined; - [EWorkItemEmbedAttributeNames.ENTITY_NAME]: string | undefined; -}; +export const EWorkItemEmbedAttributeNames = { + ID: "id", + ENTITY_IDENTIFIER: "entity_identifier", + PROJECT_IDENTIFIER: "project_identifier", + WORKSPACE_IDENTIFIER: "workspace_identifier", + ENTITY_NAME: "entity_name", +} as const satisfies Record<string, string>; + +export type TWorkItemEmbedAttributeName = + (typeof EWorkItemEmbedAttributeNames)[keyof typeof EWorkItemEmbedAttributeNames]; + +export type TWorkItemEmbedAttributes = + Partial<Record<TWorkItemEmbedAttributeName, string>>;packages/editor/src/core/extensions/custom-image/components/node-view.tsx (2)
6-6: Prefer const-object/union attribute names over enums (if applicable).
IfECustomImageAttributeNamesis a TSenumin../types, the repo guidance prefers erasableconstobjects/unions to avoid runtime enum output. Consider switching the definition while keeping usage unchanged.Based on learnings, the repo prefers const objects/unions over enums.
74-76: Re-check the exhaustive-deps suppression.
WithgetImageSourceandgetImageDownloadSourceexplicitly listed, confirm the lint disable is still required; remove it if the rule passes to keep hooks linting active.As per coding guidelines, keep ESLint rules enabled where possible.
packages/editor/src/core/extensions/custom-image/components/block.tsx (1)
250-286: Log the caught error for diagnostics.
Thecatchblock drops the actual error; include it so failures are actionable in logs.♻️ Suggested tweak
- } catch { + } catch (error) { setFailedToLoadImage(true); - console.error("Error while loading image", e); + console.error("Error while loading image", error, e); } finally {As per coding guidelines, log caught errors with the error object.
packages/editor/src/core/extensions/custom-image/components/block.tsx
Outdated
Show resolved
Hide resolved
packages/editor/src/core/extensions/custom-image/components/block.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/editor/src/core/extensions/custom-image/components/block.tsx`:
- Around line 245-276: The catch block in the onError handler for the image
(inside the anonymous async function in onError) currently swallows the thrown
restore error and logs the React event variable e; change the catch to capture
the thrown error (e.g., catch (err)) and log that error with
console.error("Error while loading image", err) while preserving the existing
setFailedToLoadImage(true) behavior; update references to the restore helpers
(extension.options.restoreImage, extension.options.getImageSource) only insofar
as needed to ensure the caught error is the one logged.
♻️ Duplicate comments (1)
packages/editor/src/core/extensions/custom-image/components/block.tsx (1)
222-222: Avoid duplicate DOM ids when the attribute is missing.Line 222 still falls back to
"", which can collapse multiple nodes to the sameeditor-image-block-id. Prefer omitting theidprop when the attribute is absent (or ensure IDs are always assigned upstream).
Description
Adds unique key props to all NodeViewWrapper components in the editor package to ensure stable React rendering and prevent unnecessary re-mounts during editor updates.
Problem:
TipTap NodeViewWrapper components were missing stable key props, which could cause React to re-mount components unnecessarily during re-renders, leading to loss of component state, unnecessary DOM mutations, and potential visual flickering.
Solution:
Files affected:
Core extensions: callout, code-block, custom-image, mentions, work-item-embed
Type of Change
[x] Improvement (change that would cause existing functionality to not work as expected)
[x] Code refactoring
[x] Performance improvements
Screenshots and Media (if applicable)
N/A - Internal rendering optimization with no visible UI changes
Test Scenarios
Verified all editor extensions render correctly with unique keys
Summary by CodeRabbit
Improvements
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.