-
Notifications
You must be signed in to change notification settings - Fork 159
feat: mention support in developer chat #8477
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.
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
base: main
Are you sure you want to change the base?
Conversation
bc6dfb2 to
5c2dade
Compare
6a974a3 to
ec43819
Compare
0096f76 to
58e7844
Compare
58e7844 to
1f28f16
Compare
ericpgreen2
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UXQA: I'm finding it very hard to use the keyboard to move around in the dropdown menu.
- I type
@to open the menu, I press the down arrow, and I'd expect to move to the second item in the list. Instead, the first metrics view is "opened" and a measure is in focus. Further, sometimes I've found that it focuses a random measure (e.g. see the second image below), not the first one in the list. For inspiration, Cursor does a good job with keyboard navigation in their context picker.


- Sometimes I have to hit the up/down arrow key twice in order to navigate to the next item in the dropdown menu
- When I type
@ad, I see "No matches found". That messaging makes it feel like there's nothing to select. But, in fact, in this case, I can select the metrics view or the model. We should be more specific about what's "not found".

- The resource icons disappear when the resource is in the expanded state (see image above)
|
Thanks @ericpgreen2 .
|
|
Up and down arrow is a common and standard shortcut for navigate up and down in the dropdown list. |
3.We can just show the metrics view/model/ without expand feature. So there is no right arrow icon show when hover on. Only selectable.
@aditya.hegde
For very complex dropdown lists, we could consider showing up/down/left/right arrow hints as a single helper message, rather than displaying shortcut indicators on each individual item.
WDYT? CC @ericpgreen2 @ericokuma |
This sounds good to me. @AdityaHegde, is it reasonable from an implementation perspective?
This sounds reasonable, though I'll specify that this feature will primarily be accessed via keyboard shortcuts, not via point-and-click, so I'm reading "on hover" as "when focused by keyboard controls".
I agree with this. However, I see that Metrics Views and Models are currently expandable. I don't think anyone will know that they're expandable unless we provide some indication that they are. (Though I also wonder if they even need to be expandable in the developer context.)
In this case, let's say a user adds a UXQA:
|
4bbec5e to
5bbfe46
Compare
ericpgreen2
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review: PR #8477
Thanks for the work on this feature. The mention support for models and columns is a valuable addition, and the architectural refactoring to support multiple entity types is heading in the right direction.
I have some concerns about complexity and code organization that need to be addressed before we merge.
Keyboard Navigation
The new HighlightManagerMetadata class addresses some of the original navigation issues, but there are still a few items:
-
Lines 155-158 in
highlight-manager.ts: The variablenonParentAvailableis misleading—the condition checks if index 1 IS a parent type, not a non-parent. Consider renaming tofirstChildIsParentor inverting the logic. -
Inconsistent return types:
highlightNextContext()andhighlightPreviousContext()returnnullon early exit butundefinedotherwise. Should bevoidthroughout.
UX Issues (from designer feedback)
- "No matches found" messaging: Still appears under expanded parents when only children are filtered out, which is confusing when the parent itself is selectable.
- Icon visibility: The stale comment at
PickerGroup.svelte:92says "On hover show chevron right icon" but no hover logic exists.
Complexity Concerns
The picker/ directory has grown complex. Here are specific concerns with suggested solutions:
1. Separate UI State from Data
Current problem:
// data.ts - UI state created during data fetching
return {
context: { ... },
openStore: writable(false), // UI state mixed with data
childrenQueryOptions: ...,
} satisfies InlineContextPickerParentOption;Every parent option carries its own openStore. This means:
- UI state is recreated if the query refetches
- The highlight manager must reach into data objects to check
get(parentOption.openStore) - Testing data transformations requires mocking Svelte stores
Recommended approach — create a dedicated UI state manager:
// context-picker/ui-state.ts
import { writable, derived } from "svelte/store";
const expandedParents = writable<Set<string>>(new Set());
export const pickerUIState = {
isExpanded: (parentValue: string) =>
derived(expandedParents, ($expanded) => $expanded.has(parentValue)),
expand: (parentValue: string) => {
expandedParents.update((set) => new Set(set).add(parentValue));
},
collapse: (parentValue: string) => {
expandedParents.update((set) => {
const next = new Set(set);
next.delete(parentValue);
return next;
});
},
toggle: (parentValue: string) => {
expandedParents.update((set) => {
const next = new Set(set);
if (next.has(parentValue)) next.delete(parentValue);
else next.add(parentValue);
return next;
});
},
};Then the data types become pure data:
// types.ts - no more Writable<boolean>
export type InlineContextPickerParentOption = {
context: InlineContext;
recentlyUsed?: boolean;
currentlyActive?: boolean;
children?: InlineContextPickerChildSection[];
childrenLoading?: boolean;
};2. Flatten the Data Structure
Current hierarchy (4 levels):
Section // Groups parents by type
└─ ParentOption // A metrics view or model
└─ ChildSection // Groups children by type
└─ InlineContext // The actual selectable item
Recommended flatter structure:
// types.ts
export type PickerItem = {
context: InlineContext;
parentValue?: string; // undefined for top-level items
recentlyUsed?: boolean;
currentlyActive?: boolean;
childrenLoading?: boolean;
hasChildren?: boolean;
};
export type PickerData = {
items: PickerItem[];
parentChildMap: Map<string, string[]>; // parentValue -> childValues[]
};Benefits:
- Keyboard navigation becomes trivial (iterate flat list, skip based on collapsed state)
- The highlight manager doesn't need
closestParentIndicesprecomputation - Filtering is a simple
.filter()on the flat list - Sectioning/grouping happens at render time
Render-time grouping:
// Pure function, easy to test
function getVisibleItems(
data: PickerData,
expanded: Set<string>,
searchText: string
): PickerItem[] {
return data.items.filter((item) => {
if (item.parentValue && !expanded.has(item.parentValue)) {
return false;
}
if (searchText && !matchesSearch(item, searchText)) {
return false;
}
return true;
});
}3. Split data.ts into Focused Modules
Current data.ts responsibilities:
- Fetch metrics views and transform to picker format
- Fetch models and transform to picker format
- Create lazy query options for model columns
- Combine all data sources
- Track recently used / currently active status
- Filter by search text
- Group into sections
Recommended split:
context-picker/
├── data/
│ ├── metrics-views.ts # Fetch and transform metrics views
│ ├── models.ts # Fetch and transform models
│ └── index.ts # Combine data sources
├── filters.ts # Search filtering logic
├── ui-state.ts # Expanded state management
├── keyboard-navigation.ts # Was highlight-manager.ts
└── types.ts
Example: data/metrics-views.ts
export function getMetricsViewItems(): Readable<PickerItem[]> {
const query = createQuery(getValidMetricsViewsQueryOptions(), queryClient);
return derived(query, (resp) => {
const items: PickerItem[] = [];
for (const mv of resp.data ?? []) {
const mvName = mv.meta?.name?.name ?? "";
const spec = mv.metricsView?.state?.validSpec ?? {};
// Add the parent
items.push({
context: {
type: InlineContextType.MetricsView,
value: mvName,
label: spec.displayName || mvName,
metricsView: mvName,
},
hasChildren: true,
});
// Add children inline
for (const m of spec.measures ?? []) {
items.push({
context: { type: InlineContextType.Measure, ... },
parentValue: mvName,
});
}
// ... dimensions
}
return items;
});
}4. Simplify the Reactive Chain
Current pattern (nested derived with manual subscription):
// data.ts lines 48-103
function getPickerOptions(): Readable<InlineContextPickerParentOption[]> {
return derived(
[getMetricsViewPickerOptions(), getModelsPickerOptions()],
([metricsViewOptions, filesOption], set) => {
const subOptionStores = allOptions.map((o) =>
o.childrenQueryOptions ? createQuery(...) : readable(null)
);
// Create ANOTHER derived store inside
const resolvedOptionsStore = derived(
[lastUsedMv, activeMv, activeFile, ...subOptionStores],
([...]) => { ... }
);
// Manually subscribe and return cleanup
return resolvedOptionsStore.subscribe((resolved) => set(resolved));
},
);
}Problems:
- Hard to follow the data flow
- Manual subscription management is error-prone
- Difficult to debug which store triggered an update
Recommended: Single-level derivation with explicit dependencies
export function getPickerData(): Readable<PickerData> {
const metricsViewItems = getMetricsViewItems();
const modelItems = getModelItems();
const lastUsedMv = getLastUsedMetricsViewNameStore();
const activeMv = getActiveMetricsViewNameStore();
const activeFile = getActiveFileArtifactStore();
return derived(
[metricsViewItems, modelItems, lastUsedMv, activeMv, activeFile],
([$mvItems, $modelItems, $lastUsedMv, $activeMv, $activeFile]) => {
const items = [...$mvItems, ...$modelItems];
// Mark recently used / currently active
for (const item of items) {
if (item.context.type === InlineContextType.MetricsView) {
item.recentlyUsed = item.context.value === $lastUsedMv;
item.currentlyActive = item.context.value === $activeMv;
}
}
// Build parent-child map
const parentChildMap = new Map<string, string[]>();
for (const item of items) {
if (item.parentValue) {
const siblings = parentChildMap.get(item.parentValue) ?? [];
siblings.push(item.context.value);
parentChildMap.set(item.parentValue, siblings);
}
}
return { items, parentChildMap };
}
);
}Naming Suggestions
| Current | Suggestion | Reason |
|---|---|---|
picker/ |
context-picker/ |
More explicit about what's being picked |
highlight-manager.ts |
keyboard-navigation.ts |
"Highlight" is ambiguous |
PickerOptionsHighlightManager |
KeyboardNavigationManager |
Clearer purpose |
HighlightManagerMetadata |
NavigationIndex |
Describes what it is |
highlightedContext |
focusedContext |
Distinguishes from visual highlight |
filterOptionsUpdated() |
setOptions() |
Current name sounds like an event |
ensureIsHighlighted |
createMouseFocusHandler |
Describes what it does |
Minor Items
- TODO comment at
inline-context.ts:31:columnType?: string; // TODO: is this needed here?should be resolved. - Unused method:
HighlightManagerMetadata.contains()is never called.
Happy to pair on any of these if helpful.
Developed in collaboration with Claude Code
ericpgreen2
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review: PR #8477 (Re-review)
Great work on the refactoring. The architecture is now much cleaner—UI state is separated from data, the flat PickerItem structure is easier to reason about, and the modular file organization follows good separation of concerns.
A few items to address before merge:
Architecture
-
ActiveFileArtifacttype (nav-utils.ts): This new type wrapsV1ResourceNamewith an unusedfilePathfield. Consider usingV1ResourceNamedirectly:export function getActiveResourceNameStore(): Readable<V1ResourceName | undefined> { const activeFilePathStore = derived( page, (pageState) => pageState.params?.file ?? "", ); return derived(activeFilePathStore, (filePath, set) => { if (!filePath) { set(undefined); return; } const fileArtifact = fileArtifacts.getFileArtifact(addLeadingSlash(filePath)); return fileArtifact.resourceName.subscribe(set); }); }
This is generic for any resource kind, uses an existing type, and the
ActiveFileArtifacttype can be deleted. -
getInlineChatContextMetadata()(metadata.ts): Only fetches MetricsView data, but the name suggests it handles all context types. Consider renaming togetMetricsViewMetadata(). Since this is MetricsView-specific, it may also belong inpicker/data/metrics-views.tsalongside the other MetricsView logic. -
Nested derived pattern (
data/models.ts:76-89): Still uses nested derived with manual subscription.
Naming
- "NonLeaf" / "Leaf" terminology (
tree/NonLeafOption.svelte,tree/LeafOption.svelte): "NonLeaf" is awkward because it's a negation—it describes what something isn't rather than what it is. Consider renaming both components to a matching pair:ExpandableOption/SimpleOptionParentOption/ChildOptionPickerGroup/PickerItem
Small Fixes
-
build-picker-tree.ts:39-42: Boundary logic appears inverted—adds boundaries when types match rather than when they change. -
keyboard-navigation.ts:62: Typo "Focuse" → "Focus". -
ensureInView.ts: Add a brief comment explaining this Svelte action.
UX (from designer feedback)
- "No matches found" messaging: Still shows under expanded parents when only children are filtered out.
Developed in collaboration with Claude Code
|
Thanks @ericpgreen2
Can you share more details around |
|
All sounds reasonable.
Oh, I don't see it anymore. Looks good! |
Adds the mention-like inline context to developer chat.
Adds models and their columns to dev chat for rill developer context. This is the 1st iteration, we will probably add more of these.
Known issue: Using keyboard to move to model doesnt automatically uncollapse it because the options are not loaded. Will do a follow up that does some significant changes to the highlight manager in a separate PR.
Checklist: