fix: PR #905 レビュー指摘のフォローアップ#908
Conversation
Implement CodeRabbit/Codex feedback for admin errors UX, Sentry scrubbing, GitHub Actions analyze-error hardening, Claude JSON parsing, and related tests. Co-authored-by: Cursor <cursoragent@cursor.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (22)
📝 WalkthroughWalkthroughThis PR strengthens Claude JSON parsing and tests, escapes GitHub Action annotation payloads, trims env values, cancels duplicate analyze workflows, updates admin filter/UI and SSE behavior, coalesces GitHub App token refreshes, scrubs Sentry request URLs, and improves several tests and comments. ChangesClaude Analysis Workflow Robustness
Admin Error List Management and UI
Security and Infrastructure Improvements
Test Suite Improvements
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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: 332b89e44d
ℹ️ 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".
| * @returns {string} | ||
| */ | ||
| function stripMarkdownJsonFence(raw) { | ||
| const m = raw.match(/```(?:json)?\s*([\s\S]*?)```/im); |
There was a problem hiding this comment.
stripMarkdownJsonFence currently matches the first triple-backtick block regardless of language, because (?:json)? is optional. If Claude returns any non-JSON fenced snippet before the actual payload (for example a ```text block in a preamble), this function discards the real response body and parseAndValidate fails even when valid analysis JSON is present later. This can cause the analyze workflow to reject otherwise valid runs and skip callback/issue processing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed: stripMarkdownJsonFence now matches only explicit json-labeled fences (opening delimiter plus json label). Unlabeled fences are left untouched so we do not strip arbitrary code blocks. Added regression coverage in schema.test.mjs.
![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 several improvements across the automation scripts, admin UI, and server-side logic. Key changes include a more robust JSON extraction mechanism for AI responses, escaping for GitHub Action annotations, and the implementation of request coalescing for GitHub App installation tokens to prevent redundant API calls. In the admin interface, error filtering and pluralization support were improved. On the server, Sentry event scrubbing was enhanced to redact UUIDs and opaque path segments. Feedback was provided regarding the Sentry URL scrubbing logic, specifically noting that the use of the URL constructor may fail on relative paths and that the regex for opaque IDs might inadvertently mask long filenames.
| const u = new URL(url); | ||
| u.search = ""; | ||
| const segments = u.pathname.split("/").map((seg) => { | ||
| if (seg.length === 0) return seg; | ||
| if (/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i.test(seg)) { | ||
| return "[uuid]"; | ||
| } | ||
| if (/^[A-Za-z0-9._~-]{24,}$/.test(seg)) { | ||
| return FILTERED; | ||
| } | ||
| return seg; | ||
| }); | ||
| u.pathname = segments.join("/") || "/"; | ||
| return u.toString(); |
There was a problem hiding this comment.
new URL(url) は絶対 URL を期待するため、相対パス（例: /api/pages/...）が渡された場合に TypeError を投げ、結果が一律で [Filtered] になってしまいます。Sentry SDK から送られる URL が常に絶対 URL であるとは限らないため、ベース URL を指定して相対パスも解析できるようにするのが安全です。
また、不透明な ID をマスクする正規表現 /^[A-Za-z0-9._~-]{24,}$/ は、ドットを含む長いファイル名（例: pageAccessService.test.ts）もマスクしてしまう可能性があるため、ドットを除外するか閾値を調整することを検討してください。
try {
const isAbsolute = /^https?:\/\//i.test(url);
const u = new URL(url, "http://n");
u.search = "";
u.hash = "";
const segments = u.pathname.split("/").map((seg) => {
if (seg.length === 0) return seg;
if (/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i.test(seg)) {
return "[uuid]";
}
if (/^[A-Za-z0-9_~-]{24,}$/.test(seg)) {
return FILTERED;
}
return seg;
});
u.pathname = segments.join("/") || "/";
return isAbsolute ? u.toString() : u.pathname;There was a problem hiding this comment.
Addressed: scrubRequestUrl now resolves relative paths against a synthetic origin before pathname scrubbing, so relative SDK URLs are scrubbed instead of collapsing to [Filtered]. Return type is string | undefined to satisfy Sentry typings (fixes API Typecheck). Added sentry.test.ts coverage for a relative URL.
Fix API typecheck for scrubRequestUrl, scrub relative request URLs with a synthetic base, and only unwrap json-labeled markdown fences in Claude analyze parsing. Co-authored-by: Cursor <cursoragent@cursor.com>
|
レビューコメントへの対応をコミットしました（0b83c79a）。API Typecheck の TS2322、Sentry の相対 URL スクラブ、Claude analyze の JSON フェンス抽出を修正済みです。最新の変更に対する再レビューをお願いします。 @coderabbitai review |
|
🐇 ✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
admin/src/api/admin.test.ts (1)
316-316:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate the test assertion to verify
encodeURIComponentencoding, consistent with the implementation.The
patchApiErrorStatusimplementation correctly usesencodeURIComponent(id)when building the path (line 481). Update the test assertion on line 316 to verify this encoding behavior:expect(adminFetch).toHaveBeenCalledWith(`/api/admin/errors/${encodeURIComponent(sampleErrorRow.id)}`, {While
sampleErrorRow.idis a plain UUID that yields the same string after encoding, this change documents and verifies the defensive encoding behavior consistent with the implementation and thegetApiErrorByIdtest pattern.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@admin/src/api/admin.test.ts` at line 316, The test assertion in admin.test.ts should mirror the implementation's defensive encoding: update the expect call that checks adminFetch to use encodeURIComponent(sampleErrorRow.id) in the URL; specifically change the assertion that references `/api/admin/errors/${sampleErrorRow.id}` to `/api/admin/errors/${encodeURIComponent(sampleErrorRow.id)}` so it matches patchApiErrorStatus's use of encodeURIComponent(id) and the getApiErrorById test pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/actions/claude-analyze/schema.mjs:
- Around line 125-129: The root-payload detection in looksLikeAnalysisPayload is
too strict/type-based and can skip early rejection when keys exist but types are
wrong; change the check to detect the shape by key presence (use
Object.prototype.hasOwnProperty.call(o, "severity") &&
Object.prototype.hasOwnProperty.call(o, "ai_summary")) so it returns true only
when those keys exist, and move/keep type validation later where the payload is
consumed to fail fast on malformed types; apply the same key-presence change to
the duplicate detection at the second occurrence (the block around lines
181-183) so both places test for key presence rather than immediate type
matching.
In `@admin/src/pages/errors/ErrorDetailDialog.tsx`:
- Around line 75-79: The pending state reset only runs inside the onOpenChange
handler so clicking the Cancel button (which calls onClose directly) doesn't
clear it; update ErrorDetailDialog to centralize clearing by ensuring onClose
always clears pending state—e.g., create/replace the existing onClose handler
used by the Cancel button with a wrapper that calls setPendingFor(null) then
calls the original onClose (and remove duplicate clearing from onOpenChange or
also call the wrapper there); apply the same change for the other occurrence
around the code referenced at lines 197-199 so every close path (overlay, ESC,
Cancel button) clears pending state.
---
Outside diff comments:
In `@admin/src/api/admin.test.ts`:
- Line 316: The test assertion in admin.test.ts should mirror the
implementation's defensive encoding: update the expect call that checks
adminFetch to use encodeURIComponent(sampleErrorRow.id) in the URL; specifically
change the assertion that references `/api/admin/errors/${sampleErrorRow.id}` to
`/api/admin/errors/${encodeURIComponent(sampleErrorRow.id)}` so it matches
patchApiErrorStatus's use of encodeURIComponent(id) and the getApiErrorById test
pattern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 63e5c21d-fb7c-4472-8e71-cbeb1367a3ac
📒 Files selected for processing (21)
.github/actions/claude-analyze/__tests__/schema.test.mjs.github/actions/claude-analyze/action.yml.github/actions/claude-analyze/autoIssueRunner.mjs.github/actions/claude-analyze/escapeGithubAnnotation.mjs.github/actions/claude-analyze/schema.mjs.github/workflows/analyze-error.ymladmin/src/api/admin.test.tsadmin/src/i18n/locales/en/nav.jsonadmin/src/i18n/locales/ja/nav.jsonadmin/src/pages/errors/ErrorDetailDialog.tsxadmin/src/pages/errors/ErrorsContent.tsxadmin/src/pages/errors/useApiErrors.tsserver/api/src/__tests__/routes/admin/errors.test.tsserver/api/src/__tests__/routes/pageSnapshots.test.tsserver/api/src/__tests__/routes/pages.test.tsserver/api/src/__tests__/routes/syncPages.test.tsserver/api/src/__tests__/routes/webhooks/sentry.test.tsserver/api/src/__tests__/services/pageAccessService.test.tsserver/api/src/lib/githubAppAuth.tsserver/api/src/lib/sentry.test.tsserver/api/src/lib/sentry.ts
Detect analysis root payload by own keys for fast schema failures; reset pending status on Cancel in ErrorDetailDialog. Co-authored-by: Cursor <cursoragent@cursor.com>
|
CodeRabbit の未対応スレッド 2 件を反映しました（d0b0d698）。looksLikeAnalysisPayload のルート判定強化と、ErrorDetailDialog のキャンセル経路での pending リセットです。 @coderabbitai review |
|
🐇 ✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
admin/src/pages/errors/ErrorDetailDialog.test.tsx (1)
108-151: ⚡ Quick winWell-structured test for cancel-clears-pending-state behavior.
The test correctly verifies that when a user makes a pending status change and clicks Cancel, that draft is discarded when the dialog reopens with the same row. The bilingual comments, explicit typing, and proper use of async user-event interactions align with coding guidelines.
💡 Optional: Consider additional test coverage
The current test suite has one focused test case. You may want to add tests for:
- Save button click (verify
onUpdateStatusis called with new status)- Outside-click behavior (the mock includes
data-testid="outside-close")- Error state handling when
saveErroris not null- Disabled state when
saving={true}These would provide more complete coverage of the dialog's state management, though the current test successfully validates the specific cancel behavior mentioned in the PR objectives.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@admin/src/pages/errors/ErrorDetailDialog.test.tsx` around lines 108 - 151, Add complementary tests to cover save, outside-click, error, and disabled-saving behaviors: (1) add a test that selects a new status via userEvent.selectOptions on the "status-select" then clicks the save button and asserts onUpdateStatus was called with the chosen value; (2) add a test that clicks the element with data-testid "outside-close" and asserts onClose is invoked to cover outside-click closing; (3) add a test that rerenders ErrorDetailDialog with saveError set (non-null) and asserts the error message or error UI is rendered when row and saveError are present; (4) add a test that renders with saving={true} and asserts controls (status-select and save button) are disabled/grayed out and that onUpdateStatus is not called when attempting to save; use the existing test patterns, userEvent interactions, and props (row, onClose, onUpdateStatus, saving, saveError) and reference the "status-select" test id and button labels to locate elements.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@admin/src/pages/errors/ErrorDetailDialog.test.tsx`:
- Around line 108-151: Add complementary tests to cover save, outside-click,
error, and disabled-saving behaviors: (1) add a test that selects a new status
via userEvent.selectOptions on the "status-select" then clicks the save button
and asserts onUpdateStatus was called with the chosen value; (2) add a test that
clicks the element with data-testid "outside-close" and asserts onClose is
invoked to cover outside-click closing; (3) add a test that rerenders
ErrorDetailDialog with saveError set (non-null) and asserts the error message or
error UI is rendered when row and saveError are present; (4) add a test that
renders with saving={true} and asserts controls (status-select and save button)
are disabled/grayed out and that onUpdateStatus is not called when attempting to
save; use the existing test patterns, userEvent interactions, and props (row,
onClose, onUpdateStatus, saving, saveError) and reference the "status-select"
test id and button labels to locate elements.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f89af5fc-3b8d-4e8b-bbbd-a0129344c2bd
📒 Files selected for processing (4)
.github/actions/claude-analyze/__tests__/schema.test.mjs.github/actions/claude-analyze/schema.mjsadmin/src/pages/errors/ErrorDetailDialog.test.tsxadmin/src/pages/errors/ErrorDetailDialog.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- admin/src/pages/errors/ErrorDetailDialog.tsx
- .github/actions/claude-analyze/schema.mjs
1 participant
概要 / Summary
PR #905（develop → main）に付いた Bot レビュー（CodeRabbit / Codex）のうち、対応推奨だった項目をまとめて修正します。
変更内容（対応したコメントの対応関係）
sentry_issue_idで古い run をキャンセル（cancel-in-progress: true）requireEnvの trim、JSON 抽出の頑健化（括弧バランス + ルートっぽいオブジェクトはスキーマエラーを即時化）all/__any__整合、ダイアログ閉鎖時の pending リセット、SSE でフィルタ外になった行を一覧から除去、英語 nav の複数形 ARIA、getApiErrorByIdの URL エンコード検証強化scrubSentryEventでrequest.urlのクエリ除去・パス成分のマスク、SSE テストのタイムアウト、pageAccessServiceテストのフィクスチャ修正、各種テストコメントの日英併記、sentry webhook テストの fetch stub を try/finally で確実に復元意図的に見送ったもの（PR #905 のスレッドへインライン返信済みの内容と整合）:
isNewを upsert と原子的にする件 → コード上のトレードオフとして既に文書化済みGET/PATCH /api/admin/errors/:idの成功レスポンスキーerrorの改名 → admin クライアント契約との同期が必要なため別検討テスト / Test plan
node --test .github/actions/claude-analyze/__tests__/schema.test.mjsserver/api: 関連 vitest（sentry、admin errors、webhooks、sentry.test、pageAccess、pages、pageSnapshots、syncPages）admin:admin.test.ts,useApiErrors.test.tsRelated: #905
Made with Cursor
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests