feat(webui): Use DirectoryTree for file path selection (closes #1779).#1787
feat(webui): Use DirectoryTree for file path selection (closes #1779).#1787junhaoliao wants to merge 14 commits intoy-scope:mainfrom
Conversation
WalkthroughReplaces the TreeSelect-based path picker with a new DirectoryTreeSelect (Select + DirectoryTree popup), adds popup CSS, introduces utilities for flat→hierarchy conversion and centralized error handling, implements DirectoryTreePopup, and removes the SwitcherIcon component. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant UI as PathsSelectFormItem
participant DirSel as DirectoryTreeSelect
participant Popup as DirectoryTreePopup
participant Loader as loadPath / data source
participant Util as handleLoadError
Note over UI,DirSel: User opens/selects paths
UI->>DirSel: render with `treeData`, `checkedKeys`, `onCheck`, `onLoadData`, `onExpand`
DirSel->>Popup: render hierarchical nodes (via `flatToHierarchy`)
alt user expands node
Popup->>DirSel: onExpand(keys)
DirSel->>Loader: request children for expanded path (onLoadData)
Loader-->>DirSel: children data or error
alt success
DirSel->>UI: onLoadData(children)
else error
DirSel->>Util: handleLoadError(error)
end
end
alt user checks/unchecks node(s)
Popup->>DirSel: onCheck(keys, info)
DirSel->>UI: onCheck(filteredKeys)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/index.module.css(1 hunks)components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/index.tsx(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit configuration file
- Prefer
false == <expression>rather than!<expression>.
Files:
components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/index.tsx
🧠 Learnings (1)
📚 Learning: 2025-04-08T22:32:05.366Z
Learnt from: davemarco
Repo: y-scope/clp PR: 797
File: components/log-viewer-webui/client/src/components/Layout/MainLayout.tsx:2-5
Timestamp: 2025-04-08T22:32:05.366Z
Learning: In this codebase using React Router v7.4.1, components should be imported directly from "react-router" (e.g., `import { Link, Outlet } from "react-router";`) rather than from "react-router-dom" as was common in previous versions of React Router.
Applied to files:
components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/index.tsx
🧬 Code graph analysis (1)
components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/index.tsx (2)
components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/typings.ts (1)
TreeNode(25-25)components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/utils.ts (1)
getListHeight(103-103)
🪛 Biome (2.1.2)
components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/index.tsx
[error] 128-133: Static Elements should not be interactive.
To add interactivity such as a mouse or key event listener to a static element, give the element an appropriate role value.
(lint/a11y/noStaticElementInteractions)
[error] 128-133: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
(lint/a11y/useKeyWithClickEvents)
⏰ 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: package-image
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (macos-15)
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
🔇 Additional comments (5)
components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/index.module.css (1)
1-5: LGTM!The CSS styling appropriately supports full-width clickable tree titles with proper visual feedback.
components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/index.tsx (4)
17-17: LGTM!The CSS module import is correctly structured and properly used within the component.
138-148: LGTM!The resize listener correctly recalculates list height on window resize with proper cleanup. The effect runs once on mount and properly cleans up on unmount.
172-172: LGTM!The
treeTitleRenderprop correctly integrates the custom title rendering callback with TreeSelect, enabling the full-width click interaction.
91-121: Code review verified — no issues found.The
TreeNode.isLeafproperty is correctly implemented and used. Line 105 properly follows the coding guideline by usingfalse ===comparison. The click handler logic properly distinguishes between leaf and non-leaf nodes with appropriate event handling for each case.
| const renderTreeTitle = useCallback((nodeData: TreeNode) => ( | ||
| <span | ||
| className={styles["treeTitle"]} | ||
| onClick={(ev) => { | ||
| handleTitleClick(nodeData, ev); | ||
| }} | ||
| > | ||
| {nodeData.title} | ||
| </span> | ||
| ), [handleTitleClick]); |
There was a problem hiding this comment.
Add accessibility attributes for keyboard navigation.
The interactive span lacks proper accessibility support. Non-leaf nodes should be keyboard accessible and announce their interactive nature to assistive technologies.
Apply this diff to add accessibility support:
- const renderTreeTitle = useCallback((nodeData: TreeNode) => (
- <span
- className={styles["treeTitle"]}
- onClick={(ev) => {
- handleTitleClick(nodeData, ev);
- }}
- >
- {nodeData.title}
- </span>
- ), [handleTitleClick]);
+ const renderTreeTitle = useCallback((nodeData: TreeNode) => (
+ <span
+ className={styles["treeTitle"]}
+ role={nodeData.isLeaf ? undefined : "button"}
+ tabIndex={nodeData.isLeaf ? undefined : 0}
+ onClick={(ev) => {
+ handleTitleClick(nodeData, ev);
+ }}
+ onKeyDown={(ev) => {
+ if (false === nodeData.isLeaf && ("Enter" === ev.key || " " === ev.key)) {
+ ev.preventDefault();
+ handleTitleClick(nodeData, ev as unknown as React.MouseEvent);
+ }
+ }}
+ >
+ {nodeData.title}
+ </span>
+ ), [handleTitleClick]);This addresses the static analysis warnings by:
- Adding
role="button"for non-leaf nodes to announce interactivity - Adding
tabIndex={0}for keyboard navigation - Adding
onKeyDownhandler for Enter and Space keys
Based on static analysis hints from Biome.
🧰 Tools
🪛 Biome (2.1.2)
[error] 128-133: Static Elements should not be interactive.
To add interactivity such as a mouse or key event listener to a static element, give the element an appropriate role value.
(lint/a11y/noStaticElementInteractions)
[error] 128-133: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
(lint/a11y/useKeyWithClickEvents)
🤖 Prompt for AI Agents
In
components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/index.tsx
around lines 127 to 136, the interactive span lacks accessibility attributes;
update the span so that for non-leaf nodes it has role="button", tabIndex={0},
and an onKeyDown handler that listens for Enter and Space, prevents default, and
calls handleTitleClick(nodeData, ev) to match the onClick behavior; ensure the
attributes are only set for non-leaf nodes (preserve existing non-interactive
behavior for leaf nodes) and keep the existing className and onClick intact.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/index.tsx (1)
142-151: Address accessibility attributes for keyboard navigation.The interactive
<span>lacks proper accessibility support as flagged by static analysis. This concern was raised in a previous review with a comprehensive fix includingrole,tabIndex, andonKeyDownhandler for non-leaf nodes.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/index.tsx(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit configuration file
- Prefer
false == <expression>rather than!<expression>.
Files:
components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/index.tsx
🧠 Learnings (1)
📚 Learning: 2025-04-08T22:32:05.366Z
Learnt from: davemarco
Repo: y-scope/clp PR: 797
File: components/log-viewer-webui/client/src/components/Layout/MainLayout.tsx:2-5
Timestamp: 2025-04-08T22:32:05.366Z
Learning: In this codebase using React Router v7.4.1, components should be imported directly from "react-router" (e.g., `import { Link, Outlet } from "react-router";`) rather than from "react-router-dom" as was common in previous versions of React Router.
Applied to files:
components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/index.tsx
🪛 Biome (2.1.2)
components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/index.tsx
[error] 143-148: Static Elements should not be interactive.
To add interactivity such as a mouse or key event listener to a static element, give the element an appropriate role value.
(lint/a11y/noStaticElementInteractions)
[error] 143-148: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
(lint/a11y/useKeyWithClickEvents)
🔇 Additional comments (3)
components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/index.tsx (3)
1-1: LGTM on import additions.The
Reactnamespace import is needed for theReact.MouseEventtype annotation, and the CSS module import supports the new tree title styling.Also applies to: 17-17
153-163: LGTM on resize handling.The effect properly registers and cleans up the window resize listener, ensuring
listHeightstays in sync with viewport changes.
187-188: LGTM on TreeSelect integration.The
treeTitleRenderprop correctly integrates the custom renderer, enabling full-width clickable titles as intended by the PR.
| const handleTitleClick = useCallback((nodeData: TreeNode, ev: React.MouseEvent) => { | ||
| if (nodeData.isLeaf) { | ||
| // Propagate event to let TreeSelect handle selection. | ||
| return; | ||
| } | ||
| ev.stopPropagation(); | ||
|
|
||
| const nodeValue = nodeData.value; | ||
| if (expandedKeys.includes(nodeValue)) { | ||
| setExpandedKeys(expandedKeys.filter((k) => k !== nodeValue)); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| if (false === loadedPathsRef.current.has(nodeValue)) { | ||
| loadPath(nodeValue).catch((e: unknown) => { | ||
| console.error("Failed to load directory:", e); | ||
| message.error(e instanceof Error ? | ||
| e.message : | ||
| "Unknown error while loading paths"); | ||
| }); | ||
| } | ||
|
|
||
| setExpandedKeys([ | ||
| ...expandedKeys, | ||
| nodeValue, | ||
| ]); | ||
| }, [ | ||
| expandedKeys, | ||
| loadPath, | ||
| ]); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using functional state updates for robustness.
The callback uses expandedKeys directly from the closure. While the dependency array ensures the callback is recreated on changes, using functional updates provides more reliable state management, particularly if rapid clicks occur.
const nodeValue = nodeData.value;
if (expandedKeys.includes(nodeValue)) {
- setExpandedKeys(expandedKeys.filter((k) => k !== nodeValue));
+ setExpandedKeys((prev) => prev.filter((k) => k !== nodeValue));
return;
}
if (false === loadedPathsRef.current.has(nodeValue)) {
loadPath(nodeValue).catch((e: unknown) => {
console.error("Failed to load directory:", e);
message.error(e instanceof Error ?
e.message :
"Unknown error while loading paths");
});
}
- setExpandedKeys([
- ...expandedKeys,
- nodeValue,
- ]);
+ setExpandedKeys((prev) => [
+ ...prev,
+ nodeValue,
+ ]);
}, [
- expandedKeys,
loadPath,
]);This removes the need for expandedKeys in the dependency array and ensures the latest state is always used.
📝 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 handleTitleClick = useCallback((nodeData: TreeNode, ev: React.MouseEvent) => { | |
| if (nodeData.isLeaf) { | |
| // Propagate event to let TreeSelect handle selection. | |
| return; | |
| } | |
| ev.stopPropagation(); | |
| const nodeValue = nodeData.value; | |
| if (expandedKeys.includes(nodeValue)) { | |
| setExpandedKeys(expandedKeys.filter((k) => k !== nodeValue)); | |
| return; | |
| } | |
| if (false === loadedPathsRef.current.has(nodeValue)) { | |
| loadPath(nodeValue).catch((e: unknown) => { | |
| console.error("Failed to load directory:", e); | |
| message.error(e instanceof Error ? | |
| e.message : | |
| "Unknown error while loading paths"); | |
| }); | |
| } | |
| setExpandedKeys([ | |
| ...expandedKeys, | |
| nodeValue, | |
| ]); | |
| }, [ | |
| expandedKeys, | |
| loadPath, | |
| ]); | |
| const handleTitleClick = useCallback((nodeData: TreeNode, ev: React.MouseEvent) => { | |
| if (nodeData.isLeaf) { | |
| // Propagate event to let TreeSelect handle selection. | |
| return; | |
| } | |
| ev.stopPropagation(); | |
| const nodeValue = nodeData.value; | |
| if (expandedKeys.includes(nodeValue)) { | |
| setExpandedKeys((prev) => prev.filter((k) => k !== nodeValue)); | |
| return; | |
| } | |
| if (false === loadedPathsRef.current.has(nodeValue)) { | |
| loadPath(nodeValue).catch((e: unknown) => { | |
| console.error("Failed to load directory:", e); | |
| message.error(e instanceof Error ? | |
| e.message : | |
| "Unknown error while loading paths"); | |
| }); | |
| } | |
| setExpandedKeys((prev) => [ | |
| ...prev, | |
| nodeValue, | |
| ]); | |
| }, [ | |
| loadPath, | |
| ]); |
🤖 Prompt for AI Agents
In
components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/index.tsx
around lines 106 to 136, the handleTitleClick callback reads and sets
expandedKeys from the closure which can be stale under rapid interactions;
change setExpandedKeys to use the functional updater form (prev => ...) so it
always operates on the latest state, update the branches that remove or add
nodeValue to use that functional updater, and then remove expandedKeys from the
useCallback dependency array (leaving loadPath) so the callback no longer relies
on a captured expandedKeys value.
|
fixing the function length violation |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/index.tsx (2)
94-119: Use functional state updates forsetExpandedKeys.The callback captures
expandedKeysfrom the closure. Under rapid clicks, this could lead to stale state reads. Using functional updates ensures the latest state is always used.Apply this diff:
const nodeValue = nodeData.value; - if (expandedKeys.includes(nodeValue)) { - setExpandedKeys(expandedKeys.filter((k) => k !== nodeValue)); + setExpandedKeys((prev) => { + if (prev.includes(nodeValue)) { + return prev.filter((k) => k !== nodeValue); + } - return; - } - - if (false === loadedPathsRef.current.has(nodeValue)) { - loadPath(nodeValue).catch(handleLoadError); - } + if (false === loadedPathsRef.current.has(nodeValue)) { + loadPath(nodeValue).catch(handleLoadError); + } - setExpandedKeys([ - ...expandedKeys, - nodeValue, - ]); + return [ + ...prev, + nodeValue, + ]; + }); }, [ - expandedKeys, loadPath, ]);
125-134: Add accessibility attributes for keyboard navigation.The interactive span lacks
role,tabIndex, and keyboard event handlers. This prevents keyboard-only users from toggling non-leaf nodes and triggers static analysis warnings.Apply this diff to add accessibility support:
const renderTreeTitle = useCallback((nodeData: TreeNode) => ( <span className={styles["treeTitle"]} + role={nodeData.isLeaf ? undefined : "button"} + tabIndex={nodeData.isLeaf ? undefined : 0} onClick={(ev) => { handleTitleClick(nodeData, ev); }} + onKeyDown={(ev) => { + if (false === nodeData.isLeaf && ("Enter" === ev.key || " " === ev.key)) { + ev.preventDefault(); + handleTitleClick(nodeData, ev as unknown as React.MouseEvent); + } + }} > {nodeData.title} </span> ), [handleTitleClick]);
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/index.tsx(6 hunks)components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/utils.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit configuration file
- Prefer
false == <expression>rather than!<expression>.
Files:
components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/utils.tscomponents/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/index.tsx
🧠 Learnings (1)
📚 Learning: 2025-04-08T22:32:05.366Z
Learnt from: davemarco
Repo: y-scope/clp PR: 797
File: components/log-viewer-webui/client/src/components/Layout/MainLayout.tsx:2-5
Timestamp: 2025-04-08T22:32:05.366Z
Learning: In this codebase using React Router v7.4.1, components should be imported directly from "react-router" (e.g., `import { Link, Outlet } from "react-router";`) rather than from "react-router-dom" as was common in previous versions of React Router.
Applied to files:
components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/index.tsx
🪛 Biome (2.1.2)
components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/index.tsx
[error] 126-131: Static Elements should not be interactive.
To add interactivity such as a mouse or key event listener to a static element, give the element an appropriate role value.
(lint/a11y/noStaticElementInteractions)
[error] 126-131: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
(lint/a11y/useKeyWithClickEvents)
🔇 Additional comments (3)
components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/utils.ts (1)
107-112: LGTM on the centralized error handling.The implementation correctly logs the error and displays a user-friendly message, with appropriate type narrowing for
Errorinstances.components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/index.tsx (2)
78-85: LGTM on root initialization with centralized error handling.The initialization correctly loads the root path, sets expanded keys on success, and delegates error handling to the shared utility.
154-171: LGTM on TreeSelect configuration.The integration of
treeTitleRenderwith the existing props is correct, and the component properly wires up the custom title rendering.
| useEffect(() => { | ||
| const handleResize = () => { | ||
| setListHeight(getListHeight()); | ||
| }; | ||
|
|
||
| window.addEventListener("resize", handleResize); | ||
|
|
||
| return () => { | ||
| window.removeEventListener("resize", handleResize); | ||
| }; | ||
| }, []); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider debouncing the resize handler.
The resize event can fire rapidly during window resizing. While getListHeight() is lightweight, debouncing would reduce unnecessary state updates and re-renders.
Example implementation:
useEffect(() => {
+ let timeoutId: ReturnType<typeof setTimeout>;
const handleResize = () => {
- setListHeight(getListHeight());
+ clearTimeout(timeoutId);
+ timeoutId = setTimeout(() => {
+ setListHeight(getListHeight());
+ }, 150);
};
window.addEventListener("resize", handleResize);
return () => {
+ clearTimeout(timeoutId);
window.removeEventListener("resize", handleResize);
};
}, []);📝 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.
| useEffect(() => { | |
| const handleResize = () => { | |
| setListHeight(getListHeight()); | |
| }; | |
| window.addEventListener("resize", handleResize); | |
| return () => { | |
| window.removeEventListener("resize", handleResize); | |
| }; | |
| }, []); | |
| useEffect(() => { | |
| let timeoutId: ReturnType<typeof setTimeout>; | |
| const handleResize = () => { | |
| clearTimeout(timeoutId); | |
| timeoutId = setTimeout(() => { | |
| setListHeight(getListHeight()); | |
| }, 150); | |
| }; | |
| window.addEventListener("resize", handleResize); | |
| return () => { | |
| clearTimeout(timeoutId); | |
| window.removeEventListener("resize", handleResize); | |
| }; | |
| }, []); |
| /** | ||
| * Logs and displays an error message for path loading failures. | ||
| * | ||
| * @param e | ||
| */ | ||
| const handleLoadError = (e: unknown): void => { | ||
| console.error("Failed to load path:", e); | ||
| message.error(e instanceof Error ? | ||
| e.message : | ||
| "Failed to load path"); | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Complete the JSDoc parameter description.
The @param e tag lacks a description, which reduces documentation clarity.
Apply this diff:
/**
* Logs and displays an error message for path loading failures.
*
- * @param e
+ * @param e The error thrown during path loading.
*/
const handleLoadError = (e: unknown): void => {📝 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.
| /** | |
| * Logs and displays an error message for path loading failures. | |
| * | |
| * @param e | |
| */ | |
| const handleLoadError = (e: unknown): void => { | |
| console.error("Failed to load path:", e); | |
| message.error(e instanceof Error ? | |
| e.message : | |
| "Failed to load path"); | |
| }; | |
| /** | |
| * Logs and displays an error message for path loading failures. | |
| * | |
| * @param e The error thrown during path loading. | |
| */ | |
| const handleLoadError = (e: unknown): void => { | |
| console.error("Failed to load path:", e); | |
| message.error(e instanceof Error ? | |
| e.message : | |
| "Failed to load path"); | |
| }; |
🤖 Prompt for AI Agents
In
components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/utils.ts
around lines 102 to 112, the JSDoc block for handleLoadError has an incomplete
@param e tag; update the comment to describe the parameter (e.g., "@param e The
error object or value thrown during path loading") so the function documentation
clearly explains that e represents the error encountered while loading paths.
|
I think its okay for a small improvement. I think long term we should try to copy https://ant.design/components/tree#tree-demo-directory. the open/closed folders and files, I think make more intuitive. |
junhaoliao
changed the title
davemarco
left a comment
davemarco
left a comment
There was a problem hiding this comment.
I initially expected this change to build on your previous update by adding icons, but using DirectoryTree directly works well too. I wonder if we could simplify things by using a regular Select as the base component. That way, the tree and its related logic could live in a dedicated component (e.g., DirectoryTreePopup, rendered as a popup), which might help keep the select and form components simpler and reduce some function duplication.
| } | ||
|
|
||
| /** | ||
| * TreeSelect component that uses DirectoryTree for the popup. |
There was a problem hiding this comment.
flow comment on one line.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
@components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/DirectoryTreeSelect.tsx:
- Around line 72-86: The callbacks handleTreeExpand and handleDirectoryTreeCheck
currently assert React.Key[] to string[] without runtime validation; update them
to convert/validate keys to strings before passing to onExpand, onChange, and
setCheckedKeys (e.g., map each key with String(key) and ensure the resulting
array is type string[]), and handle the checked union branch similarly so
numeric keys are safely converted to strings.
- Around line 61-66: handleTreeSelectDataLoad currently awaits onDataLoad
without error handling; wrap the await in a try/catch and call the shared
handleLoadError utility on failure so rejections are handled and users get
feedback. Specifically, in the handleTreeSelectDataLoad function, after
confirming nodeValue is a string, call await onDataLoad(nodeValue) inside a try
block and in the catch pass the caught error (and any contextual info like
nodeValue) to handleLoadError from utils.ts. Ensure handleLoadError is imported
where DirectoryTreeSelect.tsx is defined and do not swallow errors silently.
- Around line 68-70: handleDirectoryTreeLoadData currently calls the async
onDataLoad without error handling and unsafely asserts node.key as string; wrap
the await onDataLoad(...) in a try/catch to log/handle errors (consistent with
handleTreeSelectDataLoad) and replace the unsafe cast with a runtime-safe
conversion/guard (e.g., validate typeof node.key === 'string' or use
String(node.key) before passing to onDataLoad) so the function neither throws
unhandled errors nor relies on an unsafe type assertion.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/DirectoryTreeSelect.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit configuration file
- Prefer
false == <expression>rather than!<expression>.
Files:
components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/DirectoryTreeSelect.tsx
🧬 Code graph analysis (1)
components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/DirectoryTreeSelect.tsx (2)
components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/typings.ts (1)
TreeNode(25-25)components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/utils.ts (2)
getListHeight(157-157)flatToHierarchy(156-156)
⏰ 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). (4)
- GitHub Check: package-image
- GitHub Check: check-generated
- GitHub Check: lint-check (macos-15)
- GitHub Check: lint-check (ubuntu-24.04)
🔇 Additional comments (5)
components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/DirectoryTreeSelect.tsx (5)
1-19: LGTM! Imports are appropriate.The imports correctly include necessary React hooks, Ant Design components, and local utilities for this component's functionality.
22-31: LGTM! Interface definition is clear.The component interface is well-defined with appropriate types for a controlled tree selection component.
88-115: LGTM! Callbacks are well-structured.Both
handleTreeSelectChangeandrenderPopupare correctly implemented with proper memoization and complete dependency arrays.
117-128: LGTM! Resize listener properly implemented.The window resize listener is correctly set up with proper cleanup to prevent memory leaks. The empty dependency array is appropriate for this use case.
130-148: LGTM! Component return is correctly structured.The TreeSelect component is properly configured with all necessary props. The dual data approach (flat
treeDatafor TreeSelect, hierarchical data in custom popup) is appropriate for this implementation.
| const handleTreeSelectDataLoad = useCallback(async ({value: nodeValue}: {value?: unknown}) => { | ||
| if ("string" !== typeof nodeValue) { | ||
| return; | ||
| } | ||
| await onDataLoad(nodeValue); | ||
| }, [onDataLoad]); |
There was a problem hiding this comment.
Add error handling for async data loading.
The onDataLoad call can fail, but there's no error handling. Unhandled promise rejections will occur if data loading fails, and users won't receive feedback about the failure.
🛡️ Proposed fix using handleLoadError utility
According to the AI summary, utils.ts exports a handleLoadError utility. Consider wrapping the async call:
const handleTreeSelectDataLoad = useCallback(async ({value: nodeValue}: {value?: unknown}) => {
if ("string" !== typeof nodeValue) {
return;
}
- await onDataLoad(nodeValue);
+ try {
+ await onDataLoad(nodeValue);
+ } catch (error) {
+ handleLoadError(error);
+ }
}, [onDataLoad]);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
@components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/DirectoryTreeSelect.tsx
around lines 61 - 66, handleTreeSelectDataLoad currently awaits onDataLoad
without error handling; wrap the await in a try/catch and call the shared
handleLoadError utility on failure so rejections are handled and users get
feedback. Specifically, in the handleTreeSelectDataLoad function, after
confirming nodeValue is a string, call await onDataLoad(nodeValue) inside a try
block and in the catch pass the caught error (and any contextual info like
nodeValue) to handleLoadError from utils.ts. Ensure handleLoadError is imported
where DirectoryTreeSelect.tsx is defined and do not swallow errors silently.
| const handleDirectoryTreeLoadData = useCallback(async (node: {key: React.Key}) => { | ||
| await onDataLoad(node.key as string); | ||
| }, [onDataLoad]); |
There was a problem hiding this comment.
Add error handling and validate type assertion.
Two concerns:
- Missing error handling for the async
onDataLoadcall (same issue ashandleTreeSelectDataLoad). - The type assertion
node.key as stringis unsafe.React.Keyisstring | number, but the code assumes it's always a string without runtime validation.
🛡️ Proposed fix with error handling and type guard
const handleDirectoryTreeLoadData = useCallback(async (node: {key: React.Key}) => {
- await onDataLoad(node.key as string);
+ const key = String(node.key);
+ try {
+ await onDataLoad(key);
+ } catch (error) {
+ handleLoadError(error);
+ }
}, [onDataLoad]);Using String(node.key) ensures safe conversion to string regardless of the actual type.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
@components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/DirectoryTreeSelect.tsx
around lines 68 - 70, handleDirectoryTreeLoadData currently calls the async
onDataLoad without error handling and unsafely asserts node.key as string; wrap
the await onDataLoad(...) in a try/catch to log/handle errors (consistent with
handleTreeSelectDataLoad) and replace the unsafe cast with a runtime-safe
conversion/guard (e.g., validate typeof node.key === 'string' or use
String(node.key) before passing to onDataLoad) so the function neither throws
unhandled errors nor relies on an unsafe type assertion.
...nents/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/DirectoryTreeSelect.tsx
Outdated
Show resolved
Hide resolved
makes a lot of sense. i'm making the modifications and testing now |
… optimized key handling functions
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
@components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/DirectoryTreeSelect.tsx:
- Around line 27-39: The JSDoc for the DirectoryTreeSelect component includes an
empty @return tag; remove the stray "@return" line from the comment block above
the DirectoryTreeSelect component so the JSDoc lists only relevant param tags
(e.g., props.checkedKeys, props.expandedKeys, props.treeData, props.onCheck,
props.onExpand, props.onLoadData) and no return annotation.
In
@components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/utils.ts:
- Around line 207-217: In the BFS loop over queue, replace the two-step access
pattern using queue[0] and queue.shift() with a single call to queue.shift() and
use its return value as the current node (avoid the separate type assertion);
update the loop to handle possible undefined from shift() (e.g., continue if
null/undefined), then proceed to get children from childrenMap and add them to
toRemove and queue as before (referencing variables queue, current, childrenMap,
toRemove in the function).
- Around line 115-151: The flatToHierarchy function uses implicit truthiness
checks for treeNode and parent.children; change these to explicit comparisons to
match file style: when retrieving treeNode and parent from nodeMap, check using
strict !== undefined (e.g., treeNode !== undefined and parent !== undefined &&
parent.children !== undefined) before pushing into roots or parent.children;
reference symbols: flatToHierarchy, nodeMap, roots, treeNode, parent.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/DirectoryTreeSelect.tsxcomponents/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/index.tsxcomponents/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/utils.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit configuration file
- Prefer
false == <expression>rather than!<expression>.
Files:
components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/DirectoryTreeSelect.tsxcomponents/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/index.tsxcomponents/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/utils.ts
🧠 Learnings (2)
📚 Learning: 2025-05-29T20:33:40.653Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 937
File: components/log-viewer-webui/client/src/AntdApp.tsx:16-24
Timestamp: 2025-05-29T20:33:40.653Z
Learning: In components/log-viewer-webui React codebase: Return type annotations (like `: JSX.Element`) are unnecessary and not preferred for React components in JSX/TSX files.
Applied to files:
components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/DirectoryTreeSelect.tsx
📚 Learning: 2025-07-18T20:00:50.288Z
Learnt from: hoophalab
Repo: y-scope/clp PR: 1108
File: components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlQueryInput/index.tsx:15-15
Timestamp: 2025-07-18T20:00:50.288Z
Learning: In the y-scope/clp React webui client codebase, for Zustand store usage: use `useStore.getState().method` for callbacks since the output is not reactive and doesn't need state as a dependency in the hook, and use `useStore((state) => state.property)` with proper selectors for reactive components that need to re-render when state changes.
Applied to files:
components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/index.tsx
🧬 Code graph analysis (2)
components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/index.tsx (2)
components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/typings.ts (3)
TreeNode(25-25)ROOT_NODE(27-27)ROOT_PATH(28-28)components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/utils.ts (2)
handleLoadError(228-228)ROOT_PATH(230-230)
components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/utils.ts (1)
components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/typings.ts (1)
TreeNode(25-25)
⏰ 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: package-image
- GitHub Check: build (macos-15)
- GitHub Check: check-generated
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
🔇 Additional comments (5)
components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/DirectoryTreeSelect.tsx (1)
40-104: LGTM!The component implementation is well-structured with appropriate memoization. The use of
useMemofor derived data anduseCallbackfor handlers ensures efficient re-rendering. All dependency arrays are correctly specified.components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/index.tsx (2)
69-80: LGTM!The new handlers are well-implemented with proper use of
useCallbackand correct dependencies. The centralized error handling viahandleLoadErrorimproves consistency, and the form integration inhandleCheckcorrectly updates both local state and the form field value.
97-103: LGTM!The
DirectoryTreeSelectcomponent is properly integrated with all required props correctly wired to the component's state and handlers.components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/utils.ts (2)
103-113: LGTM!The centralized error handler appropriately logs to the console for debugging and displays a user-friendly message. The error message handling correctly checks the error type.
154-180: LGTM!The function correctly filters checked keys to only include topmost ancestors. All comparisons follow the explicit comparison pattern consistently.
| /** | ||
| * Renders a select component that uses DirectoryTree for the dropdown. Provides folder/file | ||
| * icons and intuitive directory navigation. | ||
| * | ||
| * @param props | ||
| * @param props.checkedKeys | ||
| * @param props.expandedKeys | ||
| * @param props.treeData | ||
| * @param props.onCheck | ||
| * @param props.onExpand | ||
| * @param props.onLoadData | ||
| * @return | ||
| */ |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove the empty @return tag.
The JSDoc includes an @return tag without a description. Since return type annotations are not preferred for React components in this codebase, and the tag provides no value, it should be removed.
📝 Proposed fix
/**
* Renders a select component that uses DirectoryTree for the dropdown. Provides folder/file
* icons and intuitive directory navigation.
*
* @param props
* @param props.checkedKeys
* @param props.expandedKeys
* @param props.treeData
* @param props.onCheck
* @param props.onExpand
* @param props.onLoadData
- * @return
*/📝 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.
| /** | |
| * Renders a select component that uses DirectoryTree for the dropdown. Provides folder/file | |
| * icons and intuitive directory navigation. | |
| * | |
| * @param props | |
| * @param props.checkedKeys | |
| * @param props.expandedKeys | |
| * @param props.treeData | |
| * @param props.onCheck | |
| * @param props.onExpand | |
| * @param props.onLoadData | |
| * @return | |
| */ | |
| /** | |
| * Renders a select component that uses DirectoryTree for the dropdown. Provides folder/file | |
| * icons and intuitive directory navigation. | |
| * | |
| * @param props | |
| * @param props.checkedKeys | |
| * @param props.expandedKeys | |
| * @param props.treeData | |
| * @param props.onCheck | |
| * @param props.onExpand | |
| * @param props.onLoadData | |
| */ |
🤖 Prompt for AI Agents
In
@components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/DirectoryTreeSelect.tsx
around lines 27 - 39, The JSDoc for the DirectoryTreeSelect component includes
an empty @return tag; remove the stray "@return" line from the comment block
above the DirectoryTreeSelect component so the JSDoc lists only relevant param
tags (e.g., props.checkedKeys, props.expandedKeys, props.treeData,
props.onCheck, props.onExpand, props.onLoadData) and no return annotation.
| /** | ||
| * Converts flat tree data (simple mode) to hierarchical format for DirectoryTree. | ||
| * | ||
| * @param flatData Array of flat tree nodes with pId references | ||
| * @return Hierarchical tree structure with children arrays. | ||
| */ | ||
| const flatToHierarchy = (flatData: TreeNode[]): TreeDataNode[] => { | ||
| const nodeMap = new Map<string, TreeDataNode>(); | ||
| const roots: TreeDataNode[] = []; | ||
|
|
||
| // Create all nodes | ||
| for (const node of flatData) { | ||
| nodeMap.set(node.id, { | ||
| key: node.value, | ||
| title: node.title, | ||
| isLeaf: node.isLeaf, | ||
| children: [], | ||
| }); | ||
| } | ||
|
|
||
| // Build hierarchy | ||
| for (const node of flatData) { | ||
| const treeNode = nodeMap.get(node.id); | ||
| if (null === node.pId) { | ||
| if (treeNode) { | ||
| roots.push(treeNode); | ||
| } | ||
| } else { | ||
| const parent = nodeMap.get(node.pId); | ||
| if (parent?.children && treeNode) { | ||
| parent.children.push(treeNode); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return roots; | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use explicit comparisons for consistency.
Lines 139 and 144 use implicit truthiness checks (if (treeNode) and if (parent?.children && treeNode)), which are inconsistent with the explicit comparison pattern used throughout this file (e.g., lines 138, 171, 178, 200, 219) and the coding guideline preference for explicit comparisons.
♻️ Proposed fix
// Build hierarchy
for (const node of flatData) {
const treeNode = nodeMap.get(node.id);
if (null === node.pId) {
- if (treeNode) {
+ if (undefined !== treeNode) {
roots.push(treeNode);
}
} else {
const parent = nodeMap.get(node.pId);
- if (parent?.children && treeNode) {
+ if (undefined !== parent && undefined !== parent.children && undefined !== treeNode) {
parent.children.push(treeNode);
}
}
}📝 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.
| /** | |
| * Converts flat tree data (simple mode) to hierarchical format for DirectoryTree. | |
| * | |
| * @param flatData Array of flat tree nodes with pId references | |
| * @return Hierarchical tree structure with children arrays. | |
| */ | |
| const flatToHierarchy = (flatData: TreeNode[]): TreeDataNode[] => { | |
| const nodeMap = new Map<string, TreeDataNode>(); | |
| const roots: TreeDataNode[] = []; | |
| // Create all nodes | |
| for (const node of flatData) { | |
| nodeMap.set(node.id, { | |
| key: node.value, | |
| title: node.title, | |
| isLeaf: node.isLeaf, | |
| children: [], | |
| }); | |
| } | |
| // Build hierarchy | |
| for (const node of flatData) { | |
| const treeNode = nodeMap.get(node.id); | |
| if (null === node.pId) { | |
| if (treeNode) { | |
| roots.push(treeNode); | |
| } | |
| } else { | |
| const parent = nodeMap.get(node.pId); | |
| if (parent?.children && treeNode) { | |
| parent.children.push(treeNode); | |
| } | |
| } | |
| } | |
| return roots; | |
| }; | |
| /** | |
| * Converts flat tree data (simple mode) to hierarchical format for DirectoryTree. | |
| * | |
| * @param flatData Array of flat tree nodes with pId references | |
| * @return Hierarchical tree structure with children arrays. | |
| */ | |
| const flatToHierarchy = (flatData: TreeNode[]): TreeDataNode[] => { | |
| const nodeMap = new Map<string, TreeDataNode>(); | |
| const roots: TreeDataNode[] = []; | |
| // Create all nodes | |
| for (const node of flatData) { | |
| nodeMap.set(node.id, { | |
| key: node.value, | |
| title: node.title, | |
| isLeaf: node.isLeaf, | |
| children: [], | |
| }); | |
| } | |
| // Build hierarchy | |
| for (const node of flatData) { | |
| const treeNode = nodeMap.get(node.id); | |
| if (null === node.pId) { | |
| if (undefined !== treeNode) { | |
| roots.push(treeNode); | |
| } | |
| } else { | |
| const parent = nodeMap.get(node.pId); | |
| if (undefined !== parent && undefined !== parent.children && undefined !== treeNode) { | |
| parent.children.push(treeNode); | |
| } | |
| } | |
| } | |
| return roots; | |
| }; |
🤖 Prompt for AI Agents
In
@components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/utils.ts
around lines 115 - 151, The flatToHierarchy function uses implicit truthiness
checks for treeNode and parent.children; change these to explicit comparisons to
match file style: when retrieving treeNode and parent from nodeMap, check using
strict !== undefined (e.g., treeNode !== undefined and parent !== undefined &&
parent.children !== undefined) before pushing into roots or parent.children;
reference symbols: flatToHierarchy, nodeMap, roots, treeNode, parent.
| // BFS to find all descendants | ||
| const queue = [keyToRemove]; | ||
| while (0 < queue.length) { | ||
| const current = queue[0] as string; | ||
| queue.shift(); | ||
| const children = childrenMap.get(current) ?? []; | ||
| for (const child of children) { | ||
| toRemove.add(child); | ||
| queue.push(child); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Simplify queue usage.
Lines 210-211 use an inefficient pattern: accessing queue[0] followed by queue.shift(). The shift() method already returns the removed element, making the separate access and type assertion unnecessary.
♻️ Proposed fix
// BFS to find all descendants
const queue = [keyToRemove];
while (0 < queue.length) {
- const current = queue[0] as string;
- queue.shift();
+ const current = queue.shift();
+ if (undefined === current) {
+ break;
+ }
const children = childrenMap.get(current) ?? [];
for (const child of children) {
toRemove.add(child);
queue.push(child);
}
}📝 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.
| // BFS to find all descendants | |
| const queue = [keyToRemove]; | |
| while (0 < queue.length) { | |
| const current = queue[0] as string; | |
| queue.shift(); | |
| const children = childrenMap.get(current) ?? []; | |
| for (const child of children) { | |
| toRemove.add(child); | |
| queue.push(child); | |
| } | |
| } | |
| // BFS to find all descendants | |
| const queue = [keyToRemove]; | |
| while (0 < queue.length) { | |
| const current = queue.shift(); | |
| if (undefined === current) { | |
| break; | |
| } | |
| const children = childrenMap.get(current) ?? []; | |
| for (const child of children) { | |
| toRemove.add(child); | |
| queue.push(child); | |
| } | |
| } |
🤖 Prompt for AI Agents
In
@components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/utils.ts
around lines 207 - 217, In the BFS loop over queue, replace the two-step access
pattern using queue[0] and queue.shift() with a single call to queue.shift() and
use its return value as the current node (avoid the separate type assertion);
update the loop to handle possible undefined from shift() (e.g., continue if
null/undefined), then proceed to get children from childrenMap and add them to
toRemove and queue as before (referencing variables queue, current, childrenMap,
toRemove in the function).
…zing and data loading
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/DirectoryTreePopup.tsx:
- Around line 79-94: The inline style object passed to the wrapper div in
DirectoryTreePopup is recreated on each render; memoize it using React.useMemo
inside the DirectoryTreePopup component (e.g., compute a containerStyle that
returns { height, overflow: 'auto' } and depends on height) and replace the
inline style with that memoized variable so the object identity is stable across
renders.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/DirectoryTreePopup.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit configuration file
- Prefer
false == <expression>rather than!<expression>.
Files:
components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/DirectoryTreePopup.tsx
🧬 Code graph analysis (1)
components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/DirectoryTreePopup.tsx (1)
components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/utils.ts (1)
getListHeight(227-227)
⏰ 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: package-image
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: build (macos-15)
- GitHub Check: check-generated
- GitHub Check: lint-check (macos-15)
🔇 Additional comments (5)
components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/DirectoryTreePopup.tsx (5)
1-16: LGTM: Clean imports and setup.The imports are well-organized, with React hooks, Ant Design components, types, and local utilities properly imported. The DirectoryTree extraction from Tree is a standard pattern.
18-38: LGTM: Well-defined interface and documentation.The
DirectoryTreePopupPropsinterface properly types all necessary props for a controlled DirectoryTree component. The JSDoc documentation clearly describes the component's purpose and parameters.
39-47: LGTM: Proper component setup.The component properly destructures props and initializes height state with lazy initialization using
getListHeight, which is efficient as the function is only called once on mount.
49-77: LGTM: Well-implemented callbacks and resize handling.The callbacks are properly memoized with correct dependencies. The
handleCheckcorrectly handles both payload formats from Ant Design's onCheck event. The useEffect properly manages the window resize listener with appropriate cleanup to prevent memory leaks.
97-97: LGTM: Standard default export.
| return ( | ||
| <div | ||
| className={styles["directoryTreePopup"]} | ||
| style={{height: height, overflow: "auto"}} | ||
| > | ||
| <DirectoryTree | ||
| checkable={true} | ||
| checkedKeys={checkedKeys} | ||
| expandedKeys={expandedKeys} | ||
| loadData={handleLoadData} | ||
| treeData={treeData} | ||
| onCheck={handleCheck} | ||
| onExpand={handleExpand}/> | ||
| </div> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider memoizing the inline style object.
The component is well-structured with the DirectoryTree properly configured. However, the inline style object is recreated on every render.
♻️ Optional optimization to memoize the style object
+import React, {
+ useCallback,
+ useEffect,
+ useMemo,
+ useState,
+} from "react"; const DirectoryTreePopup = ({
checkedKeys,
expandedKeys,
treeData,
onCheck,
onExpand,
onLoadData,
}: DirectoryTreePopupProps) => {
const [height, setHeight] = useState<number>(getListHeight);
+
+ const containerStyle = useMemo(() => ({
+ height,
+ overflow: "auto" as const,
+ }), [height]);
// ... rest of the code
return (
<div
className={styles["directoryTreePopup"]}
- style={{height: height, overflow: "auto"}}
+ style={containerStyle}
>Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
@components/webui/client/src/pages/IngestPage/Compress/PathsSelectFormItem/DirectoryTreePopup.tsx
around lines 79 - 94, The inline style object passed to the wrapper div in
DirectoryTreePopup is recreated on each render; memoize it using React.useMemo
inside the DirectoryTreePopup component (e.g., compute a containerStyle that
returns { height, overflow: 'auto' } and depends on height) and replace the
inline style with that memoized variable so the object identity is stable across
renders.
davemarco
left a comment
davemarco
left a comment
There was a problem hiding this comment.
Does it make sense handleLoadData and similar functions directly into DirectoryTreeSelect instead of keeping them inside PathsSelectFormItem, which could make the tree component more self-contained and less passing functions. Also, I’m not entirely sure, could it work to get rid of simple mode layout entirely and storing the tree data directly in a hierarchical structure? This im less sure about
The current structure is:
Moving it would mean DirectoryTreeSelect owns data loading, state, and deduplication? While moving would encourage:
, we need to look at:
I believe the current separation is reasonable. The form item is the stateful controller, and DirectoryTreeSelect is a controlled presentation component.
Storing directly in a hierarchical structure would work, but the conversion from flat -> hierarchical is cheap (linear scan, runs only when treeData changes). The real complexity is in state updates, and flat makes those trivial. Keeping flat storage is a reasonable choice for the lazy-loaded tree. The flatToHierarchy conversion is also memoized and only runs when data changes. The hierarchical structure would add complexity to addNodes and any future operations that modify the tree |
|
for 1. I think you can do something like this maybe? https://ant.design/components/form#form-demo-customized-form-controls -Like just match what the form expects, and you shouldnt need to set the value? I will respond to the second point soon |
|
for second point
Im noticing these new functions filterToParents/removeWithDescendants look complicated, and are already managed by treeSelect. Does it make sense to just go back to treeSelect where it has treeDataSimple, and checkedStrategy which manages all this stuff for us. Then we can just icons to "fake" directory tree. Maybe eventually they will even add support. Anyways let me know ur thoughts. |
2 participants
Description
Currently, tree node expansion/collapse in the file listing UI requires clicking the small +/- switcher icon.
This PR replaces the TreeSelect's default tree rendering with Ant Design's
DirectoryTreecomponentvia
popupRender, providing a more intuitive file browser experience with folder/file icons.Changes:
DirectoryTreeSelectcomponent that wrapsTreeSelectwith aDirectoryTreepopupflatToHierarchyutility to convert flat tree data to hierarchical formatuseListHeighthook for responsive dropdown sizingSwitcherIconcomponent (DirectoryTree has built-in icons)Checklist
breaking change.
Validation performed
Confirmed the above clicking on leaf / clicking on non-leaf node behaviours matched expectations.
Summary by CodeRabbit
New Features
Behavior & Reliability
Chores
✏️ Tip: You can customize this high-level summary in your review settings.