Refactor packager.ts into smaller functions#217
Conversation
|
|
📝 WalkthroughWalkthroughThis pull request introduces several new modules and functions across the core metrics, security, and packager components of the project. The changes focus on modularizing file processing, security validation, and metrics calculation. Key additions include functions for searching files, calculating file metrics, validating file safety, writing output to disk, and copying output to clipboard. The modifications aim to improve code organization, error handling, and provide more granular control over file processing and output generation. Changes
Possibly related PRs
Suggested labels
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![standard-input[bot]](https://avatars.githubusercontent.com/in/1011034?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (12)
src/core/security/filterOutUntrustedFiles.ts (1)
4-8: Consider using a set for improved performance
For large lists of suspicious files, repeatedly calling.some()can be inefficient. Constructing a set of suspicious file paths before filtering can improve performance.-export const filterOutUntrustedFiles = ( - rawFiles: RawFile[], - suspiciousFilesResults: SuspiciousFileResult[], -): RawFile[] => - rawFiles.filter((rawFile) => !suspiciousFilesResults.some((result) => result.filePath === rawFile.path)); +export const filterOutUntrustedFiles = ( + rawFiles: RawFile[], + suspiciousFilesResults: SuspiciousFileResult[], +): RawFile[] => { + const suspiciousSet = new Set(suspiciousFilesResults.map(r => r.filePath)); + return rawFiles.filter((rawFile) => !suspiciousSet.has(rawFile.path)); +};src/core/packager/writeOutputToDisk.ts (2)
2-2: Use the Node.js protocol for builtin imports
Static analysis advises using thenode:protocol. It clarifies that this is a Node.js core module.- import path from 'path'; + import path from 'node:path';🧰 Tools
🪛 Biome (1.9.4)
[error] 2-2: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.(lint/style/useNodejsImportProtocol)
7-11: Return type can be simplified
ReturningPromise<undefined>is effectively the same asPromise<void>. For clarity and consistency, consider usingPromise<void>.-export const writeOutputToDisk = async (output: string, config: RepomixConfigMerged): Promise<undefined> => { +export const writeOutputToDisk = async (output: string, config: RepomixConfigMerged): Promise<void> => {src/core/packager/copyToClipboardIfEnabled.ts (1)
7-17: Consider error handling for clipboard operations
If the environment does not support a clipboard,clipboard.writecould fail. Handling errors gracefully or logging them can improve user experience and debuggability.if (config.output.copyToClipboard) { progressCallback('Copying to clipboard...'); logger.trace('Copying output to clipboard'); - await clipboard.write(output); + try { + await clipboard.write(output); + } catch (err) { + logger.warn('Clipboard write failed. Check system clipboard support.', err); + } }src/core/security/runSecurityCheckIfEnabled.ts (1)
12-15: Handle potential errors during security checkSince this is an asynchronous operation, consider wrapping
checkSecurityin a try/catch block or ensuring errors are handled and logged. This prevents unhandled promise rejections ifrunSecurityCheckthrows an error.tests/core/packager/writeOutputToDisk.test.ts (1)
2-2: Use node: protocol for consistencyStatic analysis indicates that using the
node:protocol is more explicit for built-in modules. Update your import to:- import path from 'path'; + import path from 'node:path';🧰 Tools
🪛 Biome (1.9.4)
[error] 2-2: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.(lint/style/useNodejsImportProtocol)
src/core/metrics/aggregateMetrics.ts (1)
5-29: Handle edge cases for empty or null inputsWhile this function does handle the metrics array gracefully, consider how it should behave if
processedFilesorfileMetricsare unexpectedly empty. Adding guards or logs can improve diagnostics.src/core/metrics/calculateIndividualFileMetrics.ts (3)
1-2: Organize imports for clarity
The usage ofsetTimeoutfromnode:timers/promisesandpicocolorsis clear, but be mindful of your import order for easier scanning. Usually, standard library imports can come first, followed by third-party libraries, and then local imports.
7-11: Use a more descriptive interface or doc comments
TheFileMetricsinterface is straightforward, but consider adding a doc comment or more descriptive property documentation. This helps future maintainers understand the purpose of each property, especially if they expand in future iterations.
13-29: Double-check thesetTimeout(1)approach
Including a short delay (setTimeout(1)) is a neat way to yield to the event loop, but it’s still fairly arbitrary. Consider either removing it if unneeded or adding a comment justifying why a 1ms delay is ideal for your use case.src/core/security/validateFileSafety.ts (1)
8-28: Consider better fail-fast or partial-safety strategies
IfrunSecurityCheckIfEnabledorfilterOutUntrustedFilesencounters a critical failure—such as a network issue or unrecognized file format—your flow might need to address partial results or fail safely. Think about whether you need fallback behavior or a robust plan for short-circuiting the process.tests/core/metrics/calculateIndividualFileMetrics.test.ts (1)
8-28: Validate boundary edge cases
The test looks thorough for a single file scenario. In addition, consider adding tests for an empty file, a large file, or special characters to ensure correct character counting and token processing under more edge conditions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
.devcontainer/devcontainer.json(1 hunks)src/core/file/fileSearch.ts(1 hunks)src/core/metrics/aggregateMetrics.ts(1 hunks)src/core/metrics/calculateAllFileMetrics.ts(1 hunks)src/core/metrics/calculateIndividualFileMetrics.ts(1 hunks)src/core/metrics/calculateMetrics.ts(1 hunks)src/core/packager.ts(2 hunks)src/core/packager/copyToClipboardIfEnabled.ts(1 hunks)src/core/packager/writeOutputToDisk.ts(1 hunks)src/core/security/filterOutUntrustedFiles.ts(1 hunks)src/core/security/runSecurityCheckIfEnabled.ts(1 hunks)src/core/security/validateFileSafety.ts(1 hunks)tests/core/metrics/aggregateMetrics.test.ts(1 hunks)tests/core/metrics/calculateAllFileMetrics.test.ts(1 hunks)tests/core/metrics/calculateIndividualFileMetrics.test.ts(1 hunks)tests/core/metrics/calculateMetrics.test.ts(1 hunks)tests/core/packager.test.ts(1 hunks)tests/core/packager/copyToClipboardIfEnabled.test.ts(1 hunks)tests/core/packager/writeOutputToDisk.test.ts(1 hunks)tests/core/security/filterOutUntrustedFiles.test.ts(1 hunks)tests/core/security/runSecurityCheckIfEnabled.test.ts(1 hunks)tests/core/security/validateFileSafety.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .devcontainer/devcontainer.json
🧰 Additional context used
🪛 Biome (1.9.4)
src/core/packager/writeOutputToDisk.ts
[error] 2-2: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
tests/core/packager/writeOutputToDisk.test.ts
[error] 2-2: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
🔇 Additional comments (37)
src/core/packager.ts (6)
6-11: Modular imports enhance readability
Bringing in dedicated helper modules for metrics, clipboard copying, file writing, and file safety validation significantly improves code clarity and maintainability.
31-34: Dependency injection approach is flexible
Providing the new functions in the deps object supports easy mocking and testing, enhancing maintainability.
43-47: Proper integration of validateFileSafety
This step ensures that suspicious files are properly identified and removed from subsequent processing. The call site is clearly separated from the rest of the logic, which helps keep code concerns well-partitioned.
57-57: Output writing is correctly delegated
Refactoring the file-writing logic into a separate function keeps the main pack flow concise and improves readability.
59-59: Clipboard copying logic is encapsulated
Invoking a separate function for clipboard operations is a clean solution. It avoids polluting the main packaging flow with conditional clipboard logic.
61-64: Metrics calculation is well modularized
Extracting metrics-calculation logic into its own function adheres to the single responsibility principle and simplifies future maintenance.
src/core/file/fileSearch.ts (1)
Line range hint 45-107: Asynchronous file search with robust error handling
This function thoroughly checks directory permissions, constructs patterns, and handles errors. Using globby with ignore patterns and concurrency is a clean approach. Logging at trace level further aids in debugging.
src/core/security/runSecurityCheckIfEnabled.ts (1)
1-4: Ensure config.security is always defined
Currently, this code assumes config.security is always defined. If config can be missing that property, you might encounter runtime errors. Consider providing a safe default or performing a check to ensure config.security exists.
src/core/metrics/calculateAllFileMetrics.ts (2)
1-1: Efficient concurrency approach
Kudos on using pMap for concurrency! It allows parallel processing of file metrics effectively.
13-19: Consider global error handling strategy
If a single file's metric calculation fails, pMap will reject the entire process by default. Ensure that any errors from calculateIndividualFileMetrics won't disrupt the entire batch if partial failures are acceptable, or provide user feedback if necessary.
tests/core/packager/writeOutputToDisk.test.ts (1)
10-23: Sufficient test coverage
The test sufficiently verifies that the writeOutputToDisk function writes the expected data. Good job mocking fs.promises to isolate file system side-effects.
src/core/metrics/calculateMetrics.ts (2)
1-14: Imports and interface are well-structured
The interface CalculateMetricsResult covers necessary data fields for aggregated metrics. Good job ensuring it remains standalone and easy to expand if further metrics are added later.
15-30: Ensure robust error handling
The function currently returns the aggregated metrics but does not catch or handle potential errors from calculateAllFileMetrics or aggregateMetrics. Consider wrapping these calls in a try/catch block or letting them bubble up in a controlled fashion, especially if your system requires graceful error reporting.
src/core/security/validateFileSafety.ts (1)
1-7: Security dependencies are well-defined
The direct import of filterOutUntrustedFiles and runSecurityCheckIfEnabled ensures a clean separation of concerns. This design choice enables better testing and potential future reuse or replacement of these functions.
tests/core/metrics/calculateIndividualFileMetrics.test.ts (1)
1-7: Test suite covers essential cases
You’re verifying that both the tokenCounter call and the progress callback are accurately triggered. This is excellent coverage for a core metric function.
tests/core/security/filterOutUntrustedFiles.test.ts (3)
1-5: Use descriptive test imports for clarity.
Imports are straightforward and well-structured. The use of descriptive type imports (e.g., RawFile) strengthens code readability.
6-22: Comprehensive test for filtering suspicious files.
The test effectively covers the scenario of excluding suspicious files, ensuring that only trusted files remain. The arrangement of test data, suspicious files, and expected outcomes is clear.
23-35: Coverage of scenario with no suspicious files.
Verifying that all files remain intact if the suspicious list is empty ensures comprehensive test coverage. This test is valuable for catching edge cases.
tests/core/packager/copyToClipboardIfEnabled.test.ts (4)
1-7: Mocking external dependencies enhances test isolation.
By mocking clipboardy and logger, you’re preventing side effects and ensuring tests remain self-contained and reproducible.
8-15: Use of vi.resetAllMocks() ensures a clean test environment.
Resetting mocks before each test avoids potential cross-contamination between test runs, contributing to reliable test results.
16-27: Validates behavior when clipboard feature is enabled.
The test design properly verifies that the function performs the clipboard write and callback call. Assertions are concise and comprehensive.
29-41: Confirms clipboard is skipped when disabled.
Covering the disabled state ensures full behavioral coverage. The combination of negative assertions (e.g., .not.toHaveBeenCalled()) confirms that the function respects configuration flags.
tests/core/metrics/calculateAllFileMetrics.test.ts (3)
1-7: Type usage and mocking strategy look good.
Importing the necessary types (e.g., ProcessedFile, TokenCounter) promotes clarity, and mocking calculateIndividualFileMetrics is a well-chosen approach for isolating function behavior under test.
8-12: Concurrency mock ensures predictable parallelism.
By mocking concurrency functions, you control the processing order in tests, resulting in deterministic outcomes and simpler test assertions.
13-39: Verifies aggregated metrics for multiple files.
The test thoroughly checks the computed character and token counts for each processed file, and ensures the correct number of calls to the mocked metric calculator. This approach covers the core concurrency flow and correctness of aggregated results.
tests/core/security/validateFileSafety.test.ts (3)
1-7: Imports and type definitions establish robust safety checks.
Including type references (e.g., SuspiciousFileResult, RawFile) clarifies data structures, reducing confusion about the shape of external dependencies.
8-27: Test setup mirrors realistic usage of validateFileSafety.
You effectively simulate suspicious file results and the filter function, allowing the test to verify the integration of runSecurityCheckIfEnabled and filterOutUntrustedFiles.
28-38: Asserts combined outputs for both safe and suspicious files.
Checking the integrity of returned structures (i.e., safe file list, safe paths, suspicious file results) ensures that validateFileSafety meets its contract efficiently.
tests/core/metrics/aggregateMetrics.test.ts (3)
6-6: Good structure for the testing suite.
The test suite accurately describes the function's purpose and sets the stage for thorough coverage.
7-36: Test data covers typical usage scenarios effectively.
Using distinct file paths and combining them with unique char/token counts adequately tests the normal functionality of aggregateMetrics.
38-56: Edge case test is well-defined.
Verifying empty fileMetrics and processedFiles provides necessary coverage for boundary conditions.
tests/core/security/runSecurityCheckIfEnabled.test.ts (2)
8-25: Properly tests enabled security checks.
Mocking security checks and verifying their invocation ensures robust coverage and clear expectations for the test’s outcome.
27-44: Correctly tests disabled security check scenario.
The test ensures that the function behavior remains stable even when security checks are disabled.
tests/core/metrics/calculateMetrics.test.ts (2)
13-14: Test suite naming is clear and descriptive.
Keeping the suite name aligned with the function under test helps with readability and organization.
15-56: Well-structured test for calculating metrics.
The use of mocks for TokenCounter, calculateAllFileMetrics, and aggregateMetrics ensures a focused approach that isolates and validates just the calculateMetrics function.
tests/core/packager.test.ts (2)
19-20: Appropriate reset strategy in beforeEach.
Ensuring all mocks are reset before each test helps avoid unintended side effects between test runs.
21-87: Comprehensive orchestration test for the pack process.
This single test captures the main workflow of packing files, including searching, collecting, validating, processing, generating output, writing to disk, and calculating metrics.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/core/metrics/calculateMetrics.test.ts (1)
22-48: Consider adding error handling test cases.While the happy path is well tested, consider adding test cases for error scenarios to ensure proper cleanup of TokenCounter resources.
Add a test case for error handling:
it('should free TokenCounter resources even if an error occurs', async () => { const mockTokenCounter = { countTokens: vi.fn().mockRejectedValue(new Error('Count failed')), free: vi.fn(), }; (TokenCounter as unknown as Mock).mockImplementation(() => mockTokenCounter); await expect(calculateMetrics([], '', vi.fn())).rejects.toThrow('Count failed'); expect(mockTokenCounter.free).toHaveBeenCalled(); });tests/core/packager/writeOutputToDisk.test.ts (1)
13-16: Improve type safety of the config object.Instead of type assertion, consider providing all required properties of RepomixConfigMerged.
- const config: RepomixConfigMerged = { - cwd: '/test/directory', - output: { filePath: 'output.txt' }, - } as RepomixConfigMerged; + const config: RepomixConfigMerged = { + cwd: '/test/directory', + output: { filePath: 'output.txt' }, + // Add other required properties with appropriate default values + searchPatterns: [], + exclude: [], + // ... other required properties
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/core/packager/writeOutputToDisk.ts(1 hunks)tests/core/metrics/aggregateMetrics.test.ts(1 hunks)tests/core/metrics/calculateMetrics.test.ts(1 hunks)tests/core/packager/writeOutputToDisk.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/core/packager/writeOutputToDisk.ts
- tests/core/metrics/aggregateMetrics.test.ts
🔇 Additional comments (4)
tests/core/metrics/calculateMetrics.test.ts (3)
13-21: LGTM! Well-structured test setup.
The test data preparation is clean and includes good sample cases with varying content lengths.
49-56: LGTM! Comprehensive test assertions.
The test thoroughly verifies:
- Progress reporting
- Correct function call sequence
- Resource cleanup
- Expected results
1-12: Verify if this file is within the scope of the PR.
This appears to be a new test file for metrics calculation, but the PR objectives mention refactoring packager.ts. Could you clarify if this file is part of the intended changes?
✅ Verification successful
The test file is within scope of the packager.ts refactoring
Based on the git diff and code analysis, this test file is directly related to the packager refactoring. The changes in packager.ts show that the metrics calculation has been extracted into a separate module (calculateMetrics.js), and this new test file provides test coverage for that extracted functionality. The refactoring includes:
- Moving metrics calculation from
packager.tsinto a separate module - Adding proper test coverage for the extracted metrics functionality
- Updating the packager to use the new metrics module as a dependency
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if this file is related to packager.ts changes
rg -l "calculateMetrics" | xargs rg "packager"
# Check commit history to understand the relationship
git log --oneline -- tests/core/metrics/calculateMetrics.test.ts src/core/metrics/calculateMetrics.ts
Length of output: 374
Script:
#!/bin/bash
# Let's check if calculateMetrics is used in packager.ts or related files
rg "calculateMetrics" src/core/packager.ts
# Check if there are any recent changes to packager.ts that might be related
git diff HEAD~5 -- src/core/packager.ts
# Check the PR description or related files
fd packager --exec cat {}
Length of output: 14875
tests/core/packager/writeOutputToDisk.test.ts (1)
1-9: LGTM! Imports and mocks are well structured.
The necessary imports and mocks are properly set up for testing the file writing functionality.
|
Hi, @mikelovesrobots ! You're right - packager.ts was frequently modified and caused conflicts across multiple PRs. I fixed a small linting error but your code changes look perfect. While the metrics-related splitting might seem a bit granular, I think it's actually well-suited for AI-assisted code generation, so I'm satisfied with this approach. I'll merge this in! |
| @@ -0,0 +1,57 @@ | |||
| import { describe, expect, it } from 'vitest'; | |||
| import type { FileMetrics } from '../../../lib/core/metrics/calculateIndividualFileMetrics.js'; | |||
There was a problem hiding this comment.
This line should be:
import type { FileMetrics } from '../../../src/core/metrics/calculateIndividualFileMetrics.js';There was a problem hiding this comment.
Hey guys, I saw CI was failing so took a quick look and found this. Hope it helps!
There was a problem hiding this comment.
Thanks for catching that. Not sure how it was passing here, but yeah, certainly breaking main.
Here's the fix:
#218
|
@mikelovesrobots https://github.com/yamadashy/repomix/releases/tag/v0.2.7 While this change is classified as "internal", it greatly improves maintainability for future development. |
3 participants
Hi!
I'm writing a book on refactoring. To test out the methods I promote in my book, I’ve been working on open-source projects I admire, identifying complex files and simplifying them.
In this repo, I ran
git log --name-only --pretty=format: | sort | uniq -c | sort -rg | head -40and found that packager.ts was the most frequently modified file, with around 50 commits. It’s also over 100 lines long, so I refactored it by breaking it into smaller, single-purpose functions. Each function is unit-tested, and there are no actual functional changes.The existing code is already pretty strong, but this refactor on that one hotspot should make it easier to work with in the future. This PR is unsolicited and my first time working with your coding conventions, so let me know if anything needs adjustment.
Thanks for considering this!
Functional changes
None. This PR only refactors packager.ts by extracting out smaller functions.
Checklist
npm run testnpm run lint