migrate to react 19 #2272
migrate to react 19 #2272
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughWidespread type and nullability refinements across the frontend: many Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
tsconfig.json (1)
2-2: Fix likely typo in include globs: "emain//" → "main/**/".**This will exclude your Electron main-process sources from type-checking if the real path is "main/**". Please correct the include pattern.
Apply this diff:
- "include": ["frontend/**/*", "emain/**/*"], + "include": ["frontend/**/*", "main/**/*"],frontend/app/view/term/ijson.tsx (1)
15-31: Return type should allow null; prefer React.ReactNode for flexibilityconvertNodeToTag returns null on Line 17, but the signature excludes it. React.ReactNode also naturally covers strings, numbers, and fragments, reducing future friction.
Apply:
-function convertNodeToTag(node: IJsonNode | string, idx?: number): React.ReactElement | string { +function convertNodeToTag(node: IJsonNode | string, idx?: number): React.ReactNode {frontend/app/modals/userinputmodal.tsx (1)
118-118: Fix extra brace in htmlFor — label association is brokenThe label’s htmlFor has an extra } which breaks association and accessibility.
- <label htmlFor={`uicheckbox-${userInputRequest.requestid}}`}>{userInputRequest.checkboxmsg}</label> + <label htmlFor={`uicheckbox-${userInputRequest.requestid}`}>{userInputRequest.checkboxmsg}</label>frontend/app/element/popover.tsx (1)
126-137: Bug:asprop is destructured but never used (polymorphic rendering broken).
as: Component = "button"is pulled out but not passed to<Button>, so consumers can’t change the underlying element. Either pass it through or dropasfrom this component’s props.Apply this diff to forward
as:return ( <Button ref={ref} className={clsx("popover-button", className, { "is-active": isActive })} {...props} // Spread the rest of the props {...restReferenceProps} // Spread referenceProps without onClick + as={Component} onClick={combinedOnClick} // Assign combined onClick after spreading >Also applies to: 154-164
frontend/app/view/vdom/vdom.tsx (1)
172-180: Type-return mismatch: function returnsnullbut signature excludes it.
convertElemToTagreturnsnull(Line 174) while its signature isReact.ReactElement | string. This will either fail type-checking or mask the intent.Apply this diff:
-function convertElemToTag(elem: VDomElem, model: VDomModel): React.ReactElement | string { +function convertElemToTag(elem: VDomElem, model: VDomModel): React.ReactNode {frontend/app/element/expandablemenu.tsx (2)
112-120: Type bug:useRef<string>(null)is invalid; allownullin the ref type.Also assert non-null when reading after initialization to satisfy TS narrowing.
Apply this diff:
-const idRef = useRef<string>(null); +const idRef = useRef<string | null>(null); @@ -const id = idRef.current; +const id = idRef.current!;
4-4: Incorrectclsximport form.
clsxis a default export. Named import will be undefined in production builds.Apply this diff:
-import { clsx } from "clsx"; +import clsx from "clsx";
🧹 Nitpick comments (21)
tsconfig.json (1)
4-4: Align target/lib and resolver with modern Vite + React 19.Not blocking, but recommended for consistency and better typings:
- Use a more recent target (ES2020+). You already output ES2020 modules.
- Prefer "bundler" moduleResolution with Vite 6 to match its resolver.
- Update "lib" to match the chosen target (e.g., "es2020" or newer).
- Consider re-enabling stricter checks (at least strictNullChecks) now that nullability is explicitly modeled across refs.
Proposed diff:
- "target": "es6", + "target": "es2020", @@ - "moduleResolution": "node", + "moduleResolution": "bundler", @@ - "lib": ["dom", "dom.iterable", "es6"], + "lib": ["dom", "dom.iterable", "es2020"], @@ - "strict": false, + "strict": false, // consider enabling strictNullChecks at minimumIf you want stricter null-safety without going full strict, add:
"strictNullChecks": trueAlso applies to: 10-10, 27-30
frontend/app/modals/modalsrenderer.tsx (1)
30-32: Avoid effect churn by depending on stable booleans, not a freshly created array.Because rtn is a new array every render, this effect runs on every render and may spam globalStore.set. Depend on modals/tosOpen (or a derived boolean) instead.
Apply this diff:
- useEffect(() => { - globalStore.set(atoms.modalOpen, rtn.length > 0); - }, [rtn]); + const anyOpen = modals.length > 0 || tosOpen; + useEffect(() => { + globalStore.set(atoms.modalOpen, anyOpen); + }, [anyOpen]);frontend/app/tab/tab.tsx (3)
56-56: Prefer environment-agnostic timer typing.NodeJS.Timeout can conflict in DOM contexts and also requires Node ambient types. Use ReturnType and allow null for initialization.
Apply this diff:
- const editableTimeoutRef = useRef<NodeJS.Timeout>(null); + const editableTimeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null);This avoids reliance on Node typings and matches Electron renderer semantics.
153-155: Avoid calling an event handler with null; use a non-event callback.handleRenameTab is typed as a MouseEventHandler but is invoked manually with null. Extract a non-event function and call it from both places.
Suggested refactor (outside this hunk):
const startRename = useCallback(() => { setIsEditable(true); editableTimeoutRef.current = setTimeout(() => { selectEditableText(); }, 0); }, [selectEditableText]); const handleRenameTab: React.MouseEventHandler<HTMLDivElement> = (event) => { event.stopPropagation(); startRename(); }; // ... { label: "Rename Tab", click: () => startRename() },
76-84: Null-guard selection APIs.window.getSelection() can return null. Guard before using removeAllRanges/addRange to avoid rare runtime errors.
Possible tweak:
const selection = window.getSelection(); if (!selection) return; selection.removeAllRanges(); selection.addRange(range);frontend/app/view/term/ijson.tsx (2)
47-55: Broaden children type to React.ReactNode[]childrenComps will hold results from convertNodeToTag (now ReactNode). Using React.ReactNode[] avoids unnecessary narrowing and covers numbers, fragments, etc.
- let childrenComps: (string | React.ReactElement)[] = []; + let childrenComps: React.ReactNode[] = [];
93-93: Typo in CSS reset: “sgv” → “svg”This prevents the intended display reset from applying to SVG elements.
-img, picture, video, canvas, sgv { display: block; } +img, picture, video, canvas, svg { display: block; }frontend/app/view/preview/directorypreview.tsx (1)
414-416: Null-initialized refs: add guard in onScroll and allow null in downstream prop typeGood move initializing refs with null. Two follow-ups:
- onScroll uses osRef.current without a null check; add a guard to avoid a rare early-call crash.
- TableBodyProps.osRef is non-nullable but receives osRef.current (can be null). Make it OverlayScrollbarsComponentRef | null to reflect reality.
Add a guard (outside this hunk):
const onScroll = useCallback( debounce(2, () => { const viewportTop = osRef.current?.osInstance()?.elements().viewport.scrollTop; if (viewportTop == null) return; setScrollHeight(viewportTop); }), [] );Adjust the prop type (outside this hunk):
interface TableBodyProps { // ... osRef: OverlayScrollbarsComponentRef | null; }And keep passing osRef={osRef.current} as you do now.
frontend/app/modals/userinputmodal.tsx (1)
105-122: optionalCheckbox useMemo has empty deps; may render stale contentThis block depends on userInputRequest.checkboxmsg and requestid. If the modal can receive different requests, memoize with deps; otherwise consider removing useMemo for simplicity.
Proposed update (outside this hunk):
const optionalCheckbox = useMemo(() => { if (userInputRequest.checkboxmsg == "") return <></>; return ( <div className="userinput-checkbox-container"> <div className="userinput-checkbox-row"> <input type="checkbox" id={`uicheckbox-${userInputRequest.requestid}`} className="userinput-checkbox" ref={checkboxRef} /> <label htmlFor={`uicheckbox-${userInputRequest.requestid}`}>{userInputRequest.checkboxmsg}</label> </div> </div> ); }, [userInputRequest.checkboxmsg, userInputRequest.requestid]);frontend/types/jsx.d.ts (1)
4-4: Ambient JSX runtime types inclusion verified
- The
includepattern in tsconfig.json ("frontend/**/*") already encompassesfrontend/types/jsx.d.ts, so the/// <reference types="react/jsx-runtime" />directive will be picked up by TypeScript.- You have
"jsx": "preserve"configured, which is correct for the new JSX transform.- Currently, your
"types"array only lists"vite/client"and"vite-plugin-svgr/client", meaning other ambient type packages aren’t auto-included.Consider this optional refactor to ensure editors and IDEs pick up the JSX runtime types without relying solely on the triple-slash directive:
// tsconfig.json "compilerOptions": { // … "types": [ "vite/client", "vite-plugin-svgr/client", + "react/jsx-runtime" ] }This addition makes the dependency on the JSX runtime types explicit in your tsconfig, improving consistency across tooling.
frontend/app/element/popover.tsx (3)
69-77: Avoid mutating themiddlewareprop array.Mutating props can cause surprising bugs for parents reusing the same array reference. Create a new array instead.
Apply this diff:
- middleware ??= []; - middleware.push(offsetMiddleware(offset)); + const allMiddleware = [...(middleware ?? []), offsetMiddleware(offset)];And below:
- middleware: middleware, + middleware: allMiddleware,
65-68: Redundant defaulting ofoffset.
offset = 3is already the default parameter value; the undefined check is unnecessary.Apply this diff:
- if (offset === undefined) { - offset = 3; - }
117-118: DuplicatedisplayNameassignment forPopover.
Popover.displayNameis set twice (Line 117 and again near Line 191). Keep one for clarity.Apply this diff to remove the earlier duplicate:
-Popover.displayName = "Popover";Also applies to: 191-193
frontend/app/element/button.tsx (1)
12-13: Type-only JSX migration is correct.
keyof React.JSX.IntrinsicElementsis the right target for React 19.If you plan to rely on the polymorphic
asprop broadly, consider a typed-polymorphic pattern so therefand intrinsic props match the chosen element (preventsHTMLButtonElementref types leaking onto non-button tags). I can draft a minimal, compatible refactor if you want to pursue this.frontend/app/view/sysinfo/sysinfo.tsx (1)
508-515: Optional: guard against a null ref before appending.Defensive check avoids rare timing issues in strict/concurrent mode.
Apply this diff:
-React.useEffect(() => { - containerRef.current.append(plot); +React.useEffect(() => { + if (containerRef.current) { + containerRef.current.append(plot); + }frontend/app/view/vdom/vdom.tsx (1)
34-36: Nit: consider broadeningVDomReactTagTypetoReact.ReactNode.Wave-specific tags may legitimately return strings or null in the future;
React.ReactNodegives you that flexibility. Not required for this PR since current tags return elements.frontend/app/element/expandablemenu.tsx (1)
149-156: OnClick order change acknowledged — confirm intended UX.You now call the child’s
onClickbefore toggling the group open state. If the child expects the new open state inside its handler, invert the order. Otherwise, LGTM.frontend/app/block/blockframe.tsx (2)
107-113: Broaden event type to support both div contextmenu and button click sourcescomputeEndIcons passes onContextMenu into IconButton as a click handler, which will receive React.MouseEvent. The current signature expects React.MouseEvent, which is unnecessarily narrow and can trip type checks.
Apply this focused change in the current hunk:
- onContextMenu: (e: React.MouseEvent<HTMLDivElement>) => void + onContextMenu: (e: React.MouseEvent<HTMLElement>) => void ): React.ReactElement[] {And outside this hunk, adjust the helper and callback to match:
// make the event target generic function handleHeaderContextMenu( e: React.MouseEvent<HTMLElement>, blockData: Block, viewModel: ViewModel, magnified: boolean, onMagnifyToggle: () => void, onClose: () => void ) { /* ... */ } // when creating the callback: const onContextMenu = React.useCallback( (e: React.MouseEvent<HTMLElement>) => { handleHeaderContextMenu(e, blockData, viewModel, magnified, nodeModel.toggleMagnify, nodeModel.onClose); }, [magnified] );
214-225: Prefer ReactNode for header element collections to keep flexibilityTyping headerTextElems and renderHeaderElements as React.ReactElement[] excludes valid React children (strings, numbers, fragments) and forces non-null elements. Since HeaderTextElem can render null and you also compose text, React.ReactNode[] is a better fit.
- const headerTextElems: React.ReactElement[] = []; + const headerTextElems: React.ReactNode[] = [];-function renderHeaderElements(headerTextUnion: HeaderElem[], preview: boolean): React.ReactElement[] { - const headerTextElems: React.ReactElement[] = []; +function renderHeaderElements(headerTextUnion: HeaderElem[], preview: boolean): React.ReactNode[] { + const headerTextElems: React.ReactNode[] = [];Note: No downstream changes are required where you render {headerTextElems}; React can render ReactNode[] directly.
Also applies to: 313-315
frontend/app/element/flyoutmenu.tsx (2)
17-19: Loosen custom renderer return types to ReactNodeConfining renderMenu/renderMenuItem to React.ReactElement disallows returning strings, numbers, fragments, or null (useful for conditional omission). React.ReactNode is more ergonomic and idiomatic for render props.
- renderMenu?: (subMenu: React.ReactElement, props: any) => React.ReactElement; - renderMenuItem?: (item: MenuItem, props: any) => React.ReactElement; + renderMenu?: (subMenu: React.ReactElement, props: any) => React.ReactNode; + renderMenuItem?: (item: MenuItem, props: any) => React.ReactNode;
217-219: Mirror ReactNode in SubMenuProps for consistencySame rationale as above; this keeps the public API consistent and flexible.
- renderMenu?: (subMenu: React.ReactElement, props: any) => React.ReactElement; - renderMenuItem?: (item: MenuItem, props: any) => React.ReactElement; + renderMenu?: (subMenu: React.ReactElement, props: any) => React.ReactNode; + renderMenuItem?: (item: MenuItem, props: any) => React.ReactNode;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (18)
frontend/app/block/block.tsx(1 hunks)frontend/app/block/blockframe.tsx(5 hunks)frontend/app/element/button.tsx(1 hunks)frontend/app/element/expandablemenu.tsx(2 hunks)frontend/app/element/flyoutmenu.stories.tsx(1 hunks)frontend/app/element/flyoutmenu.tsx(2 hunks)frontend/app/element/popover.tsx(1 hunks)frontend/app/modals/modalsrenderer.tsx(1 hunks)frontend/app/modals/userinputmodal.tsx(1 hunks)frontend/app/tab/tab.tsx(1 hunks)frontend/app/view/preview/directorypreview.tsx(2 hunks)frontend/app/view/sysinfo/sysinfo.tsx(2 hunks)frontend/app/view/term/ijson.tsx(2 hunks)frontend/app/view/vdom/vdom.tsx(3 hunks)frontend/layout/tests/model.ts(1 hunks)frontend/types/jsx.d.ts(1 hunks)package.json(2 hunks)tsconfig.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/app/view/vdom/vdom.tsx (2)
pkg/vdom/vdom_types.go (1)
VDomElem(26-32)frontend/app/view/vdom/vdom-model.tsx (1)
VDomModel(105-691)
frontend/app/block/blockframe.tsx (1)
frontend/app/element/iconbutton.tsx (1)
IconButton(12-34)
⏰ 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). (5)
- GitHub Check: merge-gatekeeper
- GitHub Check: Build Docsite
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (10)
frontend/app/block/block.tsx (1)
66-66: Return type switch to React.ReactElement looks good.The function only returns elements (never null/undefined/strings), so React.ReactElement is the precise type here and aligns with the React 19 typing pass.
package.json (1)
54-56: Critical: Add missing Node and React ambient types to tsconfig.jsonThe project’s tsconfig.json currently restricts global type inclusion to Vite clients only, but the codebase uses
NodeJS.Timeoutextensively (e.g. foruseRef<NodeJS.Timeout>,useState<NodeJS.Timeout>, andsetTimeout()callbacks) and relies on React’s JSX/DOM types. Without adding these, type-checking will fail.Please update
tsconfig.json(around the"types"array) as follows:"types": [ "vite/client", "vite-plugin-svgr/client", + "node", + "react", + "react-dom" ],Alternatively, you may remove the
"types"field entirely to allow TypeScript to auto-include all installed@types/*packages.Key usages needing the
nodeand React types:
- frontend/app/tab/tab.tsx (useRef<NodeJS.Timeout> at line 56)
- frontend/app/tab/updatebanner.tsx (useState<NodeJS.Timeout> at line 10)
- frontend/app/view/preview/csvview.tsx (let tid: NodeJS.Timeout at line 132)
- frontend/app/store/wshrpcutil.ts (timeoutId: NodeJS.Timeout at line 22)
- frontend/app/store/keymodel.ts (chordTimeout: NodeJS.Timeout at line 41)
- frontend/app/store/ws.ts (onOpenTimeoutId: NodeJS.Timeout at line 49)
- frontend/app/element/copybutton.tsx (useRef<NodeJS.Timeout | null> at line 17)
- frontend/layout/lib/layoutModelHooks.ts (innerRectDebounceTimeout: NodeJS.Timeout at line 76)
- emain/emain-tabview.ts (resetChordModeTimeout: NodeJS.Timeout at line 50)
- emain/updater.ts (autoCheckInterval: NodeJS.Timeout | null at line 38)
Likely an incorrect or invalid review comment.
frontend/layout/tests/model.ts (1)
10-11: pendingBackendActions type correctness verifiedThe
LayoutTreeStateinterface already declarespendingBackendActions: LayoutActionData[];and there are no occurrences of
any[]for this field. ThenewLayoutTreeStatedefault initializer (pendingBackendActions: []) is correctly inferred asLayoutActionData[]per its return type. No further changes needed.frontend/app/view/preview/directorypreview.tsx (1)
521-523: LGTM on null-initialized row refsdummyLineRef and warningBoxRef now reflect possible null on first render and match typical React 19 patterns. No further changes required here.
frontend/app/modals/userinputmodal.tsx (1)
16-16: Ref initialization to null aligns with React 19 typingThis avoids undefined in .current and simplifies null checks. Looks good.
frontend/app/element/popover.tsx (1)
123-124: Type-only JSX migration looks good.Switching to
keyof React.JSX.IntrinsicElementsaligns with React 19’s typings. No runtime impact here.frontend/app/view/sysinfo/sysinfo.tsx (1)
523-524: LGTM: explicitnullinit forosRef.This matches the broader React 19 typings migration and avoids undefined ref states.
frontend/app/block/blockframe.tsx (2)
85-92: Consistent React 19 element typing — looks goodUsing React.ReactElement for getViewIconElem aligns with the repo-wide migration and keeps return types precise. No runtime behavior change.
539-539: Null-initialized ref is the right move for React 19 typingsInitializing connBtnRef with null removes the implicit-undefined state and plays well with strict ref typing.
frontend/app/element/flyoutmenu.stories.tsx (1)
266-266: Story type aligns with component API update — looks goodrenderMenu’s parameter typed as React.ReactElement matches the component’s current signature. If you adopt the ReactNode return-type suggestion in the component, no change is needed here.
| let preIconButtonElem: React.ReactElement = null; | ||
| if (preIconButton) { | ||
| preIconButtonElem = <IconButton decl={preIconButton} className="block-frame-preicon-button" />; | ||
| } |
There was a problem hiding this comment.
Type mismatch: React.ReactElement cannot be null
preIconButtonElem is initialized with null but typed as React.ReactElement. This will fail under strict typing. Use a nullable union.
- let preIconButtonElem: React.ReactElement = null;
+ let preIconButtonElem: React.ReactElement | null = 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 preIconButtonElem: React.ReactElement = null; | |
| if (preIconButton) { | |
| preIconButtonElem = <IconButton decl={preIconButton} className="block-frame-preicon-button" />; | |
| } | |
| let preIconButtonElem: React.ReactElement | null = null; | |
| if (preIconButton) { | |
| preIconButtonElem = <IconButton decl={preIconButton} className="block-frame-preicon-button" />; | |
| } |
🤖 Prompt for AI Agents
In frontend/app/block/blockframe.tsx around lines 209 to 212, the variable
preIconButtonElem is declared as React.ReactElement but initialized to null
which violates strict typing; change its type to a nullable union (e.g.
React.ReactElement | null) or alternatively to React.ReactElement | undefined
and keep the null/undefined initialization, ensuring subsequent usage checks for
null/undefined before rendering.
| targetLen, | ||
| }: SingleLinePlotProps) { | ||
| const containerRef = React.useRef<HTMLInputElement>(); | ||
| const containerRef = React.useRef<HTMLInputElement>(null); |
There was a problem hiding this comment.
Wrong ref element type: should be HTMLDivElement, not HTMLInputElement.
The ref is attached to a <div> (Line 516), and you call .append(plot) on it. Using HTMLInputElement is incorrect and can hide real type errors.
Apply this diff:
-const containerRef = React.useRef<HTMLInputElement>(null);
+const containerRef = React.useRef<HTMLDivElement>(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.
| const containerRef = React.useRef<HTMLInputElement>(null); | |
| const containerRef = React.useRef<HTMLDivElement>(null); |
🤖 Prompt for AI Agents
In frontend/app/view/sysinfo/sysinfo.tsx around line 411, the ref is typed as
HTMLInputElement but it is attached to a div and used with .append(plot); change
the ref type to HTMLDivElement (preferably React.useRef<HTMLDivElement |
null>(null)) so the element type matches usage and avoid type errors, and update
any related null checks or usages accordingly.
frontend/app/view/vdom/vdom.tsx
Outdated
| function convertChildren(elem: VDomElem, model: VDomModel): (string | React.ReactElement)[] { | ||
| if (elem.children == null || elem.children.length == 0) { | ||
| return null; | ||
| } | ||
| let childrenComps: (string | JSX.Element)[] = []; | ||
| let childrenComps: (string | React.ReactElement)[] = []; | ||
| for (let child of elem.children) { | ||
| if (child == null) { |
There was a problem hiding this comment.
Type-return mismatch: convertChildren returns null but signature excludes it.
The function returns null at Lines 300 and 310, but the signature is (string | React.ReactElement)[]. Use React.ReactNode which naturally includes arrays, strings, elements, and null.
Apply this diff:
-function convertChildren(elem: VDomElem, model: VDomModel): (string | React.ReactElement)[] {
+function convertChildren(elem: VDomElem, model: VDomModel): React.ReactNode {
@@
- let childrenComps: (string | React.ReactElement)[] = [];
+ let childrenComps: React.ReactNode[] = [];📝 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.
| function convertChildren(elem: VDomElem, model: VDomModel): (string | React.ReactElement)[] { | |
| if (elem.children == null || elem.children.length == 0) { | |
| return null; | |
| } | |
| let childrenComps: (string | JSX.Element)[] = []; | |
| let childrenComps: (string | React.ReactElement)[] = []; | |
| for (let child of elem.children) { | |
| if (child == null) { | |
| function convertChildren(elem: VDomElem, model: VDomModel): React.ReactNode { | |
| if (elem.children == null || elem.children.length == 0) { | |
| return null; | |
| } | |
| let childrenComps: React.ReactNode[] = []; | |
| for (let child of elem.children) { | |
| if (child == null) { | |
| // ... | |
| } | |
| // ... | |
| } | |
| // ... | |
| } |
🤖 Prompt for AI Agents
In frontend/app/view/vdom/vdom.tsx around lines 298 to 304 (and the later return
at ~310), the function signature currently declares a return type of (string |
React.ReactElement)[] but the implementation returns null in some code paths;
change the return type to React.ReactNode (which already permits arrays,
strings, elements and null), update any local variables (e.g. childrenComps) to
use React.ReactNode or React.ReactNode[] as appropriate, and ensure all return
statements and the function signature consistently use React.ReactNode so null
returns are valid.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/types/vite-env.d.ts (1)
4-5: Optional: Centralize type refs via tsconfig instead of triple-slash.To reduce reliance on file inclusion order and make types explicit, consider moving these to tsconfig and dropping the triple-slash lines:
Example tsconfig change (adjust the file you actually use, e.g., frontend/tsconfig.json):
{ "compilerOptions": { "types": ["vite/client", "vite-plugin-svgr/client"] } }If you adopt this, you can remove the two reference lines from this file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
frontend/types/vite-env.d.ts(1 hunks)tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tsconfig.json
⏰ 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). (5)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Build Docsite
- GitHub Check: Build for TestDriver.ai
- GitHub Check: merge-gatekeeper
🔇 Additional comments (3)
frontend/types/vite-env.d.ts (3)
1-2: LGTM: License header is clear and correct.Apache-2.0 SPDX tag and copyright line are fine.
4-5: LGTM: Ambient Vite and SVGR client types are correctly referenced.This is the conventional way to make ImportMetaEnv and SVG React component typings available; no runtime impact.
4-5: All set: SVG typings are correctly configuredEverything checks out:
- Only one
vite-env.d.tsexists underfrontend/types/and it’s covered by thetsconfig.jsoninclude globs ("frontend/**/*").vite-plugin-svgris listed as a devDependency (^4.3.0) in the rootpackage.json.- Multiple
.svgimports in React code confirm the plugin is in use.- No custom
declare module "*.svg"definitions were found to conflict.No action needed.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
frontend/app/modals/userinputmodal.tsx (2)
105-122: optionalCheckbox memo has stale props due to empty deps; also use truthy check over == "".With an empty dependency array, label text, presence, and id won’t update if a new UserInputRequest is shown while the component stays mounted. Also prefer a truthy check to avoid surprising coercions.
Apply this diff:
- const optionalCheckbox = useMemo(() => { - if (userInputRequest.checkboxmsg == "") { + const optionalCheckbox = useMemo(() => { + if (!userInputRequest.checkboxmsg) { return <></>; } return ( <div className="userinput-checkbox-container"> <div className="userinput-checkbox-row"> <input type="checkbox" id={`uicheckbox-${userInputRequest.requestid}`} className="userinput-checkbox" ref={checkboxRef} /> <label htmlFor={`uicheckbox-${userInputRequest.requestid}`}>{userInputRequest.checkboxmsg}</label> </div> </div> ); - }, []); + }, [userInputRequest.checkboxmsg, userInputRequest.requestid]);
67-79: handleKeyDown declares boolean return but returns undefined on Escape; make return consistent.Inconsistent return breaks the declared signature and can confuse wrappers that rely on a boolean. Return true on Escape and an explicit false by default.
Apply this diff:
const handleKeyDown = useCallback( (waveEvent: WaveKeyboardEvent): boolean => { if (keyutil.checkKeyPressed(waveEvent, "Escape")) { handleSendErrResponse(); - return; + return true; } if (keyutil.checkKeyPressed(waveEvent, "Enter")) { handleSubmit(); return true; } + return false; }, [handleSendErrResponse, handleSubmit] );frontend/app/element/markdown.tsx (2)
321-341: rehype-sanitize: attributes for responsive images are attached to span instead of source/img.Currently
srcset,media, andtypeare allowed onspan, whilesource/imglack the needed whitelisting. This likely strips critical attributes from<source>and breaks responsive<picture>content, while unnecessarily allowing odd attributes on<span>.Apply this diff to move/extend the attribute allow-list:
() => rehypeSanitize({ ...defaultSchema, attributes: { ...defaultSchema.attributes, - span: [ + span: [ ...(defaultSchema.attributes?.span || []), // Allow all class names starting with `hljs-`. ["className", /^hljs-./], - ["srcset"], - ["media"], - ["type"], ], + // Ensure responsive images survive sanitization. + img: [ + ...(defaultSchema.attributes?.img || []), + ["srcset"], + ["sizes"], + ], + source: [ + ...(defaultSchema.attributes?.source || []), + ["srcset"], + ["media"], + ["type"], + ["sizes"], + ], waveblock: [["blockkey"]], }, tagNames: [...(defaultSchema.tagNames || []), "span", "waveblock", "picture", "source"], }),
180-181: Fix nullability on state type.
useState<string>(null)violates strict null checks and can break TS builds. Use a nullable union.- const [resolvedStr, setResolvedStr] = useState<string>(null); + const [resolvedStr, setResolvedStr] = useState<string | null>(null);
🧹 Nitpick comments (11)
frontend/app/modals/userinputmodal.tsx (3)
16-16: Null-initialized ref looks good; tighten the type for clarity.Using a null-initialized ref is the right call for React 19. Consider reflecting nullability in the generic to make the intent explicit and keep TS happy in strict mode.
Apply this diff:
- const checkboxRef = useRef<HTMLInputElement>(null); + const checkboxRef = useRef<HTMLInputElement | null>(null);
18-27: Cleanup hook deps: remove unused dep and include the callback used in the effect.
- handleSendErrResponse doesn’t use responseText, so it shouldn’t be in the deps array.
- The timeout effect calls handleSendErrResponse but doesn’t list it as a dependency.
This reduces unnecessary re-creations and aligns with exhaustive-deps.
Apply these diffs:
@@ - }, [responseText, userInputRequest]); + }, [userInputRequest]);@@ - }, [countdown]); + }, [countdown, handleSendErrResponse]);Also applies to: 124-136
100-101: Avoid per-render wrappers in JSX props.You can pass stable function references directly to reduce needless allocations and ease memoization.
Apply this diff:
- onKeyDown={(e) => keyutil.keydownWrapper(handleKeyDown)(e)} + onKeyDown={keyutil.keydownWrapper(handleKeyDown)}- onOk={() => handleSubmit()} - onCancel={() => handleNegativeResponse()} - onClose={() => handleSendErrResponse()} + onOk={handleSubmit} + onCancel={handleNegativeResponse} + onClose={handleSendErrResponse}Also applies to: 151-153
frontend/app/element/markdown.tsx (3)
126-135: Update useEffect deps to include resolveOpts.If
resolveOptschanges (e.g., base URL), the srcset won’t re-resolve. Add it to the dependency list.- }, [props.srcSet]); + }, [props.srcSet, resolveOpts]);
183-209: Same dependency issue in MarkdownImg.Re-resolve when
resolveOptschanges.- }, [props.src, props.srcSet]); + }, [props.src, props.srcSet, resolveOpts]);
35-42: Guard against missing href in Link click handler.If
hrefis undefined,startsWithwill throw. Low probability with ReactMarkdown, but trivial to harden.- const onClick = (e: React.MouseEvent) => { - e.preventDefault(); - if (props.href.startsWith("#")) { + const onClick = (e: React.MouseEvent) => { + if (!props.href) { + return; + } + e.preventDefault(); + if (props.href.startsWith("#")) { setFocusedHeading(props.href); } else { openLink(props.href); } };frontend/app/element/linkbutton.tsx (1)
23-29: Optional: add rel when target="_blank".If consumers pass
target="_blank"via{...rest}, we should defensively addrel="noopener noreferrer"to avoid tab-nabbing. You can merge it conditionally without breaking existingrel.-const LinkButton = ({ children, className, ...rest }: LinkButtonProps) => { +const LinkButton = ({ children, className, rel, target, ...rest }: LinkButtonProps) => { + const safeRel = target === "_blank" ? clsx("noopener", "noreferrer", rel) : rel; return ( - <a {...rest} className={clsx("button grey solid link-button", className)}> + <a {...rest} target={target} rel={safeRel} className={clsx("button grey solid link-button", className)}> <span className="button-inner">{children}</span> </a> ); };frontend/app/element/windowdrag.tsx (1)
15-21: Optional: give the forwarded component a displayName for better DevTools traces.Add after the component definition:
WindowDrag.displayName = "WindowDrag";frontend/app/element/copybutton.tsx (1)
16-18: Use browser-safe timer type for refs.
NodeJS.Timeoutcan cause TS friction in DOM builds. PreferReturnType<typeof setTimeout>.- const timeoutRef = useRef<NodeJS.Timeout | null>(null); + const timeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null);frontend/app/view/preview/csvview.tsx (1)
196-203: Ref callback change is a no-op; consider stabilizing the callback to avoid extra mount/unmount churn.Inline arrow refs change identity every render, which can cause React to call the ref with null and then with the element again, especially noticeable on large tables when sorting or filtering. Optional tweak below keeps ref functions stable per row index.
Apply within this range:
- ref={(el) => { - rowRef.current[index] = el; - }} + ref={setRowRef(index)}And add this inside the component (outside the map) to support the above:
import { useCallback } from "react"; const setRowRef = useCallback( (idx: number) => (el: HTMLTableRowElement | null) => { rowRef.current[idx] = el; }, [] );Alternative (if rows can reorder often): store by row.id instead of index to decouple refs from ordering.
pkg/waveobj/wtypemeta.go (1)
77-82: Consider documenting expected range/default for editor font sizeNot required for this PR, but adding a MetaDataDecl (in the place where these are defined) with a sane range (e.g., 8–48) and a default would prevent out-of-range values from config/meta updates.
I can draft a MetaDataDecl entry and proposed defaults if you point me to where meta key declarations are centralized.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (14)
frontend/app/element/copybutton.tsx(1 hunks)frontend/app/element/expandablemenu.tsx(3 hunks)frontend/app/element/linkbutton.tsx(1 hunks)frontend/app/element/markdown.tsx(1 hunks)frontend/app/element/typingindicator.tsx(1 hunks)frontend/app/element/windowdrag.tsx(1 hunks)frontend/app/modals/userinputmodal.tsx(2 hunks)frontend/app/tab/tab.tsx(2 hunks)frontend/app/view/preview/csvview.tsx(2 hunks)frontend/app/view/term/ijson.tsx(2 hunks)frontend/app/view/vdom/vdom.tsx(3 hunks)frontend/types/gotypes.d.ts(1 hunks)pkg/waveobj/metaconsts.go(1 hunks)pkg/waveobj/wtypemeta.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/app/view/term/ijson.tsx
- frontend/app/element/expandablemenu.tsx
- frontend/app/tab/tab.tsx
- frontend/app/view/vdom/vdom.tsx
⏰ 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). (5)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Build Docsite
- GitHub Check: Build for TestDriver.ai
- GitHub Check: merge-gatekeeper
🔇 Additional comments (11)
frontend/app/modals/userinputmodal.tsx (1)
118-118: Correct htmlFor/id pairing.Nice fix—label now correctly targets the checkbox id. Improves a11y and click-to-toggle behavior.
frontend/app/element/markdown.tsx (1)
13-13: Interop flags verified & no legacy named clsx imports found
tsconfig.json already enables
esModuleInterop: true(line 7)allowSyntheticDefaultImports: true(line 11)
And a repo-wide search confirms there are no remaining named imports ofclsx.frontend/app/element/linkbutton.tsx (1)
4-4: Default-importing clsx is the right call.Consistent with clsx’s API and the repo-wide standardization.
frontend/app/element/windowdrag.tsx (1)
4-4: clsx default import LGTM.frontend/app/element/typingindicator.tsx (1)
4-4: clsx default import LGTM.frontend/app/element/copybutton.tsx (1)
4-4: clsx default import LGTM.frontend/app/view/preview/csvview.tsx (1)
12-12: Approve default import for clsx – configuration verified
tsconfig.json has bothesModuleInteropandallowSyntheticDefaultImportsenabled, and there are no named-import occurrences ofclsxin thefrontend/directory. LGTM.pkg/waveobj/metaconsts.go (1)
75-81: New editor font-size meta key is consistent and well-scopedAdding MetaKey_EditorFontSize = "editor:fontsize" fits the existing naming scheme and aligns with corresponding Go and TS changes. No issues spotted.
frontend/types/gotypes.d.ts (1)
558-564: Resolved – “editor:fontsize” is wired through to the editor widgetI’ve verified that the new
editor:fontsizemeta key is:
- Read from overrides via
useOverrideConfigAtom(blockId, "editor:fontsize")in
frontend/app/view/codeeditor/codeeditor.tsx(lines 123–126)- Clamped with
boundNumber(..., 6, 64)and assigned toopts.fontSize(line 175)- Supplied to the Monaco
<Editor>via itsoptionsprop in the same fileAdditionally, the preview pane (
frontend/app/view/preview/preview.tsx, lines 689–692 & 729–733) correctly readseditor:fontsizefrom block metadata, displays the current value, and writes updates back viaRpcApi.SetMetaCommand. No further changes are needed here.pkg/waveobj/wtypemeta.go (2)
77-82: Server-side meta struct addition looks good; float64 type makes senseThe new EditorFontSize float64 with json:"editor:fontsize,omitempty" matches the TS number type and mirrors MarkdownFontSize (float64). This allows fractional sizes, which is useful for editors. No functional concerns.
77-82: Zero-value semantics safe forEditorFontSize
I ran a search across persisted metadata (including all test fixtures) for any explicit"editor:fontsize": 0entries and found none. Since no wave files have ever relied on a zero font‐size override, usingfloat64withomitemptyhere is safe and requires no pointer indirection.
1 participant
No description provided.