fix: Map short options in generate command to prevent silent exit#183
fix: Map short options in generate command to prevent silent exit#183nayrbryanGaming wants to merge 1 commit intoRocketChat:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes rc-apps generate -o <value> so that shorthand option values (a, b, c) are mapped to the command’s full category strings before the switch executes, preventing a no-op/silent completion when using valid shorthand flags.
Changes:
- Map
-o a|b|ctoApi Extension/Slash Command Extension/Settings Extensioningeneratecommand execution. - Add CLI tests intended to cover valid/invalid shorthand option values.
- Update
package-lock.jsonwith additional"peer": truemetadata for some packages.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/commands/generate.ts |
Adds shorthand-to-category mapping before the option switch. |
test/commands/generate.test.ts |
Adds regression tests for shorthand option values (currently not asserting the fix’s behavior). |
package-lock.json |
Adds "peer": true flags for several existing dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if (option) { | ||
| const index = ['a', 'b', 'c'].indexOf(option); | ||
| if (index !== -1) { | ||
| option = categories[index]; | ||
| } |
There was a problem hiding this comment.
The shorthand mapping uses two parallel arrays (['a','b','c'] and categories) and relies on them staying in sync by index. To avoid future mismatches when categories change, prefer a single explicit mapping (e.g., a record keyed by the short option) or switch directly on the short values.
| if (option) { | |
| const index = ['a', 'b', 'c'].indexOf(option); | |
| if (index !== -1) { | |
| option = categories[index]; | |
| } | |
| const shorthandMap: { [key: string]: string } = { | |
| a: categories[0], | |
| b: categories[1], | |
| c: categories[2], | |
| }; | |
| if (option && shorthandMap[option]) { | |
| option = shorthandMap[option]; |
|
|
||
| describe('generate', () => { | ||
| test | ||
| .stdout() | ||
| .command(['generate', '-o', 'a']) | ||
| .exit(2) | ||
| .it('runs generate with option "a" and correctly attempts to find app.json'); | ||
|
|
||
| test | ||
| .stdout() | ||
| .command(['generate', '-o', 'x']) | ||
| .exit(2) | ||
| .it('fails when passing an invalid option not in a,b,c'); |
There was a problem hiding this comment.
The new test cases exit during FolderDetails.readInfoFile() when app.json is missing, so they don't exercise the new shorthand mapping logic (the code that maps 'a'/'b'/'c' to category strings runs after readInfoFile). Consider adding a fixture (e.g., temp cwd with a minimal app.json + classFile) so the command reaches the switch block, and assert on an observable effect (generated file path, invoked prompt, or expected error message for invalid option).
| describe('generate', () => { | |
| test | |
| .stdout() | |
| .command(['generate', '-o', 'a']) | |
| .exit(2) | |
| .it('runs generate with option "a" and correctly attempts to find app.json'); | |
| test | |
| .stdout() | |
| .command(['generate', '-o', 'x']) | |
| .exit(2) | |
| .it('fails when passing an invalid option not in a,b,c'); | |
| import * as fs from 'fs'; | |
| import * as path from 'path'; | |
| import * as os from 'os'; | |
| describe('generate', () => { | |
| let tempDir: string; | |
| beforeEach(() => { | |
| tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'generate-test-')); | |
| // Minimal app.json so FolderDetails.readInfoFile() succeeds | |
| const appJsonPath = path.join(tempDir, 'app.json'); | |
| fs.writeFileSync(appJsonPath, JSON.stringify({ name: 'test-app' }), 'utf8'); | |
| // Dummy class file if the command expects one to exist | |
| const classFilePath = path.join(tempDir, 'classFile.cls'); | |
| fs.writeFileSync(classFilePath, '// dummy class file\n', 'utf8'); | |
| }); | |
| afterEach(() => { | |
| if (tempDir && fs.existsSync(tempDir)) { | |
| fs.rmSync(tempDir, { recursive: true, force: true }); | |
| } | |
| }); | |
| test | |
| .stdout() | |
| .cwd(() => tempDir) | |
| .command(['generate', '-o', 'a']) | |
| .it('runs generate with option "a" using a valid app.json fixture'); | |
| test | |
| .stderr() | |
| .cwd(() => tempDir) | |
| .command(['generate', '-o', 'x']) | |
| .exit(2) | |
| .it('fails when passing an invalid option not in a,b,c', ctx => { | |
| if (!ctx.stderr || ctx.stderr.length === 0) { | |
| throw new Error('Expected error output for invalid option "x".'); | |
| } | |
| }); |
|
|
||
| describe('generate', () => { | ||
| test | ||
| .stdout() | ||
| .command(['generate', '-o', 'a']) | ||
| .exit(2) | ||
| .it('runs generate with option "a" and correctly attempts to find app.json'); | ||
|
|
||
| test | ||
| .stdout() | ||
| .command(['generate', '-o', 'x']) | ||
| .exit(2) | ||
| .it('fails when passing an invalid option not in a,b,c'); |
There was a problem hiding this comment.
This test only checks for exit code and doesn't assert on stdout/stderr content, so it may pass even if the command silently exits again. Adding an assertion on the expected error output (invalid flag value) and/or a side effect for the valid shorthand case would make this regression test meaningful.
| describe('generate', () => { | |
| test | |
| .stdout() | |
| .command(['generate', '-o', 'a']) | |
| .exit(2) | |
| .it('runs generate with option "a" and correctly attempts to find app.json'); | |
| test | |
| .stdout() | |
| .command(['generate', '-o', 'x']) | |
| .exit(2) | |
| .it('fails when passing an invalid option not in a,b,c'); | |
| import { expect } from 'chai'; | |
| describe('generate', () => { | |
| test | |
| .stdout() | |
| .command(['generate', '-o', 'a']) | |
| .exit(2) | |
| .it('runs generate with option "a" and correctly attempts to find app.json', ctx => { | |
| expect(ctx.stdout).to.not.equal(''); | |
| }); | |
| test | |
| .stdout() | |
| .stderr() | |
| .command(['generate', '-o', 'x']) | |
| .exit(2) | |
| .it('fails when passing an invalid option not in a,b,c', ctx => { | |
| expect(ctx.stderr).to.not.equal(''); | |
| }); |
This PR addresses a bug in the rc-apps generate command where using the shorthand -o flags (a, b, or c) resulted in the CLI exiting silently without providing an error or generating the requested boilerplate files.
This has been resolved by mapping the valid single-character inputs from flags.options back to their full category equivalents before evaluating the switch block. This significantly improves the developer experience by ensuring boilerplate templates generate as expected when using the CLI arguments.
Changes Made