diff --git a/CLAUDE.md b/CLAUDE.md index d73d2a8..7d792ac 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -126,6 +126,13 @@ const globPattern = normalize(path.join(root, '**/*.liquid')); **Important: `normalize-path` is for filesystem paths only, NOT URIs.** It collapses multiple slashes (e.g., `file:///` becomes `file:/`), which breaks URI semantics. For URI strings (`file://...`), use the `normalize()` function from `platformos-check-common/src/path.ts` which works with `vscode-uri`. For raw backslash replacement in URIs where you can't use the common normalize, use `.replace(/\\/g, '/')`. +## Test Assertion Guidelines + +- Always use `.to.equal()` for message assertions, never `.to.include()` — assert the entire expected string +- Do not use regex for matching in tests unless absolutely necessary +- For array assertions (e.g., `applySuggestions` results), use `.to.deep.equal([...])` instead of `.to.include(element)` +- When multiple `.include()` calls check the same value, collapse them into a single `.to.equal()` + ## Development Workflows ### Online Store Web Integration diff --git a/UPSTREAM-PROPOSALS.md b/UPSTREAM-PROPOSALS.md new file mode 100644 index 0000000..13b637b --- /dev/null +++ b/UPSTREAM-PROPOSALS.md @@ -0,0 +1,791 @@ +# Upstream Feature Proposals — pos-cli check and pos-cli LSP + +Features identified during plugin development that belong at the platform tooling layer +rather than in the agent plugin. Each proposal includes rationale for placement, full +implementation detail, and expected impact. + +**Why layer placement matters:** features that detect code quality problems should live in +`pos-cli check` or the LSP so they fire universally — in CI, in editors, in the agent plugin +via auto-diagnostics — without requiring the plugin to re-implement analysis logic. The plugin +gets them for free via `pos-cli check` output. Features that exist only in the plugin are +invisible outside agent sessions. + +--- + +## pos-cli check proposals + +### 1. N+1 GraphQL query detection + +**Check code:** `NestedGraphQLQuery` (or `GraphQLInLoop`) +**Severity:** WARNING +**Type:** `SourceCodeType.LiquidHtml` + +#### The problem + +A `{% graphql %}` tag inside a `{% for %}` or `{% tablerow %}` loop executes one database +request per loop iteration. With 100 records in the outer loop, that is 100 sequential +GraphQL requests instead of one batch query — a catastrophic performance footprint that is +completely invisible at the template level and produces no error at runtime. The page simply +loads slowly (or times out under load). + +This is the single most damaging performance pattern in platformOS templates and it is not +currently detected by any tool. + +#### Why it belongs in pos-cli check + +This is a pure static analysis problem: detect a graphql tag whose AST ancestor chain +contains a `for` or `tablerow` tag. It requires no runtime information, no network calls, +and no session state. It should fire in CI, in the editor diagnostics panel, and during +`pos-cli check` runs — not only when an agent happens to be active. Putting it in the +plugin means it only fires in agent sessions and is invisible everywhere else. + +#### Implementation + +The check uses entry and exit visitors to maintain a for-loop nesting depth counter. When +a `graphql` tag is encountered at any nesting depth > 0, an offense is reported. + +```js +import { SourceCodeType, Severity } from '@platformos/platformos-check-common'; +import { NamedTags, NodeTypes } from '@platformos/liquid-html-parser'; + +export const NestedGraphQLQuery = { + meta: { + code: 'NestedGraphQLQuery', + name: 'GraphQL query inside a loop', + docs: { + description: 'A {% graphql %} tag inside a {% for %} loop executes one ' + + 'database request per iteration (N+1 pattern). Move the query before ' + + 'the loop and pass results as a variable.', + recommended: true, + url: 'https://documentation.platformos.com/best-practices/performance/graphql-in-loops', + }, + type: SourceCodeType.LiquidHtml, + severity: Severity.WARNING, + schema: {}, + targets: [], + }, + create(context) { + const loopStack = []; // tracks open for/tablerow nodes + + return { + async LiquidTag(node) { + if (node.name === NamedTags.for || node.name === NamedTags.tablerow) { + loopStack.push(node.name); + return; + } + + if (node.name !== NamedTags.graphql) return; + if (loopStack.length === 0) return; + + const outerLoop = loopStack[loopStack.length - 1]; + const markup = node.markup; + const resultVar = markup.type === NodeTypes.GraphQLMarkup + ? markup.name // the 'result' in {% graphql result = 'path' %} + : null; + + context.report({ + message: + `N+1 pattern: {% graphql ${resultVar ? resultVar + ' = ' : ''}... %} ` + + `is inside a {% ${outerLoop} %} loop. ` + + `This executes one database request per iteration. ` + + `Move the query before the loop and pass data as a variable.`, + startIndex: node.position.start, + endIndex: node.position.end, + }); + }, + + async 'LiquidTag:exit'(node) { + if (node.name === NamedTags.for || node.name === NamedTags.tablerow) { + loopStack.pop(); + } + }, + }; + }, +}; +``` + +#### Edge cases to handle + +- **Nested loops:** the `loopStack` correctly handles `{% for %}` inside `{% for %}` — + depth > 1 is even more dangerous (exponential requests) and should still warn. +- **Dynamic graphql (inline):** `GraphQLInlineMarkup` (inline queries without a file + reference) should also be detected — the markup type check handles both. +- **`background` tag:** a `{% background %}` tag inside a loop is less harmful (async) + but still worth a separate INFO-level note. Could be a separate check or a branch here. +- **`cache` wrapping graphql:** if the `graphql` tag is inside both a `{% for %}` and a + `{% cache %}` block, the severity could be downgraded to INFO since caching mitigates + the repeated requests. Requires checking ancestors for `NamedTags.cache`. + +#### Expected output + +``` +WARNING NestedGraphQLQuery app/views/pages/products.liquid:14 + N+1 pattern: {% graphql result = 'products/related' %} is inside a {% for %} + loop. This executes one database request per iteration. Move the query before + the loop and pass data as a variable. +``` + +--- + +### 2. Render parameter validation (`@param` contracts) + +**Check code:** `UndeclaredRenderParameter` + `MissingRequiredParameter` +**Severity:** WARNING (unknown param) / ERROR (missing required param) +**Type:** `SourceCodeType.LiquidHtml` + +#### The problem + +LiquidDoc supports `@param` annotations that declare a partial's expected inputs: + +```liquid +{% doc %} + @param {string} title - The card heading (required) + @param {string} subtitle - Secondary text (optional) + @param {object} cta - Call-to-action object with url and label +{% enddoc %} +``` + +Currently there is no tool that validates whether a `{% render %}` call actually provides +the required parameters. A caller can omit `title` entirely and get a silent blank value +at runtime. A caller can pass `ttle: "typo"` and the typo is silently ignored. These +are two of the most common causes of "blank partial" bugs in platformOS templates. + +#### Why it belongs in pos-cli check + +This is cross-file static analysis: read the callee's declaration (`@param` nodes in the +partial), inspect the caller's argument list (the `{% render %}` tag's named arguments), +and compare. The check engine already provides `context.fs.readFile()` for reading +dependency files. This pattern is identical to how `MissingTemplate` works — it reads +the referenced file to verify it exists. Here we go one step further and read it to +verify the interface is respected. + +As a check offense it fires in CI (preventing broken renders from reaching staging), +in the editor (inline annotations on the render tag), and in agent sessions without +any plugin-level logic. + +#### Implementation + +```js +import { SourceCodeType, Severity } from '@platformos/platformos-check-common'; +import { NamedTags, NodeTypes, toLiquidHtmlAST, walk } from '@platformos/liquid-html-parser'; +import path from 'node:path'; + +function resolvePartialPath(fileUri, partialName) { + // partialName: "sections/hero" + // resolved: /app/views/partials/sections/hero.liquid + const projectRoot = fileUri.replace(/\/app\/.*$/, ''); + return path.join(projectRoot, 'app', 'views', 'partials', partialName + '.liquid'); +} + +function extractParams(ast) { + // Returns { name, required, type, description }[] + // @param without a default or "optional" marker → required: true + const params = []; + walk(ast, (node) => { + if (node.type === NodeTypes.LiquidDocParamNode) { + params.push({ + name: node.name, + type: node.paramType?.value ?? 'any', + description: node.description?.value ?? '', + // Convention: params without "(optional)" in description are required. + // This matches the emerging LiquidDoc convention — adjust per pos-cli's + // own @param semantics when they are formalised. + required: !node.description?.value?.toLowerCase().includes('optional'), + }); + } + }); + return params; +} + +export const RenderParameterValidation = { + meta: { + code: 'RenderParameterValidation', + name: 'Render call violates @param contract', + docs: { + description: 'Validates that {% render %} calls provide all required ' + + '@param arguments declared by the target partial and do not pass ' + + 'undeclared arguments.', + recommended: true, + }, + type: SourceCodeType.LiquidHtml, + severity: Severity.WARNING, + schema: {}, + targets: [], + }, + create(context) { + return { + async LiquidTag(node) { + if (node.name !== NamedTags.render) return; + + const markup = node.markup; + + // Skip dynamic partials: {% render variable %} — can't resolve statically + if (!markup.partial || markup.partial.type !== NodeTypes.String) return; + + const partialName = markup.partial.value; + const partialPath = resolvePartialPath(context.file.uri, partialName); + + if (!await context.fileExists(partialPath)) return; // MissingTemplate handles this + + const partialSource = await context.fs.readFile(partialPath); + let partialAST; + try { + partialAST = toLiquidHtmlAST(partialSource, { mode: 'tolerant' }); + } catch { + return; // malformed partial — other checks will catch it + } + + const declaredParams = extractParams(partialAST); + + // If the partial declares no @param at all, skip validation. + // Unannotated partials have an implicit "accept anything" interface. + if (declaredParams.length === 0) return; + + const providedArgs = new Map( + (markup.args ?? []).map(arg => [arg.name, arg]) + ); + const declaredNames = new Set(declaredParams.map(p => p.name)); + + // Check 1: missing required params + for (const param of declaredParams) { + if (param.required && !providedArgs.has(param.name)) { + context.report({ + message: + `Missing required @param '${param.name}' ` + + `(${param.type}) for partial '${partialName}'. ` + + (param.description ? `Description: ${param.description}` : ''), + startIndex: node.position.start, + endIndex: node.position.end, + }); + } + } + + // Check 2: unknown params (caller passes something the partial doesn't declare) + for (const [argName, argNode] of providedArgs) { + if (!declaredNames.has(argName)) { + context.report({ + message: + `Unknown parameter '${argName}' passed to '${partialName}'. ` + + `Declared @params: ${[...declaredNames].join(', ')}. ` + + `Either add @param ${argName} to the partial's LiquidDoc or remove this argument.`, + startIndex: argNode.position?.start ?? node.position.start, + endIndex: argNode.position?.end ?? node.position.end, + }); + } + } + }, + }; + }, +}; +``` + +#### Adoption path + +For this check to be useful, the codebase needs to have `@param` annotations. It cannot +penalise render calls to unannotated partials (hence the early return when `declaredParams.length === 0`). The check is opt-in per partial: annotate a partial's interface +and callers are immediately validated. This creates a natural incremental adoption path — +annotate the most-called partials first. + +A companion check `UnannotatedPartial` (INFO severity) could flag partials with no LiquidDoc +block at all, encouraging annotation coverage over time. + +--- + +### 3. Dead translation keys + +**Check code:** `UnusedTranslationKey` +**Severity:** INFO (or WARNING — configurable) +**Type:** cross-file, runs at `onCodePathEnd` + +#### The problem + +`TranslationKeyExists` already catches keys that are *used but not defined*. The inverse +problem — keys that are *defined but never used* — goes completely undetected. Translation +files accumulate dead keys every time a template is renamed, a UI element is removed, or +a feature is sunset. These dead keys: + +- Bloat translation files, making them harder to maintain +- Create confusion when translators work on entries that are never displayed +- Make it impossible to know which translations actually need to be kept in sync + across languages + +#### Why it belongs in pos-cli check + +This requires building two sets across the entire project: +1. All translation keys *used* in templates (`{{ 'key' | t }}` expressions) +2. All translation keys *defined* in `.yml` files + +The difference (defined ∖ used) is the dead set. The check engine's cross-file lifecycle +(`onCodePathStart` / `onCodePathEnd`) is exactly designed for this pattern. The existing +`TranslationKeyExists` check already builds set 1; the machinery for set 2 would require +reading YAML files via `context.fs`. + +#### Implementation sketch + +```js +export const UnusedTranslationKey = { + meta: { + code: 'UnusedTranslationKey', + name: 'Translation key defined but never used', + type: SourceCodeType.LiquidHtml, // processes liquid files to build usage set + severity: Severity.INFO, + schema: { + translationFiles: { + type: 'string', + default: 'app/translations/*.yml', + description: 'Glob for translation files to analyse', + }, + }, + targets: [], + }, + create(context) { + const usedKeys = new Set(); + + return { + async LiquidFilter(node) { + // Detect: {{ 'some.key' | t }} — the filter named 't' applied to a string + if (node.name !== 't') return; + const variable = node.parent; // the LiquidVariable containing this filter + if (variable?.expression?.type === NodeTypes.String) { + usedKeys.add(variable.expression.value); + } + }, + + async onCodePathEnd() { + // After all .liquid files are processed, load and flatten all .yml files + const projectRoot = context.file.uri.replace(/\/app\/.*$/, ''); + const translationDir = path.join(projectRoot, 'app', 'translations'); + + const ymlFiles = await context.fs.readDirectory(translationDir); + for (const ymlFile of ymlFiles.filter(f => f.endsWith('.yml'))) { + const raw = await context.fs.readFile(ymlFile); + const parsed = yaml.parse(raw); // js-yaml + const definedKeys = flattenYamlKeys(parsed); // { key, value, line }[] + + for (const { key, line } of definedKeys) { + if (!usedKeys.has(key)) { + context.report({ + message: `Translation key '${key}' is defined but never used in any template.`, + uri: ymlFile, + startIndex: line, // approximate — YAML parser gives line, not index + endIndex: line, + }); + } + } + } + }, + }; + }, +}; + +function flattenYamlKeys(obj, prefix = '') { + const keys = []; + for (const [k, v] of Object.entries(obj)) { + const full = prefix ? `${prefix}.${k}` : k; + if (typeof v === 'object' && v !== null) { + keys.push(...flattenYamlKeys(v, full)); + } else { + keys.push({ key: full, value: v }); + } + } + return keys; +} +``` + +#### Caveats + +- **Dynamic keys:** `{{ some_variable | t }}` — the key is not a string literal and + cannot be statically resolved. The check must conservatively treat all non-literal + `| t` usages as "might use any key" and exclude them from the dead-key analysis + (or emit an INFO note that dynamic key usage prevents complete analysis). +- **Interpolated keys:** `{{ 'user.greeting' | t: name: current_user.name }}` — + the key IS a literal and can be tracked normally. +- **Multi-language files:** the check should union keys across all language files + (`en.yml`, `pl.yml`, etc.) — a key defined only in one language is not dead, + just untranslated. + +--- + +### 4. `TranslationKeyExists` — nearest-key suggestion + +**Modification to:** existing `TranslationKeyExists` check +**Change:** add `suggest[]` entries with closest matching keys + +#### The problem + +When `TranslationKeyExists` fires today, the offense message is: + +``` +Translation key 'app.hero.titel' does not exist. +``` + +The developer (or agent) then has to manually open the translation file, search for +similar keys, and figure out the correct spelling. In the case above the key is obviously +`app.hero.title` — a one-character typo. The check already has all the information needed +to surface this suggestion. + +#### Why it belongs in pos-cli check + +The `Offense` type already has a `suggest[]` field specifically for this purpose. A +suggestion is additional information attached to an offense that editors and tools can +surface inline. This is zero-cost to add — the check already reads all translation keys +to verify existence; finding the nearest match is just a few more lines. + +#### Implementation + +```js +// Levenshtein distance — simple O(nm) implementation, keys are short strings +function levenshtein(a, b) { + const dp = Array.from({ length: a.length + 1 }, (_, i) => + Array.from({ length: b.length + 1 }, (_, j) => (i === 0 ? j : j === 0 ? i : 0)) + ); + for (let i = 1; i <= a.length; i++) { + for (let j = 1; j <= b.length; j++) { + dp[i][j] = a[i-1] === b[j-1] + ? dp[i-1][j-1] + : 1 + Math.min(dp[i-1][j], dp[i][j-1], dp[i-1][j-1]); + } + } + return dp[a.length][b.length]; +} + +function findNearestKeys(missingKey, allKeys, maxDistance = 3, maxResults = 3) { + return allKeys + .map(key => ({ key, distance: levenshtein(missingKey, key) })) + .filter(({ distance }) => distance <= maxDistance) + .sort((a, b) => a.distance - b.distance) + .slice(0, maxResults) + .map(({ key }) => key); +} + +// In the existing TranslationKeyExists check, replace the bare context.report() with: +const nearest = findNearestKeys(missingKey, [...allDefinedKeys]); + +context.report({ + message: `Translation key '${missingKey}' does not exist.`, + startIndex: node.position.start, + endIndex: node.position.end, + suggest: nearest.map(key => ({ + message: `Did you mean '${key}'? (value: "${allDefinedKeys.get(key)}")`, + fix: { + startIndex: node.position.start, + endIndex: node.position.end, + newText: `'${key}'`, + }, + })), +}); +``` + +The `fix` on each suggestion means editors and `pos-cli check --fix` can apply the +correction automatically when the user picks a suggestion. + +#### Segment-based fallback + +For keys that have no close Levenshtein match (completely wrong key, not a typo), a +segment-based search is useful: split the missing key by `.` and find defined keys that +share at least one segment. For example, `app.header.titre` has no close match by edit +distance but shares `app.header` with several defined keys. This fallback catches +namespace errors vs typo errors. + +--- + +### 5. `HardcodedRoutes` — autofix + +**Modification to:** existing `HardcodedRoutes` check +**Change:** add `fix` to offenses so `pos-cli check --fix` can correct them + +#### The problem + +`HardcodedRoutes` fires when a literal path like `/products` or `/` appears in a template +context where `{{ routes.products_url }}` (or `{{ '/' | route_url }}`) should be used. +This is already detected. What is missing is an automatic fix. + +The check already knows: +- The offending string (e.g. `"/products"` or just `"/"`) +- Its exact position in the source +- The platformOS routes object keys (via `context.platformosDocset`) + +Everything needed to construct and apply the replacement is already present. + +#### Why it belongs in pos-cli check + +The `Offense.fix` field + `pos-cli check --fix` is the standard platformOS mechanism +for auto-correctable offenses. Adding autofix here means: +- Editors with pos-cli LSP integration can offer a one-click fix +- CI can run `pos-cli check --fix` to auto-correct before failing the build +- `pos-cli check --fix` in a batch migration can update an entire legacy codebase + +Implementing "suggest the route_url replacement" in the plugin is a workaround for a +missing feature in the tool that owns the detection. + +#### Implementation sketch + +```js +// In the existing HardcodedRoutes check, determine the fix based on the offense type: + +// Case 1: literal href="/products" — the path matches a known route slug +const matchingRouteKey = findRouteKey(literalPath, availableRoutes); +if (matchingRouteKey) { + fix = { + startIndex: literalValueStart, // the position of the string content (not the quotes) + endIndex: literalValueEnd, + newText: `{{ routes.${matchingRouteKey} }}`, + }; +} + +// Case 2: literal href="/" — root URL +if (literalPath === '/') { + fix = { + startIndex: literalValueStart, + endIndex: literalValueEnd, + newText: `{{ routes.root_url }}`, + }; +} + +// Case 3: path with no matching route key — suggest route_url filter (less specific) +if (!fix) { + fix = { + startIndex: literalValueStart, + endIndex: literalValueEnd, + newText: `{{ '${literalPath}' | route_url }}`, + }; +} + +context.report({ + message: `Hardcoded route '${literalPath}' — use {{ routes.${matchingRouteKey ?? '...'} }}`, + startIndex: node.position.start, + endIndex: node.position.end, + fix, +}); +``` + +The `StringCorrector` / `applyFixToString` infrastructure in `platformos-check-common` +handles the actual text substitution when `--fix` is invoked. + +--- + +## pos-cli LSP proposals + +### 6. GraphQL result shape in hover + +**LSP method:** `textDocument/hover` on graphql result variables +**Affected files:** `.liquid` files containing `{% graphql result = 'path/to/query' %}` + +#### The problem + +Today, hovering over the result variable `g` in: + +```liquid +{% graphql g = 'records/users' %} +{% for user in g.records.results %} +``` + +returns nothing useful. The agent (and developer) must either: +1. Open the `.graphql` file and mentally trace `query GetUsers { records { results { ... } } }` + back to the access path +2. Guess — and `g.users.results` vs `g.records.results` is the single most common source + of `UnknownProperty` errors in platformOS templates + +The correct access path depends on the query's root field name, which is determined by +the GraphQL schema and the specific query — information the LSP already has. + +#### What the hover should return + +```markdown +**`g`** ← `records/users` (GetUsers) + +**Access pattern:** +{{ g.records.results }} — array of Record objects +{{ g.records.total_entries }} — total count (for pagination) +{{ g.records.total_pages }} + +**Each record has:** +id · created_at · updated_at · properties_object · table +``` + +This eliminates a whole class of runtime errors by making the correct access path +explicit at the point of use. + +#### Implementation approach + +When the LSP receives `hover` on a variable that is the result of a `{% graphql %}` tag: + +1. **Resolve the query file:** parse the `GraphQLMarkup` node to get the query path + (e.g. `records/users`) and resolve to `app/graphql/records/users.graphql` +2. **Parse the query:** use the GraphQL parser to identify the operation name and + root selection field (`records`, `users`, `pages`, etc.) +3. **Look up the return type:** cross-reference the root selection field against the + schema to find its return type (`RecordConnection`, `UserConnection`, etc.) +4. **Build the access path:** from the connection type, derive: + - `g..results` — the items array + - `g..total_entries` — pagination count + - The shape of each item (fields selected in the query) +5. **Return as hover markdown** + +The LSP already performs steps 1–3 for GraphQL diagnostics and completions. Step 4 is +new but straightforward given the type information already resolved. + +#### Liquid variable tracking + +The LSP needs to track that `g` in the Liquid context is bound to the result of the +graphql tag, then recognise `g` in downstream `{{ g.something }}` expressions as the +same binding. This requires a lightweight Liquid variable binding tracker — the LSP +likely already has this for its existing `UndefinedObject` and `UnknownProperty` +diagnostics. + +--- + +### 7. GraphQL type field listing in hover and completions + +**LSP methods:** `textDocument/hover` on type names, `textDocument/completion` inside +selection sets +**Affected files:** `.graphql` files + +#### The problem + +When writing a GraphQL query, the agent (and developer) does not know which fields are +available on a given type without either: +- Running the query and inspecting the result +- Reading the schema file (`app/graphql/schema.graphql`) manually +- Guessing, leading to `UnknownProperty` errors + +Standard GraphQL LSP implementations provide field-level completions and hover out of +the box. The pos-cli LSP has schema awareness (it validates queries against the schema) +but may not surface this at the hover and completion level. + +#### What this should provide + +**Hover on a type name** (`Record`, `User`, `OrderItem`) in a query: +```markdown +**Record** + +Fields: +- `id` — ID +- `table` — String — the table/model name +- `created_at` — String — ISO 8601 timestamp +- `updated_at` — String +- `properties` — JSON — raw properties hash +- `properties_object` — Object — typed property access +- `related_records` — [Record] — associated records +``` + +**Completion inside a selection set** (cursor after `{`): +``` +id +table +created_at +updated_at +properties +properties_object +related_records { ... } +``` + +#### Why this belongs in the LSP + +The LSP already has the schema. GraphQL field completion is standard LSP behavior +(`CompletionItemKind.Field`). The implementation is a standard GraphQL LSP feature — +libraries like `graphql-language-service` implement this and could be integrated or +referenced. The pos-cli LSP could either implement this natively or delegate to +`graphql-language-service-interface` which is schema-aware. + +Implementing this in the plugin would require the plugin to parse the schema, +resolve types, and format completions — all work the LSP already does internally +but does not expose at the hover/completion level. + +--- + +### 8. Circular render detection via `appGraph` + +**LSP feature:** `textDocument/publishDiagnostics` on cyclic render references +**Trigger:** on file save / `didChange` for `.liquid` files involved in a cycle + +#### The problem + +If partial A renders partial B, which renders partial C, which renders partial A, the +result is an infinite loop at runtime — the page request never completes and eventually +times out or crashes. The linter does not detect this. The plugin's `ProjectIndex` knows +each partial's `renders[]` list but cycle detection is not implemented there. + +This is a correctness bug, not a style issue. A render cycle will break any page that +includes any partial in the cycle. + +#### Why it belongs in the LSP + +The LSP already builds and maintains the `appGraph` — the full render dependency graph +for the project. `appGraph/dependencies` gives the transitive dependency tree from any +file. Cycle detection over an already-built graph is a DFS with a visited set — a +trivial algorithm on top of an existing data structure. + +When the LSP detects a cycle, it can publish a diagnostic via +`textDocument/publishDiagnostics` on the specific `{% render %}` tag that closes the +cycle, pointing exactly at the offending line. + +#### Implementation approach + +After rebuilding the `appGraph` (on file save or project index refresh): + +```js +function detectCycles(graph) { + // graph: Map> (file → files it renders) + const cycles = []; + const visited = new Set(); + const stack = []; + + function dfs(node) { + if (stack.includes(node)) { + // Found a cycle — extract the cycle path + const cycleStart = stack.indexOf(node); + cycles.push(stack.slice(cycleStart).concat(node)); + return; + } + if (visited.has(node)) return; + visited.add(node); + stack.push(node); + for (const neighbour of (graph.get(node) ?? [])) { + dfs(neighbour); + } + stack.pop(); + } + + for (const node of graph.keys()) { + if (!visited.has(node)) dfs(node); + } + return cycles; +} +``` + +For each detected cycle, the LSP: +1. Identifies the `{% render %}` tag in the closing file that references back to a + file earlier in the cycle +2. Publishes a diagnostic (ERROR severity) on that specific tag: + ``` + Circular render detected: sections/hero → atoms/icon → sections/hero + This will cause an infinite loop at runtime. + ``` +3. Also publishes the same diagnostic on the opening file's render tag, so both ends + of the cycle are highlighted in the editor + +#### Incremental update + +When a file is saved, only re-run cycle detection for the connected component containing +that file — not the entire graph. The `appGraph` already supports incremental updates; +cycle detection can follow the same invalidation scope. + +--- + +## Summary + +| # | Feature | Target | Severity | Complexity | +|---|---------|--------|----------|------------| +| 1 | N+1 GraphQL query detection | pos-cli check | WARNING | Low — ~60 lines | +| 2 | Render `@param` validation | pos-cli check | ERROR/WARNING | Medium — ~120 lines + cross-file reads | +| 3 | Dead translation keys | pos-cli check | INFO | Medium — needs YAML + cross-file accumulation | +| 4 | `TranslationKeyExists` nearest-key suggest | pos-cli check | (modify existing) | Low — ~30 lines added to existing check | +| 5 | `HardcodedRoutes` autofix | pos-cli check | (modify existing) | Low — fix field on existing offense | +| 6 | GraphQL result shape in hover | pos-cli LSP | — | Medium — query→type resolution | +| 7 | GraphQL type field listing | pos-cli LSP | — | Medium — schema traversal for hover/completions | +| 8 | Circular render detection | pos-cli LSP | ERROR diagnostic | Low — DFS on existing appGraph | + +**Priority recommendation:** #1 (N+1), #4 (suggest), #5 (autofix), #8 (cycles) are the +highest ratio of value to effort. #2 (@param validation) has the most transformative +long-term impact but requires the codebase to adopt `@param` annotations first. diff --git a/packages/platformos-check-common/src/checks/graphql/index.spec.ts b/packages/platformos-check-common/src/checks/graphql/index.spec.ts index 35c837f..56fcf15 100644 --- a/packages/platformos-check-common/src/checks/graphql/index.spec.ts +++ b/packages/platformos-check-common/src/checks/graphql/index.spec.ts @@ -98,7 +98,7 @@ describe('Module: GraphQLCheck', () => { const offenses = await check(files, [GraphQLCheck], mockDependencies); expect(offenses).to.have.length(1); - expect(offenses[0].message).to.include('Syntax Error'); + expect(offenses[0].message).to.equal('Syntax Error: Expected Name, found .'); }); it('syntax error offense points to the actual error line, not the whole file', async () => { @@ -113,7 +113,7 @@ describe('Module: GraphQLCheck', () => { const offenses = await check(files, [GraphQLCheck], mockDependencies); expect(offenses).to.have.length(1); - expect(offenses[0].message).to.include('Syntax Error'); + expect(offenses[0].message).to.equal('Syntax Error: Expected Name, found .'); // Offense spans exactly one line (the error line), NOT the whole file expect(offenses[0].start.line).to.equal(offenses[0].end.line); // And that line is not the last line of the file (i.e. not spanning to the end) diff --git a/packages/platformos-check-common/src/checks/index.ts b/packages/platformos-check-common/src/checks/index.ts index a160c2d..6763048 100644 --- a/packages/platformos-check-common/src/checks/index.ts +++ b/packages/platformos-check-common/src/checks/index.ts @@ -36,6 +36,8 @@ import { GraphQLCheck } from './graphql'; import { UnknownProperty } from './unknown-property'; import { InvalidHashAssignTarget } from './invalid-hash-assign-target'; import { DuplicateFunctionArguments } from './duplicate-function-arguments'; +import { MissingRenderPartialArguments } from './missing-render-partial-arguments'; +import { NestedGraphQLQuery } from './nested-graphql-query'; import { MissingPage } from './missing-page'; export const allChecks: ( @@ -74,6 +76,8 @@ export const allChecks: ( GraphQLCheck, UnknownProperty, InvalidHashAssignTarget, + MissingRenderPartialArguments, + NestedGraphQLQuery, MissingPage, ]; diff --git a/packages/platformos-check-common/src/checks/missing-render-partial-arguments/index.spec.ts b/packages/platformos-check-common/src/checks/missing-render-partial-arguments/index.spec.ts new file mode 100644 index 0000000..de32059 --- /dev/null +++ b/packages/platformos-check-common/src/checks/missing-render-partial-arguments/index.spec.ts @@ -0,0 +1,74 @@ +import { describe, it, expect } from 'vitest'; +import { applySuggestions, runLiquidCheck } from '../../test'; +import { MissingRenderPartialArguments } from '.'; + +function check(partial: string, source: string) { + return runLiquidCheck( + MissingRenderPartialArguments, + source, + undefined, + {}, + { 'app/views/partials/card.liquid': partial }, + ); +} + +const partialWithRequiredParams = ` +{% doc %} + @param {string} title - The card title + @param {string} [subtitle] - Optional subtitle +{% enddoc %} +`; + +describe('Module: MissingRenderPartialArguments', () => { + it('should not report when partial has no LiquidDoc', async () => { + const offenses = await check('

card

', `{% render 'card' %}`); + expect(offenses).to.have.length(0); + }); + + it('should not report when all required params are provided', async () => { + const offenses = await check(partialWithRequiredParams, `{% render 'card', title: 'Hello' %}`); + expect(offenses).to.have.length(0); + }); + + it('should not report for missing optional params', async () => { + const offenses = await check(partialWithRequiredParams, `{% render 'card', title: 'Hello' %}`); + expect(offenses).to.have.length(0); + }); + + it('should report ERROR when a required param is missing', async () => { + const offenses = await check(partialWithRequiredParams, `{% render 'card' %}`); + expect(offenses).to.have.length(1); + expect(offenses[0].message).to.equal( + "Missing required argument 'title' in render tag for partial 'card'.", + ); + }); + + it('should suggest adding the missing required param', async () => { + const source = `{% render 'card' %}`; + const offenses = await check(partialWithRequiredParams, source); + expect(offenses[0].suggest).to.have.length(1); + expect(offenses[0].suggest![0].message).to.equal("Add required argument 'title'"); + const fixed = applySuggestions(source, offenses[0]); + expect(fixed).to.not.be.undefined; + expect(fixed![0]).to.equal("{% render 'card', title: '' %}"); + }); + + it('should report one ERROR per missing required param', async () => { + const partial = ` + {% doc %} + @param {string} title - title + @param {string} body - body + {% enddoc %} + `; + const offenses = await check(partial, `{% render 'card' %}`); + expect(offenses).to.have.length(2); + }); + + it('should not report for dynamic partials', async () => { + const offenses = await runLiquidCheck( + MissingRenderPartialArguments, + `{% render partial_name %}`, + ); + expect(offenses).to.have.length(0); + }); +}); diff --git a/packages/platformos-check-common/src/checks/missing-render-partial-arguments/index.ts b/packages/platformos-check-common/src/checks/missing-render-partial-arguments/index.ts new file mode 100644 index 0000000..f503722 --- /dev/null +++ b/packages/platformos-check-common/src/checks/missing-render-partial-arguments/index.ts @@ -0,0 +1,44 @@ +import { RenderMarkup } from '@platformos/liquid-html-parser'; +import { LiquidCheckDefinition, Severity, SourceCodeType } from '../../types'; +import { + getLiquidDocParams, + getPartialName, + reportMissingArguments, +} from '../../liquid-doc/arguments'; + +export const MissingRenderPartialArguments: LiquidCheckDefinition = { + meta: { + code: 'MissingRenderPartialArguments', + name: 'Missing Required Render Partial Arguments', + aliases: ['MissingRenderPartialParams'], + docs: { + description: + 'This check ensures that all required @param arguments declared by a partial are provided at the call site.', + recommended: true, + url: 'https://documentation.platformos.com/developer-guide/platformos-check/checks/missing-render-partial-arguments', + }, + type: SourceCodeType.LiquidHtml, + severity: Severity.ERROR, + schema: {}, + targets: [], + }, + + create(context) { + return { + async RenderMarkup(node: RenderMarkup) { + const partialName = getPartialName(node); + if (!partialName) return; + + const liquidDocParameters = await getLiquidDocParams(context, partialName); + if (!liquidDocParameters) return; + + const providedNames = new Set(node.args.map((a) => a.name)); + const missingRequired = [...liquidDocParameters.values()].filter( + (p) => p.required && !providedNames.has(p.name), + ); + + reportMissingArguments(context, node, missingRequired, partialName); + }, + }; + }, +}; diff --git a/packages/platformos-check-common/src/checks/nested-graphql-query/index.spec.ts b/packages/platformos-check-common/src/checks/nested-graphql-query/index.spec.ts new file mode 100644 index 0000000..cf5e905 --- /dev/null +++ b/packages/platformos-check-common/src/checks/nested-graphql-query/index.spec.ts @@ -0,0 +1,175 @@ +import { describe, it, expect } from 'vitest'; +import { runLiquidCheck, check } from '../../test'; +import { NestedGraphQLQuery } from '.'; + +describe('Module: NestedGraphQLQuery', () => { + it('should not report graphql outside a loop', async () => { + const offenses = await runLiquidCheck( + NestedGraphQLQuery, + `{% graphql result = 'products/list' %}`, + ); + expect(offenses).to.have.length(0); + }); + + it('should report graphql inside a for loop', async () => { + const offenses = await runLiquidCheck( + NestedGraphQLQuery, + `{% for item in items %}{% graphql result = 'products/get' %}{% endfor %}`, + ); + expect(offenses).to.have.length(1); + expect(offenses[0].message).to.equal( + "N+1 pattern: {% graphql result = 'result' %} is inside a {% for %} loop. This executes at least one database request per iteration. Move the query before the loop and pass data as a variable.", + ); + }); + + it('should report graphql inside a tablerow loop', async () => { + const offenses = await runLiquidCheck( + NestedGraphQLQuery, + `{% tablerow item in items %}{% graphql result = 'products/get' %}{% endtablerow %}`, + ); + expect(offenses).to.have.length(1); + expect(offenses[0].message).to.equal( + "N+1 pattern: {% graphql result = 'result' %} is inside a {% tablerow %} loop. This executes at least one database request per iteration. Move the query before the loop and pass data as a variable.", + ); + }); + + it('should report graphql inside nested loops', async () => { + const offenses = await runLiquidCheck( + NestedGraphQLQuery, + `{% for a in items %}{% for b in a.children %}{% graphql result = 'foo' %}{% endfor %}{% endfor %}`, + ); + expect(offenses).to.have.length(1); + }); + + it('should not report graphql inside a loop when wrapped in cache', async () => { + const offenses = await runLiquidCheck( + NestedGraphQLQuery, + `{% for item in items %}{% cache 'key' %}{% graphql result = 'foo' %}{% endcache %}{% endfor %}`, + ); + expect(offenses).to.have.length(0); + }); + + it('should not report background tag inside a loop', async () => { + const offenses = await runLiquidCheck( + NestedGraphQLQuery, + `{% for item in items %}{% background %}{% graphql result = 'foo' %}{% endbackground %}{% endfor %}`, + ); + expect(offenses).to.have.length(0); + }); + + it('should report graphql inline markup inside a for loop', async () => { + const offenses = await runLiquidCheck( + NestedGraphQLQuery, + `{% for item in items %}{% graphql result %}query { records { results { id } } }{% endgraphql %}{% endfor %}`, + ); + expect(offenses).to.have.length(1); + expect(offenses[0].message).to.equal( + "N+1 pattern: {% graphql result = 'result' %} is inside a {% for %} loop. This executes at least one database request per iteration. Move the query before the loop and pass data as a variable.", + ); + }); + + it('should report multiple graphql tags inside one loop', async () => { + const offenses = await runLiquidCheck( + NestedGraphQLQuery, + `{% for item in items %}{% graphql a = 'foo' %}{% graphql b = 'bar' %}{% endfor %}`, + ); + expect(offenses).to.have.length(2); + }); + + it('should report function call inside loop that transitively calls graphql', async () => { + const offenses = await check( + { + 'app/views/pages/index.liquid': `{% for item in items %}{% function res = 'my_partial' %}{% endfor %}`, + 'app/lib/my_partial.liquid': `{% graphql result = 'products/get' %}`, + }, + [NestedGraphQLQuery], + ); + expect(offenses).to.have.length(1); + expect(offenses[0].message).to.equal( + "N+1 pattern: {% function 'my_partial' %} inside a {% for %} loop transitively calls a GraphQL query (my_partial). Move the query before the loop and pass data as a variable.", + ); + }); + + it('should report render call inside loop that transitively calls graphql', async () => { + const offenses = await check( + { + 'app/views/pages/index.liquid': `{% for item in items %}{% render 'my_partial' %}{% endfor %}`, + 'app/views/partials/my_partial.liquid': `{% graphql result = 'products/get' %}`, + }, + [NestedGraphQLQuery], + ); + expect(offenses).to.have.length(1); + expect(offenses[0].message).to.equal( + "N+1 pattern: {% render 'my_partial' %} inside a {% for %} loop transitively calls a GraphQL query (my_partial). Move the query before the loop and pass data as a variable.", + ); + }); + + it('should report function call that transitively calls graphql through another function', async () => { + const offenses = await check( + { + 'app/views/pages/index.liquid': `{% for item in items %}{% function res = 'outer' %}{% endfor %}`, + 'app/lib/outer.liquid': `{% function inner_res = 'inner' %}`, + 'app/lib/inner.liquid': `{% graphql result = 'products/get' %}`, + }, + [NestedGraphQLQuery], + ); + expect(offenses).to.have.length(1); + expect(offenses[0].message).to.equal( + "N+1 pattern: {% function 'outer' %} inside a {% for %} loop transitively calls a GraphQL query (outer \u2192 inner). Move the query before the loop and pass data as a variable.", + ); + }); + + it('should not report function call inside loop that does not call graphql', async () => { + const offenses = await check( + { + 'app/views/pages/index.liquid': `{% for item in items %}{% function res = 'safe_partial' %}{% endfor %}`, + 'app/lib/safe_partial.liquid': `{{ 'hello' }}`, + }, + [NestedGraphQLQuery], + ); + expect(offenses).to.have.length(0); + }); + + it('should not report function call inside loop when partial does not exist', async () => { + const offenses = await check( + { + 'app/views/pages/index.liquid': `{% for item in items %}{% function res = 'nonexistent' %}{% endfor %}`, + }, + [NestedGraphQLQuery], + ); + expect(offenses).to.have.length(0); + }); + + it('should not report function call inside loop with cache wrapping', async () => { + const offenses = await check( + { + 'app/views/pages/index.liquid': `{% for item in items %}{% cache 'key' %}{% function res = 'my_partial' %}{% endcache %}{% endfor %}`, + 'app/lib/my_partial.liquid': `{% graphql result = 'products/get' %}`, + }, + [NestedGraphQLQuery], + ); + expect(offenses).to.have.length(0); + }); + + it('should handle circular function calls without infinite loop', async () => { + const offenses = await check( + { + 'app/views/pages/index.liquid': `{% for item in items %}{% function res = 'partial_a' %}{% endfor %}`, + 'app/lib/partial_a.liquid': `{% function res = 'partial_b' %}`, + 'app/lib/partial_b.liquid': `{% function res = 'partial_a' %}`, + }, + [NestedGraphQLQuery], + ); + expect(offenses).to.have.length(0); + }); + + it('should skip function calls with dynamic partial names', async () => { + const offenses = await check( + { + 'app/views/pages/index.liquid': `{% for item in items %}{% function res = partial_name %}{% endfor %}`, + }, + [NestedGraphQLQuery], + ); + expect(offenses).to.have.length(0); + }); +}); diff --git a/packages/platformos-check-common/src/checks/nested-graphql-query/index.ts b/packages/platformos-check-common/src/checks/nested-graphql-query/index.ts new file mode 100644 index 0000000..6ab6490 --- /dev/null +++ b/packages/platformos-check-common/src/checks/nested-graphql-query/index.ts @@ -0,0 +1,203 @@ +import { + LiquidHtmlNode, + NamedTags, + NodeTypes, + toLiquidHtmlAST, +} from '@platformos/liquid-html-parser'; +import { DocumentsLocator } from '@platformos/platformos-common'; +import { URI } from 'vscode-uri'; +import { LiquidCheckDefinition, Severity, SourceCodeType } from '../../types'; +import { isLoopLiquidTag } from '../utils'; + +const SKIP_IF_ANCESTOR_TAGS = [NamedTags.cache]; + +type GraphQLFound = { + type: 'graphql'; + partialChain: string[]; +}; + +type FunctionOrRenderFound = { + type: 'function' | 'render'; + partialName: string; +}; + +type FoundNode = GraphQLFound | FunctionOrRenderFound; + +function findNodesInAST(ast: LiquidHtmlNode[]): FoundNode[] { + const results: FoundNode[] = []; + const stack: LiquidHtmlNode[] = [...ast]; + + while (stack.length > 0) { + const node = stack.pop()!; + + if (node.type === NodeTypes.LiquidTag) { + if (node.name === NamedTags.graphql) { + results.push({ type: 'graphql', partialChain: [] }); + } else if ( + (node.name === NamedTags.function || node.name === NamedTags.render) && + typeof node.markup !== 'string' && + 'partial' in node.markup && + node.markup.partial.type !== NodeTypes.VariableLookup + ) { + results.push({ type: node.name, partialName: node.markup.partial.value }); + } + + if ('children' in node && Array.isArray(node.children)) { + stack.push(...node.children); + } + } else if ('children' in node && Array.isArray((node as any).children)) { + stack.push(...(node as any).children); + } + } + + return results; +} + +async function containsGraphQLTransitively( + locator: DocumentsLocator, + fs: { readFile(uri: string): Promise }, + rootUri: URI, + partialName: string, + tagType: 'function' | 'render', + visited: Set, +): Promise { + if (visited.has(partialName)) return null; + visited.add(partialName); + + const location = await locator.locate(rootUri, tagType, partialName); + if (!location) return null; + + let source: string; + try { + source = await fs.readFile(location); + } catch { + return null; + } + + let ast; + try { + ast = toLiquidHtmlAST(source); + } catch { + return null; + } + + const nodes = findNodesInAST(ast.children); + + for (const found of nodes) { + if (found.type === 'graphql') { + return [partialName]; + } + } + + for (const found of nodes) { + if (found.type === 'function' || found.type === 'render') { + const chain = await containsGraphQLTransitively( + locator, + fs, + rootUri, + found.partialName, + found.type, + visited, + ); + if (chain) { + return [partialName, ...chain]; + } + } + } + + return null; +} + +export const NestedGraphQLQuery: LiquidCheckDefinition = { + meta: { + code: 'NestedGraphQLQuery', + name: 'Prevent N+1 GraphQL queries in loops', + docs: { + description: + 'This check detects {% graphql %} tags placed inside loop tags ({% for %}, {% tablerow %}), which causes one database request per loop iteration (N+1 pattern). It also follows {% function %} and {% render %} calls transitively to detect indirect GraphQL queries.', + recommended: true, + url: 'https://documentation.platformos.com/developer-guide/platformos-check/checks/nested-graphql-query', + }, + type: SourceCodeType.LiquidHtml, + severity: Severity.WARNING, + schema: {}, + targets: [], + }, + + create(context) { + const locator = new DocumentsLocator(context.fs); + const rootUri = URI.parse(context.config.rootUri); + + function isInsideLoopWithoutCacheOrBackground(ancestors: LiquidHtmlNode[]) { + const ancestorTags = ancestors.filter((a) => a.type === NodeTypes.LiquidTag); + const loopAncestor = ancestorTags.find(isLoopLiquidTag); + if (!loopAncestor) return null; + + const inBackground = ancestorTags.some((a) => a.name === NamedTags.background); + if (inBackground) return null; + + const shouldSkip = ancestorTags.some((a) => + SKIP_IF_ANCESTOR_TAGS.map((a) => a.toString()).includes(a.name), + ); + if (shouldSkip) return null; + + return loopAncestor; + } + + return { + async LiquidTag(node, ancestors) { + if (node.name === NamedTags.graphql) { + const loopAncestor = isInsideLoopWithoutCacheOrBackground(ancestors); + if (!loopAncestor) return; + + let resultName = ''; + if ( + typeof node.markup !== 'string' && + (node.markup.type === NodeTypes.GraphQLMarkup || + node.markup.type === NodeTypes.GraphQLInlineMarkup) + ) { + resultName = node.markup.name ? ` result = '${node.markup.name}'` : ''; + } + + const graphqlStr = resultName ? `{% graphql${resultName} %}` : '{% graphql %}'; + context.report({ + message: `N+1 pattern: ${graphqlStr} is inside a {% ${loopAncestor.name} %} loop. This executes at least one database request per iteration. Move the query before the loop and pass data as a variable.`, + startIndex: node.position.start, + endIndex: node.position.end, + }); + } else if (node.name === NamedTags.function || node.name === NamedTags.render) { + const loopAncestor = isInsideLoopWithoutCacheOrBackground(ancestors); + if (!loopAncestor) return; + + if ( + typeof node.markup === 'string' || + !('partial' in node.markup) || + node.markup.partial.type === NodeTypes.VariableLookup + ) { + return; + } + + const partialName = node.markup.partial.value; + const visited = new Set(); + const chain = await containsGraphQLTransitively( + locator, + context.fs, + rootUri, + partialName, + node.name, + visited, + ); + + if (chain) { + const chainStr = chain.join(' → '); + context.report({ + message: `N+1 pattern: {% ${node.name} '${partialName}' %} inside a {% ${loopAncestor.name} %} loop transitively calls a GraphQL query (${chainStr}). Move the query before the loop and pass data as a variable.`, + startIndex: node.position.start, + endIndex: node.position.end, + }); + } + } + }, + }; + }, +}; diff --git a/packages/platformos-check-common/src/checks/parser-blocking-script/index.spec.ts b/packages/platformos-check-common/src/checks/parser-blocking-script/index.spec.ts index 6403b91..06c3fd9 100644 --- a/packages/platformos-check-common/src/checks/parser-blocking-script/index.spec.ts +++ b/packages/platformos-check-common/src/checks/parser-blocking-script/index.spec.ts @@ -20,7 +20,9 @@ describe('Module: ParserBlockingScript', () => { expect(offenses).to.have.length(1); const { check, message, start, end } = offenses[0]; expect(check).to.equal(ParserBlockingScript.meta.code); - expect(message).to.contain('Avoid parser blocking scripts by adding `defer`'); + expect(message).to.equal( + 'Avoid parser blocking scripts by adding `defer` or `async` on this tag', + ); expect(start.index).to.equal(startIndex); expect(end.index).to.equal(endIndex); }); @@ -45,8 +47,10 @@ describe('Module: ParserBlockingScript', () => { }); const suggestions = applySuggestions(file, offense); - expect(suggestions).to.include(''); - expect(suggestions).to.include(''); + expect(suggestions).to.deep.equal([ + '', + '', + ]); }); }); diff --git a/packages/platformos-check-common/src/checks/translation-key-exists/index.spec.ts b/packages/platformos-check-common/src/checks/translation-key-exists/index.spec.ts index e45f6de..3860cac 100644 --- a/packages/platformos-check-common/src/checks/translation-key-exists/index.spec.ts +++ b/packages/platformos-check-common/src/checks/translation-key-exists/index.spec.ts @@ -38,7 +38,84 @@ describe('Module: TranslationKeyExists', () => { ); expect(offenses).to.have.length(1); - expect(offenses[0].message).to.include('missing.key'); - expect(offenses[0].message).to.include('does not have a matching translation entry'); + expect(offenses[0].message).to.equal( + "'missing.key' does not have a matching translation entry", + ); + }); + + it('should suggest nearest key when the key is a typo', async () => { + const offenses = await check( + { + 'app/translations/en.yml': 'en:\n general:\n title: Hello', + 'code.liquid': `{{"general.titel" | t}}`, + }, + [TranslationKeyExists], + ); + + expect(offenses).to.have.length(1); + expect(offenses[0].suggest).to.have.length(1); + expect(offenses[0].suggest![0].message).to.equal("Did you mean 'general.title'?"); + }); + + it('should not add suggestions when there is no close key', async () => { + const offenses = await check( + { + 'app/translations/en.yml': 'en:\n general:\n title: Hello', + 'code.liquid': `{{"completely.different.xyz" | t}}`, + }, + [TranslationKeyExists], + ); + + expect(offenses).to.have.length(1); + expect(offenses[0].suggest ?? []).to.have.length(0); + }); + + it('should not report a module translation key that exists', async () => { + const offenses = await check( + { + 'app/modules/user/public/translations/en.yml': 'en:\n greeting: Hello', + 'code.liquid': '{{"modules/user/greeting" | t}}', + }, + [TranslationKeyExists], + ); + expect(offenses).to.have.length(0); + }); + + it('should report a module translation key that does not exist', async () => { + const offenses = await check( + { + 'app/modules/user/public/translations/en.yml': 'en:\n greeting: Hello', + 'code.liquid': '{{"modules/user/missing" | t}}', + }, + [TranslationKeyExists], + ); + expect(offenses).to.have.length(1); + expect(offenses[0].message).to.equal( + "'modules/user/missing' does not have a matching translation entry", + ); + }); + + it('should suggest nearest module key for typos', async () => { + const offenses = await check( + { + 'app/modules/user/public/translations/en.yml': 'en:\n greeting: Hello', + 'code.liquid': '{{"modules/user/greating" | t}}', + }, + [TranslationKeyExists], + ); + expect(offenses).to.have.length(1); + expect(offenses[0].suggest).to.have.length(1); + expect(offenses[0].suggest![0].message).to.equal("Did you mean 'modules/user/greeting'?"); + }); + + it('should find keys in legacy modules/ path', async () => { + const offenses = await check( + { + 'modules/core/public/translations/en.yml': 'en:\n label: Label', + 'code.liquid': '{{"modules/core/label" | t}}', + }, + [TranslationKeyExists], + ); + expect(offenses).to.have.length(0); }); }); diff --git a/packages/platformos-check-common/src/checks/translation-key-exists/index.ts b/packages/platformos-check-common/src/checks/translation-key-exists/index.ts index 24fb8f4..97ca213 100644 --- a/packages/platformos-check-common/src/checks/translation-key-exists/index.ts +++ b/packages/platformos-check-common/src/checks/translation-key-exists/index.ts @@ -1,22 +1,6 @@ -import { TranslationProvider } from '@platformos/platformos-common'; import { LiquidCheckDefinition, Severity, SourceCodeType } from '../../types'; -import { URI } from 'vscode-uri'; - -function keyExists(key: string, pointer: any) { - for (const token of key.split('.')) { - if (typeof pointer !== 'object') { - return false; - } - - if (!pointer.hasOwnProperty(token)) { - return false; - } - - pointer = pointer[token]; - } - - return true; -} +import { findNearestKeys } from '../../utils/levenshtein'; +import { loadAllDefinedKeys } from '../translation-utils'; export const TranslationKeyExists: LiquidCheckDefinition = { meta: { @@ -35,7 +19,6 @@ export const TranslationKeyExists: LiquidCheckDefinition = { create(context) { const nodes: { translationKey: string; startIndex: number; endIndex: number }[] = []; - const translationProvider = new TranslationProvider(context.fs); return { async LiquidVariable(node) { @@ -55,21 +38,29 @@ export const TranslationKeyExists: LiquidCheckDefinition = { }, async onCodePathEnd() { - for (const { translationKey, startIndex, endIndex } of nodes) { - const translation = await translationProvider.translate( - URI.parse(context.config.rootUri), - translationKey, - ); + if (nodes.length === 0) return; + + // Load all defined keys (app + modules) once per file + const allDefinedKeys = await loadAllDefinedKeys(context); + const definedKeySet = new Set(allDefinedKeys); - if (!!translation) { - return; - } + for (const { translationKey, startIndex, endIndex } of nodes) { + if (definedKeySet.has(translationKey)) continue; + const nearest = findNearestKeys(translationKey, allDefinedKeys); const message = `'${translationKey}' does not have a matching translation entry`; + context.report({ message, startIndex, endIndex, + suggest: + nearest.length > 0 + ? nearest.map((key) => ({ + message: `Did you mean '${key}'?`, + fix: (fixer: any) => fixer.replace(startIndex, endIndex, `'${key}'`), + })) + : undefined, }); } }, diff --git a/packages/platformos-check-common/src/checks/translation-utils.ts b/packages/platformos-check-common/src/checks/translation-utils.ts new file mode 100644 index 0000000..bed4c2c --- /dev/null +++ b/packages/platformos-check-common/src/checks/translation-utils.ts @@ -0,0 +1,63 @@ +import { FileType, TranslationProvider } from '@platformos/platformos-common'; +import { flattenTranslationKeys } from '../utils/levenshtein'; + +/** + * Discovers all module names by listing app/modules/ and modules/ directories. + * Returns a deduplicated set of module names. + */ +export async function discoverModules( + fs: { readDirectory(uri: string): Promise<[string, FileType][]> }, + ...moduleDirUris: string[] +): Promise> { + const modules = new Set(); + for (const dirUri of moduleDirUris) { + try { + const entries = await fs.readDirectory(dirUri); + for (const [entryUri, entryType] of entries) { + if (entryType === FileType.Directory) { + modules.add(entryUri.split('/').pop()!); + } + } + } catch (error) { + console.error(`[translation-utils] Failed to read module directory ${dirUri}:`, error); + } + } + return modules; +} + +export interface TranslationContext { + fs: { readDirectory(uri: string): Promise<[string, FileType][]> }; + toUri(relativePath: string): string; + getTranslationsForBase(uri: string, locale: string): Promise>; +} + +/** + * Loads all defined translation keys (app-level + module-level) and returns + * them as a flat string array. Module keys are prefixed with `modules/{name}/`. + */ +export async function loadAllDefinedKeys(context: TranslationContext): Promise { + const definedKeys: string[] = []; + + // App-level translations + for (const base of TranslationProvider.getSearchPaths()) { + const translations = await context.getTranslationsForBase(context.toUri(base), 'en'); + definedKeys.push(...flattenTranslationKeys(translations)); + } + + // Module translations + const modules = await discoverModules( + context.fs, + context.toUri('app/modules'), + context.toUri('modules'), + ); + for (const moduleName of modules) { + for (const base of TranslationProvider.getSearchPaths(moduleName)) { + const translations = await context.getTranslationsForBase(context.toUri(base), 'en'); + for (const key of flattenTranslationKeys(translations)) { + definedKeys.push(`modules/${moduleName}/${key}`); + } + } + } + + return definedKeys; +} diff --git a/packages/platformos-check-common/src/checks/undefined-object/index.spec.ts b/packages/platformos-check-common/src/checks/undefined-object/index.spec.ts index 96d3835..ebd4e8f 100644 --- a/packages/platformos-check-common/src/checks/undefined-object/index.spec.ts +++ b/packages/platformos-check-common/src/checks/undefined-object/index.spec.ts @@ -541,4 +541,34 @@ describe('Module: UndefinedObject', () => { expect(offenses).toHaveLength(1); expect(offenses[0].message).toBe("Unknown object 'groups_data' used."); }); + + it('should not report an offense for catch variable inside catch block', async () => { + const sourceCode = ` + {% try %} + {{ "something" }} + {% catch error %} + {{ error }} + {% endtry %} + `; + + const offenses = await runLiquidCheck(UndefinedObject, sourceCode); + + expect(offenses).toHaveLength(0); + }); + + it('should report an offense for catch variable used outside catch block', async () => { + const sourceCode = ` + {% try %} + {{ "something" }} + {% catch error %} + {{ error }} + {% endtry %} + {{ error }} + `; + + const offenses = await runLiquidCheck(UndefinedObject, sourceCode); + + expect(offenses).toHaveLength(1); + expect(offenses[0].message).toBe("Unknown object 'error' used."); + }); }); diff --git a/packages/platformos-check-common/src/checks/undefined-object/index.ts b/packages/platformos-check-common/src/checks/undefined-object/index.ts index 60a7f23..f151b31 100644 --- a/packages/platformos-check-common/src/checks/undefined-object/index.ts +++ b/packages/platformos-check-common/src/checks/undefined-object/index.ts @@ -9,7 +9,6 @@ import { LiquidTagIncrement, LiquidTagTablerow, LiquidVariableLookup, - LiquidTagFunction, NamedTags, NodeTypes, Position, @@ -23,6 +22,7 @@ import { import { LiquidCheckDefinition, Severity, SourceCodeType, PlatformOSDocset } from '../../types'; import { isError, last } from '../../utils'; import { isWithinRawTagThatDoesNotParseItsContents } from '../utils'; +import yaml from 'js-yaml'; type Scope = { start?: number; end?: number }; @@ -158,6 +158,24 @@ export const UndefinedObject: LiquidCheckDefinition = { } }, + async LiquidBranch(node, ancestors) { + if (isWithinRawTagThatDoesNotParseItsContents(ancestors)) return; + + // {% try %} ... {% catch error %} registers the error variable + if ( + node.name === NamedTags.catch && + node.markup && + typeof node.markup !== 'string' && + 'name' in node.markup && + node.markup.name + ) { + indexVariableScope(node.markup.name, { + start: node.blockStartPosition.end, + end: node.blockEndPosition?.start, + }); + } + }, + async VariableLookup(node, ancestors) { if (isWithinRawTagThatDoesNotParseItsContents(ancestors)) return; @@ -166,6 +184,8 @@ export const UndefinedObject: LiquidCheckDefinition = { if (isLiquidTag(parent) && isLiquidTagParseJson(parent)) return; // Skip the result variable of function tags (it's a definition, not a usage) if (isFunctionMarkup(parent) && parent.name === node) return; + // Skip the error variable definition in catch branches + if (isLiquidBranchCatch(parent) && parent.markup === node) return; variables.push(node); }, @@ -313,3 +333,9 @@ function isLiquidTagBackground( function isFunctionMarkup(node?: LiquidHtmlNode): node is FunctionMarkup { return node?.type === NodeTypes.FunctionMarkup; } + +function isLiquidBranchCatch( + node?: LiquidHtmlNode, +): node is LiquidHtmlNode & { type: typeof NodeTypes.LiquidBranch; name: 'catch'; markup: any } { + return node?.type === NodeTypes.LiquidBranch && (node as any).name === NamedTags.catch; +} diff --git a/packages/platformos-check-common/src/checks/unused-assign/index.spec.ts b/packages/platformos-check-common/src/checks/unused-assign/index.spec.ts index 70d4269..11ddcad 100644 --- a/packages/platformos-check-common/src/checks/unused-assign/index.spec.ts +++ b/packages/platformos-check-common/src/checks/unused-assign/index.spec.ts @@ -56,7 +56,7 @@ describe('Module: UnusedAssign', () => { {{ usedVar }} `; - expect(suggestions).to.include(expectedFixedCode); + expect(suggestions).to.deep.equal([expectedFixedCode]); }); it('should not report unused assigns for things used in a capture tag', async () => { diff --git a/packages/platformos-check-common/src/checks/unused-doc-param/index.spec.ts b/packages/platformos-check-common/src/checks/unused-doc-param/index.spec.ts index 9153f75..bac5e8b 100644 --- a/packages/platformos-check-common/src/checks/unused-doc-param/index.spec.ts +++ b/packages/platformos-check-common/src/checks/unused-doc-param/index.spec.ts @@ -51,14 +51,16 @@ describe('Module: UnusedDocParam', () => { const offenses = await runLiquidCheck(UnusedDocParam, sourceCode); const suggestions = applySuggestions(sourceCode, offenses[0]); - expect(suggestions).to.include(` + expect(suggestions).to.deep.equal([ + ` {% doc %} @param param1 - Example param {% enddoc %} {{ param1 }} - `); + `, + ]); }); LoopNamedTags.forEach((tag) => { diff --git a/packages/platformos-check-common/src/checks/valid-doc-param-types/index.spec.ts b/packages/platformos-check-common/src/checks/valid-doc-param-types/index.spec.ts index 2e66619..c5f45bc 100644 --- a/packages/platformos-check-common/src/checks/valid-doc-param-types/index.spec.ts +++ b/packages/platformos-check-common/src/checks/valid-doc-param-types/index.spec.ts @@ -69,7 +69,7 @@ describe('Module: ValidDocParamTypes', () => { expect(offenses).to.have.length(1); const suggestions = applySuggestions(source, offenses[0]); - expect(suggestions).to.include(`{% doc %} @param param1 - Example param {% enddoc %}`); + expect(suggestions).to.deep.equal([`{% doc %} @param param1 - Example param {% enddoc %}`]); } }); }); diff --git a/packages/platformos-check-common/src/checks/valid-render-partial-argument-types/index.spec.ts b/packages/platformos-check-common/src/checks/valid-render-partial-argument-types/index.spec.ts index fd2b376..10b7eaa 100644 --- a/packages/platformos-check-common/src/checks/valid-render-partial-argument-types/index.spec.ts +++ b/packages/platformos-check-common/src/checks/valid-render-partial-argument-types/index.spec.ts @@ -34,7 +34,7 @@ describe('Module: ValidRenderPartialParamTypes', () => { { value: "'hello'", actualType: BasicParamTypes.String }, { value: '123', actualType: BasicParamTypes.Number }, { value: 'true', actualType: BasicParamTypes.Boolean }, - { value: 'empty', actualType: BasicParamTypes.Boolean }, + { value: 'empty', actualType: BasicParamTypes.String }, ], }, ]; @@ -142,6 +142,29 @@ describe('Module: ValidRenderPartialParamTypes', () => { expect(offenses).toHaveLength(0); }); + it('should not report null/nil as type mismatch for any type', async () => { + for (const type of ['string', 'number', 'object', 'boolean']) { + for (const literal of ['nil', 'null']) { + const sourceCode = `{% render 'card', param: ${literal} %}`; + const offenses = await runLiquidCheck( + ValidRenderPartialArgumentTypes, + sourceCode, + undefined, + {}, + { + 'app/views/partials/card.liquid': ` + {% doc %} + @param {${type}} param - Description + {% enddoc %} +
{{ param }}
+ `, + }, + ); + expect(offenses, `${literal} should be valid for ${type}`).toHaveLength(0); + } + } + }); + it('should not enforce unsupported types', async () => { const sourceCode = `{% render 'card', title: 123 %}`; const offenses = await runLiquidCheck( diff --git a/packages/platformos-check-common/src/checks/valid-render-partial-argument-types/index.ts b/packages/platformos-check-common/src/checks/valid-render-partial-argument-types/index.ts index f3a26c4..4c7e964 100644 --- a/packages/platformos-check-common/src/checks/valid-render-partial-argument-types/index.ts +++ b/packages/platformos-check-common/src/checks/valid-render-partial-argument-types/index.ts @@ -1,7 +1,7 @@ import { LiquidCheckDefinition, Severity, SourceCodeType } from '../../types'; import { NodeTypes, RenderMarkup } from '@platformos/liquid-html-parser'; import { LiquidDocParameter } from '../../liquid-doc/liquidDoc'; -import { inferArgumentType, isTypeCompatible } from '../../liquid-doc/utils'; +import { inferArgumentType, isNullLiteral, isTypeCompatible } from '../../liquid-doc/utils'; import { findTypeMismatchParams, generateTypeMismatchSuggestions, @@ -41,7 +41,8 @@ export const ValidRenderPartialArgumentTypes: LiquidCheckDefinition = { if ( node.alias && node.variable?.name && - node.variable.name.type !== NodeTypes.VariableLookup + node.variable.name.type !== NodeTypes.VariableLookup && + !isNullLiteral(node.variable.name) ) { const paramIsDefinedWithType = liquidDocParameters .get(node.alias.value) diff --git a/packages/platformos-check-common/src/checks/variable-name/index.spec.ts b/packages/platformos-check-common/src/checks/variable-name/index.spec.ts index 5071b79..01276e9 100644 --- a/packages/platformos-check-common/src/checks/variable-name/index.spec.ts +++ b/packages/platformos-check-common/src/checks/variable-name/index.spec.ts @@ -40,7 +40,7 @@ describe('Module: VariableName', () => { const expectedFixedCode = `{% assign variable_name = "value" %}`; - expect(suggestions).to.include(expectedFixedCode); + expect(suggestions).to.deep.equal([expectedFixedCode]); }); it('should not report an error for variables starting with underscore', async () => { diff --git a/packages/platformos-check-common/src/context-utils.ts b/packages/platformos-check-common/src/context-utils.ts index d3b2413..624f700 100644 --- a/packages/platformos-check-common/src/context-utils.ts +++ b/packages/platformos-check-common/src/context-utils.ts @@ -171,7 +171,13 @@ export async function recursiveReadDirectory( uri: string, filter: (fileTuple: FileTuple) => boolean, ): Promise { - const allFiles = await fs.readDirectory(uri); + let allFiles: FileTuple[]; + try { + allFiles = await fs.readDirectory(uri); + } catch (err: any) { + if (err?.code === 'ENOENT') return []; + throw err; + } const files = allFiles.filter((ft) => !isIgnored(ft) && (isDirectory(ft) || filter(ft))); const results = await Promise.all( diff --git a/packages/platformos-check-common/src/liquid-doc/arguments.ts b/packages/platformos-check-common/src/liquid-doc/arguments.ts index f97a6c8..eb315c8 100644 --- a/packages/platformos-check-common/src/liquid-doc/arguments.ts +++ b/packages/platformos-check-common/src/liquid-doc/arguments.ts @@ -13,6 +13,7 @@ import { BasicParamTypes, getDefaultValueForType, inferArgumentType, + isNullLiteral, isTypeCompatible, } from './utils'; import { isLiquidString } from '../checks/utils'; @@ -133,6 +134,11 @@ export function findTypeMismatchParams( continue; } + // null/nil is compatible with any type — skip type checking + if (isNullLiteral(arg.value)) { + continue; + } + const liquidDocParamDef = liquidDocParameters.get(arg.name); if (liquidDocParamDef && liquidDocParamDef.type) { const paramType = liquidDocParamDef.type.toLowerCase(); diff --git a/packages/platformos-check-common/src/liquid-doc/utils.ts b/packages/platformos-check-common/src/liquid-doc/utils.ts index caec8fa..870ba6f 100644 --- a/packages/platformos-check-common/src/liquid-doc/utils.ts +++ b/packages/platformos-check-common/src/liquid-doc/utils.ts @@ -18,6 +18,11 @@ export enum BasicParamTypes { Object = 'object', } +/** Inferred type for null/nil literals — not a valid @param type, only used in type mismatch messages. */ +export const InferredNull = 'null' as const; + +export type InferredParamType = BasicParamTypes | typeof InferredNull; + export enum SupportedDocTagTypes { Param = 'param', Example = 'example', @@ -44,7 +49,7 @@ export function getDefaultValueForType(type: string | null) { /** * Casts the value of a LiquidNamedArgument to a string representing the type of the value. */ -export function inferArgumentType(arg: LiquidExpression | LiquidVariable): BasicParamTypes { +export function inferArgumentType(arg: LiquidExpression | LiquidVariable): InferredParamType { if (arg.type === NodeTypes.LiquidVariable) { // A variable with filters — delegate to the base expression if there are no filters, // otherwise we can't statically determine the filtered output type. @@ -59,6 +64,8 @@ export function inferArgumentType(arg: LiquidExpression | LiquidVariable): Basic case NodeTypes.Number: return BasicParamTypes.Number; case NodeTypes.LiquidLiteral: + if (arg.value === null) return InferredNull; + if (arg.value === '') return BasicParamTypes.String; return BasicParamTypes.Boolean; case NodeTypes.Range: case NodeTypes.VariableLookup: @@ -71,12 +78,29 @@ export function inferArgumentType(arg: LiquidExpression | LiquidVariable): Basic } } +/** + * Checks if a LiquidExpression is a null/nil literal. + * null/nil is compatible with any type — it represents "no value". + */ +export function isNullLiteral(arg: LiquidExpression | LiquidVariable): boolean { + if (arg.type === NodeTypes.LiquidVariable) { + if (arg.filters.length > 0) return false; + const expr = arg.expression; + if (expr.type === NodeTypes.BooleanExpression) return false; + return isNullLiteral(expr); + } + if (arg.type === NodeTypes.LiquidLiteral) { + return arg.value === null; + } + return false; +} + /** * Checks if the provided argument type is compatible with the expected type. * Makes certain types more permissive: * - Boolean accepts any value, since everything is truthy / falsy in Liquid */ -export function isTypeCompatible(expectedType: string, actualType: BasicParamTypes): boolean { +export function isTypeCompatible(expectedType: string, actualType: InferredParamType): boolean { const normalizedExpectedType = expectedType.toLowerCase(); if (normalizedExpectedType === BasicParamTypes.Boolean) { diff --git a/packages/platformos-check-common/src/utils/index.ts b/packages/platformos-check-common/src/utils/index.ts index 0a2062a..5f22560 100644 --- a/packages/platformos-check-common/src/utils/index.ts +++ b/packages/platformos-check-common/src/utils/index.ts @@ -5,3 +5,4 @@ export * from './error'; export * from './types'; export * from './memo'; export * from './indexBy'; +export * from './levenshtein'; diff --git a/packages/platformos-check-common/src/utils/levenshtein.ts b/packages/platformos-check-common/src/utils/levenshtein.ts new file mode 100644 index 0000000..eb8d190 --- /dev/null +++ b/packages/platformos-check-common/src/utils/levenshtein.ts @@ -0,0 +1,41 @@ +export function levenshtein(a: string, b: string): number { + const dp: number[][] = Array.from({ length: a.length + 1 }, (_, i) => + Array.from({ length: b.length + 1 }, (_, j) => (i === 0 ? j : j === 0 ? i : 0)), + ); + for (let i = 1; i <= a.length; i++) { + for (let j = 1; j <= b.length; j++) { + dp[i][j] = + a[i - 1] === b[j - 1] + ? dp[i - 1][j - 1] + : 1 + Math.min(dp[i - 1][j], dp[i][j - 1], dp[i - 1][j - 1]); + } + } + return dp[a.length][b.length]; +} + +export function flattenTranslationKeys(obj: Record, prefix = ''): string[] { + const keys: string[] = []; + for (const [k, v] of Object.entries(obj)) { + const full = prefix ? `${prefix}.${k}` : k; + if (typeof v === 'object' && v !== null) { + keys.push(...flattenTranslationKeys(v, full)); + } else { + keys.push(full); + } + } + return keys; +} + +export function findNearestKeys( + missingKey: string, + allKeys: string[], + maxDistance = 3, + maxResults = 3, +): string[] { + return allKeys + .map((key) => ({ key, distance: levenshtein(missingKey, key) })) + .filter(({ distance }) => distance <= maxDistance) + .sort((a, b) => a.distance - b.distance) + .slice(0, maxResults) + .map(({ key }) => key); +} diff --git a/packages/platformos-check-node/configs/all.yml b/packages/platformos-check-node/configs/all.yml index 1011470..05cc370 100644 --- a/packages/platformos-check-node/configs/all.yml +++ b/packages/platformos-check-node/configs/all.yml @@ -49,6 +49,12 @@ MissingPartial: enabled: true severity: 0 ignoreMissing: [] +MissingRenderPartialArguments: + enabled: true + severity: 0 +NestedGraphQLQuery: + enabled: true + severity: 1 OrphanedPartial: enabled: true severity: 1 diff --git a/packages/platformos-check-node/configs/recommended.yml b/packages/platformos-check-node/configs/recommended.yml index 1011470..05cc370 100644 --- a/packages/platformos-check-node/configs/recommended.yml +++ b/packages/platformos-check-node/configs/recommended.yml @@ -49,6 +49,12 @@ MissingPartial: enabled: true severity: 0 ignoreMissing: [] +MissingRenderPartialArguments: + enabled: true + severity: 0 +NestedGraphQLQuery: + enabled: true + severity: 1 OrphanedPartial: enabled: true severity: 1 diff --git a/packages/platformos-check-node/src/backfill-docs/doc-generator.ts b/packages/platformos-check-node/src/backfill-docs/doc-generator.ts index 44c4c44..4c6c158 100644 --- a/packages/platformos-check-node/src/backfill-docs/doc-generator.ts +++ b/packages/platformos-check-node/src/backfill-docs/doc-generator.ts @@ -1,4 +1,4 @@ -import { BasicParamTypes } from '@platformos/platformos-check-common'; +import { InferredParamType } from '@platformos/platformos-check-common'; /** * Generate a single @param line for a doc tag. @@ -10,7 +10,7 @@ import { BasicParamTypes } from '@platformos/platformos-check-common'; */ export function generateParamLine( name: string, - type: BasicParamTypes, + type: InferredParamType, isOptional: boolean = true, ): string { const paramName = isOptional ? `[${name}]` : name; diff --git a/packages/platformos-check-node/src/backfill-docs/types.ts b/packages/platformos-check-node/src/backfill-docs/types.ts index 063328f..467ee85 100644 --- a/packages/platformos-check-node/src/backfill-docs/types.ts +++ b/packages/platformos-check-node/src/backfill-docs/types.ts @@ -1,10 +1,10 @@ -import { BasicParamTypes } from '@platformos/platformos-check-common'; +import { InferredParamType } from '@platformos/platformos-check-common'; export type TagType = 'function' | 'render' | 'include'; export interface ArgumentInfo { name: string; - inferredType: BasicParamTypes; + inferredType: InferredParamType; usageCount: number; } diff --git a/packages/platformos-common/src/translation-provider/TranslationProvider.ts b/packages/platformos-common/src/translation-provider/TranslationProvider.ts index ee13f85..9616574 100644 --- a/packages/platformos-common/src/translation-provider/TranslationProvider.ts +++ b/packages/platformos-common/src/translation-provider/TranslationProvider.ts @@ -52,7 +52,7 @@ export class TranslationProvider { return key ? { isModule: true, moduleName, key } : { isModule: false, key: translationKey }; } - private getSearchPaths(moduleName?: string): string[] { + static getSearchPaths(moduleName?: string): string[] { if (!moduleName) { return ['app/translations']; } @@ -76,7 +76,9 @@ export class TranslationProvider { return [undefined, undefined]; } - const searchPaths = this.getSearchPaths(parsed.isModule ? parsed.moduleName : undefined); + const searchPaths = TranslationProvider.getSearchPaths( + parsed.isModule ? parsed.moduleName : undefined, + ); for (const basePath of searchPaths) { // Strategy A: single locale file ({basePath}/{locale}.yml) diff --git a/packages/platformos-language-server-common/package.json b/packages/platformos-language-server-common/package.json index 62454ea..92002ee 100644 --- a/packages/platformos-language-server-common/package.json +++ b/packages/platformos-language-server-common/package.json @@ -31,6 +31,8 @@ "@platformos/platformos-check-common": "0.0.12", "@platformos/platformos-graph": "0.0.12", "@vscode/web-custom-data": "^0.4.6", + "graphql": "^16.12.0", + "graphql-language-service": "^5.2.2", "vscode-json-languageservice": "^5.7.1", "vscode-languageserver": "^9.0.1", "vscode-css-languageservice": "^6.3.9", diff --git a/packages/platformos-language-server-common/src/TypeSystem.ts b/packages/platformos-language-server-common/src/TypeSystem.ts index 64f2743..cc3100b 100644 --- a/packages/platformos-language-server-common/src/TypeSystem.ts +++ b/packages/platformos-language-server-common/src/TypeSystem.ts @@ -29,8 +29,6 @@ import { ReturnType, SourceCodeType, PlatformOSDocset, - isError, - parseJSON, path, BasicParamTypes, getValidParamTypes, @@ -43,10 +41,8 @@ import { inferShapeFromJSONString, inferShapeFromJsonLiteral, inferShapeFromGraphQL, - lookupPropertyPath, mergeShapes, shapeToTypeString, - shapeToDetailString, shapeToJSONPlaceholder, } from './PropertyShapeInference'; import { AbstractFileSystem, DocumentsLocator } from '@platformos/platformos-common'; diff --git a/packages/platformos-language-server-common/src/completions/CompletionsProvider.ts b/packages/platformos-language-server-common/src/completions/CompletionsProvider.ts index 7727fda..402ab2e 100644 --- a/packages/platformos-language-server-common/src/completions/CompletionsProvider.ts +++ b/packages/platformos-language-server-common/src/completions/CompletionsProvider.ts @@ -27,6 +27,7 @@ import { TranslationCompletionProvider, } from './providers'; import { GetPartialNamesForURI } from './providers/PartialCompletionProvider'; +import { GraphQLFieldCompletionProvider } from './providers/GraphQLFieldCompletionProvider'; export interface CompletionProviderDependencies { documentManager: DocumentManager; @@ -47,6 +48,7 @@ export interface CompletionProviderDependencies { export class CompletionsProvider { private providers: Provider[] = []; + private graphqlFieldCompletionProvider: GraphQLFieldCompletionProvider; readonly documentManager: DocumentManager; readonly platformosDocset: PlatformOSDocset; readonly log: (message: string) => void; @@ -66,6 +68,10 @@ export class CompletionsProvider { this.platformosDocset = platformosDocset; this.log = log; const typeSystem = new TypeSystem(platformosDocset, fs, documentsLocator, findAppRootURI); + this.graphqlFieldCompletionProvider = new GraphQLFieldCompletionProvider( + platformosDocset, + documentManager, + ); this.providers = [ new HtmlTagCompletionProvider(), @@ -88,6 +94,11 @@ export class CompletionsProvider { const uri = params.textDocument.uri; const document = this.documentManager.get(uri); + // GraphQL files get dedicated completion support + if (document?.type === SourceCodeType.GraphQL) { + return this.graphqlFieldCompletionProvider.completions(params); + } + // Supports only Liquid resources if (document?.type !== SourceCodeType.LiquidHtml) { return []; diff --git a/packages/platformos-language-server-common/src/completions/providers/GraphQLFieldCompletionProvider.ts b/packages/platformos-language-server-common/src/completions/providers/GraphQLFieldCompletionProvider.ts new file mode 100644 index 0000000..0852533 --- /dev/null +++ b/packages/platformos-language-server-common/src/completions/providers/GraphQLFieldCompletionProvider.ts @@ -0,0 +1,78 @@ +import { CompletionItem, CompletionItemKind, CompletionParams } from 'vscode-languageserver'; +import { DocumentManager } from '../../documents'; +import { PlatformOSDocset } from '@platformos/platformos-check-common'; + +/** + * graphql-language-service and graphql must be loaded dynamically to avoid + * the "Cannot use GraphQLList from another module or realm" error that occurs + * when vitest transforms the graphql ESM module into separate instances. + */ +let _glsMod: typeof import('graphql-language-service') | undefined; +function getGLS(): typeof import('graphql-language-service') { + if (!_glsMod) _glsMod = require('graphql-language-service'); + return _glsMod!; +} + +let _graphqlMod: typeof import('graphql') | undefined; +function getGraphQL(): typeof import('graphql') { + if (!_graphqlMod) _graphqlMod = require('graphql'); + return _graphqlMod!; +} + +export class GraphQLFieldCompletionProvider { + private schemaCache: any; + private schemaLoaded = false; + + constructor( + private platformosDocset: PlatformOSDocset, + private documentManager: DocumentManager, + ) {} + + private async getSchema(): Promise { + if (!this.schemaLoaded) { + const sdl = await this.platformosDocset.graphQL(); + if (sdl) { + try { + this.schemaCache = getGraphQL().buildSchema(sdl); + } catch { + // Invalid schema SDL + } + } + this.schemaLoaded = true; + } + return this.schemaCache; + } + + async completions(params: CompletionParams): Promise { + const uri = params.textDocument.uri; + const document = this.documentManager.get(uri); + if (!document) return []; + + const schema = await this.getSchema(); + if (!schema) return []; + + const content = document.textDocument.getText(); + const gls = getGLS(); + const position = new gls.Position(params.position.line, params.position.character); + + try { + const suggestions = gls.getAutocompleteSuggestions(schema, content, position); + + return suggestions.map((suggestion) => ({ + label: suggestion.label, + kind: toCompletionItemKind(suggestion.kind), + detail: suggestion.detail ?? undefined, + documentation: suggestion.documentation ?? undefined, + })); + } catch { + return []; + } + } +} + +function toCompletionItemKind(kind: number | undefined): CompletionItemKind { + if (kind !== undefined && kind >= 1 && kind <= 25) { + return kind as CompletionItemKind; + } + return CompletionItemKind.Field; +} diff --git a/packages/platformos-language-server-common/src/hover/HoverProvider.ts b/packages/platformos-language-server-common/src/hover/HoverProvider.ts index ba37b49..99b5643 100644 --- a/packages/platformos-language-server-common/src/hover/HoverProvider.ts +++ b/packages/platformos-language-server-common/src/hover/HoverProvider.ts @@ -23,10 +23,12 @@ import { import { HtmlAttributeValueHoverProvider } from './providers/HtmlAttributeValueHoverProvider'; import { findCurrentNode } from '@platformos/platformos-check-common'; import { LiquidDocTagHoverProvider } from './providers/LiquidDocTagHoverProvider'; +import { GraphQLFieldHoverProvider } from './providers/GraphQLFieldHoverProvider'; import { TranslationProvider } from '@platformos/platformos-common'; import { FindAppRootURI } from '../../src/internal-types'; export class HoverProvider { private providers: BaseHoverProvider[] = []; + private graphqlFieldHoverProvider: GraphQLFieldHoverProvider; constructor( readonly documentManager: DocumentManager, @@ -37,6 +39,10 @@ export class HoverProvider { readonly findAppRootURI: FindAppRootURI = async () => null, ) { const typeSystem = new TypeSystem(platformosDocset); + this.graphqlFieldHoverProvider = new GraphQLFieldHoverProvider( + platformosDocset, + documentManager, + ); this.providers = [ new LiquidTagHoverProvider(platformosDocset), new LiquidFilterArgumentHoverProvider(platformosDocset), @@ -57,6 +63,11 @@ export class HoverProvider { const uri = params.textDocument.uri; const document = this.documentManager.get(uri); + // GraphQL files get dedicated hover support + if (document?.type === SourceCodeType.GraphQL) { + return this.graphqlFieldHoverProvider.hover(params); + } + // Supports only Liquid resources if (document?.type !== SourceCodeType.LiquidHtml || document.ast instanceof Error) { return null; diff --git a/packages/platformos-language-server-common/src/hover/providers/GraphQLFieldHoverProvider.spec.ts b/packages/platformos-language-server-common/src/hover/providers/GraphQLFieldHoverProvider.spec.ts new file mode 100644 index 0000000..0c8fc44 --- /dev/null +++ b/packages/platformos-language-server-common/src/hover/providers/GraphQLFieldHoverProvider.spec.ts @@ -0,0 +1,124 @@ +import { describe, beforeEach, it, expect } from 'vitest'; +import { DocumentManager } from '../../documents'; +import { HoverProvider } from '../HoverProvider'; +import { TranslationProvider } from '@platformos/platformos-common'; +import { MockFileSystem } from '@platformos/platformos-check-common/src/test'; +import { HoverParams } from 'vscode-languageserver'; + +const SCHEMA = ` +type Query { + records: RecordList + user(id: ID!): User +} + +type RecordList { + results: [Record] + total_entries: Int +} + +type Record { + id: ID + name: String + email: String +} + +type User { + id: ID + name: String +} +`; + +describe('Module: GraphQLFieldHoverProvider', () => { + let provider: HoverProvider; + let documentManager: DocumentManager; + + beforeEach(() => { + documentManager = new DocumentManager(); + provider = new HoverProvider( + documentManager, + { + graphQL: async () => SCHEMA, + filters: async () => [], + objects: async () => [], + liquidDrops: async () => [], + tags: async () => [], + }, + new TranslationProvider(new MockFileSystem({})), + ); + }); + + it('should return hover info for a top-level field in a .graphql file', async () => { + const source = '{\n records {\n total_entries\n }\n}'; + const uri = 'file:///app/graphql/test.graphql'; + documentManager.open(uri, source, 0); + + const params: HoverParams = { + position: { line: 1, character: 5 }, + textDocument: { uri }, + }; + + const result = await provider.hover(params); + expect(result).not.toBeNull(); + expect((result!.contents as any).value).toContain('records'); + expect((result!.contents as any).value).toContain('RecordList'); + }); + + it('should return hover info for a nested field', async () => { + const source = 'query {\n records {\n results {\n name\n }\n }\n}'; + const uri = 'file:///app/graphql/nested.graphql'; + documentManager.open(uri, source, 0); + + const params: HoverParams = { + position: { line: 3, character: 8 }, + textDocument: { uri }, + }; + + const result = await provider.hover(params); + expect(result).not.toBeNull(); + expect((result!.contents as any).value).toContain('Record.name'); + expect((result!.contents as any).value).toContain('String'); + }); + + it('should return null when no schema is available', async () => { + const noSchemaProvider = new HoverProvider( + documentManager, + { + graphQL: async () => null, + filters: async () => [], + objects: async () => [], + liquidDrops: async () => [], + tags: async () => [], + }, + new TranslationProvider(new MockFileSystem({})), + ); + + const source = 'query {\n records {\n results {\n name\n }\n }\n}'; + const uri = 'file:///app/graphql/test.graphql'; + documentManager.open(uri, source, 0); + + const params: HoverParams = { + position: { line: 1, character: 5 }, + textDocument: { uri }, + }; + + const result = await noSchemaProvider.hover(params); + expect(result).toBeNull(); + }); + + it('should not interfere with .liquid file hover', async () => { + const source = '{{ product }}'; + const uri = 'file:///app/views/partials/test.liquid'; + documentManager.open(uri, source, 0); + + const textDocument = documentManager.get(uri)!.textDocument; + const params: HoverParams = { + position: textDocument.positionAt(source.indexOf('product')), + textDocument: { uri }, + }; + + // Should not crash — goes through normal Liquid pipeline + const result = await provider.hover(params); + // product is not in the docset, so hover returns null + expect(result).toBeNull(); + }); +}); diff --git a/packages/platformos-language-server-common/src/hover/providers/GraphQLFieldHoverProvider.ts b/packages/platformos-language-server-common/src/hover/providers/GraphQLFieldHoverProvider.ts new file mode 100644 index 0000000..b21ec8c --- /dev/null +++ b/packages/platformos-language-server-common/src/hover/providers/GraphQLFieldHoverProvider.ts @@ -0,0 +1,90 @@ +import { Hover, HoverParams } from 'vscode-languageserver'; +import { DocumentManager } from '../../documents'; +import { PlatformOSDocset } from '@platformos/platformos-check-common'; + +/** + * graphql-language-service and graphql must be loaded dynamically to avoid + * the "Cannot use GraphQLList from another module or realm" error that occurs + * when vitest transforms the graphql ESM module into separate instances. + * By requiring at runtime, we ensure a single shared module instance. + */ +let _glsMod: typeof import('graphql-language-service') | undefined; +function getGLS(): typeof import('graphql-language-service') { + if (!_glsMod) _glsMod = require('graphql-language-service'); + return _glsMod!; +} + +let _graphqlMod: typeof import('graphql') | undefined; +function getGraphQL(): typeof import('graphql') { + if (!_graphqlMod) _graphqlMod = require('graphql'); + return _graphqlMod!; +} + +export class GraphQLFieldHoverProvider { + private schemaCache: any; + private schemaLoaded = false; + + constructor( + private platformosDocset: PlatformOSDocset, + private documentManager: DocumentManager, + ) {} + + private async getSchema(): Promise { + if (!this.schemaLoaded) { + const sdl = await this.platformosDocset.graphQL(); + if (sdl) { + try { + this.schemaCache = getGraphQL().buildSchema(sdl); + } catch { + // Invalid schema SDL + } + } + this.schemaLoaded = true; + } + return this.schemaCache; + } + + async hover(params: HoverParams): Promise { + const uri = params.textDocument.uri; + const document = this.documentManager.get(uri); + if (!document) return null; + + const schema = await this.getSchema(); + if (!schema) return null; + + const content = document.textDocument.getText(); + const gls = getGLS(); + + // graphql-language-service's getHoverInformation uses the character *before* the cursor + // position to identify the token. We try offset +1 first, then the original position, + // to handle the case where the cursor is at the start of a token. + const positions = [ + new gls.Position(params.position.line, params.position.character + 1), + new gls.Position(params.position.line, params.position.character), + ]; + + try { + let hoverInfo: string | undefined; + for (const position of positions) { + const info = gls.getHoverInformation(schema, content, position); + if (info && info !== '' && info !== 'null') { + hoverInfo = typeof info === 'string' ? info : String(info); + break; + } + } + + if (!hoverInfo) { + return null; + } + + return { + contents: { + kind: 'markdown', + value: hoverInfo, + }, + }; + } catch { + return null; + } + } +} diff --git a/packages/platformos-language-server-common/src/server/AppGraphManager.ts b/packages/platformos-language-server-common/src/server/AppGraphManager.ts index 388fce6..41e43f3 100644 --- a/packages/platformos-language-server-common/src/server/AppGraphManager.ts +++ b/packages/platformos-language-server-common/src/server/AppGraphManager.ts @@ -23,7 +23,6 @@ import { FindAppRootURI } from '../internal-types'; export class AppGraphManager { graphs: Map> = new Map(); - constructor( private connection: Connection, private documentManager: DocumentManager, @@ -153,9 +152,7 @@ export class AppGraphManager { const rootUri = await this.findAppRootURI(anyUri); if (!rootUri) return; - const graph = await this.graphs.get(rootUri); - if (!graph) return; - + // Delete existing graph to force rebuild this.graphs.delete(rootUri); await this.getAppGraphForURI(rootUri); this.connection.sendNotification(AppGraphDidUpdateNotification.type, { uri: rootUri }); @@ -166,7 +163,8 @@ export class AppGraphManager { await documentManager.preload(rootUri); const dependencies = await this.graphDependencies(rootUri); - return buildAppGraph(rootUri, dependencies, entryPoints); + const graph = await buildAppGraph(rootUri, dependencies, entryPoints); + return graph; }; private getSourceCode = async (uri: string) => { diff --git a/yarn.lock b/yarn.lock index 4223bbc..7f76c4d 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3219,6 +3219,11 @@ data-urls@^7.0.0: whatwg-mimetype "^5.0.0" whatwg-url "^16.0.0" +debounce-promise@^3.1.2: + version "3.1.2" + resolved "https://registry.yarnpkg.com/debounce-promise/-/debounce-promise-3.1.2.tgz#320fb8c7d15a344455cd33cee5ab63530b6dc7c5" + integrity sha512-rZHcgBkbYavBeD9ej6sP56XfG53d51CD4dnaw989YX/nZ/ZJfgRx/9ePKmTNiUiyQvh4mtrMoS3OAWW+yoYtpg== + debug@2.6.9: version "2.6.9" resolved "https://registry.npmjs.org/debug/-/debug-2.6.9.tgz" @@ -4240,6 +4245,15 @@ graphemer@^1.4.0: resolved "https://registry.npmjs.org/graphemer/-/graphemer-1.4.0.tgz" integrity sha512-EtKwoO6kxCL9WO5xipiHTZlSzBm7WLT627TqC/uVRd0HKmq8NXyebnNYxDoBi7wt8eTWrUrKXCOVaFq9x1kgag== +graphql-language-service@^5.2.2: + version "5.5.0" + resolved "https://registry.yarnpkg.com/graphql-language-service/-/graphql-language-service-5.5.0.tgz#2e68b23bd5384f2fbf1e9391e907106e8246d025" + integrity sha512-9EvWrLLkF6Y5e29/2cmFoAO6hBPPAZlCyjznmpR11iFtRydfkss+9m6x+htA8h7YznGam+TtJwS6JuwoWWgb2Q== + dependencies: + debounce-promise "^3.1.2" + nullthrows "^1.0.0" + vscode-languageserver-types "^3.17.1" + graphql@^16.12.0: version "16.12.0" resolved "https://registry.npmjs.org/graphql/-/graphql-16.12.0.tgz" @@ -5499,6 +5513,11 @@ nth-check@^2.0.1: dependencies: boolbase "^1.0.0" +nullthrows@^1.0.0: + version "1.1.1" + resolved "https://registry.yarnpkg.com/nullthrows/-/nullthrows-1.1.1.tgz#7818258843856ae971eae4208ad7d7eb19a431b1" + integrity sha512-2vPPEi+Z7WqML2jZYddDIfy5Dqb0r2fze2zTxNNknZaFpVHU3mFB3R+DWeJWGVx0ecvttSGlJTI+WG+8Z4cDWw== + object-inspect@^1.13.3: version "1.13.4" resolved "https://registry.npmjs.org/object-inspect/-/object-inspect-1.13.4.tgz" @@ -7413,7 +7432,7 @@ vscode-languageserver-textdocument@^1.0.12: resolved "https://registry.npmjs.org/vscode-languageserver-textdocument/-/vscode-languageserver-textdocument-1.0.12.tgz" integrity sha512-cxWNPesCnQCcMPeenjKKsOCKQZ/L6Tv19DTRIGuLWe32lyzWhihGVJ/rcckZXJxfdKCFvRLS3fpBIsV/ZGX4zA== -vscode-languageserver-types@3.17.5, vscode-languageserver-types@^3.17.5: +vscode-languageserver-types@3.17.5, vscode-languageserver-types@^3.17.1, vscode-languageserver-types@^3.17.5: version "3.17.5" resolved "https://registry.npmjs.org/vscode-languageserver-types/-/vscode-languageserver-types-3.17.5.tgz" integrity sha512-Ld1VelNuX9pdF39h2Hgaeb5hEZM2Z3jUrrMgWQAu82jMtZp7p3vJT3BzToKtZI7NgQssZje5o0zryOrhQvzQAg==