evals: contain the GitHub Checks availability read in a boundary - #3873
Conversation
`useGithubChecksAvailability` throws by design — the backend refuses,
rather than answers, for a caller it will not serve. The suite settings
sheet called it directly in `SuiteIterationsView`, so every refusal took
the whole /evals suite page down instead of hiding one section.
Move the read into `SuiteGithubChecksSettingsSection`, which owns the
section chrome too, and wrap it in `ErrorBoundary fallback={null}`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
GitHub Actions
Inactive
dosubot
Bot
added
size:M
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Walkthrough
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: 2
🤖 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
`@mcpjam-inspector/client/src/components/evals/__tests__/suite-iterations-github-checks.test.tsx`:
- Around line 90-118: The renderSettingsSheet helper currently hardcodes an
organization identifier, so it does not cover missing values. Update
renderSettingsSheet to accept and pass an optional organizationId, then add null
and empty-string test cases verifying the settings sheet remains visible, GitHub
Checks stays hidden, and no boundary error is reported while preserving the
existing valid-identifier coverage.
In `@mcpjam-inspector/client/src/components/evals/suite-iterations-view.tsx`:
- Around line 1653-1659: Reset the suite_github_checks ErrorBoundary when
organizationId changes by keying it with organizationId or using the component’s
reset-key support, so SuiteGithubChecksSettingsSection remounts after a rejected
organization becomes valid. Add a regression test covering that organization
transition while SuiteIterationsView remains mounted.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 26a3f7e1-f253-4574-975b-f99135d231c6
📒 Files selected for processing (3)
mcpjam-inspector/client/src/components/evals/__tests__/suite-iterations-github-checks.test.tsxmcpjam-inspector/client/src/components/evals/suite-iterations-view.tsxmcpjam-inspector/client/src/hooks/useGithubChecksSettings.ts
| function renderSettingsSheet() { | ||
| return render( | ||
| <SuiteIterationsView | ||
| suite={baseSuite} | ||
| cases={[]} | ||
| iterations={[]} | ||
| allIterations={[]} | ||
| runs={[]} | ||
| runsLoading={false} | ||
| aggregate={null} | ||
| onRerun={vi.fn()} | ||
| onCancelRun={vi.fn()} | ||
| onDelete={vi.fn()} | ||
| onDeleteRun={vi.fn()} | ||
| onDirectDeleteRun={vi.fn().mockResolvedValue(undefined)} | ||
| connectedServerNames={new Set()} | ||
| canDeleteSuite | ||
| rerunningSuiteId={null} | ||
| cancellingRunId={null} | ||
| deletingSuiteId={null} | ||
| deletingRunId={null} | ||
| availableModels={[]} | ||
| organizationId="org-1" | ||
| projectId="project-1" | ||
| route={{ type: "suite-edit", suiteId: "suite-1" }} | ||
| navigation={noopNav} | ||
| /> | ||
| ); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Cover missing organization identifiers.
organizationId is optional, but renderSettingsSheet always passes "org-1". Make the helper accept an organization identifier. Add null and empty-string cases. Verify that the settings sheet remains visible, GitHub Checks stays hidden, and no boundary error is reported.
As per coding guidelines, “Tests should cover the happy path, validation errors, error handling, and edge cases such as null or empty values.”
🤖 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
`@mcpjam-inspector/client/src/components/evals/__tests__/suite-iterations-github-checks.test.tsx`
around lines 90 - 118, The renderSettingsSheet helper currently hardcodes an
organization identifier, so it does not cover missing values. Update
renderSettingsSheet to accept and pass an optional organizationId, then add null
and empty-string test cases verifying the settings sheet remains visible, GitHub
Checks stays hidden, and no boundary error is reported while preserving the
existing valid-identifier coverage.
Source: Coding guidelines
Internal previewPreview URL: https://mcp-inspector-pr-3873.up.railway.app |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Two review findings on the boundary added here:
The boundary holds its error state forever once tripped, and `fallback={null}`
exposes no reset. The org id comes from client state, so the exact failure this
PR handles — a stale org id — would hide the section for the rest of the
session even after the id corrects itself. Keying the boundary by org id
remounts it when the id changes, which re-asks availability.
The two throwing tests exercise the real componentDidCatch, which logs the full
error and component stack on every passing run. Silenced via a console.error
spy, restored after each case.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
dosubot
Bot
removed
the
size:M
dosubot
Bot
added
the
size:L
3 participants
The bug
Sentry,
prod, release2.35.0, first seen ~3h after that release: 9 events / 4 users, all on/evals/suite/<id>, all the same shape.The users lose the whole evals suite page — the global boundary catches it and swaps the page for an error state.
Why it throws
getGithubChecksSettingsAvailabilityis asignedInQuerythat runsrequireOrgRole(..., 'member')before it answers. It refuses rather than answers for a caller it will not serve, and that refusal is deliberate, documented backend behaviour: answeringdisabledto a non-member would confirm the org exists. Three ordinary situations produce a throw:signedInQueryrejects guests at the boundaryThe org id in question comes from
useEvalTabContext→scopedProject?.organizationId, i.e. the project's org as held in client state, so a stale or mismatched value lands in the same throw.The message reads only
Server Errorbecause Convex scrubs non-ConvexErrormessages in production. The PostHog path is not involved:isOrganizationFlagEnabledcatches everything and returnsfalse.The backend is correct here and is unchanged by this PR.
Why it crashed the page
useQueryre-throws query errors during render, so a hook called inSuiteIterationsViewitself fails the whole subtree. The hook's own docstring already said callers must treat a throw as "unavailable" and wrap it in anErrorBoundary— and the two settings call sites (IntegrationsRoute,GithubChecksRoute) do exactly that. The call site added to the suite settings sheet in #3745 did not, which is why Sentry's suspect commit is right even though the crash only started with the 2.35.0 release.The fix
Move the availability read out of
SuiteIterationsViewand into a newSuiteGithubChecksSettingsSection, which owns the section chrome as well, and wrap that component in<ErrorBoundary name="suite_github_checks" fallback={null}>.A refused beta gate now means "no section", never "no page". The boundary still reports to Sentry (
reportBoundaryErrorruns regardless of which fallback renders), so nothing is lost from telemetry — the errors will keep arriving, now taggedreact_boundary:suite_github_checksand no longer taking a page down. That is also how we will confirm which of the three refusals these four users are hitting; the Convex prod logs for the request ids carry the real message.Also updates the hook's docstring: the boundary is a requirement of every call site, not defence in depth, and the previously named call sites had been renamed.
Tests
New
client/src/components/evals/__tests__/suite-iterations-github-checks.test.tsx— four cases over the suite settings sheet:suite_github_checksdisabled→ section hidden, nothing reportedenabled→ section rendersExisting
suite-iterations-master-detail,suite-github-checks-sectionandcomponents/settingssuites pass unchanged (55 tests).tsc --noEmitclean for the touched files.🤖 Generated with Claude Code
Summary by cubic
Prevents the evals suite page from crashing by isolating the GitHub Checks availability read inside a section wrapped in an error boundary. The boundary is now keyed by org id so stale org state doesn’t hide the section for the whole session.
SuiteGithubChecksSettingsSectionand wrapped it with<ErrorBoundary name="suite_github_checks" fallback={null}>; returns null when unavailable.suite_github_checkstag; only the section is suppressed.useGithubChecksAvailabilitydocs to require a boundary at every call site; tests cover enabled/disabled/throw and silence boundary logs.Written for commit e8af2ca. Summary will update on new commits.