Skip to content

feat: added search navigate to selected item #1711#2428

Merged
kieronlanning merged 2 commits intokieron/shared-filesfrom
kieron/highlight-search-result
Dec 5, 2025
Merged

feat: added search navigate to selected item #1711#2428
kieronlanning merged 2 commits intokieron/shared-filesfrom
kieron/highlight-search-result

Conversation

@kieronlanning
Copy link
Copy Markdown
Collaborator

Checklist

  • I've thoroughly read the latest contribution guidelines.
  • I've rebased my branch onto main before creating this PR.
  • My commit messages follow conventional spec
  • I've added tests to cover my changes (if applicable).
  • I've verified that all new and existing tests have passed locally for mobile, tablet, and desktop screen sizes.
  • My change requires documentation updates.
  • I've updated the documentation accordingly.

Copilot AI review requested due to automatic review settings December 5, 2025 15:35
@kieronlanning kieronlanning merged commit b7a69c4 into kieron/shared-files Dec 5, 2025
5 checks passed
@kieronlanning kieronlanning deleted the kieron/highlight-search-result branch December 5, 2025 15:35
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR implements a feature that allows users to navigate to and focus on specific elements when selecting them from the search interface. When a user searches for and selects an element or view, the diagram now highlights the selected element by focusing on it, improving the search-to-diagram workflow.

Key changes:

  • Added focusOnElement parameter to navigation API to enable focusing on specific elements after view changes
  • Implemented auto-unfocus mechanism that bypasses the FocusMode feature requirement for search-initiated focus
  • Added new focusOnElement() API method for focusing elements by FQN on the current view

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
packages/diagram/src/search/components/ViewsColum.tsx Updated ViewButton to accept focusOnElement prop and handle same-view focus scenarios
packages/diagram/src/search/components/PickView.tsx Passes elementFqn as focusOnElement to ViewButton components
packages/diagram/src/search/components/ElementsColumn.tsx Modified element selection handler to focus on element when in same view or navigate with focus
packages/diagram/src/likec4diagram/state/machine.ts Added autoUnfocusTimer to context for tracking search-initiated focus
packages/diagram/src/likec4diagram/state/machine.state.ready.idle.ts Updated focus.node event handling to prioritize autoUnfocus guard over FocusMode requirement
packages/diagram/src/likec4diagram/state/machine.state.ready.focused.ts Added auto-unfocus timer start/cancel actions and focus.autoUnfocus event handler
packages/diagram/src/likec4diagram/state/machine.state.ready.focused.spec.ts Added tests for autoUnfocus flag and timer context (structure-only, no behavior tests)
packages/diagram/src/likec4diagram/state/machine.state.navigating.ts Implemented findNodeByElementFqn utility and integrated focusOnElement into navigation flow
packages/diagram/src/likec4diagram/state/machine.state.navigating.spec.ts Added tests for findNodeByElementFqn utility function
packages/diagram/src/likec4diagram/state/machine.setup.ts Extended Events and Context types with focusOnElement and autoUnfocusTimer, added autoUnfocus guard
packages/diagram/src/likec4diagram/state/machine.actions.ts Implemented startAutoUnfocusTimer and cancelAutoUnfocusTimer actions, updated assignFocusedNode
packages/diagram/src/likec4diagram/state/diagram-api.ts Added focusOnElement method and extended navigateTo with focusOnElement parameter
Comments suppressed due to low confidence (1)

packages/diagram/src/likec4diagram/state/machine.state.ready.idle.ts:2

  • Unused import log.
import { enqueueActions, log, raise } from 'xstate/actions'

Comment on lines +1 to +64
import { describe, expect, it } from 'vitest'

describe('focus.node event with autoUnfocus', () => {
it('focus.node event can include autoUnfocus flag', () => {
const focusEvent = {
type: 'focus.node' as const,
nodeId: 'node1',
autoUnfocus: true,
}

expect(focusEvent.type).toBe('focus.node')
expect(focusEvent.autoUnfocus).toBe(true)
})

it('focus.node event works without autoUnfocus flag', () => {
const focusEvent: { type: 'focus.node'; nodeId: string; autoUnfocus?: boolean } = {
type: 'focus.node' as const,
nodeId: 'node1',
}

expect(focusEvent.type).toBe('focus.node')
expect(focusEvent.autoUnfocus).toBeUndefined()
})

it('autoUnfocus flag can be false for manual focus', () => {
const focusEvent = {
type: 'focus.node' as const,
nodeId: 'node1',
autoUnfocus: false,
}

expect(focusEvent.autoUnfocus).toBe(false)
})
})

describe('focus.autoUnfocus event', () => {
it('focus.autoUnfocus event type exists', () => {
const autoUnfocusEvent = {
type: 'focus.autoUnfocus' as const,
}

expect(autoUnfocusEvent.type).toBe('focus.autoUnfocus')
})
})

describe('autoUnfocusTimer context', () => {
it('context can track autoUnfocusTimer state', () => {
const context = {
focusedNode: 'node1',
autoUnfocusTimer: true,
}

expect(context.autoUnfocusTimer).toBe(true)
})

it('autoUnfocusTimer defaults to false', () => {
const context = {
focusedNode: null,
autoUnfocusTimer: false,
}

expect(context.autoUnfocusTimer).toBe(false)
})
})
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

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

These tests only verify basic data structure properties and don't test actual state machine behavior. Consider adding integration tests that:

  1. Create an actor instance and verify the state transitions when focus.node is sent with autoUnfocus flag
  2. Test that focus.autoUnfocus event transitions from focused to idle state
  3. Test that the autoUnfocusTimer context property is correctly set/cleared
  4. Test the guard 'focus.node: autoUnfocus' works correctly

The current tests are essentially type checks disguised as unit tests and don't provide meaningful coverage of the feature's behavior.

Copilot uses AI. Check for mistakes.
Comment on lines +67 to +115
describe('navigate.to with focusOnElement', () => {
describe('event types', () => {
it('navigate.to event can include optional focusOnElement parameter', () => {
// Type test - this compiles successfully if the types are correct
const navigateEvent = {
type: 'navigate.to' as const,
viewId: 'view1' as const,
focusOnElement: 'cloud.api' as Fqn,
}

expect(navigateEvent.type).toBe('navigate.to')
expect(navigateEvent.focusOnElement).toBe('cloud.api')
})

it('navigate.to event works without focusOnElement parameter', () => {
const navigateEvent = {
type: 'navigate.to' as const,
viewId: 'view1' as const,
}

expect(navigateEvent.type).toBe('navigate.to')
expect(navigateEvent.focusOnElement).toBeUndefined()
})
})
})

describe('lastOnNavigate context', () => {
it('can store focusOnElement in navigation context', () => {
const lastOnNavigate = {
fromView: 'view1',
toView: 'view2',
fromNode: null,
focusOnElement: 'cloud.api' as Fqn,
}

expect(lastOnNavigate.focusOnElement).toBe('cloud.api')
})

it('focusOnElement can be null', () => {
const lastOnNavigate = {
fromView: 'view1',
toView: 'view2',
fromNode: null,
focusOnElement: null,
}

expect(lastOnNavigate.focusOnElement).toBeNull()
})
})
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

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

The tests for findNodeByElementFqn are good, but the tests for navigation behavior (lines 67-115) don't actually test state machine behavior. They only verify that event objects can be created with certain properties. Consider adding integration tests that:

  1. Test the actual navigation flow with focusOnElement parameter
  2. Verify that findNodeByElementFqn is called with the correct parameters during navigation
  3. Test that focus.node event is raised with autoUnfocus=true when a matching node is found
  4. Test that raiseFitDiagram is called when no focusOnElement is provided

Current tests provide coverage for the utility function but not the feature integration.

Copilot uses AI. Check for mistakes.
Comment on lines +220 to +226
focusOnElement: (elementFqn: Fqn<A>) => {
const context = actorRef.current.getSnapshot().context
const node = context.xynodes.find(n => 'modelFqn' in n.data && n.data.modelFqn === elementFqn)
if (node) {
actorRef.current.send({ type: 'focus.node', nodeId: node.id as NodeId, autoUnfocus: true })
}
},
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

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

The logic for finding a node by element FQN is duplicated here and in machine.state.navigating.ts (line 16-22). Consider extracting the findNodeByElementFqn function to a shared location (e.g., utils.ts) and reusing it here:

focusOnElement: (elementFqn: Fqn<A>) => {
  const context = actorRef.current.getSnapshot().context
  const nodeId = findNodeByElementFqn(context.xynodes, elementFqn)
  if (nodeId) {
    actorRef.current.send({ type: 'focus.node', nodeId, autoUnfocus: true })
  }
},

This improves maintainability and ensures consistency across the codebase.

Copilot uses AI. Check for mistakes.
Comment on lines +768 to +780
/**
* Auto-unfocus timer duration in milliseconds.
* Set to 0 to disable auto-unfocus (focus remains until user clicks elsewhere).
* Used for focusing nodes from search results.
*/
const AUTO_UNFOCUS_DELAY = 0

export const startAutoUnfocusTimer = () =>
machine.enqueueActions(({ context, enqueue }) => {
if (context.autoUnfocusTimer && AUTO_UNFOCUS_DELAY > 0) {
enqueue.raise({ type: 'focus.autoUnfocus' }, { delay: AUTO_UNFOCUS_DELAY, id: 'autoUnfocusTimer' })
}
})
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

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

With AUTO_UNFOCUS_DELAY set to 0, the auto-unfocus timer is effectively disabled and the condition on line 777 will never be true. This means:

  1. The startAutoUnfocusTimer action does nothing
  2. The focus.autoUnfocus event will never be raised
  3. The focused state will never automatically return to idle

This creates dead code. Either:

  • Remove the auto-unfocus timer feature entirely if it's not needed
  • Set a non-zero delay if auto-unfocus is intended
  • Add a comment explaining why this is intentionally disabled

The documentation comment suggests this is configurable, but having it disabled by default in production code is confusing.

Copilot uses AI. Check for mistakes.
Comment on lines +214 to +220
if (nodeToFocus) {
// Focus on the searched element with auto-unfocus enabled
enqueue.raise({
type: 'focus.node',
nodeId: nodeToFocus,
autoUnfocus: true,
}, { delay: 150 })
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

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

[nitpick] The delay when focusing a node (150ms) is longer than the default fit diagram delay (100ms). This inconsistency could lead to unpredictable UI behavior. Consider:

  1. Using a consistent delay value, perhaps extracted as a constant:
const NAVIGATION_DELAY = 150 // or 100
  1. Or documenting why focus needs a longer delay than fit diagram. The longer delay might be intentional to allow the view to settle, but it's not clear from the code.

Additionally, consider if these hardcoded delays could be replaced with proper event sequencing or state transitions instead of relying on setTimeout-based timing.

Copilot uses AI. Check for mistakes.
target: targetState.focused,
},
'focus.node': [
// Focus was initialed by the user searching (autoUnfocus=true) - always allowed
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

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

The comment says "Focus was initialed by the user searching" but "initialed" should be "initiated".

Suggested change
// Focus was initialed by the user searching (autoUnfocus=true) - always allowed
// Focus was initiated by the user searching (autoUnfocus=true) - always allowed

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants