Feature: Allow selecting target branch for PR#8
Conversation
- Changed template path resolution to only use the application installation directory - Added fallback to default template when template file doesn't exist - Added debug logging for template resolution - Improved error messaging
- Add getDefaultBranch function to get the repository default branch - Add getRemoteBranches function to get list of available branches - Update createPR function to accept a target branch parameter - Modify main function to prompt user for target branch - Add basic tests for the new functionality
WalkthroughAdds interactive target-branch selection for PR creation, improves template loading with an inline fallback, adds a lint-fix script, broadens Jest test matching, and ignores Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Script
participant GitHub
participant FileSystem
User->>Script: run PR creation command
Script->>GitHub: query remote HEAD (determine default branch)
Script->>GitHub: list remote branches
Script->>User: prompt to select target branch (default pre-selected)
User-->>Script: selects target branch
Script->>FileSystem: check for PULL_REQUEST_TEMPLATE file in script dir
alt template exists
FileSystem-->>Script: return file path
Script->>FileSystem: render template from file
else no template
Script-->>Script: use inline default template content
end
Script->>GitHub: create PR with selected target branch (--base) and rendered template
GitHub-->>Script: PR created (response)
Script->>User: show result / URL
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ 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: 4
🔭 Outside diff range comments (1)
index.js (1)
167-188:⚠️ Potential issuePossible shell-injection in
createPR()
title,body, andtargetBranchare interpolated into a shell string executed by/bin/sh.
A malicious title like"; rm -rf / #would be executed.Mitigate by:
- Passing the command as an array to
spawnSync/execFileSync, or- At minimum, using
--and single-quoting:-let command = `gh pr create --title "${title}" --body "${body.replace(/"/g, '\\"')}"`; +const safeBody = body.replace(/(["`\\$])/g, '\\$1'); +const safeTitle = title.replace(/(["`\\$])/g, '\\$1'); +let command = `gh pr create --title "${safeTitle}" --body "${safeBody}"`;But switching to
spawnSync('gh', ['pr','create','--title',title,'--body',body,...])is far safer.🧰 Tools
🪛 GitHub Check: test (22.x)
[warning] 176-176:
Trailing spaces not allowed
[warning] 171-171:
Trailing spaces not allowed🪛 GitHub Actions: Test Suite
[warning] 171-171: ESLint: Trailing spaces not allowed (no-trailing-spaces)
[warning] 176-176: ESLint: Trailing spaces not allowed (no-trailing-spaces)
🧹 Nitpick comments (6)
jest.config.mjs (1)
3-3: BroadentestMatchso nested sub-folders are covered.
'**/test/*.test.js'only matches files that are direct children oftest/.
If a contributor addstest/utils/foo.test.js, it will be silently skipped.
A safer glob is:- testMatch: ['**/test/*.test.js'], + testMatch: ['**/test/**/*.test.js'],This keeps current behaviour while allowing deeper structures.
index.js (4)
74-87:getRemoteBranches()– platform-dependentgrep& potential empty output.Replace the
grep -v HEADpipeline with nativegit for-each-refto stay cross-platform:- const output = execSync('git branch -r | grep -v HEAD').toString().trim(); + const output = execSync( + 'git for-each-ref --format="%(refname:strip=3)" refs/remotes' + ).toString().trim();Same behaviour, no external binaries.
🧰 Tools
🪛 GitHub Check: test (22.x)
[warning] 79-79:
Trailing spaces not allowed🪛 GitHub Actions: Test Suite
[warning] 79-79: ESLint: Trailing spaces not allowed (no-trailing-spaces)
149-164: Return type OK but docstring / JSDoc missing.
getTemplatePath()now returns an object but no documentation reflects this.
Add a JSDoc block so editors & tests know the shape:+/** + * @returns {{ isDefault: true, content: string } | { isDefault: false, path: string }} + */ export function getTemplatePath() {🧰 Tools
🪛 GitHub Check: test (22.x)
[warning] 154-154:
Trailing spaces not allowed🪛 GitHub Actions: Test Suite
[warning] 154-154: ESLint: Trailing spaces not allowed (no-trailing-spaces)
255-275:twig.twig({ data })renders sync – no need for async wrapper.Since the default template path already renders synchronously, consider using the same path for file templates and dropping
promisifyentirely.
This simplifies code and avoids mixing sync/async error handling.🧰 Tools
🪛 GitHub Check: test (22.x)
[warning] 257-257:
Trailing spaces not allowed🪛 GitHub Actions: Test Suite
[warning] 257-257: ESLint: Trailing spaces not allowed (no-trailing-spaces)
330-334: Handle failure when PR already exists.
gh pr createexits non-zero if a PR from the branch already exists.
Catcherror.stderrand hint the user togh pr view --web.test/target-branch.test.js (1)
35-45: Consider using mock testing for createPRThe current test only verifies that calling createPR with a targetBranch parameter doesn't throw an error, which is a minimal validation. Consider enhancing this test by using mocks to validate that the function properly handles the targetBranch parameter without creating actual PRs.
describe('createPR', () => { test('accepts a target branch parameter', () => { // Verify the function accepts a target branch parameter expect(typeof createPR).toBe('function'); // This is just a type check - we don't actually call the function // since that would create a real PR const pr = {title: 'Test PR', body: 'Test body', targetBranch: 'main'}; expect(() => createPR(pr.title, pr.body, pr.targetBranch)).not.toThrow(); + + // For more comprehensive testing, consider adding a mock test: + // jest.mock('../index', () => ({ + // ...jest.requireActual('../index'), + // // Mock implementation that doesn't create actual PRs + // _createGitHubPR: jest.fn().mockResolvedValue({ url: 'https://github.com/mock/pr/url' }) + // })); + // + // await createPR('Test PR', 'Test body', 'feature-branch'); + // expect(_createGitHubPR).toHaveBeenCalledWith( + // expect.objectContaining({ + // base: 'feature-branch' + // }) + // ); }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.gitignore(1 hunks)index.js(4 hunks)jest.config.mjs(1 hunks)test/basic.test.js(1 hunks)test/target-branch.test.js(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/basic.test.js (1)
index.js (2)
template(256-256)getTemplatePath(150-164)
🪛 GitHub Check: test (22.x)
test/target-branch.test.js
[failure] 5-5:
forbid file extension '.js'
index.js
[warning] 79-79:
Trailing spaces not allowed
[warning] 65-65:
Trailing spaces not allowed
[warning] 61-61:
Trailing spaces not allowed
[warning] 57-57:
Trailing spaces not allowed
[warning] 176-176:
Trailing spaces not allowed
[warning] 171-171:
Trailing spaces not allowed
[warning] 154-154:
Trailing spaces not allowed
[warning] 257-257:
Trailing spaces not allowed
[warning] 315-315:
'remoteBranches' is assigned a value but never used. Allowed unused vars must match /^_/u
[warning] 313-313:
Trailing spaces not allowed
🪛 ESLint
test/target-branch.test.js
[error] 5-5: forbid file extension '.js'.
(n/file-extension-in-import)
🪛 GitHub Actions: Test Suite
test/target-branch.test.js
[error] 5-5: ESLint: forbid file extension '.js' (n/file-extension-in-import)
test/basic.test.js
[warning] 29-29: ESLint: Trailing spaces not allowed (no-trailing-spaces)
index.js
[warning] 57-57: ESLint: Trailing spaces not allowed (no-trailing-spaces)
[warning] 61-61: ESLint: Trailing spaces not allowed (no-trailing-spaces)
[warning] 65-65: ESLint: Trailing spaces not allowed (no-trailing-spaces)
[warning] 79-79: ESLint: Trailing spaces not allowed (no-trailing-spaces)
[warning] 154-154: ESLint: Trailing spaces not allowed (no-trailing-spaces)
[warning] 171-171: ESLint: Trailing spaces not allowed (no-trailing-spaces)
[warning] 176-176: ESLint: Trailing spaces not allowed (no-trailing-spaces)
[warning] 257-257: ESLint: Trailing spaces not allowed (no-trailing-spaces)
[warning] 313-313: ESLint: Trailing spaces not allowed (no-trailing-spaces)
[warning] 315-315: ESLint: 'remoteBranches' is assigned a value but never used. Allowed unused vars must match /^_/u (no-unused-vars)
[warning] 316-316: ESLint: Trailing spaces not allowed (no-trailing-spaces)
[warning] 325-325: ESLint: Trailing spaces not allowed (no-trailing-spaces)
[warning] 352-352: ESLint: Newline required at end of file but not found (eol-last)
🪛 Biome (1.9.4)
index.js
[error] 62-62: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (5)
.gitignore (1)
1-1: No issues – entry is clear and specific.The added ignore rule is precise and does not collide with common patterns.
test/target-branch.test.js (3)
7-11: Test file organization looks goodThe test file is well-structured with clear descriptions and is focused on testing the new target branch selection functionality.
12-22: Tests for getDefaultBranch are minimal but appropriateThe tests verify the function exists and returns a string without testing the actual implementation details, which is a reasonable approach given the function interacts with git.
24-33: Tests for getRemoteBranches are minimal but appropriateSimilar to the getDefaultBranch tests, these verify the function exists and returns an array, which is suitable for functions that interact with external systems like git.
test/basic.test.js (1)
26-37: Great update to handle the new template path return typeThe test has been properly updated to handle the new object-based return type from
getTemplatePath(). The conditional check is well structured to validate both potential return scenarios (default inline template or file path).There's a small whitespace issue on line 29 that should be fixed:
const template = getTemplatePath(); - + // Check for default template case🧰 Tools
🪛 GitHub Actions: Test Suite
[warning] 29-29: ESLint: Trailing spaces not allowed (no-trailing-spaces)
| // Get default branch name | ||
| export function getDefaultBranch() { | ||
| try { | ||
| // Try to get the remote's default branch | ||
| // First get the default remote (usually origin) | ||
| const remote = execSync('git remote').toString().trim().split('\n')[0]; | ||
|
|
||
| // Then get the default branch (what HEAD points to) | ||
| const output = execSync(`git remote show ${remote} | grep "HEAD branch"`).toString().trim(); | ||
| const match = output.match(/HEAD branch:\s*(.+)$/); | ||
|
|
||
| if (match && match[1]) { | ||
| return match[1]; | ||
| } | ||
|
|
||
| // Fallback to 'main' or 'master' if we can't determine it | ||
| return 'main'; | ||
| } catch { | ||
| // Fallback to a sensible default | ||
| return 'main'; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
getDefaultBranch() ‑ brittle parsing & missing “master” fallback.
- Using
grepinsideexecSyncbreaks on Windows and whengrepis absent. - The regex parsing returns
'main'for every failure although comment mentions'master'fallback. git remotemay return an empty string when no remotes exist (fresh repo / mirror).
Consider:
-export function getDefaultBranch() {
- try {
- const remote = execSync('git remote').toString().trim().split('\n')[0];
- const output = execSync(`git remote show ${remote} | grep "HEAD branch"`).toString().trim();
- const match = output.match(/HEAD branch:\s*(.+)$/);
- if (match && match[1]) return match[1];
- return 'main';
- } catch {
- return 'main';
- }
-}
+export function getDefaultBranch() {
+ try {
+ const remote = execSync('git remote').toString().trim().split('\n').filter(Boolean)[0];
+ if (!remote) return 'main'; // no remotes
+
+ const output = execSync(`git remote show ${remote}`).toString();
+ const match = output.match(/HEAD branch:\s*(.+)$/m);
+ if (match?.[1]) return match[1];
+
+ // Fallback hierarchy
+ return execSync('git show-ref --verify --quiet refs/heads/master && echo master || echo main')
+ .toString().trim();
+ } catch {
+ return 'main';
+ }
+}This removes grep, adds remote-less protection and honours a master branch when present.
📝 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.
| // Get default branch name | |
| export function getDefaultBranch() { | |
| try { | |
| // Try to get the remote's default branch | |
| // First get the default remote (usually origin) | |
| const remote = execSync('git remote').toString().trim().split('\n')[0]; | |
| // Then get the default branch (what HEAD points to) | |
| const output = execSync(`git remote show ${remote} | grep "HEAD branch"`).toString().trim(); | |
| const match = output.match(/HEAD branch:\s*(.+)$/); | |
| if (match && match[1]) { | |
| return match[1]; | |
| } | |
| // Fallback to 'main' or 'master' if we can't determine it | |
| return 'main'; | |
| } catch { | |
| // Fallback to a sensible default | |
| return 'main'; | |
| } | |
| } | |
| // Get default branch name | |
| export function getDefaultBranch() { | |
| try { | |
| // First get the default remote (usually 'origin') | |
| const remote = execSync('git remote') | |
| .toString() | |
| .trim() | |
| .split('\n') | |
| .filter(Boolean)[0]; | |
| if (!remote) return 'main'; // no remotes | |
| // Show all remote info and extract the HEAD branch | |
| const output = execSync(`git remote show ${remote}`).toString(); | |
| const match = output.match(/HEAD branch:\s*(.+)$/m); | |
| if (match?.[1]) return match[1]; | |
| // Fallback hierarchy: prefer 'master' if it exists, otherwise 'main' | |
| return execSync( | |
| 'git show-ref --verify --quiet refs/heads/master && echo master || echo main' | |
| ) | |
| .toString() | |
| .trim(); | |
| } catch { | |
| // If anything goes wrong, default to 'main' | |
| return 'main'; | |
| } | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 62-62: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 GitHub Check: test (22.x)
[warning] 65-65:
Trailing spaces not allowed
[warning] 61-61:
Trailing spaces not allowed
[warning] 57-57:
Trailing spaces not allowed
🪛 GitHub Actions: Test Suite
[warning] 57-57: ESLint: Trailing spaces not allowed (no-trailing-spaces)
[warning] 61-61: ESLint: Trailing spaces not allowed (no-trailing-spaces)
[warning] 65-65: ESLint: Trailing spaces not allowed (no-trailing-spaces)
| import { | ||
| getDefaultBranch, | ||
| getRemoteBranches, | ||
| createPR | ||
| } from '../index.js'; |
There was a problem hiding this comment.
Update import statement to remove file extension
The import statement uses the .js file extension which is causing ESLint errors and pipeline failures. This goes against Node.js best practices for ECMAScript modules.
import {
getDefaultBranch,
getRemoteBranches,
createPR
-} from '../index.js';
+} from '../index';📝 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.
| import { | |
| getDefaultBranch, | |
| getRemoteBranches, | |
| createPR | |
| } from '../index.js'; | |
| import { | |
| getDefaultBranch, | |
| getRemoteBranches, | |
| createPR | |
| } from '../index'; |
🧰 Tools
🪛 GitHub Check: test (22.x)
[failure] 5-5:
forbid file extension '.js'
🪛 ESLint
[error] 5-5: forbid file extension '.js'.
(n/file-extension-in-import)
🪛 GitHub Actions: Test Suite
[error] 5-5: ESLint: forbid file extension '.js' (n/file-extension-in-import)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
index.js (3)
51-72: Fix brittle parsing and missing fallbacks (duplicate issue).This function still has the same issues identified in the previous review:
- Using
grepbreaks on Windows and whengrepis absent- No protection against empty remote list
- Missing proper fallback to
masterbranch- Trailing spaces present
141-147: Use fileURLToPath for robust path conversion (duplicate issue).This function still has the same issue identified in the previous review - using
new URL().pathnamecan leave URL-encoded characters and fails on Windows.
311-328: Fix unused variable and missing validation (duplicate issue).This code still has the same issues identified in the previous review:
remoteBranchesis fetched but never used for input validation- Trailing spaces present
- User input is not validated against available branches
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
index.js(6 hunks)package.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- package.json
🔇 Additional comments (5)
index.js (5)
4-4: LGTM! Import addition supports new search functionality.The addition of
searchto the imports is correctly used for the target branch selection feature.
149-164: LGTM! Well-designed template fallback mechanism.The refactor to return an object with
isDefault,path, andcontentproperties provides a clean separation between default and file-based templates with good error handling.
167-188: LGTM! Clean implementation of target branch support.The optional
targetBranchparameter is well-implemented with conditional command building that maintains backward compatibility.
255-275: LGTM! Proper handling of both template types.The conditional rendering logic correctly handles both default string templates and file-based templates while maintaining consistent template data.
330-330: LGTM! Correct integration of target branch selection.The target branch is properly passed to the
createPRfunction, completing the feature integration.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
index.js (1)
147-153: UsefileURLToPathfor robust path conversion.As mentioned in a previous review,
new URL(fileUrl).pathnamecan leave URL-encoded characters and may not work correctly on all platforms. Since you already importfileURLToPath, use it consistently:function getScriptDir() { - // Use import.meta.url to get the full URL of the current module - const fileUrl = import.meta.url; - // Convert the file URL to a system path and get the directory - return path.dirname(new URL(fileUrl).pathname); + return path.dirname(fileURLToPath(import.meta.url)); }
🧹 Nitpick comments (1)
index.js (1)
57-78: Fix trailing spaces and consider robust error handling.The function logic is sound, but there are trailing spaces on lines 57, 61, and 65 that should be removed to pass linting.
Additionally, consider adding validation for when no remotes exist:
export function getDefaultBranch() { try { // Try to get the remote's default branch // First get the default remote (usually origin) - const remote = execSync('git remote').toString().trim().split('\n')[0]; + const remotes = execSync('git remote').toString().trim().split('\n').filter(Boolean); + if (remotes.length === 0) return 'main'; + const remote = remotes[0]; // Then get the default branch (what HEAD points to) - const output = execSync(`git remote show ${remote} | grep "HEAD branch"`).toString().trim(); + const output = execSync(`git remote show ${remote} | grep "HEAD branch"`).toString().trim(); const match = output.match(/HEAD branch:\s*(.+)$/); - if (match && match[1]) { + if (match?.[1]) { return match[1]; } // Fallback to 'main' or 'master' if we can't determine it return 'main'; } catch { // Fallback to a sensible default return 'main'; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
index.js(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
index.js (1)
Learnt from: richardgaunt
PR: #8
File: index.js:80-93
Timestamp: 2025-07-24T00:36:05.712Z
Learning: Windows compatibility is not important for the github-pr-maker project, so cross-platform compatibility suggestions (like avoiding grep usage) are not necessary.
🔇 Additional comments (5)
index.js (5)
4-4: LGTM: Import addition supports new search functionality.The
searchimport is correctly added to support the new target branch selection feature.
80-93: LGTM: Function correctly retrieves remote branches.The function properly filters and cleans remote branch names. The grep usage is acceptable given that Windows compatibility is not a concern for this project.
173-194: LGTM: Correctly implements target branch support.The function properly handles the optional
targetBranchparameter and correctly builds the GitHub CLI command with the--baseoption when needed. The string escaping for the body is also handled appropriately.
261-281: LGTM: Template rendering correctly handles both cases.The conditional rendering logic properly handles both the default inline template and file-based template scenarios. The template data is consistently passed in both cases.
333-336: LGTM: PR creation with target branch is correctly implemented.The PR creation call properly passes the selected target branch to the
createPRfunction, completing the target branch selection feature implementation.
| const defaultBranch = getDefaultBranch(); | ||
| // Default branch gets added to the start of the array. | ||
| const remoteBranches = getRemoteBranches() | ||
| .sort((branchA, branchB) => { | ||
| return (branchA === defaultBranch) ? -1 : (branchB === defaultBranch) ? 1 : branchA.localeCompare(branchB); | ||
| }) | ||
| .map(branch => ({ title: branch, value: branch })); | ||
| // Ask user which branch to target for PR | ||
| // Default to the repository's default branch | ||
| console.log('\n🌿 Select target branch for PR:'); | ||
| const targetBranch = await search({ | ||
| message: '🎯 Target branch for PR:', | ||
| default: defaultBranch, | ||
| source: (input = '') => { return remoteBranches.filter(branch => branch.title.includes(input)); }, | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify search prompt source function return type.
The search prompt implementation looks good, but ensure the source function returns the correct type. The @inquirer/prompts search expects the source function to return a Promise or array directly:
const targetBranch = await search({
message: '🎯 Target branch for PR:',
default: defaultBranch,
- source: (input = '') => { return remoteBranches.filter(branch => branch.title.includes(input)); },
+ source: (input = '') => Promise.resolve(remoteBranches.filter(branch => branch.title.includes(input))),
});Or verify that the current synchronous return is supported by checking the latest documentation.
🌐 Web query:
Does @inquirer/prompts search source function support synchronous array returns or does it require Promise?
💡 Result:
The source function for the search prompt in @inquirer/prompts must return a Promise; synchronous array returns are not supported[1]. The official documentation specifies the signature as:
function(term: string | void, opt: { signal: AbortSignal }): Promise<ReadonlyArray<Choice<Value> | Separator>>;[1]
This means:
- The function you pass to
sourceshould always return a Promise that resolves to the array of choices[1]. - If you return a plain array synchronously (rather than a Promise that resolves to an array), it will not work as expected[1].
This requirement exists to support asynchronous data sources (like network requests) and to ensure prompt behavior is consistent and non-blocking for all use cases[1][7]. A typical usage, even for synchronous data, should still return a promise (e.g., Promise.resolve([...])).
In summary:
sourcemust return a Promise (even if your data is static/synchronous, wrap it in a Promise)[1][7].- Direct synchronous returns (plain arrays) are not supported.
Example for synchronous values (correct approach):
source: async (term, { signal }) => {
// Sync logic
return [{ name: 'a', value: 'a' }, { name: 'b', value: 'b' }];
}
// OR
source: (term, { signal }) => Promise.resolve([
{ name: 'a', value: 'a' }, { name: 'b', value: 'b' }
]);[1]
Returning a plain array (not wrapped in a Promise) will not match the expected contract and can cause runtime errors[1].
Citations:
- 1: https://www.npmjs.com/package/@inquirer/search
- 2: https://geshan.com.np/blog/2023/03/inquirer-js/
- 3: https://gitlab.ifi.uzh.ch/franco/ddosrecommendation/-/tree/cbf5c4f4f94f0677b8e77dcd9e1d359776ce0990/client/node_modules/eslint/node_modules/inquirer
- 4: https://javascript.plainenglish.io/how-to-inquirer-js-c10a4e05ef1f
- 5: https://npm-compare.com/enquirer,inquirer,prompt,prompt-sync,prompts,readline-sync
- 6: https://socket.dev/npm/package/inquirer-autocomplete-prompt
- 7: https://classic.yarnpkg.com/en/package/inquirer-list-search-prompt
- 8: Can we use inquirer.js synchronously? SBoudrias/Inquirer.js#157
- 9: https://npm-compare.com/inquirer,enquirer,prompt-sync
- 10: https://github.com/SBoudrias/Inquirer.js/blob/main/packages/core/README.md
Wrap source return in a Promise for @inquirer/prompts
The search prompt’s source function must return a Promise (synchronous arrays aren’t supported). Update your handler in index.js accordingly.
Files needing update:
index.js(around lines 317–331)
Proposed change:
const targetBranch = await search({
message: '🎯 Target branch for PR:',
default: defaultBranch,
- source: (input = '') => {
- return remoteBranches.filter(branch => branch.title.includes(input));
- },
+ // Must return a Promise per @inquirer/prompts signature
+ source: (input = '', { signal }) =>
+ Promise.resolve(
+ remoteBranches.filter(branch => branch.title.includes(input))
+ ),
});📝 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.
| const defaultBranch = getDefaultBranch(); | |
| // Default branch gets added to the start of the array. | |
| const remoteBranches = getRemoteBranches() | |
| .sort((branchA, branchB) => { | |
| return (branchA === defaultBranch) ? -1 : (branchB === defaultBranch) ? 1 : branchA.localeCompare(branchB); | |
| }) | |
| .map(branch => ({ title: branch, value: branch })); | |
| // Ask user which branch to target for PR | |
| // Default to the repository's default branch | |
| console.log('\n🌿 Select target branch for PR:'); | |
| const targetBranch = await search({ | |
| message: '🎯 Target branch for PR:', | |
| default: defaultBranch, | |
| source: (input = '') => { return remoteBranches.filter(branch => branch.title.includes(input)); }, | |
| }); | |
| const defaultBranch = getDefaultBranch(); | |
| // Default branch gets added to the start of the array. | |
| const remoteBranches = getRemoteBranches() | |
| .sort((branchA, branchB) => { | |
| return (branchA === defaultBranch) ? -1 : (branchB === defaultBranch) ? 1 : branchA.localeCompare(branchB); | |
| }) | |
| .map(branch => ({ title: branch, value: branch })); | |
| // Ask user which branch to target for PR | |
| // Default to the repository's default branch | |
| console.log('\n🌿 Select target branch for PR:'); | |
| const targetBranch = await search({ | |
| message: '🎯 Target branch for PR:', | |
| default: defaultBranch, | |
| // Must return a Promise per @inquirer/prompts signature | |
| source: (input = '', { signal }) => | |
| Promise.resolve( | |
| remoteBranches.filter(branch => branch.title.includes(input)) | |
| ), | |
| }); |
🤖 Prompt for AI Agents
In index.js around lines 317 to 331, the source function passed to the search
prompt must return a Promise instead of a synchronous array. Modify the source
function to return a Promise that resolves with the filtered remoteBranches
array. This can be done by wrapping the filter operation inside a
Promise.resolve call or by making the function async and returning the filtered
array directly.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8 +/- ##
========================================
+ Coverage 6.25% 8.52% +2.27%
========================================
Files 1 1
Lines 96 129 +33
========================================
+ Hits 6 11 +5
- Misses 90 118 +28
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/basic.test.js (2)
53-65: Remove redundant helper function and test the actual implementation.The test file defines local helper functions (
branchPushNeededat line 78) that duplicate logic likely present inindex.js. Tests should import and validate the actual implementation from the module rather than creating mock helpers. This approach ensures the test suite validates real, production code behavior.
37-51: Test validates mock helper function, not actual implementation.The test calls
ticketNumberFormatdefined locally at lines 71–73 as a helper function ("mimics the PR title formatting logic"), which is not imported fromindex.js. The function does not exist in the actual implementation, meaning this test validates mock logic rather than real production code behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
test/basic.test.js(1 hunks)test/git.test.js(0 hunks)test/index.test.js(0 hunks)test/template.test.js(0 hunks)
💤 Files with no reviewable changes (3)
- test/git.test.js
- test/index.test.js
- test/template.test.js
🧰 Additional context used
🧬 Code graph analysis (1)
test/basic.test.js (1)
index.js (2)
template(262-262)getTemplatePath(156-170)
🔇 Additional comments (1)
test/basic.test.js (1)
28-34: LGTM: Correctly handles both template scenarios.The test now properly validates both return cases from
getTemplatePath():
- Default inline template (checks
contentis defined)- File-based template (checks
pathends with expected filename)
| @@ -25,7 +25,13 @@ describe('GitHub PR Maker', () => { | |||
|
|
|||
| test('Template path is correctly resolved', () => { | |||
| // This test only verifies the path structure, not actual file existence | |||
There was a problem hiding this comment.
Update comment to reflect expanded test scope.
The comment states the test "only verifies the path structure," but the test now also validates template.content when using the default inline template.
Apply this diff to clarify:
- // This test only verifies the path structure, not actual file existence
+ // This test verifies the template structure (path or default content), not actual file existence📝 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.
| // This test only verifies the path structure, not actual file existence | |
| // This test verifies the template structure (path or default content), not actual file existence |
🤖 Prompt for AI Agents
In test/basic.test.js around line 27, the inline comment incorrectly states the
test "only verifies the path structure" but the test also asserts
template.content for the default inline template; update the comment to
accurately describe that the test validates both the path structure and the
inline template content (i.e., it checks filename/path structure and that
template.content matches the default inline template).
1 participant
Summary
This PR adds the ability for users to choose the branch against which a pull request will be created. Previously, PRs would always be created against the default branch.
Implemented features:
Test plan
Did you mean this?
npm test # Test a package
To see a list of supported npm commands, run:
npm help to verify all basic tests pass
3. Try creating a PR with the tool and verify it prompts for the target branch
4. Verify PR is created against the selected branch
Closes #6
Summary by CodeRabbit
New Features
Refactor
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.