fix: resolve React Doctor errors and restore its PR baseline - #9488
Open
sriramveeraghanta wants to merge 3 commits into
Open
fix: resolve React Doctor errors and restore its PR baseline#9488sriramveeraghanta wants to merge 3 commits into
sriramveeraghanta wants to merge 3 commits into
Conversation
The React Doctor check on PR #9160 reported 266 issues across 130 files. Most of that is a reporting artifact: the workflow's `actions/checkout` step used the default shallow clone, so React Doctor had no merge base to diff against and fell back to listing every pre-existing issue in every changed file rather than only what the PR introduced. Add `fetch-depth: 0` so the comparison works. Also point the `push` trigger at `preview` — the repo's default branch — instead of `main`, which does not exist here, so the health-score trend never ran. Fix the 7 genuine errors it surfaced: - use-keypress: `callback` sat in the effect deps while every one of the 10 call sites passes an inline arrow, so the document listener was torn down and re-added on every render. Latch the callback in a ref and key the subscription on `key` alone. This clears `no-effect-with-fresh-deps` at create-root.tsx:102 and create-project-modal.tsx:64 at the source. - estimates/points/preview: the dblclick listener was added with no cleanup, so listeners accumulated on every toggle. Add the matching removeEventListener. - issues/header, calendar/issue-block, pages/editor/editor-body: guard `window` reads that run during render with `typeof window !== "undefined"`, matching the pattern already used elsewhere in web and admin. The editor case previously relied on the surrounding try/catch swallowing a ReferenceError on the server. Also clears the five pre-existing oxlint warnings in calendar/issue-block that the repo's `--deny-warnings` pre-commit gate blocks on once the file is touched: rename a shadowed `issue` parameter, and mark two presentational wrapper divs with `role="presentation"` — CustomMenu already wraps the first in a real <button>, and the second exists only to stop click propagation to the surrounding ControlLink. Verified: `turbo run check:types --filter=web` passes (11/11 tasks), `oxlint --deny-warnings` reports 0 warnings and 0 errors on the changed files. Claude-Session: https://claude.ai/code/session_01Hrr1nfNBiyC256drHM8BbT
Contributor
📝 WalkthroughWalkthroughThe PR changes React Doctor to scan ChangesWeb and CI safeguards
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
React Doctor found no new issues. 🎉 Reviewed by React Doctor for commit |
The only issue React Doctor reports against this branch. `./ai` re-exports both menu.tsx and ask-pi-menu.tsx, so importing through it pulls the ask-pi menu into the page editor bundle for a symbol that lives in menu.tsx. Pre-existing on preview rather than introduced here, but it is a one-line fix in a file this branch already touches. Claude-Session: https://claude.ai/code/session_01Hrr1nfNBiyC256drHM8BbT
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/workflows/react-doctor.yml:
- Around line 49-55: Update the actions/checkout@v5 step to set
persist-credentials to false while preserving fetch-depth: 0, so React Doctor
uses its own action credentials rather than the persisted checkout token.
In `@apps/web/core/components/issues/header.tsx`:
- Around line 58-60: Update the SPACE_APP_URL initialization to avoid reading
window.location.origin during the initial client render when SPACE_BASE_URL is
empty. Use the same stable relative base URL for SSR and the first client
render, or defer applying the origin-dependent URL until after mount, while
preserving configured external base URLs and the existing SPACE_BASE_PATH
behavior.
In `@apps/web/core/components/issues/issue-layouts/calendar/issue-block.tsx`:
- Around line 61-62: Update handleIssuePeekOverview to pass
workspaceSlug?.toString() to handleRedirection, matching the workItemLink
behavior and preserving safe handling when the optional route parameter is
absent.
In `@apps/web/core/components/pages/editor/editor-body.tsx`:
- Around line 190-194: The browser-dependent realtime setup in editor-body.tsx
and header.tsx causes SSR/client markup divergence. In editor-body.tsx, keep
PageContentLoader rendered through the first client render and initialize
realtimeConfig only after mount; in header.tsx, ensure the URL is identical
during SSR and the initial client render when SPACE_BASE_URL is empty, or defer
resolving window origin until after mount.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4fe6c388-e24a-446d-8ca1-12648b9b3bd7
📒 Files selected for processing (6)
.github/workflows/react-doctor.ymlapps/web/core/components/estimates/points/preview.tsxapps/web/core/components/issues/header.tsxapps/web/core/components/issues/issue-layouts/calendar/issue-block.tsxapps/web/core/components/pages/editor/editor-body.tsxapps/web/core/hooks/use-keypress.tsx
- react-doctor.yml: set persist-credentials: false on checkout. The token otherwise stays in .git/config for the third-party millionco/react-doctor step that runs next. fetch-depth: 0 has already fetched every ref the merge-base diff needs, and the action authenticates to the API through its own credentials, so nothing depends on the persisted git credential. - calendar/issue-block: pass workspaceSlug?.toString() to handleRedirection. The param is typed string | undefined and line 89 of the same file already optional-chains it; this call site would have thrown on a missing route param. Pre-existing, but it is on a line this branch already touches. Declined the two SSR/hydration findings: apps/web sets ssr: false in react-router.config.ts and ships a client-only bundle, so there is no server render to diverge from. The typeof window guards satisfy the React Doctor rule but are defensive only. Claude-Session: https://claude.ai/code/session_01Hrr1nfNBiyC256drHM8BbT
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Resolves the React Doctor findings reported on the v1.4.0 release PR (#9160), where the check surfaced 266 issues across 130 files (7 errors, 259 warnings, score 67/100).
Most of that count is a reporting artifact, not new debt. React Doctor diffs changed files against the merge base so it only reports what a PR introduces. The workflow's
actions/checkoutstep used the default shallow clone, so there was no merge base to diff against and the action fell back to listing every pre-existing issue in every changed file. On a release PR touching ~130 files, that means it reported the accumulated backlog of the whole web app. The action said so itself in a collapsed warning at the bottom of its comment.Two workflow fixes and the seven genuine errors:
.github/workflows/react-doctor.ymlfetch-depth: 0to the checkout step so the merge-base comparison works and future PRs only see what they actually introduce.pushtrigger frommaintopreview. This repo's default branch ispreview; there is nomain, so the health-score trend on the default branch has never run.core/hooks/use-keypress.tsx— 2 errors, fixed at the sourcecallbacksat in the effect's dependency array, but all 10 call sites pass an inline arrow or an unmemoized function. The documentkeydownlistener was therefore removed and re-added on every single render of every modal using it. The callback is now latched in a ref and the subscription keys onkeyalone.This clears
no-effect-with-fresh-depsat both reported sites —inbox/modals/create-modal/create-root.tsx:102andproject/create-project-modal.tsx:64— without touching either call site, and removes the same churn from the other eight callers.core/components/estimates/points/preview.tsx—effect-needs-cleanupThe
dblclicklistener was attached with no cleanup, so a fresh anonymous listener accumulated on the same element every time the edit/delete toggles changed. Added the matchingremoveEventListenerand hoisted the handler so add and remove reference the same function.Three unguarded browser globals read during render
issues/header.tsx:58,issue-layouts/calendar/issue-block.tsx:78, andpages/editor/editor-body.tsx:193,195readwindow.location/window.innerHeightduring render. Guarded withtypeof window !== "undefined", matching the pattern already used inissue-detail-quick-actions.tsx,workspace-details.tsx, and the admin auth forms.To be precise about the impact:
apps/websetsssr: falseinreact-router.config.tsand ships a client-only bundle, so none of these ever executed on a server and none of them were failing today. The guards satisfy the React Doctor rule and make the components safe to render server-side if that ever changes; they are defensive, not a live bug fix.CodeRabbit review
Applied two of four findings:
persist-credentials: falseon checkout (major). The token otherwise stays in.git/configfor the third-partymillionco/react-doctorstep that runs next.fetch-depth: 0has already fetched every ref the merge-base diff needs and the action authenticates to the API through its own credentials, so nothing depends on the persisted git credential.workspaceSlug?.toString()incalendar/issue-block. The parameter is typedstring | undefinedand line 89 of the same file already optional-chains it, so this call site would have thrown on a missing route param.Declined the two SSR/hydration findings — both are premised on the app server-rendering. It does not:
ssr: false, client-only bundle, no server render for the first client render to diverge from. DeferringrealtimeConfigbehind a post-mount state flag as suggested would add a render cycle and delay the collaboration socket to fix a mismatch that cannot occur.issue-layouts/calendar/issue-block.tsx— pre-existing lint warningsThe repo's
--deny-warningspre-commit gate blocks on five warnings that already existed in this file once it is touched. Cleared them: renamed a shadowedissueparameter, and marked two wrapper<div>srole="presentation". Neither is the interactive element —CustomMenurenders the first inside its own<button type="button">(packages/ui/src/dropdowns/custom-menu.tsx:251), and the second exists only to stop click propagation reaching the surroundingControlLink.Type of Change
Test Scenarios
pnpm turbo run check:types --filter=web— 11/11 tasks pass.pnpm exec oxlint --deny-warningson all changed files — 0 warnings, 0 errors.push.branches: ["preview"], checkout step{uses: actions/checkout@v5, with: {fetch-depth: 0}}.fetch-depthchange verifies itself: React Doctor's run on this PR should report only this diff rather than the whole backlog.Not covered by automated checks — worth a look during review:
useKeypress(project create, cycle, module, view, webhook, inbox create, issue peek, quick-add).References
preview.Summary by CodeRabbit