[WIKI-400] feat: page navigation pane #7206
Conversation
WalkthroughThis update introduces a comprehensive "Page Navigation Pane" feature to the editor UI, including outline, info, and assets tabs, with localized support in multiple languages. It refactors editor asset management, centralizes editor references in a MobX sub-store, and streamlines version history handling. The change also improves asset tracking, updates extension storage, and removes deprecated sidebar components. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PageEditor
participant NavigationPane
participant EditorStore
participant EditorExtension
User->>PageEditor: Opens page
PageEditor->>EditorStore: Initialize editor sub-store
PageEditor->>NavigationPane: Renders sidebar (outline/info/assets)
NavigationPane->>EditorStore: Reads editorRef, assetsList
EditorExtension-->>EditorStore: Updates assetsList on asset add/remove
User->>NavigationPane: Switches tab (outline/info/assets)
NavigationPane->>EditorStore: Fetches corresponding data
User->>NavigationPane: Closes pane
NavigationPane->>PageEditor: Triggers close handler
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
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 (
|
|
Pull Request Linked with Plane Work Items
Comment Automatically Generated by Plane |
![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
🔭 Outside diff range comments (1)
web/core/components/pages/version/main-content.tsx (1)
31-38:⚠️ Potential issueSWR cache key is not unique per page – risk of cross-page data bleed
Using only
"PAGE_VERSION_${activeVersion}"ignorespageId, so visiting two different pages that happen to share a version UUID will share the same cache entry.- pageId && activeVersion ? `PAGE_VERSION_${activeVersion}` : null, - pageId && activeVersion ? () => fetchVersionDetails(pageId, activeVersion) : null + pageId && activeVersion ? ["PAGE_VERSION", pageId, activeVersion] : null, + pageId && activeVersion ? () => fetchVersionDetails(pageId, activeVersion) : nullSWR stringifies array keys, guaranteeing uniqueness and preserving the existing cache-invalidation behavior.
🧹 Nitpick comments (24)
packages/editor/src/ce/constants/extensions.ts (1)
1-1: Add documentation to the placeholder enumThe exported
ADDITIONAL_EXTENSIONSenum is currently empty—consider adding a comment describing its intended use or deferring its introduction until it contains members to avoid confusion.web/helpers/editor.helper.ts (1)
25-28: Doc-string copy-pasteThe JSDoc still says “generate the file source using assetId”. Clarify that this variant returns a download URL to avoid confusion when scanning the file.
packages/editor/src/ce/constants/assets.ts (1)
6-6: Empty placeholder constant
ADDITIONAL_ASSETS_META_DATA_RECORDis currently an empty object. If this is intentional scaffolding, add a TODO comment describing how/when it will be populated to avoid it being mistaken for dead code later.web/core/components/pages/navigation-pane/tabs-list.tsx (2)
25-31: Animation math ties to list length
leftandwidthare derived fromORDERED_PAGE_NAVIGATION_TABS_LIST.length.
If tabs are ever conditionally hidden or added at runtime, the indicator will be mis-aligned.Consider computing positions from the actual rendered tab count (
React.Children.count) or from the element’s offset width for robustness.
15-23: Minor: provideindexin map for clarity
TabsuppliesselectedIndexbut the individual button doesn’t know its own index – using the map’s index can be helpful for debugging or aria attributes.-{ORDERED_PAGE_NAVIGATION_TABS_LIST.map((tab) => ( +{ORDERED_PAGE_NAVIGATION_TABS_LIST.map((tab, idx) => ( <Tab key={tab.key} type="button" className="relative z-[1] flex-1 py-1.5 text-sm font-semibold outline-none" > {t(tab.i18n_label)} </Tab> ))}web/core/components/pages/navigation-pane/tab-panels/outline.tsx (1)
24-36: Memoise the Empty-State element & localise thealttext.
EmptyStateis recreated on every render; wrap it inuseMemoto avoid useless re-allocations.- The hard-coded
altstring bypasses i18n. Expose it viat(...)so screen-reader users get a translated description.+ const EmptyState = useMemo( + () => ( ... - <Image src={resolvedPath} width={160} height={160} alt="An image depicting the outline of a page" /> + <Image + src={resolvedPath} + width={160} + height={160} + alt={t("page_navigation_pane.tabs.outline.empty_state.image_alt")} + /> ... - ); + ), + [resolvedPath, t] + );web/core/components/pages/editor/summary/heading-components.tsx (1)
13-29: Expose full heading text viatitleto improve accessibility.Because the label is truncated with
truncate, users can’t see the full heading unless they click it. Addingtitle={marking.text}keeps the UI unchanged while providing the full text on hover & for assistive tech.- <button type="button" onClick={onClick} className={cn(COMMON_CLASSNAME, "text-sm pl-1")}> + <button + type="button" + onClick={onClick} + className={cn(COMMON_CLASSNAME, "text-sm pl-1")} + title={marking.text} + >Apply the same to
OutlineHeading2andOutlineHeading3.web/core/components/pages/navigation-pane/tab-panels/info/document-info.tsx (2)
29-36: Guard against missingeditorRef.
editorRefis optional (?.is used), but if it’sundefined, the component still subscribes & shows zeroed stats forever.
Consider an early-return fallback to avoid misleading information:if (!editorRef) { return null; // or a dedicated “unavailable” state }
38-42:secondsToReadableTimeignores i18n & plural rules.Formatting
"60s"vs"1m"is language-dependent and may need pluralisation.
Expose it through the translation layer or at least allow overriding the units.web/core/components/pages/navigation-pane/tab-panels/root.tsx (1)
24-35: All tab panels stay mounted – could waste resources.
@headlessui/reactwill keep every<Tab.Panel>in the DOM unless you addunmount.
Panels like “info” subscribe to editor updates and can run expensive observers. Mounting only the active tab avoids unnecessary work.- <Tab.Panel + <Tab.Panel + unmount={true}web/core/components/pages/editor/summary/content-browser.tsx (1)
39-44: Micro-perf: moveHeadingComponentmapping out of render
HeadingComponentis recreated on every render. Memoising (or defining it outside the component) avoids needless allocations and keeps referential equality stable.-const HeadingComponent: { - [key: number]: React.FC<THeadingComponentProps>; -} = { +// move this *above* the component definition +export const HEADING_COMPONENT: { + [key: number]: React.FC<THeadingComponentProps>; +} = { }; // inside the component -const HeadingComponent = HEADING_COMPONENT;packages/editor/src/core/hooks/use-read-only-editor.ts (1)
75-75: Stabilise imperative handle with dependencies
useImperativeHandleis called without a deps array, soforwardedRef.currentis reassigned on every render even wheneditoris unchanged.
Providing[editor, provider]keeps the same object reference unless something relevant really changed.-useImperativeHandle(forwardedRef, () => getEditorRefHelpers({ editor, provider })); +useImperativeHandle( + forwardedRef, + () => getEditorRefHelpers({ editor, provider }), + [editor, provider] +);web/core/components/pages/navigation-pane/tab-panels/info/actors-info.tsx (1)
30-66: Duplication between “edited by” and “created by” blocks – extract a small presentational componentThe two sections share identical structure. Extracting something like
<UserMeta label="Edited by" userId={...} date={...} />will cut the duplication, simplify maintenance, and reduce future divergence in styling.web/ce/components/pages/navigation-pane/index.ts (1)
1-22: Useas constto retain literal types & avoid wideningAdding
as consttoPAGE_NAVIGATION_PANE_TABS_LISTpreserves literal string unions for callers and eliminates the need for repetitivekeyproperties.> = { outline: { key: "outline", i18n_label: "page_navigation_pane.tabs.outline.label", }, ... -}; +} as const;This also allows
ORDERED_PAGE_NAVIGATION_TABS_LISTto derive types without additional assertions.web/core/components/pages/editor/toolbar/root.tsx (2)
54-60: Addaria-label& translation to the visible navigation-pane toggle buttonThe floating button (lines 69-76) is accessible, but the primary toolbar button is missing an
aria-label, making it invisible to screen-reader users and inconsistent with the hidden-toolbar variant.-<button +<button type="button" className="flex-shrink-0 size-6 grid place-items-center rounded text-custom-text-200 hover:text-custom-text-100 hover:bg-custom-background-80 transition-colors" onClick={handleOpenNavigationPane} + aria-label={t("page_navigation_pane.open_button")} >
53-77: DRY: duplicate toggle-button markup appears twiceThe two identical button blocks (inside toolbar & floating) could be abstracted into a small
<NavigationPaneToggle />component to prevent drift and simplify future style updates.web/core/components/pages/navigation-pane/tab-panels/info/version-history.tsx (1)
85-89: Consider deterministic ordering of versions
fetchAllVersionsmay return unsorted data. Sorting (e.g. bylast_saved_atdescending) guarantees a stable UI and prevents accidental re-ordering when the backend changes its default.const { data: versionsList } = useSWR( id ? `PAGE_VERSIONS_LIST_${id}` : null, id ? () => versionHistory.fetchAllVersions(id).then(v => v?.sort((a,b) => Date.parse(b.last_saved_at) - Date.parse(a.last_saved_at))) : null, );web/core/components/pages/navigation-pane/root.tsx (1)
48-56: Userouter.replace+ shallow routing to avoid full page navigations on tab switchTab changes only mutate query parameters.
router.pushtriggers a full navigation, which is heavier and resets scroll position. Replacing in place keeps history lean and improves UX.- const updatedRoute = updateQueryParams({ - paramsToAdd: { [PAGE_NAVIGATION_PANE_TABS_QUERY_PARAM]: updatedTab }, - }); - router.push(updatedRoute); + router.replace( + updateQueryParams({ paramsToAdd: { [PAGE_NAVIGATION_PANE_TABS_QUERY_PARAM]: updatedTab } }), + { scroll: false } + );web/core/components/pages/navigation-pane/tab-panels/assets.tsx (1)
101-110: Effect dependency should includepageoreditorRefstable referenceIf
page.editorRefchanges, the cleanup of the previous subscription is skipped because the dependency array only trackseditorRefreference identity. Ifpageis re-created elsewhere, explicitly include it to avoid leaks.-}, [editorRef]); +}, [page, editorRef]);web/core/components/pages/version/root.tsx (1)
50-51: Avoid layout coupling with a magic widthThe overlay always reserves
100% - 294px, even when the navigation pane is closed, creating dead space and complicating responsive layouts.
Compute the width conditionally or derive it from a CSS variable controlled by the nav-pane state.web/core/components/pages/editor/page-root.tsx (2)
103-108: Memoise derived flag
isValidNavigationPaneTabrecalculates on every render.
Wrap it inuseMemowithnavigationPaneQueryParamas dep to avoid useless renders once the editor grows heavier.
110-114: Userouter.replaceto prevent history spamOpening the navigation pane just toggles a UI element; adding a new entry to browser history on every toggle (
router.push) clutters the back-stack.
router.replace(or{ scroll:false }) keeps the URL in sync without polluting history.packages/editor/src/core/types/editor.ts (1)
108-109: Consistent casing for markdown helperThe method is
getMarkDown, but the underlying storage helper isgetMarkdown(). Align the casing (getMarkdown) to avoid confusion.packages/editor/src/core/hooks/use-editor.ts (1)
220-227: Throttle expensive document-info computations
onDocumentInfoChangerecomputes counts on every transaction, traversing the doc each time.
Wrap the handler in athrottle/debounce(e.g. 250 ms) to avoid UI jank on rapid typing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
packages/editor/src/ce/constants/assets.ts(1 hunks)packages/editor/src/ce/constants/extensions.ts(1 hunks)packages/editor/src/ce/types/storage.ts(2 hunks)packages/editor/src/core/extensions/custom-image/components/image-block.tsx(2 hunks)packages/editor/src/core/helpers/assets.ts(1 hunks)packages/editor/src/core/helpers/editor-ref.ts(1 hunks)packages/editor/src/core/hooks/use-editor.ts(4 hunks)packages/editor/src/core/hooks/use-read-only-editor.ts(3 hunks)packages/editor/src/core/plugins/drag-handle.ts(3 hunks)packages/editor/src/core/types/editor.ts(2 hunks)web/ce/components/pages/navigation-pane/index.ts(1 hunks)web/ce/components/pages/navigation-pane/tab-panels/root.tsx(1 hunks)web/core/components/pages/editor/editor-body.tsx(5 hunks)web/core/components/pages/editor/page-root.tsx(4 hunks)web/core/components/pages/editor/summary/content-browser.tsx(3 hunks)web/core/components/pages/editor/summary/heading-components.tsx(1 hunks)web/core/components/pages/editor/toolbar/index.ts(0 hunks)web/core/components/pages/editor/toolbar/info-popover.tsx(0 hunks)web/core/components/pages/editor/toolbar/root.tsx(3 hunks)web/core/components/pages/header/actions.tsx(1 hunks)web/core/components/pages/navigation-pane/index.ts(1 hunks)web/core/components/pages/navigation-pane/root.tsx(1 hunks)web/core/components/pages/navigation-pane/tab-panels/assets.tsx(1 hunks)web/core/components/pages/navigation-pane/tab-panels/info/actors-info.tsx(1 hunks)web/core/components/pages/navigation-pane/tab-panels/info/document-info.tsx(1 hunks)web/core/components/pages/navigation-pane/tab-panels/info/root.tsx(1 hunks)web/core/components/pages/navigation-pane/tab-panels/info/version-history.tsx(1 hunks)web/core/components/pages/navigation-pane/tab-panels/outline.tsx(1 hunks)web/core/components/pages/navigation-pane/tab-panels/root.tsx(1 hunks)web/core/components/pages/navigation-pane/tabs-list.tsx(1 hunks)web/core/components/pages/version/editor.tsx(3 hunks)web/core/components/pages/version/index.ts(0 hunks)web/core/components/pages/version/main-content.tsx(5 hunks)web/core/components/pages/version/root.tsx(1 hunks)web/core/components/pages/version/sidebar-list-item.tsx(0 hunks)web/core/components/pages/version/sidebar-list.tsx(0 hunks)web/core/components/pages/version/sidebar-root.tsx(0 hunks)web/core/hooks/use-query-params.ts(2 hunks)web/helpers/editor.helper.ts(1 hunks)web/styles/globals.css(1 hunks)
💤 Files with no reviewable changes (6)
- web/core/components/pages/editor/toolbar/index.ts
- web/core/components/pages/version/index.ts
- web/core/components/pages/version/sidebar-list.tsx
- web/core/components/pages/version/sidebar-list-item.tsx
- web/core/components/pages/version/sidebar-root.tsx
- web/core/components/pages/editor/toolbar/info-popover.tsx
🧰 Additional context used
🧬 Code Graph Analysis (17)
web/ce/components/pages/navigation-pane/tab-panels/root.tsx (2)
web/ce/components/pages/navigation-pane/index.ts (1)
TPageNavigationPaneTab(1-1)web/core/store/pages/base-page.ts (1)
TPageInstance(67-70)
web/helpers/editor.helper.ts (2)
apiserver/plane/settings/storage.py (1)
url(15-16)web/core/store/router.store.ts (2)
projectId(85-87)workspaceSlug(69-71)
web/core/components/pages/navigation-pane/tabs-list.tsx (3)
packages/i18n/src/hooks/use-translation.ts (1)
useTranslation(23-35)web/ce/components/pages/navigation-pane/index.ts (1)
ORDERED_PAGE_NAVIGATION_TABS_LIST(24-31)packages/i18n/src/store/index.ts (1)
t(211-232)
packages/editor/src/ce/constants/assets.ts (1)
packages/editor/src/core/helpers/assets.ts (1)
TAssetMetaDataRecord(12-12)
packages/editor/src/core/hooks/use-read-only-editor.ts (1)
packages/editor/src/core/helpers/editor-ref.ts (1)
getEditorRefHelpers(20-57)
web/core/components/pages/navigation-pane/tab-panels/outline.tsx (4)
web/core/store/pages/base-page.ts (1)
TPageInstance(67-70)web/core/hooks/use-resolved-asset-path.tsx (1)
useResolvedAssetPath(10-26)packages/i18n/src/hooks/use-translation.ts (1)
useTranslation(23-35)web/core/components/pages/editor/summary/content-browser.tsx (1)
PageContentBrowser(16-81)
web/core/components/pages/editor/editor-body.tsx (2)
packages/i18n/src/hooks/use-translation.ts (1)
useTranslation(23-35)web/core/components/pages/editor/summary/content-browser.tsx (1)
PageContentBrowser(16-81)
web/core/components/pages/navigation-pane/index.ts (1)
web/ce/components/pages/navigation-pane/index.ts (1)
ORDERED_PAGE_NAVIGATION_TABS_LIST(24-31)
web/core/components/pages/version/main-content.tsx (1)
web/helpers/date-time.helper.ts (1)
renderFormattedTime(79-94)
web/core/components/pages/editor/summary/heading-components.tsx (1)
packages/editor/src/core/extensions/headings-list.ts (1)
IMarking(6-11)
web/core/components/pages/version/root.tsx (3)
web/core/components/pages/version/editor.tsx (1)
TVersionEditorProps(17-20)web/core/hooks/use-query-params.ts (1)
useQueryParams(8-37)web/core/components/pages/navigation-pane/index.ts (2)
PAGE_NAVIGATION_PANE_VERSION_QUERY_PARAM(9-9)PAGE_NAVIGATION_PANE_WIDTH(6-6)
web/core/components/pages/navigation-pane/tab-panels/info/version-history.tsx (7)
web/core/store/pages/base-page.ts (1)
TPageInstance(67-70)web/core/components/pages/editor/page-root.tsx (1)
TPageRootHandlers(32-39)packages/i18n/src/hooks/use-translation.ts (1)
useTranslation(23-35)web/helpers/date-time.helper.ts (1)
renderFormattedTime(79-94)packages/ui/src/avatar/avatar.tsx (1)
Avatar(116-169)web/core/components/pages/navigation-pane/index.ts (1)
PAGE_NAVIGATION_PANE_VERSION_QUERY_PARAM(9-9)web/core/hooks/use-query-params.ts (1)
useQueryParams(8-37)
web/core/components/pages/navigation-pane/tab-panels/assets.tsx (7)
web/core/store/pages/base-page.ts (1)
TPageInstance(67-70)packages/editor/src/core/types/editor.ts (1)
TEditorAsset(82-89)packages/i18n/src/hooks/use-translation.ts (1)
useTranslation(23-35)web/core/store/router.store.ts (1)
workspaceSlug(69-71)web/helpers/editor.helper.ts (2)
getEditorAssetSrc(14-23)getEditorAssetDownloadSrc(29-38)packages/utils/src/attachment.ts (1)
convertBytesToSize(22-32)web/core/hooks/use-resolved-asset-path.tsx (1)
useResolvedAssetPath(10-26)
packages/editor/src/core/helpers/assets.ts (3)
packages/editor/src/core/types/editor.ts (1)
TEditorAsset(82-89)packages/editor/src/core/extensions/custom-image/components/image-block.tsx (1)
getImageBlockId(41-41)packages/editor/src/ce/constants/assets.ts (1)
ADDITIONAL_ASSETS_META_DATA_RECORD(6-6)
web/core/components/pages/editor/toolbar/root.tsx (4)
web/core/store/pages/base-page.ts (1)
TPageInstance(67-70)packages/i18n/src/hooks/use-translation.ts (1)
useTranslation(23-35)web/core/components/pages/editor/toolbar/toolbar.tsx (1)
PageToolbar(66-164)web/ce/components/pages/header/collaborators-list.tsx (1)
PageCollaboratorsList(10-10)
packages/editor/src/core/types/editor.ts (1)
packages/editor/src/core/extensions/headings-list.ts (1)
IMarking(6-11)
packages/editor/src/core/hooks/use-editor.ts (5)
packages/editor/src/core/helpers/editor-ref.ts (1)
getEditorRefHelpers(20-57)packages/editor/src/core/helpers/get-extension-storage.ts (1)
getExtensionStorage(5-8)packages/editor/src/core/helpers/assets.ts (1)
getAllEditorAssets(39-54)packages/editor/src/core/helpers/common.ts (1)
getParagraphCount(92-99)packages/editor/src/core/helpers/insert-content-at-cursor-position.ts (1)
insertContentAtSavedSelection(3-22)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (19)
web/styles/globals.css (1)
500-500: Enable smooth scrolling globallyAdding
scroll-behavior: smoothat the global selector aligns with the new navigation pane's scroll-to-section feature.web/core/components/pages/header/actions.tsx (1)
5-5: Imported PageOptionsDropdown for new options menuThe
PageOptionsDropdownreplaces the removedPageInfoPopoverto surface page actions—import path and usage look correct.packages/editor/src/ce/types/storage.ts (1)
1-1: Integrate character count into extension storageThe
CharacterCountStorageis now mapped underCORE_EXTENSIONS.CHARACTER_COUNT, matching the new document info API. Imports and type mapping appear correct.Also applies to: 19-19
web/helpers/editor.helper.ts (1)
25-38: Avoid duplicating logic & tighten return type
getEditorAssetDownloadSrcrepeats almost the exact branch logic ofgetEditorAssetSrc. Consider extracting the common URL prefix builder or passing a boolean flag indicating “download” to a single helper – this prevents two functions from silently diverging.In addition, the function is typed to return
string | undefinedbut always returns a string (""at worst). Either change the initial value toundefinedor narrow the return type tostringfor consistency with actual behavior.-export const getEditorAssetDownloadSrc = (args: TEditorSrcArgs): string | undefined => { +export const getEditorAssetDownloadSrc = (args: TEditorSrcArgs): string => { const { assetId, projectId, workspaceSlug } = args; - let url: string | undefined = ""; + let url = "";[ suggest_essential_refactor ]
packages/editor/src/ce/constants/assets.ts (1)
1-7: Potential incorrect import alias
import { TAssetMetaDataRecord } from "@/helpers/assets";Inside
packages/editor, the alias@/usually resolves topackages/editor/src.
The actual file lives atcore/helpers/assets.ts, so the correct path should likely be:import { TAssetMetaDataRecord } from "@/core/helpers/assets";Please verify the ts-config path mapping; otherwise this will break type-checking and builds.
web/core/components/pages/navigation-pane/tab-panels/outline.tsx (1)
18-18:editorRefmight not exist onTPageInstance.
editorRefis destructured directly frompage, yet the snippet that definesTPageInstancedoesn’t list this property.
If the type wasn’t extended elsewhere, this will raise a TypeScript error.Please confirm that
TPageInstancehas been augmented (e.g., via declaration merging) to includeeditorRef, or update the prop typing accordingly.web/core/components/pages/editor/summary/heading-components.tsx (1)
10-12: Good consolidation of duplicated class names.Extracting
COMMON_CLASSNAMEremoves repetition and eases future tweaks.packages/editor/src/core/plugins/drag-handle.ts (1)
419-420: Switched to publicserializeForClipboard– nice!Using the public API removes the reliance on an internal symbol and future-proofs the plugin.
web/core/components/pages/navigation-pane/tab-panels/root.tsx (1)
7-8: Potential circular / duplicate import path.This file is itself
.../navigation-pane/tab-panels/root.tsxyet imports
@/plane-web/components/pages/navigation-pane/tab-panels/root.
Double-check that it refers to a different module and not to itself through another alias, which would cause unexpected bundler behaviour.web/core/components/pages/editor/summary/content-browser.tsx (1)
46-46: Fail-safe early-return looks goodGracefully returning the provided
emptyState(ornull) when there are no headings prevents unnecessary DOM nodes.
No action required.packages/editor/src/core/hooks/use-read-only-editor.ts (1)
43-46: SwitchingimmediatelyRendertofalsecan delay the first paintWith read-only editors this is usually fine, but if you rely on the initial HTML being present for layout calculations (e.g. scroll height) you may see a one-frame flash.
Please verify in the page/version screens that the document no longer flickers on mount.web/core/components/pages/version/editor.tsx (1)
92-98: Rendering nothing when description is empty might be confusingIf a version exists but its
description_htmlis empty, the component returnsnull, so users see a blank pane instead of a placeholder or “No content” message. Consider falling back to the empty paragraph you already pass to the editor, or showing an explicit notice.-if (description === undefined || description?.trim() === "") return null; +if (description === undefined || description.trim() === "") { + return ( + <p className="text-sm text-custom-text-400 px-5 py-4"> + This version contains no content. + </p> + ); +}web/core/components/pages/navigation-pane/tab-panels/info/root.tsx (1)
16-26: Component structure looks solidClear separation of document info, actors info and version history keeps the tab panel cohesive. No issues spotted.
web/core/components/pages/editor/editor-body.tsx (1)
92-94: Translation hook addition LGTMGood to see i18n support for the floating outline button.
web/core/components/pages/navigation-pane/index.ts (1)
1-11: Import path looks CE-specific – confirm alias correctness
"@/plane-web/components/pages/navigation-pane"resolves to the CE layer. If this file is compiled in OSS/EE builds that don’t include that alias, the import will fail. Please double-check thattsconfig.pathsmaps this alias in every build target.web/core/components/pages/version/main-content.tsx (1)
111-113: Render guard forVersionEditorduring loading
<VersionEditor>receivesversionDetails={undefined}on initial render. If the implementation assumes a non-null object this can cause runtime errors. Consider short-circuiting untilversionDetailsis available or ensure the editor handlesundefined.packages/editor/src/core/helpers/assets.ts (1)
8-9: Path alias likely wrong
"@/plane-editor/constants/assets"looks out of place—the file lives inpackages/editor/src/ce/constants/assets.ts.
Double-check the tsconfig path alias; a wrong import will silently yield an empty object, compounding the issue above.web/core/components/pages/version/root.tsx (1)
31-33: Validate query-param before use
activeVersioncomes straight fromsearchParams. If a malformed value is injected it will propagate toPageVersionsMainContent, potentially triggering an unnecessary network call or even a crash.
Add a simple format check (e.g. UUID/number regex) before passing it downstream.web/core/components/pages/editor/page-root.tsx (1)
152-160: Double-check prop drilling of version API
PageNavigationPaneRootreceives bothisNavigationPaneOpenandversionHistorywhile the overlay independently fetches version details via the same handlers. Ensure concurrent fetches don’t race or duplicate network traffic.
packages/editor/src/core/extensions/custom-image/components/image-block.tsx
Show resolved
Hide resolved
packages/editor/src/core/extensions/custom-image/components/image-block.tsx
Show resolved
Hide resolved
| const updateQueryParams = useCallback( | ||
| ({ paramsToAdd = {}, paramsToRemove = [] }: { paramsToAdd?: TParamsToAdd; paramsToRemove?: string[] }) => { | ||
| const currentParams = new URLSearchParams(searchParams.toString()); | ||
|
|
||
| // add or update query parameters | ||
| Object.keys(paramsToAdd).forEach((key) => { | ||
| currentParams.set(key, paramsToAdd[key]); | ||
| }); | ||
| // add or update query parameters | ||
| Object.keys(paramsToAdd).forEach((key) => { | ||
| currentParams.set(key, paramsToAdd[key]); | ||
| }); | ||
|
|
||
| // remove specified query parameters | ||
| paramsToRemove.forEach((key) => { | ||
| currentParams.delete(key); | ||
| }); | ||
| // remove specified query parameters | ||
| paramsToRemove.forEach((key) => { | ||
| currentParams.delete(key); | ||
| }); | ||
|
|
||
| // construct the new route with the updated query parameters | ||
| const newRoute = `${pathname}?${currentParams.toString()}`; | ||
| return newRoute; | ||
| }; | ||
| // construct the new route with the updated query parameters | ||
| const newRoute = `${pathname}?${currentParams.toString()}`; | ||
| return newRoute; | ||
| }, | ||
| [pathname, searchParams] | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Trailing “?” when no params
new URLSearchParams().toString() returns an empty string, so routes with no query parameters become ${pathname}? (e.g. /docs?). This yields visually odd URLs and can confuse caching layers.
Add a conditional:
-const newRoute = `${pathname}?${currentParams.toString()}`;
+const query = currentParams.toString();
+const newRoute = query ? `${pathname}?${query}` : pathname;📝 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.
| const updateQueryParams = useCallback( | |
| ({ paramsToAdd = {}, paramsToRemove = [] }: { paramsToAdd?: TParamsToAdd; paramsToRemove?: string[] }) => { | |
| const currentParams = new URLSearchParams(searchParams.toString()); | |
| // add or update query parameters | |
| Object.keys(paramsToAdd).forEach((key) => { | |
| currentParams.set(key, paramsToAdd[key]); | |
| }); | |
| // add or update query parameters | |
| Object.keys(paramsToAdd).forEach((key) => { | |
| currentParams.set(key, paramsToAdd[key]); | |
| }); | |
| // remove specified query parameters | |
| paramsToRemove.forEach((key) => { | |
| currentParams.delete(key); | |
| }); | |
| // remove specified query parameters | |
| paramsToRemove.forEach((key) => { | |
| currentParams.delete(key); | |
| }); | |
| // construct the new route with the updated query parameters | |
| const newRoute = `${pathname}?${currentParams.toString()}`; | |
| return newRoute; | |
| }; | |
| // construct the new route with the updated query parameters | |
| const newRoute = `${pathname}?${currentParams.toString()}`; | |
| return newRoute; | |
| }, | |
| [pathname, searchParams] | |
| ); | |
| const updateQueryParams = useCallback( | |
| ({ paramsToAdd = {}, paramsToRemove = [] }: { paramsToAdd?: TParamsToAdd; paramsToRemove?: string[] }) => { | |
| const currentParams = new URLSearchParams(searchParams.toString()); | |
| // add or update query parameters | |
| Object.keys(paramsToAdd).forEach((key) => { | |
| currentParams.set(key, paramsToAdd[key]); | |
| }); | |
| // remove specified query parameters | |
| paramsToRemove.forEach((key) => { | |
| currentParams.delete(key); | |
| }); | |
| // construct the new route with the updated query parameters | |
| const query = currentParams.toString(); | |
| const newRoute = query ? `${pathname}?${query}` : pathname; | |
| return newRoute; | |
| }, | |
| [pathname, searchParams] | |
| ); |
🤖 Prompt for AI Agents
In web/core/hooks/use-query-params.ts around lines 13 to 32, the function
updateQueryParams appends a trailing "?" to the pathname even when there are no
query parameters, resulting in URLs like "/docs?". To fix this, add a
conditional check before constructing newRoute: if currentParams.toString() is
empty, return pathname without the "?", otherwise append "?" followed by the
query string. This prevents trailing question marks on URLs with no parameters.
| const scrollableParent = getScrollParent(dragHandleElement!); | ||
| if (!scrollableParent) return; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Null-safety over non-null assertion.
dragHandleElement! will crash if, for any reason, the element hasn’t been initialised. A defensive early return is cheaper than a runtime exception.
- const scrollableParent = getScrollParent(dragHandleElement!);
+ if (!dragHandleElement) return;
+ const scrollableParent = getScrollParent(dragHandleElement);📝 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.
| const scrollableParent = getScrollParent(dragHandleElement!); | |
| if (!scrollableParent) return; | |
| if (!dragHandleElement) return; | |
| const scrollableParent = getScrollParent(dragHandleElement); | |
| if (!scrollableParent) return; |
🤖 Prompt for AI Agents
In packages/editor/src/core/plugins/drag-handle.ts around lines 173 to 175,
replace the non-null assertion on dragHandleElement with a null check before
calling getScrollParent. Add a defensive early return if dragHandleElement is
null or undefined to prevent runtime exceptions and ensure null-safety.
| return ( | ||
| <a | ||
| href={asset.scrollId} | ||
| className="relative group/asset-item h-12 flex items-center gap-2 rounded border border-custom-border-200 hover:bg-custom-background-80 transition-colors" | ||
| > | ||
| <div | ||
| className="flex-shrink-0 w-11 h-12 rounded-l bg-cover bg-no-repeat bg-center" | ||
| style={{ | ||
| backgroundImage: `url('${getAssetSrc(asset.src)}')`, | ||
| }} | ||
| /> | ||
| <div className="flex-1 space-y-0.5"> | ||
| <p className="text-sm font-medium">{asset.name}</p> | ||
| <p className="text-xs text-custom-text-200">{convertBytesToSize(Number(asset.size || 0))}</p> | ||
| </div> | ||
| <a | ||
| href={getAssetDownloadSrc(asset.src)} | ||
| target="_blank" | ||
| rel="noreferrer noopener" | ||
| className="shrink-0 self-end mb-1 mr-1 py-0.5 px-1 flex items-center gap-1 rounded text-custom-text-200 hover:text-custom-text-100 opacity-0 pointer-events-none group-hover/asset-item:opacity-100 group-hover/asset-item:pointer-events-auto transition-all" | ||
| > | ||
| <Download className="shrink-0 size-3" /> | ||
| <span className="text-xs font-medium">{t("page_navigation_pane.tabs.assets.download_button")}</span> | ||
| </a> | ||
| </a> |
There was a problem hiding this comment.
Nested anchors are invalid HTML – convert the outer wrapper to a <div>
An <a> element cannot contain another interactive <a>. This breaks accessibility and can lead to unpredictable click behaviour.
-<a
- href={asset.scrollId}
- className="relative group/asset-item h-12 flex items-center gap-2 ..."
->
+<div
+ role="link"
+ tabIndex={0}
+ onClick={() => document.querySelector(asset.scrollId)?.scrollIntoView({ behavior: "smooth" })}
+ className="relative group/asset-item h-12 flex items-center gap-2 ..."
+>
...
-</a>
+</div>Keep the inner download link as the only anchor.
📝 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.
| return ( | |
| <a | |
| href={asset.scrollId} | |
| className="relative group/asset-item h-12 flex items-center gap-2 rounded border border-custom-border-200 hover:bg-custom-background-80 transition-colors" | |
| > | |
| <div | |
| className="flex-shrink-0 w-11 h-12 rounded-l bg-cover bg-no-repeat bg-center" | |
| style={{ | |
| backgroundImage: `url('${getAssetSrc(asset.src)}')`, | |
| }} | |
| /> | |
| <div className="flex-1 space-y-0.5"> | |
| <p className="text-sm font-medium">{asset.name}</p> | |
| <p className="text-xs text-custom-text-200">{convertBytesToSize(Number(asset.size || 0))}</p> | |
| </div> | |
| <a | |
| href={getAssetDownloadSrc(asset.src)} | |
| target="_blank" | |
| rel="noreferrer noopener" | |
| className="shrink-0 self-end mb-1 mr-1 py-0.5 px-1 flex items-center gap-1 rounded text-custom-text-200 hover:text-custom-text-100 opacity-0 pointer-events-none group-hover/asset-item:opacity-100 group-hover/asset-item:pointer-events-auto transition-all" | |
| > | |
| <Download className="shrink-0 size-3" /> | |
| <span className="text-xs font-medium">{t("page_navigation_pane.tabs.assets.download_button")}</span> | |
| </a> | |
| </a> | |
| return ( | |
| - <a | |
| - href={asset.scrollId} | |
| - className="relative group/asset-item h-12 flex items-center gap-2 rounded border border-custom-border-200 hover:bg-custom-background-80 transition-colors" | |
| - > | |
| + <div | |
| + role="link" | |
| + tabIndex={0} | |
| + onClick={() => | |
| + document | |
| + .querySelector(asset.scrollId) | |
| + ?.scrollIntoView({ behavior: "smooth" }) | |
| + } | |
| + className="relative group/asset-item h-12 flex items-center gap-2 rounded border border-custom-border-200 hover:bg-custom-background-80 transition-colors" | |
| + > | |
| <div | |
| className="flex-shrink-0 w-11 h-12 rounded-l bg-cover bg-no-repeat bg-center" | |
| style={{ | |
| backgroundImage: `url('${getAssetSrc(asset.src)}')`, | |
| }} | |
| /> | |
| <div className="flex-1 space-y-0.5"> | |
| <p className="text-sm font-medium">{asset.name}</p> | |
| <p className="text-xs text-custom-text-200"> | |
| {convertBytesToSize(Number(asset.size || 0))} | |
| </p> | |
| </div> | |
| <a | |
| href={getAssetDownloadSrc(asset.src)} | |
| target="_blank" | |
| rel="noreferrer noopener" | |
| className="shrink-0 self-end mb-1 mr-1 py-0.5 px-1 flex items-center gap-1 rounded text-custom-text-200 hover:text-custom-text-100 opacity-0 pointer-events-none group-hover/asset-item:opacity-100 group-hover/asset-item:pointer-events-auto transition-all" | |
| > | |
| <Download className="shrink-0 size-3" /> | |
| <span className="text-xs font-medium"> | |
| {t("page_navigation_pane.tabs.assets.download_button")} | |
| </span> | |
| </a> | |
| - </a> | |
| + </div> | |
| ) |
🤖 Prompt for AI Agents
In web/core/components/pages/navigation-pane/tab-panels/assets.tsx around lines
65 to 89, the outer <a> tag wraps an inner <a> tag, which is invalid HTML and
breaks accessibility. Change the outer <a> element to a <div> to remove nested
anchors, keeping the inner download link as the only anchor. Adjust styling and
event handling if necessary to preserve the original behavior.
| export type TEditorAsset = { | ||
| id: string; | ||
| name: string; | ||
| scrollId: string; | ||
| size: number; | ||
| src: string; | ||
| type: keyof typeof CORE_EXTENSIONS | keyof typeof ADDITIONAL_EXTENSIONS; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Narrow the type union to asset-extensions only
type: keyof typeof CORE_EXTENSIONS | keyof typeof ADDITIONAL_EXTENSIONS admits non-asset extensions (e.g. PARAGRAPH).
Create a dedicated ASSET_EXTENSION_KEYS union so tooling & autocomplete don’t accept invalid values.
🤖 Prompt for AI Agents
In packages/editor/src/core/types/editor.ts around lines 82 to 89, the type
property currently allows keys from both CORE_EXTENSIONS and
ADDITIONAL_EXTENSIONS, which includes non-asset extensions like PARAGRAPH. To
fix this, define a new union type ASSET_EXTENSION_KEYS that includes only
asset-related extension keys, and update the type property to use this new
union. This will restrict the allowed values to asset extensions only and
improve tooling and autocomplete accuracy.
| onHeadingChange: (callback) => { | ||
| const handleHeadingChange = () => { | ||
| if (!editor) return; | ||
| const headings = getExtensionStorage(editor, CORE_EXTENSIONS.HEADINGS_LIST)?.headings; | ||
| if (headings) { | ||
| callback(headings); | ||
| } | ||
| }); | ||
| }; | ||
|
|
There was a problem hiding this comment.
Notify when heading list becomes empty
Callback fires only when headings is truthy. Consumers never learn that the document now has zero headings.
Invoke the callback unconditionally.
🤖 Prompt for AI Agents
In packages/editor/src/core/hooks/use-editor.ts around lines 238 to 246, the
callback for heading changes is only invoked when the headings list is truthy,
so consumers are not notified when the headings list becomes empty. Modify the
code to call the callback unconditionally, passing the headings list even if it
is empty or undefined, so consumers are always informed of the current heading
state.
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
web/core/components/pages/editor/toolbar/options-dropdown.tsx (1)
70-79: Handle copy-to-clipboard failures
copyTextToClipboardreturns a promise but any rejection is silently ignored. Surfacing the error helps debugging and gives the user feedback:- copyTextToClipboard(editorRef.getMarkDown()).then(() => - setToast({ - type: TOAST_TYPE.SUCCESS, - title: "Success!", - message: "Markdown copied to clipboard.", - }) - ); + copyTextToClipboard(editorRef.getMarkDown()) + .then(() => + setToast({ + type: TOAST_TYPE.SUCCESS, + title: "Success!", + message: "Markdown copied to clipboard.", + }), + ) + .catch(() => + setToast({ + type: TOAST_TYPE.ERROR, + title: "Failed!", + message: "Unable to copy markdown to clipboard.", + }), + );
♻️ Duplicate comments (4)
packages/editor/src/core/helpers/assets.ts (1)
13-36: Key-name mismatch withnode.type.namestill unfixed
(previously reported)The record is keyed by enum values (
IMAGE,CUSTOM_IMAGE) but elsewhere look-ups are done withnode.type.name("image","customImage").
Unless every caller maps the case manually (they currently don’t, see file-restore/delete plugins), asset discovery will silently fail.Fix options:
- Normalise on access:
const key = node.type.name.toUpperCase() as keyof typeof CORE_ASSETS_META_DATA_RECORD; CORE_ASSETS_META_DATA_RECORD[key]?.(attrs)
- Store the record with the actual node names.
Please address globally.
web/core/components/pages/navigation-pane/tab-panels/assets.tsx (1)
65-90: Nested anchors are still present – invalid HTML & accessibility issue
The outer<a>(href={asset.scrollId}) wraps another<a>used for downloads. This was flagged in the previous review and has not been addressed.Convert the wrapper to a non-anchor element (
<div>or<button>with role="link") and keep the download link as the only anchor.packages/editor/src/core/hooks/use-editor.ts (2)
194-204:insertTextstill bails when the selection is emptyUsers expect pasted/inserted content to appear at the caret even when nothing is selected.
Re-apply the previously suggested fallback:- if (empty) return; + if (empty) { + editor.chain().focus().insertContent(contentHTML).run(); + return; + }
238-245: Heading-change callback not fired for empty listThe callback is only invoked when
headingsis truthy, so consumers never learn that the document now has zero headings.Invoke the callback unconditionally:
- if (headings) { - callback(headings); - } + callback(headings ?? []);
🧹 Nitpick comments (5)
packages/editor/src/core/hooks/use-collaborative-editor.ts (1)
16-17: Dependency awareness
onAssetChangeis forwarded touseEditor, but it’s not memo-ised. If callers pass a new lambda on every render, it could force unnecessary re-subscriptions insideuseEditor. Recommend wrapping inuseCallbackat call-site or documenting this expectation.packages/editor/src/core/extensions/custom-image/components/image-uploader.tsx (1)
177-178: Missingeditorin dependency array
useCallbackdepends oneditor, but onlyeditor.isEditableis listed.
While the instance is usually stable, eslint will still warn and future refactors might break the assumption.-}, [draggedInside, editor.isEditable, failedToLoadImage, isImageBeingUploaded]); +}, [draggedInside, editor, failedToLoadImage, isImageBeingUploaded]);web/core/store/pages/page-editor-info.ts (1)
30-40: RedundantrunInAction– action decorators already batch
setEditorRef/updateAssetsListare declared as MobX actions via the decorator, so wrapping the body in an extrarunInActionis superfluous.- setEditorRef: ... = (editorRef) => { - runInAction(() => { - this.editorRef = editorRef; - }); - }; + setEditorRef: ... = (editorRef) => { + this.editorRef = editorRef; + };Same for
updateAssetsList.web/core/store/pages/base-page.ts (1)
104-107: Provide a cleanup hook for the neweditorsub-store
PageEditorInstanceis created but never torn down.
If the class later adds RxJS/MobX reactions or event listeners, the parentBasePage.cleanup()won’t dispose them, leading to memory leaks when pages are unmounted or swapped.+ cleanup = () => { + this.disposers.forEach((disposer) => disposer()); + + // optional — call a disposer on the sub-store once it exists + if ("cleanup" in this.editor && typeof this.editor.cleanup === "function") { + this.editor.cleanup(); + } + };Adding a pass-through now prevents future surprises and keeps ownership boundaries clear.
web/core/components/pages/navigation-pane/tab-panels/assets.tsx (1)
34-62: Duplicate URL-builder logic – consolidate to avoid drift
getAssetSrcandgetAssetDownloadSrccontain identical branching logic differing only in the helper they call. Reduce duplication:-const buildSrc = ( - path: string, - builder: typeof getEditorAssetSrc | typeof getEditorAssetDownloadSrc -) => { +const buildSrc = ( + path: string, + builder: typeof getEditorAssetSrc | typeof getEditorAssetDownloadSrc +) => { if (!path || !workspaceSlug) return ""; if (path.startsWith("http")) return path; return ( builder({ assetId: path, projectId: project_ids?.[0], workspaceSlug: workspaceSlug.toString(), }) ?? "" ); }; -const getAssetSrc = (path: string) => buildSrc(path, getEditorAssetSrc); -const getAssetDownloadSrc = (path: string) => - buildSrc(path, getEditorAssetDownloadSrc); +const getAssetSrc = (path: string) => buildSrc(path, getEditorAssetSrc); +const getAssetDownloadSrc = (path: string) => + buildSrc(path, getEditorAssetDownloadSrc);This keeps the two paths in sync and makes future changes less error-prone.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
packages/editor/src/core/components/editors/document/collaborative-editor.tsx(2 hunks)packages/editor/src/core/extensions/custom-image/components/image-uploader.tsx(3 hunks)packages/editor/src/core/extensions/utility.ts(3 hunks)packages/editor/src/core/helpers/assets.ts(1 hunks)packages/editor/src/core/helpers/editor-ref.ts(1 hunks)packages/editor/src/core/hooks/use-collaborative-editor.ts(2 hunks)packages/editor/src/core/hooks/use-editor.ts(7 hunks)packages/editor/src/core/plugins/file/delete.ts(1 hunks)packages/editor/src/core/plugins/file/restore.ts(2 hunks)packages/editor/src/core/types/collaboration.ts(2 hunks)packages/editor/src/core/types/editor.ts(3 hunks)packages/editor/src/index.ts(0 hunks)web/core/components/pages/editor/editor-body.tsx(7 hunks)web/core/components/pages/editor/page-root.tsx(6 hunks)web/core/components/pages/editor/toolbar/options-dropdown.tsx(1 hunks)web/core/components/pages/editor/toolbar/root.tsx(2 hunks)web/core/components/pages/navigation-pane/tab-panels/assets.tsx(1 hunks)web/core/components/pages/navigation-pane/tab-panels/info/document-info.tsx(1 hunks)web/core/components/pages/navigation-pane/tab-panels/outline.tsx(1 hunks)web/core/store/pages/base-page.ts(4 hunks)web/core/store/pages/page-editor-info.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/editor/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- web/core/components/pages/navigation-pane/tab-panels/info/document-info.tsx
- web/core/components/pages/navigation-pane/tab-panels/outline.tsx
- packages/editor/src/core/helpers/editor-ref.ts
- web/core/components/pages/editor/page-root.tsx
- web/core/components/pages/editor/editor-body.tsx
- packages/editor/src/core/types/editor.ts
- web/core/components/pages/editor/toolbar/root.tsx
🧰 Additional context used
🧬 Code Graph Analysis (8)
packages/editor/src/core/types/collaboration.ts (1)
packages/editor/src/core/types/editor.ts (1)
TEditorAsset(82-89)
packages/editor/src/core/plugins/file/restore.ts (1)
packages/editor/src/core/helpers/assets.ts (1)
CORE_ASSETS_META_DATA_RECORD(13-37)
packages/editor/src/core/extensions/custom-image/components/image-uploader.tsx (1)
packages/editor/src/core/helpers/assets.ts (1)
CORE_ASSETS_META_DATA_RECORD(13-37)
packages/editor/src/core/helpers/assets.ts (3)
packages/editor/src/core/types/editor.ts (1)
TEditorAsset(82-89)packages/editor/src/core/extensions/custom-image/components/image-block.tsx (1)
getImageBlockId(41-41)packages/editor/src/ce/constants/assets.ts (1)
ADDITIONAL_ASSETS_META_DATA_RECORD(6-6)
packages/editor/src/core/extensions/utility.ts (2)
packages/editor/src/core/types/config.ts (1)
TFileHandler(7-19)packages/editor/src/core/types/editor.ts (1)
TEditorAsset(82-89)
web/core/store/pages/page-editor-info.ts (1)
packages/editor/src/core/types/editor.ts (2)
TEditorAsset(82-89)EditorRefApi(112-129)
web/core/components/pages/navigation-pane/tab-panels/assets.tsx (7)
web/core/store/pages/base-page.ts (1)
TPageInstance(68-71)packages/editor/src/core/types/editor.ts (1)
TEditorAsset(82-89)packages/i18n/src/hooks/use-translation.ts (1)
useTranslation(23-35)web/core/store/router.store.ts (1)
workspaceSlug(69-71)web/helpers/editor.helper.ts (2)
getEditorAssetSrc(14-23)getEditorAssetDownloadSrc(29-38)packages/utils/src/attachment.ts (1)
convertBytesToSize(22-32)web/core/hooks/use-resolved-asset-path.tsx (1)
useResolvedAssetPath(10-26)
web/core/store/pages/base-page.ts (1)
web/core/store/pages/page-editor-info.ts (1)
PageEditorInstance(14-41)
🔇 Additional comments (6)
packages/editor/src/core/types/collaboration.ts (1)
38-38: 👍 NewonAssetChangeprop looks goodThe optional callback is clearly typed and keeps backward compatibility.
packages/editor/src/core/plugins/file/delete.ts (1)
60-63: Risk of recursive transactions insideappendTransactionInvoking
editor.commands.updateAssetsListfrom withinappendTransactiondispatches a new transaction, which ProseMirror will feed back through all plugins, including this one. IfupdateAssetsListsetsdocChangedfalse, you’re probably safe, but a future change could create an infinite loop.Safer pattern: build and return a transaction instead of dispatching, or add a guard meta flag:
editor.commands.updateAssetsList?.({ idToRemove: node.attrs.id, + meta: { skipAssetPlugin: true }, });and skip when that meta is present.
Consider revisiting the command implementation to eliminate dispatch from plugin callbacks.
packages/editor/src/core/plugins/file/restore.ts (1)
48-53: Same recursion caveat as delete plugin
editor.commands.updateAssetsListis dispatched fromappendTransaction, carrying the same recursion / performance risk highlighted for the delete plugin. Consider the same mitigation (returning a transaction or flagging with meta).packages/editor/src/core/components/editors/document/collaborative-editor.tsx (1)
33-35: Prop drilling looks correct – double-check interface update
onAssetChangeand the (re-added)onTransactionare successfully threaded through touseCollaborativeEditor.
Please make sureICollaborativeDocumentEditorhas been updated so both callbacks are declared as optional and properly typed – otherwise TS consumers will wobble.Also applies to: 64-66
packages/editor/src/core/hooks/use-editor.ts (2)
139-151: Missed empty-state notification & unnecessary re-renders in asset change effect
if (!assets || !onAssetChange) return;skips the callback whenassetsis an empty array, hiding “no assets” transitions.useEditorStatereturns a new object each time; comparingassetsList?.assetsby reference may trigger theuseEffecteven when contents are unchanged.- const assets = assetsList?.assets; - if (!assets || !onAssetChange) return; - onAssetChange(assets); + const assets = assetsList?.assets ?? []; + if (!onAssetChange) return; + onAssetChange(assets);Consider using a stable comparison (e.g., hashing or
lodash/isEqual) insideselectorto avoid spurious updates.
217-218: 👍 Guard clause implemented
listenToRealTimeUpdatenow safely returnsundefinedwhenprovideris absent, preventing runtime errors flagged earlier.
packages/editor/src/core/extensions/custom-image/components/image-uploader.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
space/styles/globals.css (1)
370-370: Global smooth scrolling may conflict with user preferences
Applyingscroll-behavior: smooth;on*overridesprefers-reduced-motionand can cause unexpected behavior in nested scroll containers. Consider scoping it tohtmland respecting motion-reduction preferences:/* remove from universal selector */ * { /* … */ /* scroll-behavior: smooth; */ } /* add scoped, accessible smooth scrolling */ @media (prefers-reduced-motion: no-preference) { html { scroll-behavior: smooth; } } @media (prefers-reduced-motion: reduce) { html { scroll-behavior: auto; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
admin/styles/globals.css(1 hunks)space/styles/globals.css(2 hunks)web/core/components/pages/editor/editor-body.tsx(7 hunks)web/core/components/pages/navigation-pane/root.tsx(1 hunks)web/core/components/pages/navigation-pane/tab-panels/assets.tsx(1 hunks)web/core/components/pages/navigation-pane/tab-panels/info/version-history.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- admin/styles/globals.css
🚧 Files skipped from review as they are similar to previous changes (4)
- web/core/components/pages/editor/editor-body.tsx
- web/core/components/pages/navigation-pane/root.tsx
- web/core/components/pages/navigation-pane/tab-panels/assets.tsx
- web/core/components/pages/navigation-pane/tab-panels/info/version-history.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
space/styles/globals.css (1)
50-75: Reformatted shadow variable declarations
The move from single‐line to multi‐line for--color-shadow-*enhances readability without altering values. Indentation and trailing commas appear consistent and valid. No functional impact detected.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
web/core/components/pages/navigation-pane/tab-panels/assets.tsx (1)
66-91: Nested<a>elements – invalid & inaccessible HTMLThe outer anchor (
<a href={asset.href}>) wraps another anchor (<a href={getAssetDownloadSrc(...)}), re-introducing the exact issue called out in the previous review: nested anchors are not allowed and break keyboard / screen-reader semantics.
Convert the outer anchor into a non-anchor element (<div>or<button>withrole="link"+ click handler) so that only one interactive anchor remains.- return ( - <a - href={asset.href} - className="relative group/asset-item h-12 flex items-center gap-2 pr-2 rounded border border-custom-border-200 hover:bg-custom-background-80 transition-colors" - > + return ( + <div + role="link" + tabIndex={0} + onClick={() => document.querySelector(asset.href)?.scrollIntoView({ behavior: "smooth" })} + className="relative group/asset-item h-12 flex items-center gap-2 pr-2 rounded border border-custom-border-200 hover:bg-custom-background-80 transition-colors" + > ... - </a> + </div>
🧹 Nitpick comments (2)
web/core/components/pages/navigation-pane/tab-panels/assets.tsx (2)
34-61: Duplicate URL-resolution logic – extract helper to reduce noise
getAssetSrcandgetAssetDownloadSrcare identical except for the wrapped util. Consider a single factory/helper to keep the component lean:- const getAssetSrc = (path: string) => { ... } - const getAssetDownloadSrc = (path: string) => { ... } + const resolveSrc = ( + path: string, + resolver: typeof getEditorAssetSrc | typeof getEditorAssetDownloadSrc, + ) => { + if (!path || !workspaceSlug) return ""; + return path.startsWith("http") + ? path + : resolver({ + assetId: path, + projectId: project_ids?.[0], + workspaceSlug: workspaceSlug.toString(), + }) ?? ""; + };This removes duplication and prevents future drift between the two implementations.
81-83: Adddownloadattribute for explicit download behaviourBrowsers will attempt to navigate if the
Content-Dispositionheader is missing. Addingdownloadhints the intent and improves UX:- <a + <a + download
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/editor/src/ce/types/asset.ts(1 hunks)packages/editor/src/core/helpers/assets.ts(1 hunks)packages/editor/src/core/types/asset.ts(1 hunks)packages/editor/src/core/types/editor.ts(4 hunks)packages/editor/src/core/types/index.ts(1 hunks)packages/editor/src/index.ts(1 hunks)web/core/components/pages/navigation-pane/tab-panels/assets.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- packages/editor/src/ce/types/asset.ts
- packages/editor/src/core/types/index.ts
- packages/editor/src/core/types/asset.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/editor/src/index.ts
- packages/editor/src/core/helpers/assets.ts
- packages/editor/src/core/types/editor.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
web/core/components/pages/navigation-pane/tab-panels/assets.tsx (6)
web/core/store/pages/base-page.ts (1)
TPageInstance(68-71)packages/editor/src/core/types/asset.ts (1)
TEditorAsset(14-14)packages/i18n/src/hooks/use-translation.ts (1)
useTranslation(23-35)packages/utils/src/editor.ts (1)
getEditorAssetDownloadSrc(29-38)web/ce/components/pages/navigation-pane/tab-panels/assets.tsx (1)
AdditionalPageNavigationPaneAssetItem(11-11)web/core/hooks/use-resolved-asset-path.tsx (1)
useResolvedAssetPath(10-26)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
| @@ -140,27 +135,26 @@ export const useEditor = (props: CustomEditorProps) => { | |||
| editor.commands.updateAssetsUploadStatus?.(assetsUploadStatus); | |||
| }, [editor, fileHandler.assetsUploadStatus]); | |||
|
|
|||
| // subscribe to assets list changes | |||
| const assetsList = useEditorState({ | |||
There was a problem hiding this comment.
let's render this in a different component since it'll rerender the entire editor
Description
This PR includes the implementation of a new navigation pane for pages consisting of a wide range of information of the current page, including-
Type of Change
Media
Screen.Recording.2025-06-16.at.19.54.46.mov
Fixes
#7169
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Refactor
Documentation
Style