feat: added search navigate to selected item #1711#2428
feat: added search navigate to selected item #1711#2428kieronlanning merged 2 commits intokieron/shared-filesfrom
Conversation
There was a problem hiding this comment.
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
focusOnElementparameter 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'
| 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) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
These tests only verify basic data structure properties and don't test actual state machine behavior. Consider adding integration tests that:
- Create an actor instance and verify the state transitions when focus.node is sent with autoUnfocus flag
- Test that focus.autoUnfocus event transitions from focused to idle state
- Test that the autoUnfocusTimer context property is correctly set/cleared
- 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.
| 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() | ||
| }) | ||
| }) |
There was a problem hiding this comment.
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:
- Test the actual navigation flow with focusOnElement parameter
- Verify that findNodeByElementFqn is called with the correct parameters during navigation
- Test that focus.node event is raised with autoUnfocus=true when a matching node is found
- Test that raiseFitDiagram is called when no focusOnElement is provided
Current tests provide coverage for the utility function but not the feature integration.
| 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 }) | ||
| } | ||
| }, |
There was a problem hiding this comment.
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.
| /** | ||
| * 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' }) | ||
| } | ||
| }) |
There was a problem hiding this comment.
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:
- The
startAutoUnfocusTimeraction does nothing - The
focus.autoUnfocusevent will never be raised - 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.
| if (nodeToFocus) { | ||
| // Focus on the searched element with auto-unfocus enabled | ||
| enqueue.raise({ | ||
| type: 'focus.node', | ||
| nodeId: nodeToFocus, | ||
| autoUnfocus: true, | ||
| }, { delay: 150 }) |
There was a problem hiding this comment.
[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:
- Using a consistent delay value, perhaps extracted as a constant:
const NAVIGATION_DELAY = 150 // or 100- 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.
| target: targetState.focused, | ||
| }, | ||
| 'focus.node': [ | ||
| // Focus was initialed by the user searching (autoUnfocus=true) - always allowed |
There was a problem hiding this comment.
The comment says "Focus was initialed by the user searching" but "initialed" should be "initiated".
| // Focus was initialed by the user searching (autoUnfocus=true) - always allowed | |
| // Focus was initiated by the user searching (autoUnfocus=true) - always allowed |
Checklist
mainbefore creating this PR.