fix(config): output filename not matching style option#949
fix(config): output filename not matching style option#949pranc1ngpegasus wants to merge 2 commits intoyamadashy:mainfrom
Conversation
Summary of ChangesHello @pranc1ngpegasus, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a configuration inconsistency where the default output filename's extension would not update to reflect the chosen output style if the user did not explicitly specify an output file. The change ensures that the system automatically aligns the output file extension with the selected style, providing a more intuitive and consistent user experience without requiring manual intervention for default cases. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug where the output filename did not automatically update to match the selected style option. Previously, when users specified a style like --style plain, the output filename would incorrectly remain as repomix-output.xml instead of updating to repomix-output.txt.
Key Changes:
- Modified the config merging logic to automatically update the output filename when the style is changed from default
- Added comprehensive test coverage for all style options (plain, xml, markdown, json)
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/config/configLoad.ts | Updated mergeConfigs to detect when style changes from default and automatically update the output filename accordingly |
| tests/cli/actions/defaultAction.buildCliConfig.test.ts | Added test cases for all supported style options to ensure proper style handling |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughUpdate configLoad to reassign the default output file path not only when filePath is missing but also when the chosen style changes while the filePath remains the default. Add tests covering style handling and merge behavior for various style/filePath combinations. Changes
Sequence Diagram(s)sequenceDiagram
actor User
User->>ConfigLoad: provide merged config + base config
rect rgb(240, 248, 255)
note over ConfigLoad: compute values
ConfigLoad->>ConfigLoad: style = mergedOutput.style ?? baseConfig.output.style
ConfigLoad->>ConfigLoad: isFilePathDefault = (mergedOutput.filePath === baseConfig.output.filePath)
ConfigLoad->>ConfigLoad: isStyleChanged = (style !== baseConfig.output.style)
end
alt filePath is null OR (isFilePathDefault AND isStyleChanged)
rect rgb(220, 245, 220)
note over ConfigLoad: assign style-specific default filePath
ConfigLoad->>ConfigLoad: mergedOutput.filePath = defaultFilePathMap[style]
ConfigLoad->>ConfigLoad: log assigned default path
end
else otherwise
note over ConfigLoad: keep existing filePath
end
ConfigLoad-->>User: return merged config
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly fixes a bug where the output filename did not match the chosen style when an explicit output file was not specified. The logic change in mergeConfigs is clear and effectively resolves the issue. My review includes feedback on the accompanying tests. While it's great that tests were added, they are repetitive and, more importantly, do not cover the core logic of the fix in mergeConfigs. I've provided suggestions to parameterize the existing tests for better maintainability and to add new unit tests to ensure the fix is robust and prevent future regressions.
| describe('style option', () => { | ||
| it('should handle plain style', () => { | ||
| const options: CliOptions = { | ||
| style: 'plain', | ||
| }; | ||
|
|
||
| const result = buildCliConfig(options); | ||
|
|
||
| expect(result.output?.style).toBe('plain'); | ||
| }); | ||
|
|
||
| it('should handle xml style', () => { | ||
| const options: CliOptions = { | ||
| style: 'xml', | ||
| }; | ||
|
|
||
| const result = buildCliConfig(options); | ||
|
|
||
| expect(result.output?.style).toBe('xml'); | ||
| }); | ||
|
|
||
| it('should handle markdown style', () => { | ||
| const options: CliOptions = { | ||
| style: 'markdown', | ||
| }; | ||
|
|
||
| const result = buildCliConfig(options); | ||
|
|
||
| expect(result.output?.style).toBe('markdown'); | ||
| }); | ||
|
|
||
| it('should handle json style', () => { | ||
| const options: CliOptions = { | ||
| style: 'json', | ||
| }; | ||
|
|
||
| const result = buildCliConfig(options); | ||
|
|
||
| expect(result.output?.style).toBe('json'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Thank you for adding tests. These tests correctly verify that the style option from the CLI is passed to the config object. However, the core logic for the bug fix, which is in the mergeConfigs function in src/config/configLoad.ts, is not covered by these tests. The fix ensures the output filename is updated based on the style, and this behavior should be tested directly.
Please consider adding unit tests for the mergeConfigs function to cover the following scenarios:
- When
--styleis provided without--output, thefilePathis correctly updated to match the style's default extension. - When both
--styleand--outputare provided, the explicitly providedfilePathfrom--outputis respected. - When a
filePathis set in a config file, it is respected even if a different--styleis provided via CLI.
This will ensure the fix is robust and prevent future regressions.
| describe('style option', () => { | ||
| it('should handle plain style', () => { | ||
| const options: CliOptions = { | ||
| style: 'plain', | ||
| }; | ||
|
|
||
| const result = buildCliConfig(options); | ||
|
|
||
| expect(result.output?.style).toBe('plain'); | ||
| }); | ||
|
|
||
| it('should handle xml style', () => { | ||
| const options: CliOptions = { | ||
| style: 'xml', | ||
| }; | ||
|
|
||
| const result = buildCliConfig(options); | ||
|
|
||
| expect(result.output?.style).toBe('xml'); | ||
| }); | ||
|
|
||
| it('should handle markdown style', () => { | ||
| const options: CliOptions = { | ||
| style: 'markdown', | ||
| }; | ||
|
|
||
| const result = buildCliConfig(options); | ||
|
|
||
| expect(result.output?.style).toBe('markdown'); | ||
| }); | ||
|
|
||
| it('should handle json style', () => { | ||
| const options: CliOptions = { | ||
| style: 'json', | ||
| }; | ||
|
|
||
| const result = buildCliConfig(options); | ||
|
|
||
| expect(result.output?.style).toBe('json'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The tests for the style option are very similar and contain duplicated code. To improve maintainability and reduce boilerplate, you could use parameterized tests. Most test runners, including Vitest, support this feature (e.g., with it.each).
describe('style option', () => {
it.each(['plain', 'xml', 'markdown', 'json'] as const)('should handle %s style', (style) => {
const options: CliOptions = {
style,
};
const result = buildCliConfig(options);
expect(result.output?.style).toBe(style);
});
});![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/config/configLoad.ts(1 hunks)tests/cli/actions/defaultAction.buildCliConfig.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/cli/actions/defaultAction.buildCliConfig.test.ts (2)
src/cli/types.ts (1)
CliOptions(4-62)src/cli/actions/defaultAction.ts (1)
buildCliConfig(114-296)
🔇 Additional comments (1)
src/config/configLoad.ts (1)
198-202: LGTM! Logic correctly fixes the output filename/style mismatch.The updated condition properly handles the case where style is changed without an explicit output path. The logic preserves user-specified output filenames while auto-updating the default filename to match the selected style.
Key behaviors verified:
--style plainwithout--outputnow correctly generatesrepomix-output.txtinstead ofrepomix-output.xml✓- Explicit
--output custom.txtis preserved regardless of style ✓- File config style changes without filePath override now work correctly ✓
| describe('style option', () => { | ||
| it('should handle plain style', () => { | ||
| const options: CliOptions = { | ||
| style: 'plain', | ||
| }; | ||
|
|
||
| const result = buildCliConfig(options); | ||
|
|
||
| expect(result.output?.style).toBe('plain'); | ||
| }); | ||
|
|
||
| it('should handle xml style', () => { | ||
| const options: CliOptions = { | ||
| style: 'xml', | ||
| }; | ||
|
|
||
| const result = buildCliConfig(options); | ||
|
|
||
| expect(result.output?.style).toBe('xml'); | ||
| }); | ||
|
|
||
| it('should handle markdown style', () => { | ||
| const options: CliOptions = { | ||
| style: 'markdown', | ||
| }; | ||
|
|
||
| const result = buildCliConfig(options); | ||
|
|
||
| expect(result.output?.style).toBe('markdown'); | ||
| }); | ||
|
|
||
| it('should handle json style', () => { | ||
| const options: CliOptions = { | ||
| style: 'json', | ||
| }; | ||
|
|
||
| const result = buildCliConfig(options); | ||
|
|
||
| expect(result.output?.style).toBe('json'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Tests don't verify the actual bug fix: missing filePath assertions.
According to the PR objectives, the bug was that "when using non-default styles (e.g., --style plain previously produced repomix-output.xml; now produces repomix-output.txt for plain, repomix-output.md for markdown)." However, these tests only verify that the style property is set correctly—they don't verify that the output filename matches the style.
These tests would have passed even before the fix, so they don't provide confidence that the core bug is actually resolved.
Add assertions to verify the filePath for each style:
describe('style option', () => {
it('should handle plain style', () => {
const options: CliOptions = {
style: 'plain',
};
const result = buildCliConfig(options);
expect(result.output?.style).toBe('plain');
+ expect(result.output?.filePath).toBe('repomix-output.txt');
});
it('should handle xml style', () => {
const options: CliOptions = {
style: 'xml',
};
const result = buildCliConfig(options);
expect(result.output?.style).toBe('xml');
+ expect(result.output?.filePath).toBe('repomix-output.xml');
});
it('should handle markdown style', () => {
const options: CliOptions = {
style: 'markdown',
};
const result = buildCliConfig(options);
expect(result.output?.style).toBe('markdown');
+ expect(result.output?.filePath).toBe('repomix-output.md');
});
it('should handle json style', () => {
const options: CliOptions = {
style: 'json',
};
const result = buildCliConfig(options);
expect(result.output?.style).toBe('json');
+ expect(result.output?.filePath).toBe('repomix-output.json');
});
+
+ it('should respect explicit output path regardless of style', () => {
+ const options: CliOptions = {
+ style: 'plain',
+ output: 'custom-output.xml',
+ };
+
+ const result = buildCliConfig(options);
+
+ expect(result.output?.style).toBe('plain');
+ expect(result.output?.filePath).toBe('custom-output.xml');
+ });
});📝 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.
| describe('style option', () => { | |
| it('should handle plain style', () => { | |
| const options: CliOptions = { | |
| style: 'plain', | |
| }; | |
| const result = buildCliConfig(options); | |
| expect(result.output?.style).toBe('plain'); | |
| }); | |
| it('should handle xml style', () => { | |
| const options: CliOptions = { | |
| style: 'xml', | |
| }; | |
| const result = buildCliConfig(options); | |
| expect(result.output?.style).toBe('xml'); | |
| }); | |
| it('should handle markdown style', () => { | |
| const options: CliOptions = { | |
| style: 'markdown', | |
| }; | |
| const result = buildCliConfig(options); | |
| expect(result.output?.style).toBe('markdown'); | |
| }); | |
| it('should handle json style', () => { | |
| const options: CliOptions = { | |
| style: 'json', | |
| }; | |
| const result = buildCliConfig(options); | |
| expect(result.output?.style).toBe('json'); | |
| }); | |
| }); | |
| describe('style option', () => { | |
| it('should handle plain style', () => { | |
| const options: CliOptions = { | |
| style: 'plain', | |
| }; | |
| const result = buildCliConfig(options); | |
| expect(result.output?.style).toBe('plain'); | |
| expect(result.output?.filePath).toBe('repomix-output.txt'); | |
| }); | |
| it('should handle xml style', () => { | |
| const options: CliOptions = { | |
| style: 'xml', | |
| }; | |
| const result = buildCliConfig(options); | |
| expect(result.output?.style).toBe('xml'); | |
| expect(result.output?.filePath).toBe('repomix-output.xml'); | |
| }); | |
| it('should handle markdown style', () => { | |
| const options: CliOptions = { | |
| style: 'markdown', | |
| }; | |
| const result = buildCliConfig(options); | |
| expect(result.output?.style).toBe('markdown'); | |
| expect(result.output?.filePath).toBe('repomix-output.md'); | |
| }); | |
| it('should handle json style', () => { | |
| const options: CliOptions = { | |
| style: 'json', | |
| }; | |
| const result = buildCliConfig(options); | |
| expect(result.output?.style).toBe('json'); | |
| expect(result.output?.filePath).toBe('repomix-output.json'); | |
| }); | |
| it('should respect explicit output path regardless of style', () => { | |
| const options: CliOptions = { | |
| style: 'plain', | |
| output: 'custom-output.xml', | |
| }; | |
| const result = buildCliConfig(options); | |
| expect(result.output?.style).toBe('plain'); | |
| expect(result.output?.filePath).toBe('custom-output.xml'); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In tests/cli/actions/defaultAction.buildCliConfig.test.ts around lines 28 to 68,
the test cases only assert the style property and do not verify the output
filename, so they don't confirm the bugfix that output filePath changes per
style; update each test block to also assert result.output?.filePath (or the
correct property name used by buildCliConfig) equals the expected filename for
that style (e.g., repomix-output.txt for plain, repomix-output.xml for xml,
repomix-output.md for markdown, repomix-output.json for json), keeping existing
style assertions and using the same result variable.
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/config/configLoad.test.ts (1)
305-375: Excellent test coverage for style-based filename generation!The new tests comprehensively validate the fix for automatic filename updates when style changes. The scenarios covered are:
- Automatic filename updates for plain/markdown/json styles (tests at 305-315, 317-327, 329-339)
- Preservation of explicit CLI output paths (341-351)
- Preservation of file config paths (353-365)
- Default behavior when no style specified (367-375)
Optional: Consider adding edge case tests
To further strengthen coverage, consider adding tests for these scenarios:
- File config with style but no filePath:
test('should update filePath when fileConfig has style but no filePath', () => { const fileConfig: RepomixConfigFile = { output: { style: 'plain' }, }; const cliConfig: RepomixConfigCli = {}; const result = mergeConfigs(process.cwd(), fileConfig, cliConfig); expect(result.output.style).toBe('plain'); expect(result.output.filePath).toBe('repomix-output.txt'); });
- CLI style overrides file config style:
test('should update filePath when CLI style overrides fileConfig style', () => { const fileConfig: RepomixConfigFile = { output: { style: 'plain' }, }; const cliConfig: RepomixConfigCli = { output: { style: 'markdown' }, }; const result = mergeConfigs(process.cwd(), fileConfig, cliConfig); expect(result.output.style).toBe('markdown'); expect(result.output.filePath).toBe('repomix-output.md'); });Optional: Consider test parameterization
Tests at lines 305-315, 317-327, and 329-339 follow a similar pattern and could be parameterized using
test.each()to reduce duplication. However, the current explicit approach is also fine for readability.test.each([ { style: 'plain', expectedPath: 'repomix-output.txt' }, { style: 'markdown', expectedPath: 'repomix-output.md' }, { style: 'json', expectedPath: 'repomix-output.json' }, ])('should update filePath to $expectedPath for $style style', ({ style, expectedPath }) => { const result = mergeConfigs(process.cwd(), {}, { output: { style } }); expect(result.output.style).toBe(style); expect(result.output.filePath).toBe(expectedPath); });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/cli/actions/defaultAction.buildCliConfig.test.ts(1 hunks)tests/config/configLoad.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/cli/actions/defaultAction.buildCliConfig.test.ts
|
Hi @pranc1ngpegasus! Thank you so much for the detailed analysis of the issue and this fix PR! I'm sorry, but #947 was submitted earlier with a similar fix, so we've prioritized that PR. Unfortunately, this means I won't be able to merge this PR. I'm closing this, but I wanted to show my appreciation for your contribution with a small sponsorship. Thank you again for taking the time to investigate and fix this issue! 🙏 |
|
Hi, @yamadashy ! Thanks for letting me know! No worries at all — I’m glad the issue got resolved, whether through my PR or another one. |
Summary
Fix output filename not matching the style option. When using
--style plain(or other non-default styles)without explicitly specifying
--output, the output filename now correctly matches the style (e.g.,repomix-output.txtfor plain,repomix-output.mdfor markdown).Before:
repomix --style plain # Output: repomix-output.xml (wrong extension)After:
repomix --style plain # Output: repomix-output.txt (correct extension)Changes
when the style is changed
Root Cause
The default config hardcodes filePath: 'repomix-output.xml'. When users specified --style plain, the style was
correctly set but the filename remained repomix-output.xml because the merge logic only updated the filename when
filePath was null, not when it was still the default value.
Technical Details
The fix checks if:
If either condition is true, the filename is automatically updated to match the style.
Note: Explicitly specified output filenames via --output are still respected regardless of the style option.
Checklist