Skip to content
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.

Sign up for GitHub

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

feat: Enhance dbt-power-user Side Panel to Show Model Info for Non-SQL Files #1475

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

feat: Enhance dbt-power-user Side Panel to Show Model Info for Non-SQL Files #1475

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0e8750f
Open Panel other than SQL files
toukoudo Oct 21, 2024
1e8243e
Open the appropriate model in a yaml file with multiple models
toukoudo Oct 27, 2024
adc62fc
fix YAML error check (CodeRabbit)
toukoudo Nov 10, 2024
5984671
fix format(Code Rabbit)
toukoudo Nov 10, 2024
148516a
Merge branch 'master' into open-panel-other-than-sql
anandgupta42 Dec 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 38 additions & 11 deletions src/treeview_provider/modelTreeviewProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { unmanaged } from "inversify";
import { provide } from "inversify-binding-decorators";
import * as path from "path";

import {
Command,
Disposable,
Expand Down Expand Up @@ -29,7 +30,7 @@ import {
ManifestCacheChangedEvent,
ManifestCacheProjectAddedEvent,
} from "../manifest/event/manifestCacheChangedEvent";
import { provideSingleton } from "../utils";
import { getModelNameInActiveEditor, provideSingleton } from "../utils";

@provide(ModelTreeviewProvider)
abstract class ModelTreeviewProvider
Expand All @@ -54,6 +55,9 @@ abstract class ModelTreeviewProvider
this.dbtProjectContainer.onManifestChanged((event) =>
this.onManifestCacheChanged(event),
),
window.onDidChangeTextEditorSelection(() => {
this._onDidChangeTreeData.fire();
}),
);
}

Expand Down Expand Up @@ -104,15 +108,28 @@ abstract class ModelTreeviewProvider
if (element) {
return Promise.resolve(this.getTreeItems(element.key, event));
}
const fileName = path.basename(
window.activeTextEditor!.document.fileName,
".sql",

const model_by_file_content = event.nodeMetaMap.lookupByBaseName(
getModelNameInActiveEditor(),
);
const model = event.nodeMetaMap.lookupByBaseName(fileName);
if (!model) {
return Promise.resolve([]);

if (model_by_file_content) {
return Promise.resolve(
this.getTreeItems(model_by_file_content.uniqueId, event),
);
}
return Promise.resolve(this.getTreeItems(model.uniqueId, event));

const fileName = path.parse(
window.activeTextEditor!.document.fileName,
).name;
const model_by_file_name = event.nodeMetaMap.lookupByBaseName(fileName);
if (model_by_file_name) {
return Promise.resolve(
this.getTreeItems(model_by_file_name.uniqueId, event),
);
}
Comment on lines +112 to +130
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Extract shared model lookup logic to reduce code duplication

The model lookup logic is duplicated between ModelTreeviewProvider and DocumentationTreeviewProvider. Consider extracting it into a shared utility function.

Create a new utility function:

async function lookupModelByEditorContent(
  nodeMetaMap: NodeMetaMap,
  fileName: string
): Promise<Node | undefined> {
  try {
    // Try content-based lookup first
    const modelName = await getModelNameInActiveEditor();
    const model = nodeMetaMap.lookupByBaseName(modelName);
    if (model) return model;

    // Fall back to filename-based lookup
    return nodeMetaMap.lookupByBaseName(path.parse(fileName).name);
  } catch (error) {
    console.warn('Failed to lookup model:', error);
    return undefined;
  }
}

Then use it in both providers:

-   const model_by_file_content = event.nodeMetaMap.lookupByBaseName(
-     getModelNameInActiveEditor(),
-   );
-
-   if (model_by_file_content) {
-     return Promise.resolve(
-       this.getTreeItems(model_by_file_content.uniqueId, event),
-     );
-   }
-
-   const fileName = path.parse(
-     window.activeTextEditor!.document.fileName,
-   ).name;
-   const model_by_file_name = event.nodeMetaMap.lookupByBaseName(fileName);
-   if (model_by_file_name) {
-     return Promise.resolve(
-       this.getTreeItems(model_by_file_name.uniqueId, event),
-     );
-   }
+   const currentNode = await lookupModelByEditorContent(
+     event.nodeMetaMap,
+     window.activeTextEditor!.document.fileName
+   );
+   if (currentNode) {
+     return this.getTreeItems(currentNode.uniqueId, event);
+   }


return Promise.resolve([]);
}

private getNodeTreeItem(node: Node): NodeTreeItem {
Expand Down Expand Up @@ -180,6 +197,9 @@ class DocumentationTreeviewProvider implements TreeDataProvider<DocTreeItem> {
this.dbtProjectContainer.onManifestChanged((event) =>
this.onManifestCacheChanged(event),
),
window.onDidChangeTextEditorSelection(() => {
this._onDidChangeTreeData.fire();
}),
);
}

Expand Down Expand Up @@ -222,14 +242,21 @@ class DocumentationTreeviewProvider implements TreeDataProvider<DocTreeItem> {
const { nodeMetaMap } = event;

if (!element) {
const modelName = path.basename(
const fileName = path.parse(
window.activeTextEditor!.document.fileName,
".sql",
).name;
const currentNodeByFileName =
event.nodeMetaMap.lookupByBaseName(fileName);
const currentNodeByFileContent = event.nodeMetaMap.lookupByBaseName(
getModelNameInActiveEditor(),
);
const currentNode = nodeMetaMap.lookupByBaseName(modelName);

const currentNode = currentNodeByFileContent || currentNodeByFileName;
if (currentNode === undefined) {
return Promise.resolve([]);
}
const modelName = currentNode.name;

Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Align implementation with ModelTreeviewProvider and add validation

There are two concerns with the current implementation:

  1. The model lookup logic differs from ModelTreeviewProvider. While ModelTreeviewProvider tries content-based lookup first and falls back to filename-based lookup, this implementation uses OR operator which might lead to unexpected behavior.

  2. The model name is used without validation, which could cause issues if both lookups fail.

Consider this improvement:

-     const currentNodeByFileName =
-       event.nodeMetaMap.lookupByBaseName(fileName);
-     const currentNodeByFileContent = event.nodeMetaMap.lookupByBaseName(
-       getModelNameInActiveEditor(),
-     );
-
-     const currentNode = currentNodeByFileContent || currentNodeByFileName;
+     // Try content-based lookup first
+     const currentNodeByFileContent = event.nodeMetaMap.lookupByBaseName(
+       getModelNameInActiveEditor().catch(error => {
+         console.warn('Failed to parse editor content:', error);
+         return undefined;
+       })
+     );
+     
+     // Fall back to filename-based lookup
+     const currentNode = currentNodeByFileContent ?? 
+       event.nodeMetaMap.lookupByBaseName(fileName);

      if (currentNode === undefined) {
        return Promise.resolve([]);
      }
-     const modelName = currentNode.name;
+     const modelName = currentNode?.name;

Committable suggestion skipped: line range outside the PR's diff.

const children = [];

if (Object.keys(currentNode.columns).length !== 0) {
Expand Down
40 changes: 39 additions & 1 deletion src/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,10 @@ import {
TextDocument,
Uri,
workspace,
window,
} from "vscode";
import { readFileSync } from "fs";
import { parse } from "yaml";
import { parse, parseDocument } from "yaml";
import {
TestMetadataAcceptedValues,
TestMetadataRelationships,
Expand Down Expand Up @@ -359,3 +360,40 @@ export const getStringSizeInMb = (str: string): number => {
const sizeInMB = sizeInBytes / (1024 * 1024);
return sizeInMB;
};

export function getModelNameInActiveEditor(): string {
if (
window.activeTextEditor === undefined ||
window.activeTextEditor.document.languageId !== "yaml"
) {
return "";
}

const parsedYaml = parseDocument(window.activeTextEditor.document.getText());
if (parsedYaml.contents === null) {
return "";
}
const cursorPosition = window.activeTextEditor.selection.active;
const offset = window.activeTextEditor.document.offsetAt(cursorPosition);

// parseDocument returns Pair of Key-Value
// So, we need to find the node with key "models", and extract items of it
const models = (parsedYaml.contents as any).items.find(
(y: any) => y.key.value === "models",
).value.items;

// Find a model at the current position
for (const model of models) {
// each element of models is a Pair of Key-Value, and Value is Scalar Type.
// So, we need to find the node with key "name", and extract value of it by toString
const name = model.items
.find((x: any) => x.key.value === "name")
.value.toString();

if (model.range[0] < offset && offset < model.range[1]) {
return name;
}
}

return "";
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add error handling and type safety improvements.

The current implementation has several potential points of failure that should be handled more gracefully:

  1. YAML parsing errors
  2. Missing "models" key
  3. Missing "name" field in model
  4. Unsafe type assertions

Consider applying these improvements:

 export function getModelNameInActiveEditor(): string {
   if (
     window.activeTextEditor === undefined ||
     window.activeTextEditor.document.languageId !== "yaml"
   ) {
     return "";
   }

-  const parsedYaml = parseDocument(window.activeTextEditor.document.getText());
-  if (parsedYaml.contents === null) {
-    return "";
-  }
+  try {
+    const parsedYaml = parseDocument(window.activeTextEditor.document.getText());
+    if (parsedYaml.contents === null) {
+      return "";
+    }
+    const cursorPosition = window.activeTextEditor.selection.active;
+    const offset = window.activeTextEditor.document.offsetAt(cursorPosition);
+
+    const contents = parsedYaml.contents as { items?: Array<any> };
+    if (!contents.items) {
+      return "";
+    }
+
+    const modelsNode = contents.items.find(
+      (item) => item?.key?.value === "models"
+    );
+    if (!modelsNode?.value?.items) {
+      return "";
+    }
+
+    // Find a model at the current position
+    for (const model of modelsNode.value.items) {
+      if (!model?.items) continue;
+
+      const nameNode = model.items.find(
+        (item) => item?.key?.value === "name"
+      );
+      if (!nameNode?.value) continue;
+
+      if (model.range?.[0] < offset && offset < model.range?.[1]) {
+        return nameNode.value.toString();
+      }
+    }
+  } catch (error) {
+    console.error("Error parsing YAML document:", error);
+  }
+  return "";
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export function getModelNameInActiveEditor(): string {
if (
window.activeTextEditor === undefined ||
window.activeTextEditor.document.languageId !== "yaml"
) {
return "";
}
const parsedYaml = parseDocument(window.activeTextEditor.document.getText());
if (parsedYaml.contents === null) {
return "";
}
const cursorPosition = window.activeTextEditor.selection.active;
const offset = window.activeTextEditor.document.offsetAt(cursorPosition);
// parseDocument returns Pair of Key-Value
// So, we need to find the node with key "models", and extract items of it
const models = (parsedYaml.contents as any).items.find(
(y: any) => y.key.value === "models",
).value.items;
// Find a model at the current position
for (const model of models) {
// each element of models is a Pair of Key-Value, and Value is Scalar Type.
// So, we need to find the node with key "name", and extract value of it by toString
const name = model.items
.find((x: any) => x.key.value === "name")
.value.toString();
if (model.range[0] < offset && offset < model.range[1]) {
return name;
}
}
return "";
}
export function getModelNameInActiveEditor(): string {
if (
window.activeTextEditor === undefined ||
window.activeTextEditor.document.languageId !== "yaml"
) {
return "";
}
try {
const parsedYaml = parseDocument(window.activeTextEditor.document.getText());
if (parsedYaml.contents === null) {
return "";
}
const cursorPosition = window.activeTextEditor.selection.active;
const offset = window.activeTextEditor.document.offsetAt(cursorPosition);
const contents = parsedYaml.contents as { items?: Array<any> };
if (!contents.items) {
return "";
}
const modelsNode = contents.items.find(
(item) => item?.key?.value === "models"
);
if (!modelsNode?.value?.items) {
return "";
}
// Find a model at the current position
for (const model of modelsNode.value.items) {
if (!model?.items) continue;
const nameNode = model.items.find(
(item) => item?.key?.value === "name"
);
if (!nameNode?.value) continue;
if (model.range?.[0] < offset && offset < model.range?.[1]) {
return nameNode.value.toString();
}
}
} catch (error) {
console.error("Error parsing YAML document:", error);
}
return "";
}

💡 Codebase verification

Based on the search results, I can now provide a final response:

The YAML structure assumption in getModelNameInActiveEditor() needs revision

The function assumes all YAML files will have a specific structure with models key containing items with name fields. However, the codebase shows:

  • The function is used specifically for dbt model YAML files (indicated by the languageId check)
  • The only YAML file found (sweep.yaml) has a completely different structure
  • The code could crash when processing non-dbt YAML files due to unsafe assumptions:
    • Unchecked access to items property
    • Direct array access without null checks
    • Assumption that models key exists and has specific structure
🔗 Analysis chain

Verify YAML structure assumptions across the codebase.

Let's verify that all dbt YAML files in the project follow the expected structure.

Let me gather more information about the YAML files in the codebase to better understand their structure and usage.

Let me try one more search to specifically understand how the YAML parsing is used in the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for YAML files that might not follow the expected structure
# Expected structure: Should have 'models' key with array of objects containing 'name' field

# Find all YAML files
echo "Checking YAML files structure..."
fd -e yml -e yaml --exec sh -c '
  echo "Checking: {}"
  if ! yq eval ".models" {} >/dev/null 2>&1; then
    echo "Warning: {} - missing or invalid models key"
  elif ! yq eval ".models[].name" {} >/dev/null 2>&1; then
    echo "Warning: {} - models missing name field"
  fi
'

Length of output: 430

Script:

#!/bin/bash
# Let's examine the content of YAML files to understand their structure
echo "=== Searching for YAML files ==="
fd -e yml -e yaml -x cat {}

echo -e "\n=== Searching for YAML file usage in code ==="
rg -g '*.{ts,js}' 'parseDocument.*\.ya?ml' -A 2 -B 2

Length of output: 1734

Script:

#!/bin/bash
# Search for parseDocument usage and YAML handling
echo "=== Searching for parseDocument usage ==="
rg "parseDocument" -A 5 -B 5

echo -e "\n=== Searching for YAML-related code ==="
rg "languageId.*yaml" -A 5 -B 5

Length of output: 3542