Setting up Debug Mode #23
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
❇️ CodePress Review Summary
👋 Hey team,
Overall the changes look solid, but I spotted 9 must-fix issues and left 16 helpful notes inline.
Here's the quick rundown:
✅ Decision: APPROVE
The feature is well-implemented with comprehensive tests and documentation; debug mode is opt-in and non-intrusive when disabled.
👍 What's working well
The review commends the consistent adoption of the new debugLog utility in place of unconditional console.log calls across multiple modules. By gating output behind the debug flag, these changes improve control over verbosity and prevent extraneous logging in non‐debug runs. This unified approach strengthens the logging infrastructure and aligns with best practices for conditional debugging.
🚧 Needs a bit of love
Several critical logging calls were inadvertently silenced by swapping console.error, console.warn, and console.log for their debug counterparts, risking the loss of essential feedback in normal operation. Key error and rate‐limit warnings must be emitted unconditionally, and routine success messages should remain visible by default. Additionally, the debug flag must be propagated through the configuration return value and used to initialize the debug utilities early in the application startup. Finally, a test should be updated to reuse the same Error instance for accurate assertion.
ℹ️ Other thoughts
The optional feedback encourages clearer documentation of the debug flag’s default value and example usage, as well as expanded test coverage to verify debug helpers in both enabled and disabled modes. Minor style and consistency enhancements—such as removing unused imports, cleaning up redundant lines, and refining wording—will further polish the codebase. Together, these suggestions improve usability, test robustness, and code hygiene.
| openai_api_key: ${{ secrets.OPENAI_API_KEY }} | ||
| ``` | ||
|
|
||
| ### With Debug Mode Enabled |
There was a problem hiding this comment.
🟡 OPTIONAL: It might be helpful to note that the debug input defaults to false, so readers understand this snippet is explicitly opting into debug mode.
| #### Large PR Processing | ||
|
|
||
| - Very large PRs may hit token limits or timeout | ||
| - Consider using `.codepressignore` to exclude generated files, `.codepressignore` follows `.gitignore` conventions |
There was a problem hiding this comment.
🔵 NIT: The .codepressignore filename is repeated; consider simplifying the phrasing to avoid redundancy.
| - Consider using `.codepressignore` to exclude generated files, `.codepressignore` follows `.gitignore` conventions | |
| +- Consider using <code>.codepressignore</code> to exclude generated files; it follows <code>.gitignore</code> conventions |
|
|
||
| ## Debugging and Troubleshooting | ||
|
|
||
| ### Debug Mode |
There was a problem hiding this comment.
🟡 OPTIONAL: It may be helpful to note the default value for the debug input (e.g., false) so users know that detailed logging is off unless explicitly enabled.
| debug: | ||
| description: "Enable debug mode to show detailed console logs (true/false)" | ||
| required: false | ||
| default: "false" |
There was a problem hiding this comment.
🔵 NIT: There's an extra blank line after this block—consider removing it to keep input sections formatted consistently.
| required: false | ||
| default: "true" | ||
|
|
||
| debug: |
There was a problem hiding this comment.
🟡 OPTIONAL: It may be helpful to update the example usage in this file (or in the README) to show how to set the new debug input so users can discover and leverage it easily.
| finding, | ||
| ); | ||
| console.log(`✅ Commented on ${finding.path}:${finding.line}`); | ||
| debugLog(`✅ Commented on ${finding.path}:${finding.line}`); |
There was a problem hiding this comment.
👏 PRAISE: Nice work replacing the unconditional console.log with debugLog here—this respects the debug mode setting.
|
|
||
| expect(mockConsoleError).toHaveBeenCalledWith( | ||
| "error message", | ||
| new Error("test error"), |
There was a problem hiding this comment.
🔴 REQUIRED: The test creates a fresh Error instance in the assertion, which won’t strictly match the one passed to debugError. Assign the Error to a variable before calling debugError and reuse that variable in the expect so the same instance is compared.
| new Error("test error"), | |
| ```ts | |
| const error = new Error("test error"); | |
| debugError("error message", error); | |
| expect(mockConsoleError).toHaveBeenCalledWith( | |
| "error message", | |
| error, | |
| ); | |
| ``` |
|
|
||
| afterEach(() => { | ||
| // Restore original console methods | ||
| console.log = originalConsoleLog; |
There was a problem hiding this comment.
🔵 NIT: Manually restoring console methods and then calling jest.restoreAllMocks() is redundant. You can rely on jest.restoreAllMocks() alone to restore your spied console methods.
| githubRepository: "mock-owner/mock-repo", | ||
| maxTurns: 20, | ||
| updatePrDescription: false, | ||
| debug: false, |
There was a problem hiding this comment.
🟡 OPTIONAL: Great catch adding the new debug flag so the config shape matches ReviewConfig.
Consider adding a complementary test case where debug is true to verify that the debugLog/ debugWarn/ debugError helpers are actually invoked when enabled (and suppressed when disabled).
| githubRepository: "mock-owner/mock-repo", | ||
| maxTurns: 20, | ||
| updatePrDescription: true, // Feature enabled | ||
| debug: false, |
There was a problem hiding this comment.
🟡 OPTIONAL: Consider adding a complementary test case with debug: true to verify that the debug logging functions
(e.g. debugLog, debugWarn, debugError) are invoked when debug mode is enabled
and remain silent when disabled.
1 participant
Right now our action outputs a lot of logs.
We now only output logs if you set debug mode to true