feat: show warning in coverage comment when coverage report too long - #298
feat: show warning in coverage comment when coverage report too long#298mschoettle wants to merge 1 commit into
Conversation
🤖 CodeAnt AI — Review Status
|
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
PR Summary by QodoSurface truncation warnings in oversized coverage comments
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Code Review by Qodo
1. Notice can exceed limit
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| const runUrl = context.runId | ||
| ? `${options.repoUrl}/actions/runs/${context.runId}` | ||
| : null; | ||
| tooLongHtml = tooLongNotice(MAX_COMMENT_LENGTH, runUrl); |
There was a problem hiding this comment.
1. Notice can exceed limit 🐞 Bug ☼ Reliability
After adding tooLongHtml, the fallback only removes failed-test blocks; if the hidden coverage output plus the summary or multiple-files table remains over 65,536 characters, the new notice leaves the body oversized and the GitHub comment operation fails. This also affects cases that previously fit after hiding the report but cross the limit solely because of the added notice and uncounted final separators/watermark.
Agent Prompt
## Issue description
Adding the truncation notice can leave the fallback comment above `MAX_COMMENT_LENGTH`, because the existing reductions are conditional and there is no final size enforcement before posting.
## Issue Context
`commentLength()` includes the notice but omits the watermark and assembly separators. After hiding the detailed coverage report, the code only removes failed-test content; an oversized summary or multiple-files table remains untouched and is posted.
## Fix Focus Areas
- src/index.ts[437-443]
- src/index.ts[480-508]
- src/index.ts[511-553]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| summaryReport.length + | ||
| failedTestsHtml.length + | ||
| multipleFilesHtml.length; | ||
| multipleFilesHtml.length + |
There was a problem hiding this comment.
Suggestion: The new size accounting can enter this truncation path when multipleFilesHtml itself exceeds the limit, but the fallback only removes failed-test sections when they are present. If the multiple-files report has no failed-test block, it remains oversized and is appended unchanged, so the final GitHub comment can still exceed MAX_COMMENT_LENGTH and be rejected. Add a final fallback that truncates or removes the remaining oversized section before assembling the body. [incorrect control logic]
Severity Level: Major ⚠️
- ❌ Large multiple-file comments can be rejected by GitHub.
- ❌ Coverage comments fail despite the warning notice.
- ⚠️ `getMultipleReport()` has no size truncation fallback.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/index.ts
**Line:** 442:442
**Comment:**
*Incorrect Control Logic: The new size accounting can enter this truncation path when `multipleFilesHtml` itself exceeds the limit, but the fallback only removes failed-test sections when they are present. If the multiple-files report has no failed-test block, it remains oversized and is appended unchanged, so the final GitHub comment can still exceed `MAX_COMMENT_LENGTH` and be rejected. Add a final fallback that truncates or removes the remaining oversized section before assembling the body.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
WalkthroughThe change adds the exported Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: 🔵 Low · up to Coverage comments near the configured size limit may still exceed that limit because assembly separators are not included in the check, allowing truncation behavior to remain inaccurate. The change is otherwise mergeable with explicit owner awareness and a small fix to calculate the length from the final assembled comment. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2 files. Full details: Description checkExplanation The description follows the required template, explains the change, records local testing and npm run all, and includes relevant implementation details. The Related Issue field is not linked to a specific issue, but the description is otherwise complete. ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1bf13a3d-bd9e-4ca1-94a6-b5402189e2c1
⛔ Files ignored due to path filters (1)
dist/index.jsis excluded by!**/dist/**
📒 Files selected for processing (2)
__tests__/index.test.tssrc/index.ts
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.
| let tooLongHtml = ''; | ||
| const commentLength = (): number => | ||
| html.length + | ||
| summaryReport.length + | ||
| failedTestsHtml.length + | ||
| multipleFilesHtml.length; | ||
| multipleFilesHtml.length + | ||
| tooLongHtml.length; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Include final assembly separators in the length check.
commentLength() adds tooLongHtml.length, but the final assembly adds \n\n before the notice at Line 513 and before the summary at Line 515. Near MAX_COMMENT_LENGTH, the check can pass while the emitted comment exceeds the configured limit. Calculate the length from the assembled comment, including separators.
Also applies to: 512-514
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/index.ts">
<violation number="1" location="src/index.ts:513">
P1: Enforce `MAX_COMMENT_LENGTH` after assembling the fallback comment. Hiding the coverage report does not guarantee that the summary or multiple-files section fits, so this path can still submit an oversized comment.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| finalHtml += html; | ||
| if (tooLongHtml) { | ||
| finalHtml += finalHtml.length ? `\n\n${tooLongHtml}` : tooLongHtml; |
There was a problem hiding this comment.
P1: Enforce MAX_COMMENT_LENGTH after assembling the fallback comment. Hiding the coverage report does not guarantee that the summary or multiple-files section fits, so this path can still submit an oversized comment.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/index.ts, line 513:
<comment>Enforce `MAX_COMMENT_LENGTH` after assembling the fallback comment. Hiding the coverage report does not guarantee that the summary or multiple-files section fits, so this path can still submit an oversized comment.</comment>
<file context>
@@ -485,6 +509,9 @@ const main = async (): Promise<void> => {
finalHtml += html;
+ if (tooLongHtml) {
+ finalHtml += finalHtml.length ? `\n\n${tooLongHtml}` : tooLongHtml;
+ }
finalHtml += finalHtml.length ? `\n\n${summaryReport}` : summaryReport;
</file context>
|
@MishaKav The issue found is pre-existing. We could fix it but I think it would be better separately. WDYT? |
1 participant
What does this PR do?
When the coverage comment is too long and gets truncated it is only visible in the job log. This PR adds a warning to the comment itself to show that it was truncated as it can otherwise go unnoticed.
Related Issue
Closes #
Checklist
npm run allSummary by cubic
Adds a warning to the coverage comment when it's too long and gets truncated, instead of only logging the truncation to the job log.
tooLongNoticedisplays a warning in the comment when the coverage report is dropped.Written for commit 516234d. Summary will update on new commits.