Conversation
- Add format/check CLI command (likec4 format, likec4 fmt) - Add FormatOptions and format() to LikeC4LanguageServices - Add LikeC4FormatOptions and format() to LikeC4 facade - Extract collectDocumentsToFormat with clear project/document filtering - Add --verbose debug logging (workspace, projects, files, unchanged) - Display paths relative to cwd (absolute when outside cwd)
🦋 Changeset detectedLatest commit: dbd9efb The changes in this PR will be included in the next version bump. This PR includes changesets to release 19 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a multi-document formatting feature: a public format API on the language service returning Map<URI, formatted>, a LikeC4.format facade accepting project names/URIs and editor options, tests, and a CLI Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Command
participant Facade as LikeC4 Facade
participant LangServices as Language Services
participant Formatter as Langium Formatter
participant FS as File System
CLI->>Facade: format(options)
Facade->>Facade: resolve project names -> projectIds
Facade->>LangServices: format(FormatOptions)
LangServices->>LangServices: collect & dedupe documents
loop per document
LangServices->>Formatter: format document (FormattingOptions)
Formatter-->>LangServices: formatted text
end
LangServices-->>Facade: Map<URI, formatted>
Facade-->>CLI: results
loop per result
CLI->>FS: read original file
FS-->>CLI: original content
alt content differs
CLI->>FS: write formatted content
FS-->>CLI: write result
else unchanged
CLI-->>CLI: skip write
end
end
CLI-->>CLI: report summary / exit code
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/likec4/src/cli/format/index.ts (2)
71-74: Inconsistent guard betweenprojectsanddocumentUris.Line 72 checks
projects?.length(falsy for empty arrays), but Line 73 checksdocumentUris &&(truthy for empty arrays). If--filesis passed with no values,documentUriscould be[], which gets passed through and changes the semantics incollectDocumentsToFormat— though it happens to work correctly since empty arrays are treated likeundefinedin the current logic. Aligning the guards would be cleaner:♻️ Proposed fix
const formatted = await languageServices.format({ ...(projects?.length && { projects }), - ...(documentUris && { documentUris }), + ...(documentUris?.length && { documentUris }), })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/likec4/src/cli/format/index.ts` around lines 71 - 74, The guards for optional args are inconsistent: change the documentUris spread guard to mirror projects by checking documentUris?.length so empty arrays aren’t passed through; update the call to languageServices.format to use ...(documentUris?.length && { documentUris }) so both projects and documentUris only get included when non-empty, ensuring collectDocumentsToFormat receives the same semantics.
92-128: Disk read for comparison is correct but has a subtle TOCTOU window.The handler reads the original file from disk to compare against the in-memory formatted output. In a CLI batch-formatting context this is fine, but if the file was modified between workspace load and
readFile, the comparison could be stale. This is unlikely in practice for a CLI tool but worth being aware of for future consideration (e.g., file-watcher integration).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/likec4/src/cli/format/index.ts` around lines 92 - 128, The current loop that reads disk (readFile) to compare against the in-memory formattedText has a TOCTOU window; fix it by validating the file wasn’t changed between the initial read and the write: after obtaining fsPath (fileURLToPath) and before writing, stat the file (fs.stat) to capture a timestamp/size, then after readFile and before writeFile re-stat and compare mtime/size (or an inode/size tuple); if they differ, treat the file as changed (increment failedFiles or log and skip) instead of blindly writing formattedText; update the code paths around formatted, readFile, writeFile, needsFormatting and checkOnly to perform this re-stat check and handle the fallback behavior (log skip or retry) when the file changed.packages/language-server/src/__tests__/format.spec.ts (1)
64-75: Inline snapshot is fragile to leading/trailing whitespace in test sources.The test input has leading newlines and trailing whitespace baked into the template literal. Because the formatter doesn't strip leading content outside of
specification/modelblocks, the snapshot captures that surrounding whitespace. If the formatter behavior changes around document boundaries, this will break. This is acceptable for now but worth noting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/language-server/src/__tests__/format.spec.ts` around lines 64 - 75, The inline snapshot is fragile due to leading/trailing whitespace in the test input; update the assertion to normalize the formatter output before snapshotting by referencing the generated value (result.values().next().value / formatted) and calling a trim-normalization (e.g. use formatted.trim() or a small normalizeWhitespace(formatted) helper) and then call expect(...).toMatchInlineSnapshot(...) so surrounding newlines/space don't break the snapshot if formatter document-boundary behavior changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/language-server/src/__tests__/format.spec.ts`:
- Around line 104-116: The test fails if langiumDocuments.getDocument throws for
unknown URIs; update collectDocumentsToFormat to defensively handle that by
calling langiumDocuments.getDocument inside a try/catch (or using a safe
existence check) and treating thrown errors as a missing document (skip it), so
format (languageServices.format) receives no document for unknown URIs and
returns an empty result; reference the collectDocumentsToFormat function and
langiumDocuments.getDocument call and ensure any thrown error is
swallowed/logged and the URI is skipped.
---
Nitpick comments:
In `@packages/language-server/src/__tests__/format.spec.ts`:
- Around line 64-75: The inline snapshot is fragile due to leading/trailing
whitespace in the test input; update the assertion to normalize the formatter
output before snapshotting by referencing the generated value
(result.values().next().value / formatted) and calling a trim-normalization
(e.g. use formatted.trim() or a small normalizeWhitespace(formatted) helper) and
then call expect(...).toMatchInlineSnapshot(...) so surrounding newlines/space
don't break the snapshot if formatter document-boundary behavior changes.
In `@packages/likec4/src/cli/format/index.ts`:
- Around line 71-74: The guards for optional args are inconsistent: change the
documentUris spread guard to mirror projects by checking documentUris?.length so
empty arrays aren’t passed through; update the call to languageServices.format
to use ...(documentUris?.length && { documentUris }) so both projects and
documentUris only get included when non-empty, ensuring collectDocumentsToFormat
receives the same semantics.
- Around line 92-128: The current loop that reads disk (readFile) to compare
against the in-memory formattedText has a TOCTOU window; fix it by validating
the file wasn’t changed between the initial read and the write: after obtaining
fsPath (fileURLToPath) and before writing, stat the file (fs.stat) to capture a
timestamp/size, then after readFile and before writeFile re-stat and compare
mtime/size (or an inode/size tuple); if they differ, treat the file as changed
(increment failedFiles or log and skip) instead of blindly writing
formattedText; update the code paths around formatted, readFile, writeFile,
needsFormatting and checkOnly to perform this re-stat check and handle the
fallback behavior (log skip or retry) when the file changed.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/language-server/src/LikeC4LanguageServices.ts (1)
357-367: Consider per-document error handling to prevent one bad document from aborting the entire batch.If
formatter.formatDocumentthrows for a single document (e.g., due to a parse error), the wholeformat()call fails and all successfully formatted results in theMapare lost. Wrapping the inner loop body in a try/catch with a warning log and falling back to the original text would make this more resilient, especially when invoked from the CLI on large workspaces.♻️ Suggested resilient loop
for (const doc of documents) { const docUri = doc.uri.toString() - const edits = await formatter.formatDocument(doc, { - options: fmtOptions, - textDocument: { uri: docUri }, - }) - const text = edits.length === 0 - ? doc.textDocument.getText() - : TextDocument.applyEdits(doc.textDocument, edits) - result.set(docUri, text) + try { + const edits = await formatter.formatDocument(doc, { + options: fmtOptions, + textDocument: { uri: docUri }, + }) + const text = edits.length === 0 + ? doc.textDocument.getText() + : TextDocument.applyEdits(doc.textDocument, edits) + result.set(docUri, text) + } catch (e) { + logger.warn(`format: failed to format ${docUri}, keeping original`, loggable(e)) + result.set(docUri, doc.textDocument.getText()) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/language-server/src/LikeC4LanguageServices.ts` around lines 357 - 367, The loop over documents should catch per-document formatting errors so one bad file doesn't abort the whole batch: wrap the body that calls formatter.formatDocument(...) and applies edits (the code that computes edits, text via TextDocument.applyEdits, and result.set(docUri, text)) in a try/catch; on error log a warning including docUri and the error, then set result.set(docUri, doc.textDocument.getText()) to preserve the original content and continue to the next document, ensuring previously formatted entries in result remain intact; reference the existing variables/functions formatter.formatDocument, fmtOptions, TextDocument.applyEdits, doc.textDocument, and result.set when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/language-server/src/LikeC4LanguageServices.ts`:
- Around line 357-367: The loop over documents should catch per-document
formatting errors so one bad file doesn't abort the whole batch: wrap the body
that calls formatter.formatDocument(...) and applies edits (the code that
computes edits, text via TextDocument.applyEdits, and result.set(docUri, text))
in a try/catch; on error log a warning including docUri and the error, then set
result.set(docUri, doc.textDocument.getText()) to preserve the original content
and continue to the next document, ensuring previously formatted entries in
result remain intact; reference the existing variables/functions
formatter.formatDocument, fmtOptions, TextDocument.applyEdits, doc.textDocument,
and result.set when making the change.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/likec4/src/cli/format/index.ts (2)
108-115: Inconsistent error serialisation inside the loopRead/write failures (lines 112, 131) interpolate
edirectly in a template literal —Error.prototype.toString()adds the redundant"Error: "prefix, producing e.g."Failed to read …: Error: ENOENT …". The outer catch blocks (lines 48, 78) already use the correct patterne instanceof Error ? e.message : String(e). Align the inner catches for consistent log output.♻️ Proposed fix
- logger.error(`Failed to read ${relPath}: ${e}`) + logger.error(`Failed to read ${relPath}: ${e instanceof Error ? e.message : String(e)}`)- logger.error(`Failed to write ${relPath}: ${e}`) + logger.error(`Failed to write ${relPath}: ${e instanceof Error ? e.message : String(e)}`)Also applies to: 127-133
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/likec4/src/cli/format/index.ts` around lines 108 - 115, The catch blocks inside the formatting loop use template interpolation `${e}` which yields "Error: ..." inconsistently; update the read and write error logging (the logger.error calls that reference relPath and the exception variable e in the block handling readFile and the block handling writeFile/failedFiles) to serialize the error like the outer handlers do (use e instanceof Error ? e.message : String(e)) so logs match the outer catch formatting and keep failedFiles/continue behavior unchanged.
26-32:normalize: trueis redundant alongside thecoercethat callsresolve()In yargs,
normalizeruns beforecoerce. Sinceresolve(f)already produces a normalized, absolute path, the precedingpath.normalizepass fromnormalize: trueis a no-op. Safe to remove to keep the option descriptor lean.♻️ Proposed change
files: { type: 'string', array: true, - normalize: true, coerce: (files: string[]) => files.map(f => resolve(f)), desc: 'specific file(s) to format (repeatable)', },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/likec4/src/cli/format/index.ts` around lines 26 - 32, The option descriptor for the CLI "files" option includes redundant normalize: true because yargs runs normalize before the coerce which calls resolve(f); remove the normalize property from the "files" option so the coerce (files: string[] => files.map(f => resolve(f))) is the sole normalization step, leaving the descriptor leaner and functionally unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/likec4/src/cli/format/index.ts`:
- Around line 138-154: When failedFiles > 0 the current handler returns before
emitting the --check summary; update the failing branch to still emit the
check-mode summary when checkOnly is true and needsFormatting/changedFiles are
populated. Specifically, in the block that inspects failedFiles, log the
existing failure message as now, but if checkOnly is true then also log the
formatted summary using needsFormatting, changedFiles and formatted.size (and
set process.exitCode = 1 as needed) before returning so users see which files
need formatting in partial runs; keep using logger, failedFiles, checkOnly,
needsFormatting, changedFiles and formatted to locate and implement the change.
---
Nitpick comments:
In `@packages/likec4/src/cli/format/index.ts`:
- Around line 108-115: The catch blocks inside the formatting loop use template
interpolation `${e}` which yields "Error: ..." inconsistently; update the read
and write error logging (the logger.error calls that reference relPath and the
exception variable e in the block handling readFile and the block handling
writeFile/failedFiles) to serialize the error like the outer handlers do (use e
instanceof Error ? e.message : String(e)) so logs match the outer catch
formatting and keep failedFiles/continue behavior unchanged.
- Around line 26-32: The option descriptor for the CLI "files" option includes
redundant normalize: true because yargs runs normalize before the coerce which
calls resolve(f); remove the normalize property from the "files" option so the
coerce (files: string[] => files.map(f => resolve(f))) is the sole normalization
step, leaving the descriptor leaner and functionally unchanged.
| if (failedFiles > 0) { | ||
| logger.error(`${failedFiles} file(s) failed to process`) | ||
| process.exitCode = 1 | ||
| return | ||
| } | ||
|
|
||
| if (checkOnly) { | ||
| if (changedFiles > 0) { | ||
| logger.error( | ||
| `${changedFiles} of ${formatted.size} file(s) need formatting:\n${ | ||
| needsFormatting.map(f => ` ${f}`).join('\n') | ||
| }`, | ||
| ) | ||
| process.exitCode = 1 | ||
| return | ||
| } | ||
| logger.info(k.green(`All ${formatted.size} file(s) are formatted`)) |
There was a problem hiding this comment.
--check result silently suppressed when reads fail
When failedFiles > 0, the handler exits at line 141 before reaching the check-mode summary (lines 144–153). By that point needsFormatting is already populated, so the information about which files need formatting exists — it's just never logged. Users see only "X file(s) failed to process" and get no hint about the formatting state of the other files.
Consider reporting the partial check result alongside the failure:
🛡️ Proposed improvement
if (failedFiles > 0) {
logger.error(`${failedFiles} file(s) failed to process`)
+ if (checkOnly && needsFormatting.length > 0) {
+ logger.error(
+ `${needsFormatting.length} file(s) also need formatting:\n${needsFormatting.map(f => ` ${f}`).join('\n')}`,
+ )
+ }
process.exitCode = 1
return
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (failedFiles > 0) { | |
| logger.error(`${failedFiles} file(s) failed to process`) | |
| process.exitCode = 1 | |
| return | |
| } | |
| if (checkOnly) { | |
| if (changedFiles > 0) { | |
| logger.error( | |
| `${changedFiles} of ${formatted.size} file(s) need formatting:\n${ | |
| needsFormatting.map(f => ` ${f}`).join('\n') | |
| }`, | |
| ) | |
| process.exitCode = 1 | |
| return | |
| } | |
| logger.info(k.green(`All ${formatted.size} file(s) are formatted`)) | |
| if (failedFiles > 0) { | |
| logger.error(`${failedFiles} file(s) failed to process`) | |
| if (checkOnly && needsFormatting.length > 0) { | |
| logger.error( | |
| `${needsFormatting.length} file(s) also need formatting:\n${needsFormatting.map(f => ` ${f}`).join('\n')}`, | |
| ) | |
| } | |
| process.exitCode = 1 | |
| return | |
| } | |
| if (checkOnly) { | |
| if (changedFiles > 0) { | |
| logger.error( | |
| `${changedFiles} of ${formatted.size} file(s) need formatting:\n${ | |
| needsFormatting.map(f => ` ${f}`).join('\n') | |
| }`, | |
| ) | |
| process.exitCode = 1 | |
| return | |
| } | |
| logger.info(k.green(`All ${formatted.size} file(s) are formatted`)) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/likec4/src/cli/format/index.ts` around lines 138 - 154, When
failedFiles > 0 the current handler returns before emitting the --check summary;
update the failing branch to still emit the check-mode summary when checkOnly is
true and needsFormatting/changedFiles are populated. Specifically, in the block
that inspects failedFiles, log the existing failure message as now, but if
checkOnly is true then also log the formatted summary using needsFormatting,
changedFiles and formatted.size (and set process.exitCode = 1 as needed) before
returning so users see which files need formatting in partial runs; keep using
logger, failedFiles, checkOnly, needsFormatting, changedFiles and formatted to
locate and implement the change.
|
@davydkov Could you please review this? |
|
Could you also mention new command in docs, just usage examples from PR description would be enough |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/docs/src/content/docs/tooling/cli.mdx (1)
273-278: Consider documenting the-palias andLIKEC4_WORKSPACEenv variable.The options table has minor gaps compared to the actual CLI implementation:
- The
--projectoption has an alias-pthat isn't documented- The
pathdescription omits theLIKEC4_WORKSPACEenvironment variable fallback📝 Suggested improvements
| Option | Description | | ------------------- | --------------------------------------------------------------- | -| `path` | Path to workspace (default: current directory) | -| `--project` | Format only specific project(s) by name (repeatable) | +| `path` | Path to workspace (default: current directory or `LIKEC4_WORKSPACE` env) | +| `--project, -p` | Format only specific project(s) by name (repeatable) | | `--files` | Format only specific files (repeatable) | | `--check` | Check if files are formatted without writing changes |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/docs/src/content/docs/tooling/cli.mdx` around lines 273 - 278, Update the CLI options table so it matches the actual implementation: add the `-p` alias to the `--project` row (e.g., show `--project / -p`) and update the `path` description to mention the `LIKEC4_WORKSPACE` environment variable fallback (e.g., "Path to workspace (default: current directory; falls back to LIKEC4_WORKSPACE)"). Ensure the displayed option names match the CLI flags `--project`/`-p` and the `path` entry references `LIKEC4_WORKSPACE`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/docs/src/content/docs/tooling/cli.mdx`:
- Around line 273-278: Update the CLI options table so it matches the actual
implementation: add the `-p` alias to the `--project` row (e.g., show `--project
/ -p`) and update the `path` description to mention the `LIKEC4_WORKSPACE`
environment variable fallback (e.g., "Path to workspace (default: current
directory; falls back to LIKEC4_WORKSPACE)"). Ensure the displayed option names
match the CLI flags `--project`/`-p` and the `path` entry references
`LIKEC4_WORKSPACE`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5b1afff0-4f70-4740-849c-1c1a8ae79219
📒 Files selected for processing (1)
apps/docs/src/content/docs/tooling/cli.mdx
…4_WORKSPACE fallback)
|
@davydkov All requested changes are done — merge conflict resolved, changeset added, and CLI docs updated. Ready for re-review when you have a chance. Thanks! |
2 participants
Summary
Add a
likec4 format(aliaslikec4 fmt) CLI command that formats.c4source files in place, with a--checkmode for CI pipelines.The feature is implemented across three layers:
@likec4/language-server—format()method onLikeC4LanguageServicesinterface, batch-formats documents via the existingLikeC4Formatterand returnsMap<string, string>(URI → formatted text)@likec4/language-services—format()method on theLikeC4facade, translates project name strings toProjectIdand delegateslikec4—formatCLI command wired into yargsUsage
Changes
packages/language-serverformat(options?: FormatOptions)toLikeC4LanguageServicesinterface andDefaultLikeC4LanguageServicesimplementationFormatOptionsinterface withprojectIds,documentUris, and LSP formatting options (tabSize,insertSpaces, etc.)FormatOptionstype fromcommon-exports.tsformat.spec.ts— unit tests covering multi-document formatting,documentUris/projectIdsfiltering, union semantics, formatting options (tabSize,insertSpaces), idempotency, and unknown URI handlingpackages/language-servicesformat(options?: LikeC4FormatOptions)toLikeC4facadeLikeC4FormatOptionsinterface (uses project name strings instead ofProjectId)packages/likec4formatCLI command (packages/likec4/src/cli/format/index.ts)packages/likec4/src/cli/index.tsDesign Notes
LikeC4Formatter(Langium'sAbstractFormatter) stores mutable state per call — concurrent invocations would corrupt each other's state.projectIds∪documentUris; omit both to format all documents.Testing
format.spec.ts) covering:projectIds+documentUristabSize,insertSpaces)command:
Checklist
mainbefore creating this PR.Summary by CodeRabbit