Skip to content

fix: workflow search keyword #7729

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wangdan-fit2cloud merged 1 commit into from
Nov 6, 2026
Merged

fix: workflow search keyword #7729

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ui/src/styles/element-plus.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -823,7 +823,7 @@
/* progress */
.el-progress {
.el-progress-bar__inner {
background: linear-gradient(90deg, rgb(var(--mk-primary-rgb) / 20%) 0%, var(--el-color-primary) 100%);
background: linear-gradient(90deg, rgb(51 112 255 / 20%) 0%, #3370ff 100%);
}
&.is-warning {
.el-progress-bar__inner {
Expand Down
6 changes: 3 additions & 3 deletions ui/src/workflow-canvas/component/NodeControl.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<script setup lang="ts">
import { nextTick, ref, watch } from 'vue'
import { nextTick, reactive, ref, watch } from 'vue'
import type LogicFlow from '@logicflow/core'
import type { BaseNodeModel } from '@logicflow/core'
import type { SelectionSelect } from '@logicflow/extension'
Expand Down Expand Up @@ -73,10 +73,10 @@ function fitView() {
props.logicFlow.fitView(40, 40)
}

// 批量展开收起不替换节点数据，保留表单内容。
// 与节点视图共用 Vue 响应式代理，批量切换时同步内容显示并保留表单数据。
function setNodesExpanded(expanded: boolean) {
props.logicFlow.graphModel.nodes.forEach((node) => {
node.properties.showNode = expanded
reactive(node).properties.showNode = expanded
})
}

@shaohuzhang1 shaohuzhang1 Nov 6, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Main issue: reactive(node) usage is problematic

  1. reactive() created inside the loop

    props.logicFlow.graphModel.nodes.forEach((node) => {
  reactive(node).properties.showNode = expanded
})
    • Calling reactive(node) on every iteration creates an ephemeral proxy. Vue caches proxies internally, so writes do go through the same cached proxy for the same raw object, but this relies on implementation details of how LogicFlow stores nodes.
    • If LogicFlow's internal models use their own reactivity system (MobX in some versions) or frozen/plain objects, reactive() may silently do nothing or cause two separate reactive systems fighting over updates.

  2. Better alternative: mutate properties via LogicFlow's own API instead of hijacking Vue reactivity:

    node.setProperty('showNode', expanded)

    or if direct mutation is truly needed, wrap once at a higher level (e.g., const nodes = reactive(props.logicFlow.graphModel.nodes)) rather than calling reactive() repeatedly inside the loop.

  3. Unavailable while batch operation — mutating every node synchronously triggers N reactive notifications; if there are many nodes, consider batching (e.g., toggling only collapsed/visible ones or using pauseTracking-style strategies).

Comment drift: the comment changed from “不替换节点数据，保留表单内容” to “与节点视图共用 Vue 响应式代理…” — that's fine, but make sure it actually describes behavior; if the "shared proxy" only works by accident of Vue's proxy cache, the comment may mislead future maintainers.

Minor suggestions

  • Type safety: node.properties may be Record<string, unknown>; ensure showNode is typed, e.g. via a custom node model type.
  • Consider extracting const { nodes } = props.logicFlow.graphModel before the loop for readability (negligible perf impact).
  • If this runs often (e.g., watch triggered frequently), add a guard to skip work when all nodes already have the target state.

Summary: the core risk is depending on reactive() against non-Vue-owned objects (LogicFlow models). Prefer LogicFlow's setProperty API; keep reactive() only for objects you create and own.

Expand Down
20 changes: 18 additions & 2 deletions ui/src/workflow-canvas/component/NodeSearch.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,20 @@ const matchedNodes = computed(() => {
function focusCurrentNode() {
const node = matchedNodes.value[currentIndex.value]
props.logicFlow.clearSelectElements()
const keyword = searchKeyword.value.trim()
props.logicFlow.graphModel.nodes.forEach((nodeModel) => {
nodeModel.clearSelectOn?.()
if (
keyword &&
String(nodeModel.properties.stepName ?? '')
.toLocaleLowerCase()
.includes(keyword.toLocaleLowerCase())
) {
nodeModel.selectOn?.(keyword)
}
})
if (!node) return
props.logicFlow.getNodeModelById(node.id)?.focusOn?.()
props.logicFlow.selectElementById(node.id)
props.logicFlow.focusOn(node.id)
}
Expand All @@ -44,7 +57,7 @@ function closeSearch() {
searchVisible.value = false
searchKeyword.value = ''
currentIndex.value = 0
props.logicFlow.clearSelectElements()
focusCurrentNode()
}

function handleShortcut(event: KeyboardEvent) {
Expand All @@ -61,7 +74,10 @@ watch(searchKeyword, () => {
})

onMounted(() => window.addEventListener('keydown', handleShortcut))
onBeforeUnmount(() => window.removeEventListener('keydown', handleShortcut))
onBeforeUnmount(() => {
window.removeEventListener('keydown', handleShortcut)
props.logicFlow.graphModel.nodes.forEach((node) => node.clearSelectOn?.())
})
</script>

<template>

@shaohuzhang1 shaohuzhang1 Nov 6, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a review of the changes:

Issues

  1. Logic change in closeSearch is questionable

    - props.logicFlow.clearSelectElements()
+ focusCurrentNode()

    focusCurrentNode() re-selects and re-highlights everything using the current searchKeyword. But closeSearch just set searchKeyword.value = '', so the keyword-based highlighting will match nothing — yet it still calls selectElementById and focusOn on matchedNodes[0]. If the intent was to clean up, this now leaves one node selected/focused instead of clearing everything. Consider whether you actually want clearSelectElements() plus a manual clearSelectOn sweep, or intentionally keep focus.

  2. Loop variable shadowing

    const node = matchedNodes.value[currentIndex.value]
...
props.logicFlow.graphModel.nodes.forEach((nodeModel) => ...)

    Good that you renamed to nodeModel, but note the later props.logicFlow.graphModel.nodes.forEach((node) => ...) in onBeforeUnmount shadows nothing problematic — still, keep naming consistent.

  3. Repeated keyword-computation cost
    keyword.toLocaleLowerCase() is computed inside the loop for every node. Hoist it:

    const kw = keyword.toLocaleLowerCase()
// ...
String(nodeModel.properties.stepName ?? '').toLocaleLowerCase().includes(kw)

  4. searchKeyword.value.trim() timing
    In focusCurrentNode you trim the keyword for matching, but the watch(searchKeyword, ...) / matchedNodes computed may use the untrimmed value. Ensure both use the same normalization or counts/indices may mismatch (e.g., keyword " x " matches differently in the two places).

  5. Redundant focus call

    props.logicFlow.getNodeModelById(node.id)?.focusOn?.()
props.logicFlow.focusOn(node.id)

    Two focus mechanisms back-to-back; likely one suffices. The getNodeModelById(...)?.focusOn?.() appears redundant — remove it unless it has side effects logicFlow.focusOn lacks.

  6. Extra loop on close/unmount could be avoided
    If clearSelectElements() triggers each node's clear internally, the manual nodeModel.clearSelectOn?.() sweep may be unnecessary duplication. Verify LogicFlow's behavior; otherwise you keep two sources of truth for selection state.

  7. Guard against missing properties
    nodeModel.properties.stepName assumes properties always exists; safer: nodeModel.properties?.stepName ?? ''.

Minor optimizations

  • Cache graphModel.nodes once instead of walking it multiple times per call.
  • Precompile lowercase stepName comparisons only when keyword is truthy (already partly done with the keyword && short-circuit — good).
  • Consider debouncing the highlight sweep on rapid typing if node lists are large.

Overall: functionally plausible, but #1 (behavior change in closeSearch) and #5 (double focus) deserve a second look.

Expand Down
29 changes: 12 additions & 17 deletions ui/src/workflow-canvas/core/node-container/index.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<script setup lang="ts">
import { reaction, type BaseNodeModel, type Model } from '@logicflow/core'
import { computed, inject, onBeforeUnmount, onMounted, ref, watch } from 'vue'
import { cloneDeep } from 'lodash'
import { cloneDeep, escape, escapeRegExp } from 'lodash'
import { iconComponent } from '@/workflow-canvas/icons/utils'
import { copyText } from '@/utils/clipboard'
import { WorkflowMode, WorkflowNodeType, type WorkflowNodeField } from '@/workflow-canvas/types'
Expand Down Expand Up @@ -218,23 +218,18 @@ const clearSelectOn = () => {
// 高亮选中关键字

const highlightedStepName = (contentText: string) => {
let res = contentText
if (keyWord.value === '') {
return res
} else {
const wordsArray = contentText.split('')
for (let i = 0; i < wordsArray.length; i++) {
const word = wordsArray[i]
if (word && keyWord.value.includes(word)) {
wordsArray[i] = currentKeyWord.value
? `<span style='background: #FF8800;'>${word}</span>`
: `<span style='background: #FFC60A;'>${word}</span>`
}
}
res = wordsArray.join('')
return res
}
if (!keyWord.value) return escape(contentText)
const keywordPattern = new RegExp(`(${escapeRegExp(keyWord.value)})`, 'gi')
return contentText
.split(keywordPattern)
.map((part, index) => {
const text = escape(part)
if (index % 2 === 0) return text
return currentKeyWord.value ? `<span style='background: var(--mk-warning);'>${text}</span>` : `<span style='background: #FFC60A'>${text}</span>`
})
.join('')
}

onMounted(() => {
model.openNodeMenu = (anchor: Model.AnchorConfig) => {
showAnchor.value && anchorData.value?.id === anchor.id ? closeNodeMenu() : openNodeMenu(anchor)

@shaohuzhang1 shaohuzhang1 Nov 6, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review: highlightedStepName Refactor

✅ Improvements in the New Version

  1. Correct whole-keyword matching — old code highlighted any character contained in the keyword (substring of chars); new code matches the full keyword as a substring. This is the right behavior.
  2. Proper regex escaping (escapeRegExp) — special characters like *, (, . no longer break or throw in new RegExp. Good.
  3. HTML escaping (escape) — prevents XSS/injection from user-provided text and keywords. Major security fix.
  4. Uses a CSS variable (--mk-warning) instead of hardcoded color for one branch — better theming consistency.
  5. Simpler and more readable via split with capture group + odd-index check.

⚠️ Issues / Potential Problems

  1. index % 2 fragility: With a capture group, String.split guarantees even indices are non-matches and odd indices are matches — but only if the regex always has exactly one capture group and never matches an empty string. Edge case: if keyWord.value can be whitespace-only or the pattern could match empty string, odd/even logic may break. The current guard !keyWord.value covers empty, but consider trimming: if (!keyWord.value.trim()).
  2. Inconsistent styling: one branch uses var(--mk-warning), the other hardcodes #FFC60A. Prefer a CSS variable for both (or move styles to a class, e.g. .highlight), which would also remove inline style duplication.
  3. Quoting style inconsistency: 'background: #FF8800;' vs 'background: #FFC60A' — one keeps trailing semicolon, new one doesn't; also mixes quote styles elsewhere in the file. Minor.
  4. Performance: for each render per node, this runs escape/regex. Fine normally, but if called very frequently (hundreds of nodes, on keystroke), consider computed-caching the compiled regex per keyword rather than rebuilding it each call.

💡 Suggested Optimization

const highlightClass = computed(() => currentKeyWord.value ? 'highlight-active' : 'highlight')

const highlightedStepName = (contentText: string) => {
  if (!keyWord.value.trim()) return escape(contentText)
  const pattern = new RegExp(`(${escapeRegExp(keyWord.value)})`, 'gi')
  return contentText
    .split(pattern)
    .map((part, i) => i % 2
      ? `<span class="${highlightClass.value}">${escape(part)}</span>`
      : escape(part))
    .join('')
}

(Move background colors into CSS classes for maintainability.)

Summary

The refactor is a clear improvement — fixes correctness, security (XSS), and readability. Remaining nits are minor: trim-check the keyword, unify styling via CSS classes/variables, and optionally cache the regex.

Expand Down
Loading