refactor: consolidate doc frontmatter validation logic (#1233)

kanadgupta · web-flow · commit 6812a54e7c1b · 2025-04-25T14:25:06.000-05:00
## 🧰 Changes i'm about to open up another PR for RM-12535 that adds a new `-confirm-autofixes` flag, and as part of that i got fed up and totally DRY'd up our duplicated validation logic so it's now basically the same between `docs:upload` and `docs:migrate`. i also added a tiny enhancement to our frontmatter autofixing logic that allows the `hidden` attribute to accept flags like `'true'` and `'false'`. ## 🧬 QA & Testing `docs:migrate` is still in dire need of more test coverage so i've been testing it locally and confirmed that it's still working as expected. `docs:upload` has great test coverage so if those tests pass we should be in good shape.
diff --git a/__tests__/lib/frontmatter.test.ts b/__tests__/lib/frontmatter.test.ts
@@ -245,6 +245,25 @@ describe('#fix', () => {
     `);
   });
 
+  it('should fix privacy (public, `hidden` is a string)', () => {
+    const data = {
+      title: 'Hello, world!',
+      hidden: 'false',
+    };
+
+    const result = fix.call(command, data, schema, emptyMappings);
+
+    expect(result.hasIssues).toBe(true);
+    expect(result.updatedData).toMatchInlineSnapshot(`
+      {
+        "privacy": {
+          "view": "public",
+        },
+        "title": "Hello, world!",
+      }
+    `);
+  });
+
   it('should fix privacy (anyone_with_link)', () => {
     const data = {
       title: 'Hello, world!',
@@ -264,6 +283,25 @@ describe('#fix', () => {
     `);
   });
 
+  it('should fix privacy (anyone_with_link, `hidden` is a string)', () => {
+    const data = {
+      title: 'Hello, world!',
+      hidden: 'true',
+    };
+
+    const result = fix.call(command, data, schema, emptyMappings);
+
+    expect(result.hasIssues).toBe(true);
+    expect(result.updatedData).toMatchInlineSnapshot(`
+      {
+        "privacy": {
+          "view": "anyone_with_link",
+        },
+        "title": "Hello, world!",
+      }
+    `);
+  });
+
   it.todo('should fix metadata object');
 
   it.todo('should fix content.link object');
diff --git a/src/commands/docs/migrate.ts b/src/commands/docs/migrate.ts
@@ -6,13 +6,11 @@ import ora from 'ora';
 import { dir } from 'tmp-promise';
 
 import BaseCommand from '../../lib/baseCommand.js';
-import { fix, writeFixes } from '../../lib/frontmatter.js';
-import isCI from '../../lib/isCI.js';
+import { writeFixes } from '../../lib/frontmatter.js';
 import { oraOptions } from '../../lib/logger.js';
-import promptTerminal from '../../lib/promptWrapper.js';
-import { fetchMappings, fetchSchema } from '../../lib/readmeAPIFetch.js';
 import { findPages } from '../../lib/readPage.js';
 import { attemptUnzip } from '../../lib/unzip.js';
+import { validateFrontmatter } from '../../lib/validateFrontmatter.js';
 
 const alphaNotice = 'This command is in an experimental alpha and is likely to change. Use at your own risk!';
 
@@ -113,96 +111,31 @@ export default class DocsMigrateCommand extends BaseCommand<typeof DocsMigrateCo
       }
     });
 
-    // todo: either DRY this validation logic up or remove it entirely
-    if (!skipValidation) {
-      const validationSpinner = ora({ ...oraOptions() }).start('🔬 Validating frontmatter data...');
+    if (skipValidation) {
+      this.debug('skipping validation');
+    } else {
+      unsortedFiles = await validateFrontmatter.call(
+        this,
+        unsortedFiles,
+        'Would you like to make these changes?',
+        outputDir,
+      );
+    }
 
-      const schema = await fetchSchema.call(this);
-      const mappings = await fetchMappings.call(this);
+    if (transformedByHooks) {
+      const fileUpdateSpinner = ora({ ...oraOptions() }).start(
+        `📝 Writing the updated files to the following directory: ${chalk.underline(outputDir)}...`,
+      );
 
-      // validate the files, prompt user to fix if necessary
-      const validationResults = unsortedFiles.map(file => {
-        this.debug(`validating frontmatter for ${file.filePath}`);
-        return fix.call(this, file.data, schema, mappings);
+      const updatedFiles = unsortedFiles.map(file => {
+        // TODO: I think that this `writeFixes` call is redundant if `validateFrontmatter` above also writes to the file,
+        // but it's not even close to being a bottleneck so I'm not going to worry about it for now
+        return writeFixes.call(this, file, file.data, outputDir);
       });
 
-      const filesWithIssues = validationResults.filter(result => result.hasIssues);
-      this.debug(`found ${filesWithIssues.length} files with issues: ${JSON.stringify(filesWithIssues)}`);
-      const filesWithFixableIssues = filesWithIssues.filter(result => result.changeCount);
-      const filesWithUnfixableIssues = filesWithIssues.filter(result => result.unfixableErrors.length);
-
-      if (filesWithIssues.length) {
-        validationSpinner.warn(`${validationSpinner.text} issues found in ${filesWithIssues.length} file(s).`);
-        if (filesWithFixableIssues.length) {
-          if (isCI()) {
-            throw new Error(
-              `${filesWithIssues.length} file(s) have issues. Please run \`${this.config.bin} ${this.id} ${pathInput} --dry-run\` in a non-CI environment to fix them.`,
-            );
-          }
-
-          const { confirm } = await promptTerminal([
-            {
-              type: 'confirm',
-              name: 'confirm',
-              message: `${filesWithFixableIssues.length} file(s) have issues that can be fixed automatically. Would you like to make these changes?`,
-            },
-          ]);
-
-          if (!confirm) {
-            throw new Error('Aborting fixes due to user input.');
-          }
-
-          const fileUpdateSpinner = ora({ ...oraOptions() }).start(
-            `📝 Writing file changes to the following directory: ${chalk.underline(outputDir)}...`,
-          );
-
-          const updatedFiles = unsortedFiles.map((file, index) => {
-            return writeFixes.call(this, file, validationResults[index].updatedData, outputDir);
-          });
-
-          fileUpdateSpinner.succeed(`${fileUpdateSpinner.text} ${updatedFiles.length} file(s) updated!`);
-
-          unsortedFiles = updatedFiles;
-        }
-
-        // also inform the user if there are files with issues that can't be fixed
-        if (filesWithUnfixableIssues.length) {
-          this.warn(
-            `${filesWithUnfixableIssues.length} file(s) have issues that cannot be fixed automatically. Please get in touch with us at support@readme.io if you need a hand.`,
-          );
-        }
-      } else if (transformedByHooks) {
-        validationSpinner.succeed(`${validationSpinner.text} no issues found!`);
-
-        const fileUpdateSpinner = ora({ ...oraOptions() }).start(
-          `📝 Writing the updated files to the following directory: ${chalk.underline(outputDir)}...`,
-        );
-
-        const updatedFiles = unsortedFiles.map((file, index) => {
-          return writeFixes.call(this, file, validationResults[index].updatedData, outputDir);
-        });
-
-        fileUpdateSpinner.succeed(`${fileUpdateSpinner.text} done!`);
-
-        unsortedFiles = updatedFiles;
-      } else {
-        validationSpinner.succeed(`${validationSpinner.text} no issues found!`);
-      }
-    } else {
-      this.debug('skipping validation');
-      if (transformedByHooks) {
-        const fileUpdateSpinner = ora({ ...oraOptions() }).start(
-          `📝 Writing the updated files to the following directory: ${chalk.underline(outputDir)}...`,
-        );
-
-        const updatedFiles = unsortedFiles.map(file => {
-          return writeFixes.call(this, file, file.data, outputDir);
-        });
+      fileUpdateSpinner.succeed(`${fileUpdateSpinner.text} done!`);
 
-        fileUpdateSpinner.succeed(`${fileUpdateSpinner.text} done!`);
-
-        unsortedFiles = updatedFiles;
-      }
+      unsortedFiles = updatedFiles;
     }
 
     return { outputDir, stats };
diff --git a/src/lib/frontmatter.ts b/src/lib/frontmatter.ts
@@ -29,15 +29,15 @@ export function fix(
    */
   mappings: Mappings,
 ): {
-  changeCount: number;
+  fixableErrorCount: number;
   errors: ErrorObject[];
   hasIssues: boolean;
   unfixableErrors: ErrorObject[];
   updatedData: PageMetadata['data'];
 } {
   if (!Object.keys(data).length) {
     this.debug('no frontmatter attributes found, skipping validation');
-    return { changeCount: 0, errors: [], hasIssues: false, unfixableErrors: [], updatedData: data };
+    return { fixableErrorCount: 0, errors: [], hasIssues: false, unfixableErrors: [], updatedData: data };
   }
 
   const ajv = new Ajv({ allErrors: true, strictTypes: false, strictTuples: false });
@@ -76,12 +76,12 @@ export function fix(
     }
   }
 
-  let changeCount = 0;
+  let fixableErrorCount = 0;
   const unfixableErrors: ErrorObject[] = [];
   const updatedData = structuredClone(data);
 
   if (typeof errors === 'undefined' || !errors.length) {
-    return { changeCount, errors: [], hasIssues: false, unfixableErrors, updatedData };
+    return { errors: [], fixableErrorCount, hasIssues: false, unfixableErrors, updatedData };
   }
 
   errors.forEach(error => {
@@ -90,14 +90,14 @@ export function fix(
       updatedData.category = {
         uri: uri || `uri-that-does-not-map-to-${data.category}`,
       };
-      changeCount += 1;
+      fixableErrorCount += 1;
     } else if (error.keyword === 'additionalProperties') {
       const badKey = error.params.additionalProperty as string;
       const extractedValue = data[badKey];
       if (error.schemaPath === '#/additionalProperties') {
         // if the bad property is at the root level, delete it
         delete updatedData[badKey];
-        changeCount += 1;
+        fixableErrorCount += 1;
         if (badKey === 'excerpt') {
           // if the `content` object exists, add to it. otherwise, create it
           if (typeof updatedData.content === 'object' && updatedData.content) {
@@ -123,7 +123,8 @@ export function fix(
             uri: extractedValue,
           };
         } else if (badKey === 'hidden') {
-          updatedData.privacy = { view: extractedValue ? 'anyone_with_link' : 'public' };
+          const hidden = typeof extractedValue === 'boolean' ? extractedValue : extractedValue === 'true';
+          updatedData.privacy = { view: hidden ? 'anyone_with_link' : 'public' };
         } else if (badKey === 'order') {
           updatedData.position = extractedValue;
         }
@@ -135,7 +136,7 @@ export function fix(
     }
   });
 
-  return { errors, changeCount, hasIssues: true, unfixableErrors, updatedData };
+  return { errors, fixableErrorCount, hasIssues: true, unfixableErrors, updatedData };
 }
 
 export function writeFixes(
diff --git a/src/lib/syncPagePath.ts b/src/lib/syncPagePath.ts
@@ -7,13 +7,10 @@ import ora from 'ora';
 import toposort from 'toposort';
 
 import { APIv2Error } from './apiError.js';
-import { fix, writeFixes } from './frontmatter.js';
-import isCI from './isCI.js';
 import { oraOptions } from './logger.js';
-import promptTerminal from './promptWrapper.js';
-import { fetchMappings, fetchSchema } from './readmeAPIFetch.js';
 import { findPages, type PageMetadata } from './readPage.js';
 import { categoryUriRegexPattern, parentUriRegexPattern } from './types/index.js';
+import { validateFrontmatter } from './validateFrontmatter.js';
 
 /**
  * Commands that leverage the APIv2 representations for pages (e.g., Guides, API Reference, etc.)
@@ -220,7 +217,7 @@ function sortFiles(files: PageMetadata<PageRepresentation>[]): PageMetadata<Page
  * @returns An array of objects with the results
  */
 export default async function syncPagePath(this: DocsUploadCommand) {
-  const { path: pathInput }: { path: string } = this.args;
+  const { path: pathInput } = this.args;
   const { key, 'dry-run': dryRun, 'max-errors': maxErrors, 'skip-validation': skipValidation } = this.flags;
 
   // check whether or not the project has bidirection syncing enabled
@@ -240,6 +237,8 @@ export default async function syncPagePath(this: DocsUploadCommand) {
     );
   }
 
+  let unsortedFiles = await findPages.call(this, pathInput);
+
   if (skipValidation) {
     if (biDiConnection) {
       this.warn(
@@ -248,64 +247,12 @@ export default async function syncPagePath(this: DocsUploadCommand) {
     } else {
       this.warn('Skipping pre-upload validation of the Markdown file(s). This is not recommended.');
     }
-  }
-
-  let unsortedFiles = await findPages.call(this, pathInput);
-
-  if (!skipValidation) {
-    const validationSpinner = ora({ ...oraOptions() }).start('🔬 Validating frontmatter data...');
-
-    const schema = await fetchSchema.call(this);
-    const mappings = await fetchMappings.call(this);
-
-    // validate the files, prompt user to fix if necessary
-    const validationResults = unsortedFiles.map(file => {
-      this.debug(`validating frontmatter for ${file.filePath}`);
-      return fix.call(this, file.data, schema, mappings);
-    });
-
-    const filesWithIssues = validationResults.filter(result => result.hasIssues);
-    this.debug(`found ${filesWithIssues.length} files with issues: ${JSON.stringify(filesWithIssues)}`);
-    const filesWithFixableIssues = filesWithIssues.filter(result => result.changeCount);
-    const filesWithUnfixableIssues = filesWithIssues.filter(result => result.unfixableErrors.length);
-
-    if (filesWithIssues.length) {
-      validationSpinner.warn(`${validationSpinner.text} issues found in ${filesWithIssues.length} file(s).`);
-      if (filesWithFixableIssues.length) {
-        if (isCI()) {
-          throw new Error(
-            `${filesWithIssues.length} file(s) have issues that should be fixed before uploading to ReadMe. Please run \`${this.config.bin} ${this.id} ${pathInput} --dry-run\` in a non-CI environment to fix them.`,
-          );
-        }
-
-        const { confirm } = await promptTerminal([
-          {
-            type: 'confirm',
-            name: 'confirm',
-            message: `${filesWithFixableIssues.length} file(s) have issues that can be fixed automatically. Would you like to make these changes and continue with the upload to ReadMe?`,
-          },
-        ]);
-
-        if (!confirm) {
-          throw new Error('Aborting upload due to user input.');
-        }
-
-        const updatedFiles = unsortedFiles.map((file, index) => {
-          return writeFixes.call(this, file, validationResults[index].updatedData);
-        });
-
-        unsortedFiles = updatedFiles;
-      }
-
-      // also inform the user if there are files with issues that can't be fixed
-      if (filesWithUnfixableIssues.length) {
-        this.warn(
-          `${filesWithUnfixableIssues.length} file(s) have issues that cannot be fixed automatically. The upload will proceed but we recommend addressing these issues. Please get in touch with us at support@readme.io if you need a hand.`,
-        );
-      }
-    } else {
-      validationSpinner.succeed(`${validationSpinner.text} no issues found!`);
-    }
+  } else {
+    unsortedFiles = await validateFrontmatter.call(
+      this,
+      unsortedFiles,
+      'Would you like to make these changes and continue with the upload to ReadMe?',
+    );
   }
 
   const uploadSpinner = ora({ ...oraOptions() }).start(
diff --git a/src/lib/validateFrontmatter.ts b/src/lib/validateFrontmatter.ts
