refactor: implement suggested improvements to codebase (#287)

* docs: update code review document with resolved issues and new findings

* chore: add copyright notice and license information to `types.ts` file

* docs: update code review document to reflect resolution of  critical issues

* chore: remove unused rule for test files in ESLint configuration

* fix: ensure `appName` is a string and handle generation errors in `icons` command

* fix: validate `appName` type and improve error handling in splashscreen generation

* test: update exit code check in E2E tests for `icons` command completion to reflect generation errors

* docs: update status of resolved issues in code review document

* refactor: move `CLI_BIN` constant to `constants.ts` file for better organization

* refactor: add comment to clarify idempotency of `mkdir` for concurrent execution in Android icon generation

* refactor: add JSDoc comments for `extractAppName`, `checkAssetFile`, and `mkdirp` functions for better documentation

* docs: update status of resolved issues in code review document for clarity

* docs: update code review document with resolved issues and overall assessment

Author: panz3r

docs/005-CODE-REVIEW-IMPROVEMENTS.md

@@ -7,11 +7,4 @@ export default [
   },
   ...tseslint.configs.recommended,
   prettier,
-  {
-    // Test files use Chai's expect().to.be.true style which triggers this rule
-    files: ["test/**/*.ts"],
-    rules: {
-      "@typescript-eslint/no-unused-expressions": "off",
-    },
-  },
 ];

eslint.config.mjs

@@ -6,10 +6,9 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/.
  */
 
+import { CLI_BIN } from '../constants.js'
 import type { CommandConfig } from './types.js'
 
-const CLI_BIN = 'rn-toolbox'
-
 /**
  * Generates help text for a single command
  */

src/cli/help.ts

@@ -62,7 +62,7 @@ The template icon file should be at least 1024x1024px.`,
   public async execute(parsed: ParsedArgs): Promise<void> {
     const { args, flags } = parsed
     const file = args.file!
-    const appName = flags.appName as string | undefined
+    const appName = typeof flags.appName === 'string' ? flags.appName : undefined
 
     const sourceFilesExists = checkAssetFile(file)
     if (!sourceFilesExists) {
@@ -89,6 +89,10 @@ The template icon file should be at least 1024x1024px.`,
       for (const err of this.errors) {
         this.log(`  - ${err}`)
       }
+      this.error(
+        `Failed to generate ${this.errors.length} asset(s)`,
+        ExitCode.GENERATION_ERROR
+      )
     }
 
     this.log(green('✔'), `Generated icons for '${cyan(appName)}' app.`)
@@ -145,6 +149,7 @@ The template icon file should be at least 1024x1024px.`,
   private async generateAndroidIconsWithDensity(inputPath: string, outputDir: string, density: string, size: number) {
     const densityFolderPath = join(outputDir, `mipmap-${density}`)
 
+    // Safe for concurrent execution - mkdir with recursive:true is idempotent
     await mkdirp(densityFolderPath)
 
     this.logVerbose(yellow('≈'), cyan('Android'), `Generating icons for density '${density}'...`)

src/commands/icons.ts

@@ -61,7 +61,7 @@ The template splashscreen file should be at least 1242x2208px.`,
   public async execute(parsed: ParsedArgs): Promise<void> {
     const { args, flags } = parsed
     const file = args.file!
-    const appName = flags.appName as string | undefined
+    const appName = typeof flags.appName === 'string' ? flags.appName : undefined
 
     const sourceFilesExists = checkAssetFile(file)
     if (!sourceFilesExists) {
@@ -88,6 +88,10 @@ The template splashscreen file should be at least 1242x2208px.`,
       for (const err of this.errors) {
         this.log(`  - ${err}`)
       }
+      this.error(
+        `Failed to generate ${this.errors.length} asset(s)`,
+        ExitCode.GENERATION_ERROR
+      )
     }
 
     this.log(green('✔'), `Generated splashscreens for '${cyan(appName)}' app.`)

src/commands/splash.ts

@@ -8,6 +8,11 @@
 
 import type { IconSizeAndroid, IconSizeIOS, SplashscreenSize } from "./types.js"
 
+/**
+ * CLI binary name
+ */
+export const CLI_BIN = 'rn-toolbox'
+
 /**
  * Android Assets Sizes
  */

src/constants.ts

@@ -1,3 +1,11 @@
+/*
+ * Copyright (c) 2025 ForWarD Software (https://forwardsoftware.solutions/)
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ */
+
 export interface SplashscreenSize {
   density?: string;
   height: number;

src/types.ts

@@ -8,6 +8,16 @@
 
 import {readFile} from 'node:fs/promises'
 
+/**
+ * Extracts the app name from app.json in the current directory.
+ * 
+ * @returns The app name if found and valid, undefined otherwise
+ * @example
+ * const name = await extractAppName()
+ * if (name) {
+ *   console.log(`Found app: ${name}`)
+ * }
+ */
 export async function extractAppName(): Promise<string | undefined> {
   try {
     const content = await readFile('./app.json', {encoding: 'utf8'})

src/utils/app.utils.ts

@@ -9,10 +9,21 @@
 import { existsSync } from 'node:fs'
 import { mkdir } from 'node:fs/promises'
 
+/**
+ * Checks if an asset file exists at the specified path.
+ * 
+ * @param filePath - The path to the asset file
+ * @returns true if the file exists, false otherwise
+ */
 export function checkAssetFile(filePath: string): boolean {
   return existsSync(filePath)
 }
 
+/**
+ * Creates a directory and all necessary parent directories.
+ * 
+ * @param path - The path to create
+ */
 export async function mkdirp(path: string): Promise<void> {
   await mkdir(path, { recursive: true })
 }

src/utils/file-utils.ts

@@ -267,7 +267,7 @@ describe('CLI E2E', () => {
       })
 
       // Command completes but reports failures
-      assert.equal(exitCode, ExitCode.SUCCESS)
+      assert.equal(exitCode, ExitCode.GENERATION_ERROR)
       assert.ok(stdout.includes('Warning') || stdout.includes('Failed'))
       assert.ok(stdout.includes('asset') || stdout.includes('generate'))
     })