feat: Enhance dbt-power-user Side Panel to Show Model Info for Non-SQL Files #1475
Conversation
|
Warning Rate limit exceeded@toukoudo has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 15 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe pull request introduces significant modifications to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 1d359c0 in 17 seconds
More details
- Looked at
30lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. src/treeview_provider/modelTreeviewProvider.ts:108
- Draft comment:
Simplify the code by directly usingpath.parsewithout the intermediate variable.
const fileName = path.parse(window.activeTextEditor!.document.fileName).name;
- Reason this comment was not posted:
Confidence changes required:10%
The change frompath.basenametopath.parseis correct and aligns with the PR description. However, the code can be simplified by directly usingpath.parsewithout the intermediate variable.
2. src/treeview_provider/modelTreeviewProvider.ts:221
- Draft comment:
Please specify a return type for thegetChildrenfunction to improve code readability and maintainability. This applies to bothModelTreeviewProviderandDocumentationTreeviewProviderclasses. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_4hKAYrB8x5iEcsgz
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/treeview_provider/modelTreeviewProvider.ts (1)
224-226: LGTM! Consistent improvement in file name parsing.The change from
path.basenametopath.parsein theDocumentationTreeviewProviderclass is consistent with the earlier modification in theModelTreeviewProviderclass. This ensures that the documentation tree view can also handle non-SQL files effectively.For improved code consistency, consider extracting the file name parsing logic into a shared utility function:
function getFileNameWithoutExtension(filePath: string): string { return path.parse(filePath).name; }This function can then be used in both
ModelTreeviewProviderandDocumentationTreeviewProviderclasses, reducing code duplication and improving maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/treeview_provider/modelTreeviewProvider.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (2)
src/treeview_provider/modelTreeviewProvider.ts (2)
107-109: LGTM! Improved file name parsing for broader file type support.The change from
path.basenametopath.parseenhances the extension's capability to handle non-SQL files, aligning with the PR's objective. This modification allows the Side Panel to display model information for various file types, including dbt documentation files.
Line range hint
1-487: Overall assessment: Changes effectively implement the PR objectives.The modifications in both
ModelTreeviewProviderandDocumentationTreeviewProviderclasses successfully enhance the dbt-power-user extension to support non-SQL files, particularly dbt documentation files. The changes are minimal, focused, and consistent, which reduces the risk of introducing new issues while effectively implementing the desired functionality.To further improve the PR:
- Consider adding unit tests to verify the behavior with different file types.
- Update the
README.mdto reflect this new capability, as mentioned in the PR checklist.- Implement the suggested shared utility function for file name parsing to improve code consistency and maintainability.
Great job on enhancing the extension's functionality!
To ensure the changes work as expected across different file types, please run the following verification script:
|
@toukoudo Thanks for your contribution. How will this work if there are multiple models defined within the same markdown file? |
|
Thanks for the comment! @anandgupta42
Sadly, this PR doesn't handle multiple models in the same markdown file. (If necessary, I would like to create another pull request to address that functionality in the future! ) |
|
We should always aim at bringing something to production that works reliably. So we need to know what's the behaviour in case of multiple models. We could use the texteditor position in case of multiple models. |
|
Also when you say markdown, you mean yaml file right? Again it is better to support only yaml files if that is the ones that you tested. In case of markdown I'm not totally sure if this currently is associated with a model based on file basename. So let's only support yaml files for now. Also the yaml file name is completely free and not linked to a model. You will need to get the model name from the yaml instead. |
| const fileName = path.parse( | ||
| window.activeTextEditor!.document.fileName, | ||
| ".sql", | ||
| ); | ||
| ).name; |
There was a problem hiding this comment.
We should adapt this code to follow this logic for sql files, but different logic for yaml files. to use the texteditor position in case of multiple models within the yaml file, or select the single model in case of a single model in the yaml file
|
Thank you for the feedback! I'll work on implementing the code to extract the model name from the YAML file itself, rather than relying on the file name. |
toukoudo
force-pushed
the
open-panel-other-than-sql
branch
from
4a99930
to
1e8243e
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
src/utils.ts (1)
379-383: Add documentation for YAML structure assumptions.The code makes assumptions about the YAML structure that should be documented for maintainability.
Add a comment block explaining the expected YAML structure:
+ // Expected YAML structure: + // ```yaml + // models: + // - name: model_name + // # ... other model properties + // ``` const models = (parsedYaml.contents as any).items.find( (y: any) => y.key.value === "models", ).value.items;src/treeview_provider/modelTreeviewProvider.ts (1)
112-130: Consider implementing position-based model selectionAs discussed in the PR comments, the current implementation doesn't handle multiple models within the same YAML file. While the
onDidChangeTextEditorSelectionevent listener is in place, the actual position-based model selection logic is missing.Consider these improvements for a future PR:
- Enhance
getModelNameInActiveEditorto accept cursor position as a parameter- Use the cursor position to determine which model definition the cursor is currently in
- Return the model name based on the YAML structure at that position
- Add tests to verify the behavior with multiple models in a single file
Would you like me to create a GitHub issue to track this enhancement?
Also applies to: 245-259
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/treeview_provider/modelTreeviewProvider.ts(6 hunks)src/utils.ts(2 hunks)
🔇 Additional comments (2)
src/utils.ts (1)
11-11: LGTM: Import additions are appropriate.
The new imports are necessary for the enhanced functionality:
windowenables active editor interactionparseDocumentprovides structured YAML parsing capabilities
Also applies to: 14-14
src/treeview_provider/modelTreeviewProvider.ts (1)
58-60: LGTM: Event listener implementation for selection changes
The addition of onDidChangeTextEditorSelection event listeners in both providers enables dynamic updates of the tree view when the cursor position changes, which is essential for handling multiple models in a file.
Let's verify the refresh behavior:
Also applies to: 200-202
✅ Verification successful
Let me gather more context about the event handling implementation to ensure consistency.
Let me check one more aspect to understand the complete refresh mechanism and its triggers.
Based on the gathered evidence, I can now provide the final response:
Tree refresh implementation is consistent and well-integrated
The onDidChangeTextEditorSelection event listener's implementation is correctly integrated with the existing refresh mechanisms:
- It properly fires the tree data change event using
_onDidChangeTreeData.fire() - The refresh behavior aligns with other refresh triggers in the codebase
- The implementation is consistent across both providers and works alongside the manifest change events
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any other places in the codebase where tree refresh is handled
# to ensure consistent behavior across the extension.
# Search for tree refresh patterns
rg -A 2 '_onDidChangeTreeData.fire'
Length of output: 1512
Script:
#!/bin/bash
# Check the full implementation of ModelTreeviewProvider to understand all refresh triggers
ast-grep --pattern 'class ModelTreeviewProvider {
$$$
constructor($$$) {
$$$
}
$$$
}'
# Also check for any window.onDidChangeTextEditorSelection usage elsewhere
rg -A 2 'onDidChangeTextEditorSelection'
Length of output: 633
Script:
#!/bin/bash
# Check the constructor implementation to understand all event subscriptions
ast-grep --pattern 'constructor($$$) {
$$$
this._disposables = [
$$$
];
$$$
}'
# Also check for any other tree refresh mechanisms in the file
rg -B 2 -A 2 'refresh|getChildren|getTreeItem' src/treeview_provider/modelTreeviewProvider.ts
Length of output: 1756
src/utils.ts
Outdated
| 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 ""; | ||
| } |
There was a problem hiding this comment.
Add error handling and type safety improvements.
The current implementation has several potential points of failure that should be handled more gracefully:
- YAML parsing errors
- Missing "models" key
- Missing "name" field in model
- 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.
| 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
itemsproperty - Direct array access without null checks
- Assumption that
modelskey exists and has specific structure
- Unchecked access to
🔗 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
| 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; | ||
|
|
There was a problem hiding this comment.
Align implementation with ModelTreeviewProvider and add validation
There are two concerns with the current implementation:
-
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.
-
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.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/utils.ts (1)
364-415: Improve type safety with interfaces.The implementation looks good but could benefit from stronger typing. Consider defining interfaces for the YAML structure:
interface DbtModelNode { key?: { value: string }; value?: { items?: DbtModelItem[] }; range?: [number, number]; } interface DbtModelItem { items?: Array<{ key?: { value: string }; value?: unknown; }>; range?: [number, number]; } interface DbtYamlContents { items?: DbtModelNode[]; }Then update the type assertions:
- const contents = parsedYaml.contents as { items?: Array<any> }; + const contents = parsedYaml.contents as DbtYamlContents; - (item: any) => item?.key?.value === "name", + (item: DbtModelItem["items"][number]) => item?.key?.value === "name",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/utils.ts(2 hunks)
🔇 Additional comments (2)
src/utils.ts (2)
11-11: LGTM: Import statements are appropriate.
The added imports are necessary for the new functionality:
windowfrom 'vscode' for accessing the active editorparseDocumentfrom 'yaml' for parsing YAML with position information
Also applies to: 14-14
394-410: Clarify behavior for multiple models.
The current implementation returns the first model that contains the cursor position. Based on the PR discussion:
- How should the function behave when multiple models are defined in the same file?
- Should we consider adding a warning when multiple models are detected?
- Would it be helpful to return all applicable model names to let the caller decide?
Let's check if multiple models in a single file is a common pattern:
|
I have updated the code to support multiple models in dbt docs. Please review again 🙇 @anandgupta42 @mdesmet |
Overview
Problem
a_model.mdfor a dbt modela_model.Solution
modelTreeviewProvider.ts,path.basename(fileName, ".sql")is used to get a basename (=model name).path.parse(fileName).nameto get a basename. In this way, we can get a basename of any extension.Screenshot/Demo
Single model in a YAML file:
Multiple mode in a YAML file. The model at the cursor is shown in a side panel:
How to test
Checklist
README.mdupdated and added information about my change.Important
Enhance dbt-power-user side panel to display model info for non-SQL files by updating file name parsing in
modelTreeviewProvider.ts.modelTreeviewProvider.tsto usepath.parse(fileName).nameinstead ofpath.basename(fileName, ".sql").getChildren()method ofModelTreeviewProviderandDocumentationTreeviewProviderclasses.This description was created by
for 1d359c0. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation