feat: implement Next.js AST analyzer and fix 'as const' support #1366
Conversation
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Localization Entries locales/en.json, locales/ko.json |
Added three new locale keys for export functionality: command.export_usage_report, command.export_no_report, and command.export_success with English and Korean translations. |
VS Code Extension Manifest package.json |
Registered new command i18n-ally.export-usage-report in contributes.commands; added view-title UI entry with group navigation@2 and condition requiring i18n-ally-has-report; reorganized existing i18n-ally.refresh-usage to group navigation@1. |
Command Module src/commands/commands.ts, src/commands/exportUsageReport.ts, src/commands/index.ts |
Added export_usage_report enum member; implemented new command handler that calls Analyst.analyzeUsage(), generates Markdown report (Summary, Active/Idle/Missing Keys), prompts save dialog, and writes file to disk with completion notification; integrated into command pipeline. |
AST Analysis Framework src/analyzers/ast-analyzer.ts |
New module providing Babel-based static analysis for translation calls; exports interfaces UseTranslationsAnalysis and AnalyzedKey; implements functions analyzeUseTranslations() (identifies variable-namespace mappings from useTranslations/getTranslations calls), analyzeTranslationUsages() (resolves t(...) usages with namespace-aware keys), foldConstant() (constant-folding resolver for literals, identifiers, and simple expressions), and parseCode() (Babel parser wrapper). |
Framework Detection System src/core/KeyDetector.ts, src/frameworks/base.ts, src/frameworks/next-intl.ts |
Added detectKeys(document) method to Framework base class; updated KeyDetector to call framework.detectKeys() early before cache/regex fallback; implemented detectKeys() in NextIntlFramework using AST analysis with namespace resolution; added createImporter() helper for path resolution; updated regex patterns to account for dot-qualified methods (.rich, .markup, .raw). |
Unit Tests test/unit/analyzers/ast-analyzer.test.ts, test/unit/analyzers/repro_issue.test.ts |
Added comprehensive test suite covering analyzeUseTranslations and analyzeTranslationUsages with scenarios for static/dynamic namespaces, template literals, constant propagation, array/object maps, as const patterns, and namespace shadowing; added reproduction tests for edge cases. |
Sequence Diagram(s)
sequenceDiagram
actor User
participant VSCode as VSCode Extension
participant Analyst
participant Generator as Markdown Generator
participant Dialog as Save Dialog
participant FileSystem as File System
participant Notif as Notification
User->>VSCode: Invoke export-usage-report command
VSCode->>Analyst: analyzeUsage(true)
Analyst-->>VSCode: UsageReport
alt Report is empty
VSCode->>Notif: Show warning (i18n: export_no_report)
Notif-->>User: Display message
else Report available
rect rgb(240, 248, 255)
VSCode->>Generator: generateMarkdown(report)
Generator-->>VSCode: Markdown string
end
VSCode->>Dialog: showSaveDialog(defaultName: i18n-usage-report.md)
Dialog-->>User: Save dialog
User->>Dialog: Choose destination
Dialog-->>VSCode: File URI
VSCode->>FileSystem: workspace.fs.writeFile(uri, markdown)
FileSystem-->>VSCode: Success
VSCode->>Notif: Show info with Open File option
Notif-->>User: Display success message
alt User clicks Open File
User->>VSCode: Open File action
VSCode->>VSCode: vscode.open(uri)
VSCode-->>User: Open in editor
end
end
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title clearly summarizes the main changes: implementing an AST analyzer for Next.js/next-intl and fixing 'as const' support, which aligns with the core objectives of the PR. |
| Docstring Coverage | ✅ Passed | Docstring coverage is 83.33% which is sufficient. The required threshold is 80.00%. |
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
src/core/KeyDetector.ts (1)
103-109: Consider caching AST-detected keys for performance.The framework-specific detection path returns early before the cache lookup, meaning every call to
getKeysfor files handled by AST-based frameworks will re-parse the AST. For large files or frequent calls (e.g., during typing), this could impact performance.Consider caching the AST-detected results similarly to how regex-based results are cached:
// Try framework-specific detection first (e.g. AST based) for (const framework of Global.enabledFrameworks) { const detected = framework.detectKeys?.(document) - if (detected) + if (detected) { + this._get_keys_cache[filepath] = detected return detected + } }This would allow the cache invalidation on document change (line 86-92) to work for AST-detected files as well.
test/unit/analyzers/repro_issue.test.ts (1)
27-36: Consider adding a negative test case for array map resolution.The array map test assumes
ITEMSis always resolvable. Consider adding a test case where the array source cannot be statically resolved (e.g., a function parameter or external import) to verify graceful handling of unresolvable dynamic values.test/unit/analyzers/ast-analyzer.test.ts (1)
204-214: Minor style: missing blank line before test case.For consistency with other test cases in this file, consider adding a blank line before this test.
src/frameworks/next-intl.ts (2)
82-127: Synchronous file operations may block the extension host.The
createImporterfunction uses synchronousfs.existsSyncandFile.readSynccalls. In a VS Code extension, synchronous I/O on the main thread can cause UI freezes, especially when resolving multiple imports in large files.Consider making this async or using a caching strategy to minimize blocking. For now, this is acceptable if the number of imports per file is typically small.
100-107: Redundant path resolution when no path exists.When none of the
possiblePathsexist, line 107 still setsresolvedPath = possiblePaths[0], which will then fail in the extension loop below. This is intentional fallback behavior but could be clarified with a comment.🔎 Suggested clarification
- // Fallback to simple join if not found, to try extensions below - if (!resolvedPath) resolvedPath = possiblePaths[0] + // Fallback: try first path with various extensions below + // This allows .ts/.tsx/.js extensions to be tested even if base path doesn't exist + if (!resolvedPath) resolvedPath = possiblePaths[0]src/commands/exportUsageReport.ts (1)
92-99: Default URI may not resolve to a meaningful location.
Uri.file('i18n-usage-report.md')creates a URI relative to the process working directory, which may not be the workspace folder. Consider using the workspace folder as the base.🔎 Proposed fix
+ const workspaceFolder = workspace.workspaceFolders?.[0]?.uri const uri = await window.showSaveDialog({ - defaultUri: Uri.file('i18n-usage-report.md'), + defaultUri: workspaceFolder + ? Uri.joinPath(workspaceFolder, 'i18n-usage-report.md') + : Uri.file('i18n-usage-report.md'), filters: { Markdown: ['md'], 'All Files': ['*'], }, title: 'Export Usage Report', })src/analyzers/ast-analyzer.ts (1)
406-408:fileNameis hardcoded when it could be derived from context.Unlike
analyzeUseTranslationswhich acceptsfilePath,analyzeTranslationUsagesdoesn't accept a file path parameter but usesfileName = 'unknown'on line 408. Consider adding an optionalfilePathparameter for consistency.
📜 Review details
Configuration used: Repository: lokalise/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
locales/en.jsonlocales/ko.jsonpackage.jsonsrc/analyzers/ast-analyzer.tssrc/commands/commands.tssrc/commands/exportUsageReport.tssrc/commands/index.tssrc/core/KeyDetector.tssrc/frameworks/base.tssrc/frameworks/next-intl.tstest/unit/analyzers/ast-analyzer.test.tstest/unit/analyzers/repro_issue.test.ts
🧰 Additional context used
🧬 Code graph analysis (5)
src/frameworks/base.ts (1)
src/core/types.ts (1)
KeyInDocument(113-118)
test/unit/analyzers/repro_issue.test.ts (1)
src/analyzers/ast-analyzer.ts (2)
analyzeUseTranslations(324-392)analyzeTranslationUsages(398-563)
src/core/KeyDetector.ts (1)
src/core/Global.ts (1)
Global(24-464)
test/unit/analyzers/ast-analyzer.test.ts (1)
src/analyzers/ast-analyzer.ts (2)
analyzeUseTranslations(324-392)analyzeTranslationUsages(398-563)
src/commands/exportUsageReport.ts (3)
src/core/types.ts (1)
UsageReport(132-136)src/modules.ts (1)
ExtensionModule(3-5)src/core/Analyst.ts (1)
Analyst(12-178)
🔇 Additional comments (17)
locales/ko.json (1)
29-31: LGTM!The new Korean localization entries for the export usage report feature are correctly structured and follow the existing naming conventions. The placeholder
{0}is properly used for the file path in the success message.locales/en.json (1)
29-31: LGTM!The new English localization entries are clear, grammatically correct, and consistent with the existing command key patterns.
src/commands/commands.ts (1)
48-48: LGTM!The new enum member follows the established naming conventions and matches the command string registered in
package.json.src/commands/index.ts (1)
15-15: LGTM!The new
exportUsageReportmodule is correctly imported and integrated into the command pipeline following the established pattern. The placement afterrefreshUsageReportis logical since users would typically refresh before exporting.Also applies to: 28-28
src/frameworks/base.ts (1)
65-70: LGTM!The new
detectKeysmethod is a well-designed extension point that follows the existing pattern of optional framework-level hooks (similar todetectHardStringsandgetScopeRange). The defaultundefinedreturn allows frameworks to opt-in to AST-based detection while maintaining backward compatibility.test/unit/analyzers/repro_issue.test.ts (1)
1-4: LGTM!The reproduction tests provide good coverage for edge cases including template literals, binary expressions, array mapping, and
as constassertions. The test structure is clean and each test case is focused on a specific scenario.package.json (1)
622-626: Thei18n-ally-has-reportcontext is correctly set insrc/views/providers/UsageReportProvider.ts(line 25). The context is enabled when a usage report contains active or idle entries, ensuring the export button appears only when data is available. Implementation is sound.test/unit/analyzers/ast-analyzer.test.ts (3)
1-14: LGTM! Well-structured test setup.The test file properly imports the analyzer functions and uses chai for assertions. The first test case validates the basic string literal namespace detection correctly.
246-263: Good test for scope-aware namespace resolution.This test validates that the analyzer correctly respects lexical scoping when the same variable name
tis used in different functions with different namespaces. The comment on line 257 accurately describes the fallback behavior.
279-299: Excellent coverage for variable shadowing.This test properly validates that inner-scope variable declarations shadow outer-scope ones during constant propagation. The use of line numbers to identify results is a pragmatic approach.
src/frameworks/next-intl.ts (2)
129-158: LGTM! Clean integration of AST analysis for key detection.The
detectKeysmethod properly:
- Checks language ID compatibility
- Creates an importer for cross-file resolution
- Builds variable-to-namespace mappings from
useTranslationscalls- Analyzes translation usages with the namespace map
The TODO on line 145 for handling dynamic namespaces is noted and appropriate for a future enhancement.
209-223: Regex fallback provides graceful degradation for namespace detection.The fallback regex
/namespace:\s*['"]([^'"`]+)['"`]/gonly matches object-property-style namespace declarations like{ namespace: 'value' }. However, this is acceptable because the primary AST analyzer handles the broader range of patterns (string literal arguments touseTranslations(), template literals, and variable resolution), and the fallback is invoked only when AST analysis yields no results. For next-intl's typical usage (useTranslations('name')orgetTranslations({ namespace: 'name' })`), the AST analyzer already captures the primary cases.src/commands/exportUsageReport.ts (1)
7-74: Well-structured Markdown generation.The
generateMarkdownfunction produces a clean, readable report with:
- Timestamped header
- Summary table with counts
- Expandable idle keys section (good UX for large lists)
- Clear categorization of active, idle, and missing keys
src/analyzers/ast-analyzer.ts (4)
1-4: LGTM! Appropriate use of @ts-ignore.The
@ts-ignorefor@babel/traverseis acceptable since the package lacks complete TypeScript definitions. Consider adding a more descriptive comment explaining why.
66-266: Well-implemented constant folding with appropriate limits.The
foldConstantfunction handles:
- TS type assertions (as const, type assertions, non-null assertions)
- Literals and template literals
- Arrays (string and object arrays)
- Identifier resolution via scope bindings
- Cross-file imports via the importer callback
- Binary concatenation
- Recursion depth limit (5) to prevent infinite loops
The implementation is robust for the stated use cases.
324-392: LGTM! Clean implementation ofanalyzeUseTranslations.The function properly:
- Parses code with error handling
- Traverses
VariableDeclaratornodes- Handles both direct calls and
awaitexpressions- Uses scope-aware namespace resolution
- Returns structured analysis results with location info
462-464: No issue found - early return is intentional design.The code correctly handles empty namespaces (line 198–202 test confirms
new Map([['t', '']])processes keys as'title'without prefix). The early return requires translation functions to be registered invarNamespaceMapor resolved via scope analysis—this is defensive design to skip untracked translation calls, not a bug. Both intentionally empty namespaces and different sources are handled correctly through the designed mechanism: calling code must pre-populatevarNamespaceMapwith known translation variables.
| else if (!res.isDynamic && typeof res.value === 'object' && res.value !== null && !Array.isArray(res.value)) { | ||
| // It's likely an object setup from ObjectExpression below, but foldConstant currently doesn't return single object. | ||
| // Let's handle ObjectExpression explicitly here for element structure | ||
| } |
There was a problem hiding this comment.
Empty conditional branch has no effect.
Lines 120-123 check for a resolved object value but don't do anything with it. The logic falls through to the ObjectExpression handling below, but this branch appears incomplete.
🔎 Suggested fix - remove dead code
if (!res.isDynamic && typeof res.value === 'string') {
if (type === 'unknown') type = 'string'
if (type === 'string') {
stringElements.push(res.value)
} else {
type = 'mixed'
break
}
}
- else if (!res.isDynamic && typeof res.value === 'object' && res.value !== null && !Array.isArray(res.value)) {
- // It's likely an object setup from ObjectExpression below, but foldConstant currently doesn't return single object.
- // Let's handle ObjectExpression explicitly here for element structure
- }
// Direct ObjectExpression handling to match previous logic
else if (el.type === 'ObjectExpression') {📝 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.
| else if (!res.isDynamic && typeof res.value === 'object' && res.value !== null && !Array.isArray(res.value)) { | |
| // It's likely an object setup from ObjectExpression below, but foldConstant currently doesn't return single object. | |
| // Let's handle ObjectExpression explicitly here for element structure | |
| } | |
| // Direct ObjectExpression handling to match previous logic | |
| else if (el.type === 'ObjectExpression') { |
🤖 Prompt for AI Agents
In src/analyzers/ast-analyzer.ts around lines 120 to 123, the else-if branch
that checks for a resolved non-array object (else if (!res.isDynamic && typeof
res.value === 'object' && res.value !== null && !Array.isArray(res.value)) { ...
}) is empty and has no effect; remove that entire conditional branch to avoid
dead code (or if you intended to handle folded single-object constants,
implement the explicit ObjectExpression handling there instead of leaving it
empty) so control falls through to the existing ObjectExpression logic below.
| let isSimpleObject = true | ||
| for (const prop of el.properties) { | ||
| if (prop.type === 'ObjectProperty' && prop.key.type === 'Identifier') { | ||
| const valRes = foldConstant(prop.value, scope, importer, depth + 1) | ||
| if (!valRes.isDynamic && typeof valRes.value === 'string') { | ||
| obj[prop.key.name] = valRes.value | ||
| } | ||
| } | ||
| // Ignore complex properties for now | ||
| } | ||
| if (isSimpleObject) { | ||
| objectElements.push(obj) | ||
| } |
There was a problem hiding this comment.
isSimpleObject is always true and has no effect.
The variable isSimpleObject is initialized to true on line 129 but never set to false, making the condition on line 139 always true. Either remove it or implement the intended logic to detect complex properties.
🔎 Suggested fix
const obj: Record<string, string> = {}
- let isSimpleObject = true
for (const prop of el.properties) {
if (prop.type === 'ObjectProperty' && prop.key.type === 'Identifier') {
const valRes = foldConstant(prop.value, scope, importer, depth + 1)
if (!valRes.isDynamic && typeof valRes.value === 'string') {
obj[prop.key.name] = valRes.value
}
}
// Ignore complex properties for now
}
- if (isSimpleObject) {
- objectElements.push(obj)
- }
+ objectElements.push(obj)📝 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.
| let isSimpleObject = true | |
| for (const prop of el.properties) { | |
| if (prop.type === 'ObjectProperty' && prop.key.type === 'Identifier') { | |
| const valRes = foldConstant(prop.value, scope, importer, depth + 1) | |
| if (!valRes.isDynamic && typeof valRes.value === 'string') { | |
| obj[prop.key.name] = valRes.value | |
| } | |
| } | |
| // Ignore complex properties for now | |
| } | |
| if (isSimpleObject) { | |
| objectElements.push(obj) | |
| } | |
| const obj: Record<string, string> = {} | |
| for (const prop of el.properties) { | |
| if (prop.type === 'ObjectProperty' && prop.key.type === 'Identifier') { | |
| const valRes = foldConstant(prop.value, scope, importer, depth + 1) | |
| if (!valRes.isDynamic && typeof valRes.value === 'string') { | |
| obj[prop.key.name] = valRes.value | |
| } | |
| } | |
| // Ignore complex properties for now | |
| } | |
| objectElements.push(obj) |
| if (binding.kind === 'module') { | ||
| if (binding.path.isImportSpecifier()) { | ||
| const importedName = binding.path.node.imported.name | ||
| const importDecl = binding.path.parentPath.node | ||
| const source = importDecl.source.value | ||
|
|
||
| if (importer) { | ||
| const code = importer(source) | ||
| if (code) { | ||
| const subAst = parseCode(code) | ||
| if (subAst) { | ||
| let importedValue: ResolvedValue = { value: null, isDynamic: true } | ||
| // Traverse the imported file to find the export | ||
| traverse(subAst, { | ||
| ExportNamedDeclaration(path: any) { | ||
| const declaration = path.node.declaration | ||
| if (declaration?.type === 'VariableDeclaration') { | ||
| for (const decl of declaration.declarations) { | ||
| if (decl.id.name === importedName) { | ||
| // Found valid export, resolve its value in its own scope (global in that file) | ||
| // We create a dummy scope or just resolve based on init node since top level | ||
| // For simplicity, we call foldConstant with a dummy scope or standard AST traversal for that node | ||
| // Note: Cross-file scope resolution is hard. We'll reuse the logic by passing the sub-file AST path | ||
| // But we don't have scope path easily. | ||
| // Simplified: Just check the init node directly if it's simple literal. | ||
| // Scope usage in imported file is not supported yet (except simple constants) | ||
| importedValue = foldConstant(decl.init, { getBinding: () => null }, undefined, depth + 1) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }) | ||
| return importedValue | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return { value: null, isDynamic: true } | ||
| } |
There was a problem hiding this comment.
Import resolution assumes named imports only.
Line 170 accesses binding.path.node.imported.name, which works for named imports like import { CATEGORIES } from './constants' but will fail for:
- Default imports:
import CATEGORIES from './constants' - Namespace imports:
import * as constants from './constants'
🔎 Suggested defensive check
if (binding.kind === 'module') {
- if (binding.path.isImportSpecifier()) {
- const importedName = binding.path.node.imported.name
+ if (binding.path.isImportSpecifier() && binding.path.node.imported?.type === 'Identifier') {
+ const importedName = binding.path.node.imported.name
// ... rest of logic
+ }
+ // TODO: Handle default imports and namespace imports if needed
return { value: null, isDynamic: true }
}🤖 Prompt for AI Agents
In src/analyzers/ast-analyzer.ts around lines 168 to 206, the code assumes
binding.path.node.imported exists (named import) and will throw for default or
namespace imports; add defensive checks and handle the three import specifier
types: use path.isImportSpecifier() before accessing node.imported and handle
ImportDefaultSpecifier by looking for ExportDefaultDeclaration or a named export
mapped to default in the imported AST, and handle ImportNamespaceSpecifier by
not trying to resolve a single identifier (return dynamic or attempt to resolve
property accesses later). Ensure all accesses to node.imported/node.local are
guarded (null checks) and fall back to returning {value: null, isDynamic: true}
when the import form or export cannot be resolved.
| for (const usage of report.missing) { | ||
| const files = [...new Set(usage.occurrences.map(o => o.filepath.split('/').pop()))].join(', ') | ||
| lines.push(`| \`${usage.keypath}\` | ${files} |`) | ||
| } |
There was a problem hiding this comment.
Path splitting fails on Windows.
Using filepath.split('/').pop() won't correctly extract filenames on Windows where paths use backslashes. Use path.basename() or a cross-platform approach.
🔎 Proposed fix
First, add the import at the top of the file:
import * as path from 'path'Then update the path extraction:
- const files = [...new Set(usage.occurrences.map(o => o.filepath.split('/').pop()))].join(', ')
+ const files = [...new Set(usage.occurrences.map(o => path.basename(o.filepath)))].join(', ')🤖 Prompt for AI Agents
In src/commands/exportUsageReport.ts around lines 66 to 69, the code uses
filepath.split('/').pop() which fails on Windows; import Node's path module at
the top of the file (e.g., import * as path from 'path') and replace the
split/pop expression with path.basename(usage.occurrences.map(...)? No — use
path.basename(o.filepath) when mapping: build the Set from
usage.occurrences.map(o => path.basename(o.filepath)) and join as before so
filenames are extracted in a cross-platform way.
1 participant
PR: Implement Next.js AST Analyzer & Fix
as constSupport📝 Description
This PR introduces a robust AST-based analyzer for the
next-intlframework ini18n-ally. It significantly improves the detection of translation keys and namespaces by supporting constant propagation, variable shadowing, and indirect usage (e.g., t(key) wherekeyis a variable).Specifically, this PR addresses a critical issue where TypeScript assertions (like
as const) prevented the analyzer from correctly identifying static values, leading to missed keys.🚀 Changes
useTranslationscalls and resolve namespaces via scope analysis.const k = 'key'; t(k)ITEMS.map(i => t(i.id))TSAsExpression,TSTypeAssertion, etc.).useTranslations('ns' as const)and t('key' as const) are now correctly resolved.detectionandscoperesolution.as constissues in test/unit/analyzers/repro_issue.test.ts.✅ Verification
as constusage is correctly handled.npm run build).npx vsce package).🔗 Related Issues
next-intlkeys were not detected when using variables oras const.Commit:
feat: implement Next.js AST analyzer and fix 'as const' supportSummary by CodeRabbit
New Features
Localization
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.