feat(@deepnote/cli): show better imports in analyze command#315
Merged
dinohamzic merged 11 commits intomainfrom Feb 23, 2026
Merged
feat(@deepnote/cli): show better imports in analyze command#315dinohamzic merged 11 commits intomainfrom
dinohamzic merged 11 commits intomainfrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThe changes extend the import analysis pipeline to track actual package names (e.g., "pandas") instead of module aliases (e.g., "pd"). The Python AST analyzer now extracts imported package names via a new field, the DAG builder propagates this data through node definitions, type schemas are updated to include the field, and CLI tests are adjusted to verify the presence of package names rather than aliases. Sequence DiagramsequenceDiagram
participant Python as Python AST<br/>Analyzer
participant DAG as DAG Builder<br/>(JavaScript)
participant Types as Type Definitions<br/>(TypeScript)
participant CLI as CLI Stats<br/>Command
Python->>Python: Extract imported_packages<br/>from Import statements
Python->>DAG: Return analyzed block with<br/>importedPackages field
DAG->>DAG: Create DagNode with<br/>importedPackages array
DAG->>Types: Populate node matching<br/>BlockDependencySchema
Types->>CLI: CLI reads DagNode with<br/>importedPackages field
CLI->>CLI: Assert output contains<br/>actual package names
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
…eepnote into cli-better-imports-in-analyze
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #315 +/- ##
=======================================
Coverage 83.47% 83.47%
=======================================
Files 114 114
Lines 6970 6971 +1
Branches 1862 1926 +64
=======================================
+ Hits 5818 5819 +1
Misses 1152 1152 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/cli/src/commands/stats.test.ts (1)
84-127:⚠️ Potential issue | 🟠 MajorExpand assertions to cover new import formatting + JSON fields.
The output now includes alias/from details, and JSON includes
packageFromImports; current assertions miss both.✅ Add JSON assertion
- expect(parsed.packageAliases).toBeDefined() + expect(parsed.packageAliases).toBeDefined() + expect(parsed.packageFromImports).toBeDefined()As per coding guidelines, "When updating code generation, update corresponding tests to match new output format".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/stats.test.ts` around lines 84 - 127, Tests for createStatsAction in stats.test.ts are outdated: they don't assert the new import formatting (alias/from details) in the human output nor the new JSON field packageFromImports; update the tests that call createStatsAction (the 'displays imports when present' and the '-o json output' cases) to assert the alias/from formatted import lines appear in console output (e.g., include "as" and "from" patterns) and add assertions that parsed.packageFromImports isDefined (and contains expected entries), and also verify parsed.imports entries include alias/from details as appropriate; keep test structure (using action = createStatsAction(program), filePath resolution, getOutput(consoleSpy), JSON.parse(output)) and only add the new expect(...) checks to these specs.packages/cli/src/commands/analyze.test.ts (1)
111-117:⚠️ Potential issue | 🟠 MajorAdd assertions for new dependency metadata + formatted output.
The JSON test only checks
packageAliases; it should also validatepackageFromImports, and a human-readable test should assert the new import formatting to prevent regressions.✅ Add JSON assertion
- expect(result.dependencies.packageAliases).toBeDefined() + expect(result.dependencies.packageAliases).toBeDefined() + expect(result.dependencies.packageFromImports).toBeDefined()As per coding guidelines, "When updating code generation, update corresponding tests to match new output format".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/analyze.test.ts` around lines 111 - 117, The test "includes dependency information" currently only asserts packageAliases and should be extended to validate the new dependency metadata and formatted output: add an assertion that result.dependencies.packageFromImports is defined and is an array (or has the expected structure) in the JSON path used by the test (where result is taken from outputJsonSpy.mock.calls[0][0]); additionally update or add the human-readable output test for the same action() invocation to assert the new import formatting (verify the printed string contains the expected formatted import lines), referencing the test case's action invocation and the outputJsonSpy/mock that capture outputs so the assertions target the correct outputs and prevent regressions in import formatting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/src/utils/analysis.ts`:
- Around line 737-785: The aggregation functions produce object keys in DAG node
order, causing nondeterministic output; update extractPackageAliasesFromDag and
extractPackageFromImportsFromDag to emit keys in a stable, sorted order: gather
all package keys (from node.packageAliases and from node.packageFromImports),
sort the keys, then build the returned Record<string,string> /
Record<string,string[]> by iterating the sorted keys so the resulting objects
have deterministic key ordering for CLI and tests.
In `@packages/reactivity/src/dag-builder.test.ts`:
- Around line 186-188: Add tests in dag-builder.test.ts that exercise non-empty
values for the new package metadata fields: create input source(s) containing an
aliased import (e.g., "import pandas as pd") and a from-import (e.g., "from
numpy import array"), run the same builder/parse helper used elsewhere in this
file (the test harness that produces the metadata object), and assert that
importedPackages contains the discovered package names, packageAliases maps "pd"
to "pandas", and packageFromImports records the specific names imported from
modules (e.g., "numpy" -> ["array"]). Locate the existing test case that
currently asserts importedPackages: [], packageAliases: {}, packageFromImports:
{} and extend or add a new Vitest it/test block to provide non-empty
expectations matching those symbols.
In `@packages/reactivity/src/scripts/ast-analyzer.py`:
- Around line 131-139: The visit_ImportFrom visitor currently skips recording
aliased from-imports into package_from_imports due to the "not alias.asname"
check; update visit_ImportFrom to always append alias.name into
package_from_imports for the top-level package (remove the alias.asname guard)
while keeping the existing imported_modules logic (alias.asname or alias.name)
and the imported_packages handling—this ensures all "from X import Y"
occurrences are tracked even when they use "as".
---
Outside diff comments:
In `@packages/cli/src/commands/analyze.test.ts`:
- Around line 111-117: The test "includes dependency information" currently only
asserts packageAliases and should be extended to validate the new dependency
metadata and formatted output: add an assertion that
result.dependencies.packageFromImports is defined and is an array (or has the
expected structure) in the JSON path used by the test (where result is taken
from outputJsonSpy.mock.calls[0][0]); additionally update or add the
human-readable output test for the same action() invocation to assert the new
import formatting (verify the printed string contains the expected formatted
import lines), referencing the test case's action invocation and the
outputJsonSpy/mock that capture outputs so the assertions target the correct
outputs and prevent regressions in import formatting.
In `@packages/cli/src/commands/stats.test.ts`:
- Around line 84-127: Tests for createStatsAction in stats.test.ts are outdated:
they don't assert the new import formatting (alias/from details) in the human
output nor the new JSON field packageFromImports; update the tests that call
createStatsAction (the 'displays imports when present' and the '-o json output'
cases) to assert the alias/from formatted import lines appear in console output
(e.g., include "as" and "from" patterns) and add assertions that
parsed.packageFromImports isDefined (and contains expected entries), and also
verify parsed.imports entries include alias/from details as appropriate; keep
test structure (using action = createStatsAction(program), filePath resolution,
getOutput(consoleSpy), JSON.parse(output)) and only add the new expect(...)
checks to these specs.
…eepnote into cli-better-imports-in-analyze
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/reactivity/src/types.ts (1)
10-19:⚠️ Potential issue | 🟡 MinorAdd
packageAliasesfield toBlockDependencySchema.
BlockDependencySchemais missing thepackageAliasesfield that the AST analyzer outputs and thatDagNodeexpects. AddpackageAliases: z.record(z.string()).optional()to the schema to ensure proper validation consistency with the AST analyzer output.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/reactivity/src/types.ts` around lines 10 - 19, BlockDependencySchema is missing the packageAliases field that the AST analyzer and DagNode expect; update the schema definition for BlockDependencySchema to include packageAliases: z.record(z.string()).optional() so validation accepts the analyzer output, placing it alongside the other optional fields (e.g., after importedPackages/usedImportedModules) and ensure imports/types remain valid for z.record usage.packages/reactivity/src/scripts/ast-analyzer.py (1)
280-301:⚠️ Potential issue | 🟡 MinorAdd missing schema fields to non-code blocks for consistency.
Code blocks emit
importedPackagesandpackageAliases, but SQL, button, big-number, notebook-function, and input blocks don't. While TypeScript consumers apply defaults, the Python output should be uniform. Add empty defaults to all non-code blocks:Example fix for SQL block (lines 280–301)
analysis.append( { "id": block["id"], "definedVariables": output_variables, "usedVariables": jinja_variables_list, "importedModules": [], + "importedPackages": [], + "packageAliases": {}, "linesOfCode": loc, } )Also applies to: button (302–319), big-number (320–343), notebook-function (344–384), input blocks (385–416)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/reactivity/src/scripts/ast-analyzer.py` around lines 280 - 301, Non-code block branches (e.g., the SQL branch that checks block["type"] == "sql" and builds the analysis entry) are missing the schema fields importedPackages and packageAliases; add these keys with empty lists to the dict appended to analysis so the Python output matches code blocks. Locate the analysis.append call in the SQL branch (and mirror the change in the other non-code block handlers: button, big-number, notebook-function, input) and add "importedPackages": [], "packageAliases": [] to the object alongside id, definedVariables, usedVariables, importedModules, and linesOfCode.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/reactivity/src/dag-builder.test.ts`:
- Around line 195-213: Add a new test case in dag-builder.test.ts that mirrors
the existing "should pass through package metadata fields" test but uses a
package name with a special character (e.g., "scikit_learn") and an alias (e.g.,
{ scikit_learn: 'sk' }); call buildDagFromBlocks with that block and assert the
resulting node.importedPackages contains "scikit_learn" and node.packageAliases
equals { scikit_learn: 'sk' } to cover underscore handling in
buildDagFromBlocks.
---
Outside diff comments:
In `@packages/reactivity/src/scripts/ast-analyzer.py`:
- Around line 280-301: Non-code block branches (e.g., the SQL branch that checks
block["type"] == "sql" and builds the analysis entry) are missing the schema
fields importedPackages and packageAliases; add these keys with empty lists to
the dict appended to analysis so the Python output matches code blocks. Locate
the analysis.append call in the SQL branch (and mirror the change in the other
non-code block handlers: button, big-number, notebook-function, input) and add
"importedPackages": [], "packageAliases": [] to the object alongside id,
definedVariables, usedVariables, importedModules, and linesOfCode.
In `@packages/reactivity/src/types.ts`:
- Around line 10-19: BlockDependencySchema is missing the packageAliases field
that the AST analyzer and DagNode expect; update the schema definition for
BlockDependencySchema to include packageAliases: z.record(z.string()).optional()
so validation accepts the analyzer output, placing it alongside the other
optional fields (e.g., after importedPackages/usedImportedModules) and ensure
imports/types remain valid for z.record usage.
Contributor
Author
|
@coderabbitai approve |
Contributor
✅ Actions performedComments resolved and changes approved. |
OlegWock
requested changes
Feb 23, 2026
Contributor
OlegWock
left a comment
OlegWock
left a comment
There was a problem hiding this comment.
Code looks good, but there is one issue with incorrect parsing of aliases 👀
packages/cli/src/utils/analysis.ts
Outdated
| * Extract package-to-alias mapping from DAG nodes. | ||
| * Only includes explicit "as" renames (e.g. pandas → pd from "import pandas as pd"). | ||
| */ | ||
| function extractPackageAliasesFromDag(dag: BlockDependencyDag): Record<string, string> { |
Contributor
There was a problem hiding this comment.
This incorrectly parses import os.path as paaaaath as Imports: os (as paaaaath)
Contributor
Author
There was a problem hiding this comment.
I simplified the PR and removed the alias altogether (going back to your original suggestion), otherwise it's becoming a bit of a headache. 😄
OlegWock
approved these changes
Feb 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Chores