Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ 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 introduces persona-aware client validation reports, enabling the generation of tailored HTML reports for different stakeholders like clients, product managers, and engineers. The changes include comprehensive documentation, an example report specification, and new logic in build-review.mjs for report normalization and rendering. The review feedback identifies several opportunities to improve the robustness of the build script, specifically by handling invalid date strings and malformed JSON files to prevent crashes, and ensuring asset URLs are properly HTML-escaped to mitigate potential injection vulnerabilities.
| subtitle: raw.subtitle || raw.summary || '', | ||
| summary: raw.summary || raw.subtitle || '', | ||
| route: raw.route || raw.url || '', | ||
| generatedAt: raw.generated_at || raw.generatedAt || new Date().toISOString(), |
There was a problem hiding this comment.
If raw.generated_at or raw.generatedAt contains an invalid date string, new Date(report.generatedAt).toISOString() will throw a RangeError later in the rendering process (line 562), crashing the build script. It is safer to validate the date string during normalization and fall back to the current date if it's invalid.
generatedAt: (function(d) { return isNaN(new Date(d).getTime()) ? new Date().toISOString() : d; })(raw.generated_at || raw.generatedAt || new Date().toISOString()),| for (const entry of readdirSync(CHANGE_REPORTS_DIR, { withFileTypes: true })) { | ||
| if (!entry.isDirectory()) continue; | ||
| const reportPath = join(CHANGE_REPORTS_DIR, entry.name, 'report.json'); | ||
| if (!existsSync(reportPath)) continue; | ||
| reports.push(normalizeChangeReport(entry.name, readJson(reportPath))); | ||
| } |
There was a problem hiding this comment.
The build script will crash if any report.json file is malformed or fails validation in normalizeChangeReport. Wrapping this call in a try-catch block and logging a warning allows the build to continue for other valid reports, providing a more robust user experience.
for (const entry of readdirSync(CHANGE_REPORTS_DIR, { withFileTypes: true })) {
if (!entry.isDirectory()) continue;
const reportPath = join(CHANGE_REPORTS_DIR, entry.name, 'report.json');
if (!existsSync(reportPath)) continue;
try {
reports.push(normalizeChangeReport(entry.name, readJson(reportPath)));
} catch (e) {
console.warn(` WARN: Failed to process change report ${entry.name}: ${e.message}`);
}
}| <a class="shot" href="${href}"> | ||
| <div class="shot-label"><strong>${escapeHtml(label)}</strong><span>${escapeHtml(shot.caption || '')}</span></div> | ||
| <img src="${href}" alt="${escapeHtml(shot.alt || shot.caption || label)}" loading="lazy" /> |
There was a problem hiding this comment.
The href generated by reportAssetHref should be HTML-escaped before being used in attributes. While reportAssetHref handles URL encoding for local paths, full URLs provided in the report JSON might contain characters like & or " that could break the HTML structure or lead to attribute injection.
<a class="shot" href="${escapeHtml(href)}">
<div class="shot-label"><strong>${escapeHtml(label)}</strong><span>${escapeHtml(shot.caption || '')}</span></div>
<img src="${escapeHtml(href)}" alt="${escapeHtml(shot.alt || shot.caption || label)}" loading="lazy" />![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a66b1efdb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const audiences = asArray(configuredAudiences).map(normalizeAudience).filter(Boolean); | ||
| if (audiences.length === 0) throw new Error(`Invalid report.json for ${id}: at least one audience is required`); | ||
| return { | ||
| id: slugify(raw.id || id), |
There was a problem hiding this comment.
Do not derive report.id from arbitrary JSON when assets are still read from change-reports/<folder>/.... If report.json sets an id different from its directory name, generated image links resolve to change-reports/<json-id>/... and 404, so before/after evidence disappears in the persona pages. This also allows two different folders with the same JSON id to collide in one output directory.
Useful? React with 👍 / 👎.
| <div class="metric"><strong>${report.changes.length}</strong><span>changes to review</span></div> | ||
| <div class="metric"><strong>${escapeHtml(report.status)}</strong><span>report status</span></div> | ||
| <div class="metric"><strong>${escapeHtml(report.route || 'n/a')}</strong><span>route / flow</span></div> | ||
| <div class="metric"><strong>${escapeHtml(new Date(report.generatedAt).toISOString().slice(0, 10))}</strong><span>generated</span></div> |
There was a problem hiding this comment.
Avoid calling toISOString() on unvalidated report.generatedAt. A malformed generated_at value in report.json produces RangeError: Invalid time value, which aborts report generation instead of falling back to a safe date display; one bad report can therefore break persona report output entirely.
Useful? React with 👍 / 👎.
1 participant
What
Why
Testing
ShipGuard Change Report
visual-tests/_results/change-reports/<report-id>/source report, or this PR is not UI-visible.visual-tests/_results/persona-reports/<report-id>/HTML is committed when the PR needs before/after review.visual-tests/_results/review.htmlandvisual-tests/_results/.server.pidare not committed.