Conversation
744fe90 to
1feff4f
Compare
d374c78 to
2efd371
Compare
6ec663c to
6411423
Compare
6411423 to
455ef4d
Compare
There was a problem hiding this comment.
Pull request overview
Implements a new PointerView feature area (route + components) to visualize DATEX pointers as a navigable tree, including UI primitives (switch/scroll-area/popover/etc.) and persisted display preferences.
Changes:
- Add
/pointersroute wiringPointerViewwith pointer data fromgetPointers(). - Introduce
PointerView+ recursivePointerTreeItemrendering, pointer reference jumping, and a preferences popover backed by localStorage. - Add pointer utilities/types to normalize pointer references and support DIF-driven type rendering; add new UI wrapper components.
Reviewed changes
Copilot reviewed 23 out of 24 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| src/router/index.ts | Adds /pointers route to render the new pointer inspector view. |
| src/lib/runtime.ts | Updates pointer sample generation and converts values to DIF while replacing pointer refs with IDs. |
| src/lib/pointer-utils.ts | Adds helpers for replacing Ref objects with pointer ID strings (plus additional pointer-map helpers). |
| src/lib/pointer-types.ts | Adds type detection helpers and pointer-id extraction utilities. |
| src/composable/usePointerPreferences.ts | Adds persisted display preferences composable (localStorage-backed). |
| src/components/ui/switch/index.ts | Exposes Switch UI wrapper. |
| src/components/ui/switch/Switch.vue | Adds Switch wrapper around reka-ui Switch components. |
| src/components/ui/scroll-area/index.ts | Exposes ScrollArea/ScrollBar UI wrappers. |
| src/components/ui/scroll-area/ScrollArea.vue | Adds ScrollArea wrapper around reka-ui ScrollArea primitives. |
| src/components/ui/scroll-area/ScrollBar.vue | Adds ScrollBar wrapper component. |
| src/components/ui/popover/index.ts | Exposes Popover UI wrappers. |
| src/components/ui/popover/Popover.vue | Adds Popover wrapper around reka-ui PopoverRoot. |
| src/components/ui/popover/PopoverContent.vue | Adds PopoverContent wrapper with styling/forwarded props. |
| src/components/ui/popover/PopoverTrigger.vue | Adds PopoverTrigger wrapper. |
| src/components/ui/label/index.ts | Exposes Label UI wrapper. |
| src/components/ui/label/Label.vue | Adds Label wrapper around reka-ui Label. |
| src/components/ui/checkbox/index.ts | Exposes Checkbox UI wrapper. |
| src/components/ui/checkbox/Checkbox.vue | Adds Checkbox wrapper around reka-ui Checkbox primitives. |
| src/components/PointerView.vue | New full-screen pointer browser with filters, search UI, scrolling, and preferences. |
| src/components/PointerTreeItem.vue | Recursive tree node renderer with expansion, tooltips, editing, and pointer-ref navigation. |
| src/components/PointerRefInline.vue | Inline chip for pointer references with preview tooltip and jump-to-definition click. |
| src/components/PointerPreferences.vue | Preferences UI (toggles) displayed in a popover. |
| package.json | Adds shadcn / shadcn-ui dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| props: { pointers: getPointers() }, | ||
| }, |
There was a problem hiding this comment.
props: { pointers: getPointers() } runs getPointers() eagerly at router creation time and the result is frozen for the lifetime of the router. If you want fresh runtime data per navigation (and to avoid doing runtime work before the route is visited), use a props: () => ({ pointers: getPointers() }) function (or fetch inside the view/composable).
| // Define props with defaults | ||
| const props = withDefaults(defineProps<PointerViewProps>(), { | ||
| searchPlaceholder: 'Search...', | ||
| visitedObjects: new WeakSet(), | ||
| }); |
There was a problem hiding this comment.
withDefaults(defineProps<PointerViewProps>(), { visitedObjects: new WeakSet() }) provides a default for a prop (visitedObjects) that is not declared in PointerViewProps. This will fail type-checking and is also an invalid default for a non-primitive (should be a factory). Remove this default or add the prop properly (as visitedObjects?: WeakSet<object> with () => new WeakSet()).
| // Create new visited set for children (includes current value) | ||
| const childVisitedObjects = computed(() => { | ||
| const newVisited = new WeakSet<object>(); | ||
| if (typeof props.value === 'object' && props.value !== null) { | ||
| newVisited.add(props.value); | ||
| } | ||
| return newVisited; | ||
| }); |
There was a problem hiding this comment.
Circular-reference tracking doesn’t work as implemented: childVisitedObjects creates a brand-new WeakSet containing only the current node, and WeakSet can’t be cloned/iterated to include ancestors. This means references back to ancestors won’t be detected and expanding cyclic structures can recurse indefinitely. Consider using a path-local Set<object> (cloneable) or an Array<object> of ancestors to pass down, so each branch can detect cycles correctly.
| export interface PointerPreferences { | ||
| show_full_pointer_ids: boolean | ||
| show_type_hints: boolean | ||
| show_array_indicies: boolean | ||
| hide_type_hints_for_primitives: boolean | ||
| hide_map_key_type_hints_for_primitives: boolean |
There was a problem hiding this comment.
show_array_indicies is misspelled (should be show_array_indices). Since this key is persisted to localStorage, it’s best to fix the spelling now and add a small migration in loadPreferences() (e.g., map old key to new key) to avoid locking in the typo.
| <label class="font-medium text-sm cursor-pointer"> | ||
| Full Pointer IDs | ||
| </label> | ||
| <span class="text-xs text-muted-foreground"> | ||
| Show complete pointer identifiers instead of shortened versions | ||
| </span> | ||
| </div> | ||
| <Switch | ||
| v-model="preferences.show_full_pointer_ids" | ||
| id="show-full-ids" | ||
| /> |
There was a problem hiding this comment.
The preference labels aren’t associated with the switches (no for attribute / no Label component binding). This reduces click target size and hurts accessibility for assistive tech. Wire each label to the corresponding switch id (or use the shared Label component) so clicking the text toggles the switch and screen readers get proper labeling.
| provide('pointers', props.pointers); | ||
|
|
||
| // State | ||
| const searchQuery = ref(''); |
There was a problem hiding this comment.
searchQuery is only bound to the input but never used to filter filteredPointers, so typing in the search box has no effect. Either implement query filtering in the computed map (e.g., by pointerId/value preview) or remove the search UI until it’s wired up.
| </div> | ||
|
|
||
| <!-- Tree Structure (Scrollable) --> | ||
| <ScrollArea class="flex-1.5 mb-15"> |
There was a problem hiding this comment.
class="flex-1.5 mb-15" uses Tailwind utilities that don’t exist in the default scale (flex-1.5, mb-15). Unless these are custom-defined, this will silently not style as intended. Consider using standard utilities like flex-1 and a valid spacing token (e.g. mb-4/mb-6/mb-16) or add them to the Tailwind theme config.
| <ScrollArea class="flex-1.5 mb-15"> | |
| <ScrollArea class="flex-1 mb-16"> |
| @node-click="emit('node-click', $event, childValue)" | ||
| @node-toggle="emit('node-toggle', $event)" | ||
| @pointer-ref-click="emit('pointer-ref-click', $event)" | ||
| @value-update="emit('value-update', $event, $event)" |
There was a problem hiding this comment.
Event forwarding in the recursive child is incorrect: @node-click="emit('node-click', $event, childValue)" only forwards the first emitted arg ($event) and replaces the actual emitted value with the loop variable. @value-update="emit('value-update', $event, $event)" forwards the nodeId twice and drops newValue. Use an inline handler that accepts both args (e.g. (id, val) => emit('node-click', id, val) and (id, newVal) => emit('value-update', id, newVal)).
| @node-click="emit('node-click', $event, childValue)" | |
| @node-toggle="emit('node-toggle', $event)" | |
| @pointer-ref-click="emit('pointer-ref-click', $event)" | |
| @value-update="emit('value-update', $event, $event)" | |
| @node-click="(id, val) => emit('node-click', id, val)" | |
| @node-toggle="emit('node-toggle', $event)" | |
| @pointer-ref-click="emit('pointer-ref-click', $event)" | |
| @value-update="(id, newVal) => emit('value-update', id, newVal)" |
| export function buildPointerMap( | ||
| pointerValuePairs: PointerValuePair[], | ||
| additionalValues: unknown[], | ||
| Datex: Runtime | ||
| ): Map<string, DIF.Definitions.DIFContainer> { | ||
| // Build values array: [value0, pointer0, value1, pointer1, ..., ...additionalValues] |
There was a problem hiding this comment.
convertValueWithSequentialIds / buildPointerMap don’t appear to be referenced anywhere in the repo (only replaceRefsWithIds is). Keeping unused exported code increases maintenance surface and makes the intended API unclear. Consider removing these until they’re needed, or add a concrete usage/tests to justify them.
| if (!difContainer) return '' | ||
|
|
||
| // Extract value from DIF container | ||
| const value = typeof difContainer === 'object' && difContainer !== null && 'value' in difContainer |
There was a problem hiding this comment.
Variable 'difContainer' is of type date, object or regular expression, but it is compared to an expression of type null.
Implements PointerView component with:
Status: 🚧 Work in progress