Feat/fix select default search #4639
Conversation
|
/next |
Walkthrough向 Select 组件添加 Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/components/src/select/index.tsx (1)
425-429: 搜索输入未随下拉关闭清空：依赖数组缺失导致逻辑失效该副作用只在首次挂载执行一次，无法在关闭面板时清空
searchInput，结合“启用外部搜索时有输入就不渲染列表”的逻辑，可能导致再次打开后列表一直不显示。- useEffect(() => { - if (!open && searchInput) { - setSearchInput(''); - } - }, []); + useEffect(() => { + if (!open && searchInput) { + setSearchInput(''); + } + }, [open, searchInput]);packages/file-tree-next/src/browser/dialog/file-dialog.view.tsx (1)
170-181: 错误使用了未定义的event变量，易引发逻辑错误回调参数名为
ev，但这里调用了hasShiftMask(event)/hasCtrlCmdMask(event)。在 React 环境下window.event不可靠，这会导致修饰键判断异常。- const shiftMask = hasShiftMask(event); - const ctrlCmdMask = hasCtrlCmdMask(event); + const shiftMask = hasShiftMask(ev); + const ctrlCmdMask = hasCtrlCmdMask(ev);
🧹 Nitpick comments (7)
packages/main-layout/src/browser/tabbar/tabbar.service.ts (1)
831-841: 改用.get()读取 currentContainerId 的修复是合理的；建议一次读取并复用以避免状态抖动当前在同一轮执行中对
this.currentContainerId.get()读取了两次，极端情况下可能出现前后不一致（例如切换发生在fastdom回调间隙），也会产生一次不必要的 observable 读取。建议将值缓存到局部变量并复用，语义更稳定也更易读。fastdom.measureAtNextFrame(() => { - if (!this.currentContainerId.get() || !this.resizeHandle) { + const currentId = this.currentContainerId.get(); + if (!currentId || !this.resizeHandle) { // 折叠时不监听变化 return; } const size = this.resizeHandle.getSize(); - if (size !== this.barSize && !this.shouldExpand(this.currentContainerId.get())) { + if (size !== this.barSize && !this.shouldExpand(currentId)) { this.prevSize = size; this.onSizeChangeEmitter.fire({ size }); } });packages/components/src/select/index.tsx (4)
536-536: 隐藏下拉列表的条件建议改用派生常量，并请确认交互是否符合预期当前条件在有输入且启用外部搜索时完全不渲染列表，可能导致用户输入期间无可选结果可视/可达。若外部会按输入异步刷新
options，通常仍希望展示更新后的列表。建议先改用局部常量；交互层面请确认：是否需要“正在搜索…”占位或保留上次结果。
- !(externalSearchBehavior && searchInput) && + !(isExternalSearch && searchInput) &&如需保留渲染并交给外部过滤，可改为始终渲染并由外部控制
options内容。需要的话我可以补提交“搜索中占位 + 空态”方案。
484-508: 外部搜索启用时可避免不必要的本地过滤计算当
isExternalSearch && searchInput时本地filterOption结果不会被渲染，仍计算会平白产生开销。- const filteredOptions = useMemo(() => { + const filteredOptions = useMemo(() => { + if (isExternalSearch && searchInput) { + // 外部托管过滤：直接返回原 options（或外部传入的结果） + return options; + } if (!searchInput) { return options; } @@ - }, [options, searchInput, filterOption]); + }, [options, searchInput, filterOption, isExternalSearch]);
516-522: useCallback 依赖包含无用的searchInput回调内未读取
searchInput，将其放入依赖会造成不必要的重建。- }, [searchInput, onSearchChange], + }, [onSearchChange],
30-33: 补充外部搜索开关的默认语义到注释为避免歧义，建议在注释中明确“未显式传入时，若提供
onSearchChange则默认启用”。/** - * 搜索行为不采用默认的 filterOptions 进行筛选，由外部托管 + * 搜索行为由外部托管（不使用默认 filterOption）。 + * 未显式传入时，若提供 onSearchChange 则默认启用。 */ externalSearchBehavior?: boolean;packages/file-tree-next/src/browser/dialog/file-dialog.view.tsx (2)
291-291: 此处可省略externalSearchBehavior（可选）在 Select 中我们已约定“未显式传入时，提供了
onSearchChange则默认启用外部搜索”，因此这里可以不必显式传true，避免样板代码。- externalSearchBehavior={true} + // 省略：提供了 onSearchChange，默认启用外部搜索
224-232: 目录路径搜索每击键即触发树模型刷新，建议去抖频繁调用
updateTreeModel可能引发性能抖动。建议对onSearchChangeHandler做轻量去抖（200ms 作为起点），同时精简依赖。- const onSearchChangeHandler = useCallback( - async (value: string) => { - setIsReady(false); - setSelectPath(value); - await model.updateTreeModel(value); - setIsReady(true); - }, - [model, isReady, selectPath, directoryList], - ); + const debounceRef = useRef<number | null>(null); + const onSearchChangeHandler = useCallback( + (value: string) => { + if (debounceRef.current) { + window.clearTimeout(debounceRef.current); + } + debounceRef.current = window.setTimeout(async () => { + setIsReady(false); + setSelectPath(value); + await model.updateTreeModel(value); + setIsReady(true); + }, 200); + }, + [model], + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/components/src/select/index.tsx(3 hunks)packages/file-tree-next/src/browser/dialog/file-dialog.view.tsx(1 hunks)packages/main-layout/src/browser/tabbar/tabbar.service.ts(1 hunks)
⏰ 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). (7)
- GitHub Check: build (macos-latest, 20.x)
- GitHub Check: build (ubuntu-latest, 20.x)
- GitHub Check: unittest (macos-latest, 18.x, jsdom)
- GitHub Check: unittest (ubuntu-latest, 18.x, node)
- GitHub Check: unittest (ubuntu-latest, 18.x, jsdom)
- GitHub Check: unittest (macos-latest, 18.x, node)
- GitHub Check: ubuntu-latest, Node.js 20.x
| externalSearchBehavior = externalSearchBehavior || !!onSearchChange | ||
|
|
There was a problem hiding this comment.
默认值覆盖显式 false，且直接改写参数，存在语义与可维护性问题
- 使用
||会在onSearchChange存在时把用户显式传入的false覆盖成true，这不符合直觉。 - 不要直接重写函数参数，改用派生的局部常量更安全清晰。
建议：用空值合并并引入局部变量，既保留“当未显式传值时默认随 onSearchChange 启用”的语义，也尊重显式 false。
export function Select<T = string>({
@@
onSearchChange,
- externalSearchBehavior
+ externalSearchBehavior
}: ISelectProps<T>) {
@@
- externalSearchBehavior = externalSearchBehavior || !!onSearchChange
+ // 派生外部搜索开关：未显式传入时，默认随 onSearchChange 启用；显式 false 必须被尊重
+ const isExternalSearch = externalSearchBehavior ?? !!onSearchChange;并同步替换后续对 externalSearchBehavior 的使用（见下方相关 diff）。
Also applies to: 297-298
🤖 Prompt for AI Agents
In packages/components/src/select/index.tsx around lines 297-306, the code
mutates the parameter and uses `||` which erroneously treats an explicit false
as missing; change to derive a local constant using nullish coalescing (e.g.
const resolvedExternalSearchBehavior = externalSearchBehavior ??
!!onSearchChange) so an explicit false is preserved and you don’t overwrite the
parameter, and then update all subsequent references to use this new local
constant instead of the original parameter.
MMhunter
force-pushed
the
feat/fix-select-default-search
branch
2 times, most recently
from
f9d6550 to
9ecabc3
Compare
|
🎉 PR Next publish successful! 3.9.1-next-1759129101.0 |
MMhunter
force-pushed
the
feat/fix-select-default-search
branch
from
9ecabc3 to
a276714
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4639 +/- ##
==========================================
+ Coverage 52.59% 52.63% +0.03%
==========================================
Files 1686 1686
Lines 104362 104571 +209
Branches 22682 22733 +51
==========================================
+ Hits 54894 55037 +143
- Misses 41097 41153 +56
- Partials 8371 8381 +10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
另外修复 suggestion 可能多一个点的问题 |
|
/next |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/components/src/select/index.tsx (1)
268-306: externalSearchBehavior 默认策略 OK，但仍不建议改写参数
externalSearchBehavior的默认逻辑从||改为：externalSearchBehavior = externalSearchBehavior ?? !!onSearchChange;这已经修复了“显式传入 false 会被覆盖”的问题，语义更合理。但这里仍然直接改写了函数参数本身，可读性和可维护性都比较差（比如后续想在 JSX 中解构同名 prop 时易混淆），也与常见的 lint 规则（如 no-param-reassign）相悖。
建议改为派生局部常量，并在后续使用中统一引用该常量，而不是重新赋值入参。例如：
export function Select<T = string>({ @@ - onSearchChange, - externalSearchBehavior + onSearchChange, + externalSearchBehavior, }: ISelectProps<T>) { @@ - externalSearchBehavior = externalSearchBehavior ?? !!onSearchChange + // 派生外部搜索开关：未显式传值时，默认随 onSearchChange 启用；显式 false 必须被尊重 + const isExternalSearch = externalSearchBehavior ?? !!onSearchChange;并在后文渲染逻辑中用
isExternalSearch替代对externalSearchBehavior的直接使用（见下方条件渲染）。这一点与之前机器人评论的建议是一致的，这里再次强调以便后续统一风格。
🧹 Nitpick comments (1)
packages/components/src/select/index.tsx (1)
484-507: externalSearchBehavior 的行为语义与注释略有偏差，建议确认是否应仅禁用内建过滤而非隐藏列表当前 filteredOptions 与渲染逻辑的组合为：
const filteredOptions = useMemo(() => { if (!searchInput) { return options; } if (isDataOptions(options)) { return options.filter((o) => filterOption(searchInput, o)); } if (isDataOptionGroups(options)) { // ... } return options; }, [options, searchInput, filterOption]); // 渲染 {open && !(externalSearchBehavior && searchInput) && (isDataOptions(filteredOptions) || isDataOptionGroups(filteredOptions) ? ( <SelectOptionsList ... /> ) : ( // children 分支 ))}配合 props 注释：
搜索行为不采用默认的 filterOptions 进行筛选，由外部托管
当：
externalSearchBehavior === true且searchInput非空时，当前行为是整个下拉列表不再渲染（
!(externalSearchBehavior && searchInput)为 false），而不是“仍然显示 options，但不再按内建filterOption二次过滤”。这可能会带来两个与注释不完全吻合的点：
- 按注释理解，“外部托管”是指筛选逻辑由调用方控制（例如在上层根据输入变更
options），但下拉列表依然由 Select 展示；而现在在输入非空时，下拉会直接消失。- 即便列表不显示，
filteredOptions仍然会在useMemo中按filterOption计算一遍，只是结果没被渲染，略有浪费。如果上层（例如文件对话框）确实希望“输入时完全不显示本组件下拉，由外部 UI 负责展示搜索结果”，那当前实现是合理的；但如果期望是“外部负责根据输入调整 options，本组件只负责按 options 显示，不再做本地过滤”，更符合注释语义的做法可能是：
- 在
useMemo中，当externalSearchBehavior为 true 时直接返回options，不执行filterOption；- 渲染条件只判断
open，而不再对searchInput做额外限制。示例（仅供参考）：
const isExternalSearch = externalSearchBehavior ?? !!onSearchChange; const filteredOptions = useMemo(() => { if (!searchInput || isExternalSearch) { // 外部托管搜索：直接使用上层传入的 options return options; } // 下面保持原有 filterOption 行为 ... }, [options, searchInput, filterOption, isExternalSearch]); ... {open && (isDataOptions(filteredOptions) || isDataOptionGroups(filteredOptions) ? ( <SelectOptionsList ... /> ) : ( ... ))}建议结合实际使用场景（尤其是文件对话框中的 Select 实例）确认预期 UX，再决定是保留“输入即隐藏下拉”的策略，还是调整为“只禁用内建过滤逻辑”。
Also applies to: 535-568
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/components/src/select/index.tsx(3 hunks)packages/extension/src/browser/vscode/api/main.thread.language.ts(1 hunks)packages/file-tree-next/src/browser/dialog/file-dialog.view.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/file-tree-next/src/browser/dialog/file-dialog.view.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). (8)
- GitHub Check: 🚀🚀🚀 Next Version for pull request
- GitHub Check: ubuntu-latest, Node.js 20.x
- GitHub Check: build (macos-latest, 20.x)
- GitHub Check: build (ubuntu-latest, 20.x)
- GitHub Check: unittest (macos-latest, 18.x, jsdom)
- GitHub Check: unittest (macos-latest, 18.x, node)
- GitHub Check: unittest (ubuntu-latest, 18.x, node)
- GitHub Check: unittest (ubuntu-latest, 18.x, jsdom)
🔇 Additional comments (1)
packages/extension/src/browser/vscode/api/main.thread.language.ts (1)
262-297: inflateSuggestDto 中 range 新逻辑基本合理，但需要确认 DTO 形态当前实现：
range: (Array.isArray(dtoRange) && dtoRange.length === 4 ? MonacoRange.lift({ ... }) : dtoRange) || defaultRange这样可以：
- 正确处理
[startLine, startCol, endLine, endCol]四元组；- 也能透传已经是
IRange或{ insert: IRange; replace: IRange }的对象；- 在
dtoRange为空时回退到defaultRange。这很好地覆盖了扩展侧更多 range 形态。但副作用是：如果上游（扩展或协议）错误传入“长度≠4 的数组”，现在会把这个数组直接作为
CompletionItem.range传给 Monaco，而不是退回defaultRange，理论上会是一个非法 range 形态。建议：
- 确认（或在类型/协议层保证）
dtoRange只会是：
- 长度为 4 的 number 数组，或
- 合法的
IRange/{ insert, replace }对象。- 如果希望更防御一点，可只接受长度为 4 的数组，其它数组统一退回
defaultRange。示例重构（仅供参考）：
const range = dtoRange == null ? defaultRange : Array.isArray(dtoRange) ? dtoRange.length === 4 ? MonacoRange.lift({ ... }) : defaultRange : dtoRange;整体来看改动方向是正确的，只是建议确认上游 DTO 不会给出异常数组即可。
MMhunter
force-pushed
the
feat/fix-select-default-search
branch
from
e59fb5e to
dba77b2
Compare
|
/next |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/extension/src/browser/vscode/api/main.thread.language.ts (1)
263-307:inflateSuggestDto使用dtoRange的方式更合理，建议稍微加强类型与健壮性整体来说，这段修改让补全项的
range不再接收“裸数组”，而是：
- 若
dtoRange存在且为数组，则通过MonacoRange.lift转成 Monaco Range；- 若
dtoRange为对象，则直接透传；- 否则回退到
defaultRange。这能避免之前 range 形状不一致导致的插入范围错位问题（例如 suggestion 多出一个
.），方向是正确的。可以考虑两点小优化，使代码更自说明、对异常数据更稳健（功能上属增强而非必须）：
- 给
dtoRange一个更明确的类型（利用你上面导入的 DTO 类型）；- 对数组形态时顺带检查长度是否为 4，防御潜在的坏数据。
示例 diff（仅供参考，可按你们的类型定义微调）：
- const dtoRange = data[ISuggestDataDtoField.range]; + const dtoRange = data[ISuggestDataDtoField.range] as RangeSuggestDataDto | undefined; @@ - let targetRange: IRange | { insert: IRange; replace: IRange } = defaultRange; + let targetRange: IRange | { insert: IRange; replace: IRange } = defaultRange; @@ - if (dtoRange) { - if (Array.isArray(dtoRange)) { - targetRange = MonacoRange.lift({ - startLineNumber: dtoRange[0], - startColumn: dtoRange[1], - endLineNumber: dtoRange[2], - endColumn: dtoRange[3], - }) - } else { - targetRange = dtoRange; - } - } + if (dtoRange) { + if (Array.isArray(dtoRange) && dtoRange.length === 4) { + const [startLineNumber, startColumn, endLineNumber, endColumn] = dtoRange; + targetRange = MonacoRange.lift({ startLineNumber, startColumn, endLineNumber, endColumn }); + } else { + // 这里假设 dtoRange 已经是 IRange 或 { insert: IRange; replace: IRange } 这类对象形态 + targetRange = dtoRange as any; + } + } @@ - range: targetRange, + range: targetRange,这样既保持了当前修复的语义，又在类型和数据形状上多了一层约束，后续有人改 DTO 时也更容易发现不匹配。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/extension/src/browser/vscode/api/main.thread.language.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/extension/src/browser/vscode/api/main.thread.language.ts (1)
packages/monaco/src/common/types.ts (1)
IRange(6-6)
⏰ 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). (8)
- GitHub Check: 🚀🚀🚀 Next Version for pull request
- GitHub Check: unittest (ubuntu-latest, 18.x, node)
- GitHub Check: unittest (ubuntu-latest, 18.x, jsdom)
- GitHub Check: unittest (macos-latest, 18.x, node)
- GitHub Check: unittest (macos-latest, 18.x, jsdom)
- GitHub Check: build (ubuntu-latest, 20.x)
- GitHub Check: build (macos-latest, 20.x)
- GitHub Check: ubuntu-latest, Node.js 20.x
🔇 Additional comments (1)
packages/extension/src/browser/vscode/api/main.thread.language.ts (1)
55-55: 引入RangeSuggestDataDto类型本身没有问题这里补充从
../../../common/vscode导出的 DTO 类型导入，保持与公共协议定义对齐是合理的扩展点，目前看不会引入任何行为变化。
|
🎉 PR Next publish successful! 3.9.1-next-1764556130.0 |
Types
Background or solution
修复由 #4306 引入的原有搜索控件的搜索筛选功能实效问题
Changelog
Summary by CodeRabbit
发行说明
新增功能
Bug 修复
✏️ Tip: You can customize this high-level summary in your review settings.