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.

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
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
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
56 changes: 55 additions & 1 deletion src/utils.ts
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,56 @@ 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 "";
}

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: any) => 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 "";
}