[shaohuzhang1]

fix: workflow search keyword

[shaohuzhang1]

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.

[shaohuzhang1]

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.

[shaohuzhang1]

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.