Skip to content

fix(schema): preserve YAML formatting when forking a schema#1130

Open
linjinze999 wants to merge 1 commit into
Fission-AI:mainfrom
linjinze999:feat/schema_fork_value_format
Open

fix(schema): preserve YAML formatting when forking a schema#1130
linjinze999 wants to merge 1 commit into
Fission-AI:mainfrom
linjinze999:feat/schema_fork_value_format

Conversation

@linjinze999
Copy link
Copy Markdown

@linjinze999 linjinze999 commented May 27, 2026

Use yaml's Document API (parseDocument + doc.set) instead of round -tripping through parseSchema/stringifyYaml so block scalars, comments and key order in the source schema.yaml survive the fork

Fix yaml:

  • A: |
    xxx
    yyy
    will be:
  • A: xxx yyy

Summary by CodeRabbit

  • Refactor
    • Enhanced the schema forking process with improved internal handling of schema name updates during schema operations.

Review Change Stack

Use yaml's Document API (parseDocument + doc.set) instead of round
-tripping through parseSchema/stringifyYaml so block scalars,
comments and key order in the source schema.yaml survive the fork
@linjinze999 linjinze999 requested a review from TabishB as a code owner May 27, 2026 07:05
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 27, 2026

📝 Walkthrough

Walkthrough

The PR refactors how the schema fork command updates a schema's name in its YAML configuration file. It switches from parsing the YAML into a typed schema model, mutating the model, and re-stringifying it—to parsing the YAML as a document, using targeted field mutation, and converting back to a string.

Changes

YAML Document-Level Editing

Layer / File(s) Summary
YAML document-level field mutation
src/commands/schema.ts
Import parseDocument from the YAML library and replace schema-model–based name field updates in schema fork with document-level doc.set('name', ...) and doc.toString() to enable targeted field edits.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 A fork once parsed the whole schema through,
But now we set just one field—clean and true!
Document-level edits, precise and lean,
The name updates faster, the structure stays keen.
Hop, hop, refactor!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix(schema): preserve YAML formatting when forking a schema' directly and clearly describes the main change: using YAML's Document API to preserve formatting (block scalars, comments, key order) when forking schemas instead of round-tripping through parseSchema/stringifyYaml.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/commands/schema.ts (1)

629-631: ⚡ Quick win

Don’t worry about silent corruption: toString() already fails on YAML parse errors

  • yaml (eemeli/yaml) Document#toString() throws when doc.errors.length > 0, so fs.writeFileSync(...) won’t run and the existing try/catch will prevent overwriting an invalid schema.yaml.
  • An explicit doc.errors check is still a useful optional improvement for clearer error messaging and to avoid mutating via doc.set.
Proposed fix
 const doc = parseDocument(schemaContent);
+if (doc.errors.length > 0) {
+  throw new Error(
+    `Failed to parse '${destSchemaPath}': ${doc.errors.map((e) => e.message).join('; ')}`
+  );
+}
 doc.set('name', destinationName);
 fs.writeFileSync(destSchemaPath, doc.toString());
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/commands/schema.ts` around lines 629 - 631, The code currently calls
parseDocument(schemaContent) then mutates the Document with doc.set('name',
destinationName) before writing, which can silently mutate an invalid Document;
update the flow in the block that uses parseDocument, Document#toString, and
fs.writeFileSync so you first check doc.errors (after parseDocument) and if any
errors exist throw or surface a clear error message, and only then call
doc.set('name', destinationName) and fs.writeFileSync(destSchemaPath,
doc.toString()); this ensures you don’t mutate an invalid Document and that
toString() won’t be relied on to detect parse problems.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/commands/schema.ts`:
- Around line 629-631: The code currently calls parseDocument(schemaContent)
then mutates the Document with doc.set('name', destinationName) before writing,
which can silently mutate an invalid Document; update the flow in the block that
uses parseDocument, Document#toString, and fs.writeFileSync so you first check
doc.errors (after parseDocument) and if any errors exist throw or surface a
clear error message, and only then call doc.set('name', destinationName) and
fs.writeFileSync(destSchemaPath, doc.toString()); this ensures you don’t mutate
an invalid Document and that toString() won’t be relied on to detect parse
problems.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: e76ea6c6-73d8-48bc-bf75-41ddf0f74f7e

📥 Commits

Reviewing files that changed from the base of the PR and between e441287 and 2485f8d.

📒 Files selected for processing (1)
  • src/commands/schema.ts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant