- 
                Notifications
    You must be signed in to change notification settings 
- Fork 0
Sets variable query refId manually #47
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
Conversation
This ensures that variable queries have their `refId` set, which is required for Grafana to associate the response with the request. I can't seem to find documentation about this, but it's reflected in other custom variable support implementations: https://github.com/grafana/grafana/blob/15293a2ceb083108c0f15490933db523aa915c7d/public/app/plugins/datasource/loki/components/VariableQueryEditor.tsx#L71
| Levitate is-compatible report: 🔍 Resolving @grafana/data@latest... 🔬 Checking compatibility between ./src/module.ts and @grafana/[email protected]... 🔬 Checking compatibility between ./src/module.ts and @grafana/[email protected]... 🔬 Checking compatibility between ./src/module.ts and @grafana/[email protected]... 🔬 Checking compatibility between ./src/module.ts and @grafana/[email protected]... 🔬 Checking compatibility between ./src/module.ts and @grafana/[email protected]... 🔬 Checking compatibility between ./src/module.ts and @grafana/[email protected]... ✔️ ./src/module.ts appears to be compatible with @grafana/data,@grafana/ui,@grafana/runtime,@grafana/schema,@grafana/e2e-selectors,@grafana/experimental | 
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.
Pull Request Overview
This PR fixes a missing refId on variable queries so that variable options populate correctly and refactors the variable support into a dedicated class.
- Sets a constant refIdon all variable query edits and on blur events to associate responses.
- Moves HaystackVariableSupportout ofdatasource.tsinto its own file with a cleaner implementation.
- Updates the DataSource constructor to wire up the new HaystackVariableSupport.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description | 
|---|---|
| src/datasource.ts | Replaced inline variable support with new class import and constructor wiring | 
| src/components/VariableQueryEditor.tsx | Added refIdhandling on change and blur; renamed exported component | 
| src/HaystackVariableSupport.ts | New file: encapsulates variable query logic from datasource.ts | 
Comments suppressed due to low confidence (4)
src/components/VariableQueryEditor.tsx:13
- [nitpick] The file name VariableQueryEditor.tsxdoes not match the exported component nameHaystackVariableQueryEditor. Consider renaming the file toHaystackVariableQueryEditor.tsxor aligning the component name with the file name for consistency.
export const HaystackVariableQueryEditor = ({ onChange, query }: Props) => {
src/HaystackVariableSupport.ts:26
- The new querylogic inHaystackVariableSupportprocesses frame data and handlesisRefconversions—consider adding unit tests to cover both string and ref scenarios to ensure correct behavior.
  query(request: DataQueryRequest<HaystackVariableQuery>): Observable<DataQueryResponse> {
src/HaystackVariableSupport.ts:19
- Check the generic parameters passed to CustomVariableSupport. The previous implementation included more type arguments for query and options—ensure this signature correctly enforces type safety for variable queries.
export class HaystackVariableSupport extends CustomVariableSupport<DataSource, HaystackVariableQuery> {
src/components/VariableQueryEditor.tsx:11
- [nitpick] Using a constant refId for all variable queries may lead to collisions when multiple variables are evaluated. Consider generating a unique refId per instance or parameterizing it to avoid duplicates.
const refId = 'HaystackVariableQueryEditor-VariableQuery';
This reworks the HaystackVariableSupport to align with the Loki plugin's use of CustomVariableSupport: https://github.com/grafana/grafana/blob/15293a2ceb083108c0f15490933db523aa915c7d/public/app/plugins/datasource/loki/LokiVariableSupport.ts#L10 Since I didn't really use any source example when implementing it initially, it ended up being kinda messy.
3218f3d    to
    03ad6cc      
    Compare
  
    
This fixes an issue where the Variable page was not populating the variable options. Although a correct response was received, since the request was missing the
refId, the response was not associated with the request.It also refactors the variable support to a cleaner implementation.
This resolves #46