[WIKI-650] fix: pane extensions close method moved into hook#7823
[WIKI-650] fix: pane extensions close method moved into hook#7823
Conversation
WalkthroughHook usePagesPaneExtensions now exposes a hook-provided handleCloseNavigationPane used by PageRoot; Lite editor gains a new "lite" layout with a LiteToolbar component and an editorClassName prop; IMAGE_ITEM constant added to editor constants; tsdown.config.clean toggled to false. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant PageRoot
participant Hook as usePagesPaneExtensions
participant Router
User->>PageRoot: Click "Close Navigation Pane"
PageRoot->>Hook: handleCloseNavigationPane()
Note right of Hook #d4f1d4: Hook removes pane query params
Hook->>Router: router.push(updated URL)
Router-->>User: URL updated (pane closed)
sequenceDiagram
autonumber
actor User
participant LiteToolbar
participant LiteEditor
participant EditorCore
User->>LiteToolbar: Click "Image"
LiteToolbar->>EditorCore: executeCommand(IMAGE_ITEM)
EditorCore-->>LiteEditor: insert image node
User->>LiteToolbar: Click "Submit"
alt Disabled (isEmpty or isSubmitting)
LiteToolbar-->>User: No action (button disabled)
else
LiteToolbar->>LiteEditor: onSubmit(event)
LiteEditor->>EditorCore: handle submit
EditorCore-->>User: Submit completes
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ 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. Comment |
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
1 similar comment
|
Linked to Plane Work Item(s) This comment was auto-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: 1
🧹 Nitpick comments (6)
apps/web/core/constants/editor.ts (1)
170-173: Single source of truth for toolbar itemsCOMPLEX_ITEMS now references IMAGE_ITEM here, but a similar list exists in packages/editor/src/core/constants/common.ts. Consider centralizing IMAGE_ITEM in the shared editor package to prevent drift.
Would you like a follow‑up PR to lift IMAGE_ITEM into packages/editor and re‑export it here?
apps/web/ce/hooks/pages/use-pages-pane-extensions.ts (1)
47-53: Good: centralized close handler clears both tab and versionMoving close logic into the hook and removing both pane params is the right direction.
Use replace to avoid polluting history for transient UI state:
- router.push(updatedRoute); + router.replace(updatedRoute);apps/web/core/components/pages/editor/page-root.tsx (1)
16-16: Remove unused router import/variableAfter delegating close/open to the hook, router isn’t used here.
-import { useAppRouter } from "@/hooks/use-app-router"; @@ - // router - const router = useAppRouter();Also applies to: 51-51
apps/web/core/components/editor/lite-text/lite-toolbar.tsx (2)
15-21: Add accessible labels to icon‑only buttonsButtons lack accessible text. Add aria‑labels (and optional titles) for a11y.
- <button - onClick={() => executeCommand(IMAGE_ITEM)} - type="button" - className="p-1 text-custom-text-300 hover:text-custom-text-200 transition-colors" - > + <button + onClick={() => executeCommand(IMAGE_ITEM)} + type="button" + aria-label="Insert image" + title="Insert image" + className="p-1 text-custom-text-300 hover:text-custom-text-200 transition-colors" + >
22-29: Expose disabled state to assistive tech; optional icon consistency
- Mirror disabled to aria-disabled.
- Optional: consider using the Image icon for consistency with IMAGE_ITEM.
- <button + <button type="button" onClick={(e) => onSubmit(e)} - disabled={isEmpty || isSubmitting} + disabled={isEmpty || isSubmitting} + aria-disabled={isEmpty || isSubmitting} className="p-1 bg-custom-primary-100 hover:bg-custom-primary-200 disabled:bg-custom-text-400 disabled:text-custom-text-200 text-custom-text-100 rounded transition-colors" > - <ArrowUp className="size-3" /> + <ArrowUp className="size-3" /> </button>apps/web/core/components/editor/lite-text/editor.tsx (1)
151-156: Guard spreading of extraProps to avoid runtime TypeErrorSpreading undefined in an object literal throws. Use nullish coalescing.
- editorRef?.executeMenuItemCommand({ - itemKey: item.itemKey, - ...item.extraProps, - }); + editorRef?.executeMenuItemCommand({ + itemKey: item.itemKey, + ...(item.extraProps ?? {}), + });Apply the same change in the full toolbar block below.
Also applies to: 176-180
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/web/ce/hooks/pages/use-pages-pane-extensions.ts(2 hunks)apps/web/core/components/editor/lite-text/editor.tsx(4 hunks)apps/web/core/components/editor/lite-text/lite-toolbar.tsx(1 hunks)apps/web/core/components/pages/editor/page-root.tsx(2 hunks)apps/web/core/constants/editor.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
apps/web/ce/hooks/pages/use-pages-pane-extensions.ts (1)
apps/web/core/components/pages/navigation-pane/index.ts (2)
PAGE_NAVIGATION_PANE_TABS_QUERY_PARAM(9-9)PAGE_NAVIGATION_PANE_VERSION_QUERY_PARAM(10-10)
apps/web/core/constants/editor.ts (1)
packages/editor/src/core/constants/common.ts (1)
COMPLEX_ITEMS(157-160)
apps/web/core/components/editor/lite-text/lite-toolbar.tsx (1)
apps/web/core/constants/editor.ts (2)
ToolbarMenuItem(38-46)IMAGE_ITEM(161-168)
apps/web/core/components/editor/lite-text/editor.tsx (2)
apps/space/helpers/editor.helper.ts (1)
getEditorFileHandlers(28-65)apps/web/core/components/editor/lite-text/lite-toolbar.tsx (1)
LiteToolbar(13-31)
apps/web/core/components/pages/editor/page-root.tsx (1)
apps/web/ce/hooks/pages/use-pages-pane-extensions.ts (1)
usePagesPaneExtensions(20-61)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Build and lint web apps
🔇 Additional comments (3)
apps/web/core/components/pages/editor/page-root.tsx (1)
81-92: Hook integration looks correctProperly wiring handleCloseNavigationPane from the hook reduces duplication and keeps URL logic in one place.
apps/web/core/components/editor/lite-text/editor.tsx (1)
34-40: Prop additions look goodThe new showLiteToolbar and editorClassName props are sensible defaults and maintain backward compatibility.
apps/web/core/constants/editor.ts (1)
161-168: Prefersatisfiesinstead ofasand drop the emptyextraProps
ashides type errors — usesatisfiesto retain compile-time checks and remove the unnecessaryextraProps. Verify TypeScript >= 4.9 in this repo (check package.json or runnpx tsc --version) before applying; if older, keepasor upgrade TS.-export const IMAGE_ITEM = { - itemKey: "image", - renderKey: "image", - name: "Image", - icon: Image, - editors: ["lite", "document"], - extraProps: {}, -} as ToolbarMenuItem<"image">; +export const IMAGE_ITEM = { + itemKey: "image", + renderKey: "image", + name: "Image", + icon: Image, + editors: ["lite", "document"], +} satisfies ToolbarMenuItem<"image">;
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
1 similar comment
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/web/core/components/editor/lite-text/editor.tsx (1)
96-96: Submit stays disabled after typing — derive emptiness from live content, not initialValue
isEmptyis computed fromprops.initialValueand never updates. This breaks both Lite and Full toolbars’ submit disabled logic once the user types.Apply the following to track emptiness live and forward upstream handlers:
- const isEmpty = isCommentEmpty(props.initialValue); + const [isEditorEmpty, setIsEditorEmpty] = useState<boolean>(isCommentEmpty(props.initialValue)); + // keep in sync if initialValue changes from outside + React.useEffect(() => { + setIsEditorEmpty(isCommentEmpty(props.initialValue)); + }, [props.initialValue]); @@ - <LiteTextEditorWithRef + <LiteTextEditorWithRef ref={ref} @@ - editorClassName={editorClassName} - {...rest} + editorClassName={editorClassName} + {...rest} + onChange={(comment_json, comment_html) => { + setIsEditorEmpty(isCommentEmpty(comment_html)); + // preserve upstream handler if provided + // @ts-expect-error upstream type may differ + rest.onChange?.(comment_json, comment_html); + }} /> @@ - isEmpty={isEmpty} + isEmpty={isEditorEmpty} /> @@ - isCommentEmpty={isEmpty} + isCommentEmpty={isEditorEmpty}Optional verification (confirms callback names and payloads in your codebase):
#!/bin/bash # Find ILiteTextEditorProps/IEditorProps and onChange signature rg -nP --type=ts -C2 'interface\s+ILiteTextEditorProps|type\s+ILiteTextEditorProps|interface\s+IEditorProps|type\s+IEditorProps' rg -nP --type=ts -C2 '\bonChange\s*\(' apps packagesAlso applies to: 147-160, 184-184
🧹 Nitpick comments (5)
apps/web/core/components/editor/lite-text/editor.tsx (5)
35-36: Variant-driven layout addition looks good; minor padding nit forvariant="none"
- Solid API:
variantandeditorClassNameare clear additions.- Nit: when
variant="none", container still gets"p-3"due to!isLiteVariant. Confirm if “no toolbar” should also mean “no extra padding.”Also applies to: 38-39, 64-75, 103-103
92-98: Forwarded ref only supports object refs — function refs won’t work; use an inner ref +useImperativeHandleCurrent
isMutableRefObjectpath ignores function refs, soeditorRefcan be null even when a parent passes a callback ref. Use an inner ref and expose it.- function isMutableRefObject<T>(ref: React.ForwardedRef<T>): ref is React.MutableRefObject<T | null> { - return !!ref && typeof ref === "object" && "current" in ref; - } - // derived values - const isEmpty = isCommentEmpty(props.initialValue); - const editorRef = isMutableRefObject<EditorRefApi>(ref) ? ref.current : null; + const innerRef = React.useRef<EditorRefApi>(null); + React.useImperativeHandle(ref, () => innerRef.current as EditorRefApi | null); + // derived values + const editorRef = innerRef.current; @@ - <LiteTextEditorWithRef - ref={ref} + <LiteTextEditorWithRef + ref={innerRef}Also add imports:
-import React, { useState } from "react"; +import React, { useEffect, useRef, useState, useImperativeHandle } from "react";Also applies to: 114-116
147-160: HonorshowSubmitButtonin lite variant (keep attach action visible)Full toolbar respects
showSubmitButton; lite toolbar ignores it. Propagate and conditionally hide only the submit button.In this file:
- {isLiteVariant && editable && ( + {isLiteVariant && editable && ( <LiteToolbar + showSubmitButton={showSubmitButton} executeCommand={executeToolbarItem} onSubmit={(e) => rest.onEnterKeyPress?.(e)} isSubmitting={isSubmitting} - isEmpty={isEditorEmpty} + isEmpty={isEditorEmpty} /> )}And in apps/web/core/components/editor/lite-text/lite-toolbar.tsx:
-export const LiteToolbar = ({ onSubmit, isSubmitting, isEmpty, executeCommand }: LiteToolbarProps) => ( +export const LiteToolbar = ({ onSubmit, isSubmitting, isEmpty, executeCommand, showSubmitButton = true }: LiteToolbarProps & { showSubmitButton?: boolean }) => ( <div className="flex items-center gap-2 pb-1"> <button ...>...</button> - <button + {showSubmitButton && ( + <button type="button" onClick={(e) => onSubmit(e)} disabled={isEmpty || isSubmitting} className="p-1 bg-custom-primary-100 hover:bg-custom-primary-200 disabled:bg-custom-text-400 disabled:text-custom-text-200 text-custom-text-100 rounded transition-colors" > <ArrowUp className="size-3" /> - </button> + </button> + )} </div> );
136-143: Avoid passing an emptyextendedEditorProps={{}}Unnecessary prop churn can cause avoidable re-renders and may override editor defaults. Drop it unless you need to set something.
- extendedEditorProps={{}} editorClassName={editorClassName} {...rest}
149-156: Extract and reuse theexecuteMenuItemCommandtype and helper to remove@ts-expect-error
Definetype MenuItemArg = Parameters<EditorRefApi["executeMenuItemCommand"]>[0]; const executeToolbarItem = (item: MenuItemArg) => editorRef?.executeMenuItemCommand(item);and then replace both inline handlers in apps/web/core/components/editor/lite-text/editor.tsx (lines 149–156, 172–181) with
<LiteToolbar executeCommand={executeToolbarItem} … /> <IssueCommentToolbar executeCommand={executeToolbarItem} … />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/core/components/editor/lite-text/editor.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/core/components/editor/lite-text/editor.tsx (2)
apps/space/helpers/editor.helper.ts (1)
getEditorFileHandlers(28-65)apps/web/core/components/editor/lite-text/lite-toolbar.tsx (1)
LiteToolbar(13-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
apps/web/core/components/editor/lite-text/editor.tsx (2)
17-20: Import and comment tweak — LGTMNo issues with the new import or comment tweak.
107-109: Focus/blur gating — double-check UX withshowToolbarInitiallyYou intentionally skip focus/blur toggling when
showToolbarInitiallyis true. Confirm this matches product behavior (toolbar will remain open even after blur).
…ne#7823) * fix: pane extensions close method moved into hook * fix: editor build breaks web everytime in dev mode * fix: variant of lite text toolbar
3 participants
Description
Pane extensions close method moved into a hook
Type of Change
Summary by CodeRabbit
New Features
Improvements
Chores