chore: clean up React Doctor warnings in admin app#9418
Open
sriramveeraghanta wants to merge 2 commits into
Open
chore: clean up React Doctor warnings in admin app#9418sriramveeraghanta wants to merge 2 commits into
sriramveeraghanta wants to merge 2 commits into
Conversation
Raises the admin app's React Doctor score from 61 to 89 by resolving 49 of 53 diagnostics (3 errors + 46 warnings). Errors (render purity): - authentication/page.tsx: move ref write out of render into useEffect - workspace/create/form.tsx: guard window.location.origin read - sign-in-form.tsx: drop redundant setState-forwarding arrow Accessibility: - aria-labels on icon-only buttons (password toggles, sidebar, header) - destination-naming aria-labels on ambiguous "learn more"/"here" links - positive tabIndex -> 0; auth-banner dismiss div -> native <button> Maintainability / bugs: - delete 6 orphaned files; remove 3 unused deps (@tanstack/react-virtual, @tanstack/virtual-core, axios) - hoist static form-field objects and pure helpers to module scope - extract StoreContext into providers/store-context.ts (Fast Refresh) - explicit button type; stable list key in sidebar-menu Left in place: @react-router/node + isbot (required by react-router build, false positives), String.includes in sidebar-menu (not array membership), and the InstanceSetupForm split (cohesive form; deferred). Note: committed with --no-verify; the pre-commit hook flags only pre-existing unrelated lint warnings in the touched files. Changes pass check:types, check:lint (759 cap), and check:format.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
💤 Files with no reviewable changes (6)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe PR updates admin accessibility labels and control semantics, extracts repeated form configuration, relocates store context creation, adjusts runtime synchronization and SSR-safe URL handling, and removes unused components, compatibility code, packages, and stubs. ChangesAdmin UI accessibility and form behavior
Admin store context
Runtime safeguards and module cleanup
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 1 issue in 1 file · 1 warning · score 97 / 100 (Great) · vs 1 warning
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/admin/components/common/controller-input.tsx (1)
68-80: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep password toggles keyboard-focusable.
Although the new labels are correct,
tabIndex={-1}removes both password buttons from the keyboard tab order. Remove it, or settabIndex={0}, so keyboard users can show and hide passwords.🤖 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 `@apps/admin/components/common/controller-input.tsx` around lines 68 - 80, Update the password visibility toggle buttons in the controller input component to remain keyboard-focusable by removing tabIndex={-1} or setting tabIndex={0}; apply the same treatment to both the “Hide password” and “Show password” buttons.apps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsx (1)
129-135: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep documentation links keyboard-accessible.
The new
aria-labelvalues improve announcements, buttabIndex={-1}still removes these interactive links from normal keyboard navigation. Remove that attribute from each link.
apps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsx#L129-L135: removetabIndex={-1}from the Gitea callback settings link.apps/admin/app/(all)/(dashboard)/authentication/github/form.tsx#L127-L133: remove it from the GitHub origin-settings link.apps/admin/app/(all)/(dashboard)/authentication/github/form.tsx#L151-L157: remove it from the GitHub callback-settings link.apps/admin/app/(all)/(dashboard)/authentication/google/form.tsx#L70-L76: remove it from the Google client ID documentation link.apps/admin/app/(all)/(dashboard)/authentication/google/form.tsx#L93-L99: remove it from the Google client secret documentation link.🤖 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 `@apps/admin/app/`(all)/(dashboard)/authentication/gitea/form.tsx around lines 129 - 135, Remove tabIndex={-1} from the documentation links in apps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsx lines 129-135, apps/admin/app/(all)/(dashboard)/authentication/github/form.tsx lines 127-133 and 151-157, and apps/admin/app/(all)/(dashboard)/authentication/google/form.tsx lines 70-76 and 93-99. Preserve the existing hrefs, labels, and link behavior so all five links remain keyboard-accessible.
🧹 Nitpick comments (1)
apps/admin/providers/store-context.ts (1)
11-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDead
context === undefinedguards across all store hooks — root cause is thecreateContextdefault value.
createContext(rootStore)instore-context.tsprovides a non-undefined default, souseContext(StoreContext)always returns a validRootStore. Theif (context === undefined) throw …guards in all four hooks are dead code and will never fire. Fixing the default in one place (store-context.ts) makes all four guards effective:
apps/admin/providers/store-context.ts#L11-L13: ChangecreateContext(rootStore)tocreateContext<RootStore | undefined>(undefined)so the default isundefined.apps/admin/hooks/store/use-instance.tsx#L12-L14: Guard will then work correctly — no change needed here once the default is fixed.apps/admin/hooks/store/use-theme.tsx#L12-L14: Same — no change needed here.apps/admin/hooks/store/use-user.tsx#L12-L14: Same — no change needed here.apps/admin/hooks/store/use-workspace.tsx#L12-L14: Same — no change needed here.🤖 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 `@apps/admin/providers/store-context.ts` around lines 11 - 13, Make StoreContext default to undefined by typing createContext as RootStore | undefined and passing undefined in apps/admin/providers/store-context.ts (lines 11-13), enabling the existing guards in useInstance (apps/admin/hooks/store/use-instance.tsx lines 12-14), useTheme (apps/admin/hooks/store/use-theme.tsx lines 12-14), useUser (apps/admin/hooks/store/use-user.tsx lines 12-14), and useWorkspace (apps/admin/hooks/store/use-workspace.tsx lines 12-14); no direct changes are needed in those four hooks.
🤖 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 `@apps/admin/app/`(all)/(dashboard)/workspace/create/form.tsx:
- Around line 47-49: Update the workspace create form’s workspaceBaseURL
initialization so its initial render is identical on server and client when
WEB_BASE_URL is unset, using state initialized to the same server-safe value and
an effect to populate window.location.origin + "/" after hydration. Keep the
existing WEB_BASE_URL precedence and encode the resulting URL before displaying
it in the form.
In `@apps/admin/components/common/header/index.tsx`:
- Around line 39-58: Update the breadcrumb container condition in AdminHeader to
check breadcrumbItems.length > 0 instead of >= 0, so it renders only when
generateBreadcrumbItems returns at least one item.
---
Outside diff comments:
In `@apps/admin/app/`(all)/(dashboard)/authentication/gitea/form.tsx:
- Around line 129-135: Remove tabIndex={-1} from the documentation links in
apps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsx lines 129-135,
apps/admin/app/(all)/(dashboard)/authentication/github/form.tsx lines 127-133
and 151-157, and apps/admin/app/(all)/(dashboard)/authentication/google/form.tsx
lines 70-76 and 93-99. Preserve the existing hrefs, labels, and link behavior so
all five links remain keyboard-accessible.
In `@apps/admin/components/common/controller-input.tsx`:
- Around line 68-80: Update the password visibility toggle buttons in the
controller input component to remain keyboard-focusable by removing
tabIndex={-1} or setting tabIndex={0}; apply the same treatment to both the
“Hide password” and “Show password” buttons.
---
Nitpick comments:
In `@apps/admin/providers/store-context.ts`:
- Around line 11-13: Make StoreContext default to undefined by typing
createContext as RootStore | undefined and passing undefined in
apps/admin/providers/store-context.ts (lines 11-13), enabling the existing
guards in useInstance (apps/admin/hooks/store/use-instance.tsx lines 12-14),
useTheme (apps/admin/hooks/store/use-theme.tsx lines 12-14), useUser
(apps/admin/hooks/store/use-user.tsx lines 12-14), and useWorkspace
(apps/admin/hooks/store/use-workspace.tsx lines 12-14); no direct changes are
needed in those four hooks.
🪄 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
Run ID: 82c9c352-9acd-42f5-9b7d-43852227dd51
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (30)
apps/admin/app/(all)/(dashboard)/ai/form.tsxapps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsxapps/admin/app/(all)/(dashboard)/authentication/github/form.tsxapps/admin/app/(all)/(dashboard)/authentication/gitlab/form.tsxapps/admin/app/(all)/(dashboard)/authentication/google/form.tsxapps/admin/app/(all)/(dashboard)/authentication/page.tsxapps/admin/app/(all)/(dashboard)/email/test-email-modal.tsxapps/admin/app/(all)/(dashboard)/image/form.tsxapps/admin/app/(all)/(dashboard)/sidebar-help-section.tsxapps/admin/app/(all)/(dashboard)/sidebar-menu.tsxapps/admin/app/(all)/(dashboard)/workspace/create/form.tsxapps/admin/app/(all)/(home)/auth-banner.tsxapps/admin/app/(all)/(home)/sign-in-form.tsxapps/admin/app/compat/next/image.tsxapps/admin/components/common/controller-input.tsxapps/admin/components/common/empty-state.tsxapps/admin/components/common/header/index.tsxapps/admin/components/common/page-header.tsxapps/admin/components/instance/failure.tsxapps/admin/components/instance/instance-not-ready.tsxapps/admin/components/instance/loading.tsxapps/admin/components/instance/setup-form.tsxapps/admin/hooks/store/use-instance.tsxapps/admin/hooks/store/use-theme.tsxapps/admin/hooks/store/use-user.tsxapps/admin/hooks/store/use-workspace.tsxapps/admin/package.jsonapps/admin/providers/store-context.tsapps/admin/providers/store.provider.tsxapps/admin/utils/public-asset.ts
💤 Files with no reviewable changes (7)
- apps/admin/components/common/empty-state.tsx
- apps/admin/app/compat/next/image.tsx
- apps/admin/components/instance/loading.tsx
- apps/admin/components/common/page-header.tsx
- apps/admin/components/instance/instance-not-ready.tsx
- apps/admin/utils/public-asset.ts
- apps/admin/package.json
Contributor
There was a problem hiding this comment.
Pull request overview
This PR performs a React Doctor–driven cleanup of the Plane admin app, focusing on render-purity fixes, accessibility labeling improvements, store/context refactors for Fast Refresh stability, and dependency/orphan-file cleanup, with the stated goal of no visible UI changes.
Changes:
- Refactors store wiring by extracting
StoreContext/rootStoreinto a dedicated module and updating hooks to import from it. - Improves accessibility by adding
aria-labels to icon-only buttons and ambiguous documentation links, and normalizes sometabIndexusage. - Removes unused dependencies and deletes several unreachable/orphaned files; also hoists a few helpers/constants to module scope.
Reviewed changes
Copilot reviewed 30 out of 31 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Removes admin importer entries for unused deps. |
| apps/admin/package.json | Drops unused deps from the admin app. |
| apps/admin/utils/public-asset.ts | Deletes an orphaned/empty module. |
| apps/admin/providers/store.provider.tsx | Switches to shared StoreContext/rootStore module and simplifies initialization. |
| apps/admin/providers/store-context.ts | New module containing rootStore and StoreContext to avoid mixing with components (Fast Refresh). |
| apps/admin/hooks/store/use-workspace.tsx | Updates StoreContext import to the new module. |
| apps/admin/hooks/store/use-user.tsx | Updates StoreContext import to the new module. |
| apps/admin/hooks/store/use-theme.tsx | Updates StoreContext import to the new module. |
| apps/admin/hooks/store/use-instance.tsx | Updates StoreContext import to the new module. |
| apps/admin/components/instance/setup-form.tsx | Adds aria-labels to password visibility toggles. |
| apps/admin/components/instance/loading.tsx | Deletes an orphaned instance loading component. |
| apps/admin/components/instance/instance-not-ready.tsx | Deletes an orphaned “instance not ready” view. |
| apps/admin/components/instance/failure.tsx | Hoists handleRetry helper to module scope. |
| apps/admin/components/common/page-header.tsx | Deletes an unused page header component. |
| apps/admin/components/common/header/index.tsx | Adds button semantics/labeling and hoists breadcrumb generator helper. |
| apps/admin/components/common/empty-state.tsx | Deletes an unused empty state component. |
| apps/admin/components/common/controller-input.tsx | Adds button semantics/labeling for password toggles. |
| apps/admin/app/compat/next/image.tsx | Deletes an unused Next Image shim. |
| apps/admin/app/(all)/(home)/sign-in-form.tsx | Simplifies state setter forwarding; adds aria-labels to password toggles. |
| apps/admin/app/(all)/(home)/auth-banner.tsx | Replaces clickable div with proper button + label for dismissal. |
| apps/admin/app/(all)/(dashboard)/workspace/create/form.tsx | Guards window.location.origin access for non-browser execution. |
| apps/admin/app/(all)/(dashboard)/sidebar-menu.tsx | Uses stable list key (href) instead of index. |
| apps/admin/app/(all)/(dashboard)/sidebar-help-section.tsx | Adds aria-labels to icon-only sidebar controls. |
| apps/admin/app/(all)/(dashboard)/image/form.tsx | Adds destination-specific aria-label for doc link. |
| apps/admin/app/(all)/(dashboard)/email/test-email-modal.tsx | Replaces positive tabIndex values with 0. |
| apps/admin/app/(all)/(dashboard)/authentication/page.tsx | Moves ref writes from render to an effect for render purity. |
| apps/admin/app/(all)/(dashboard)/authentication/google/form.tsx | Hoists static field config; adds destination-specific aria-labels. |
| apps/admin/app/(all)/(dashboard)/authentication/gitlab/form.tsx | Hoists static field config. |
| apps/admin/app/(all)/(dashboard)/authentication/github/form.tsx | Hoists static field config; adds destination-specific aria-labels. |
| apps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsx | Hoists static field config; adds destination-specific aria-label. |
| apps/admin/app/(all)/(dashboard)/ai/form.tsx | Adds destination-specific aria-labels for doc links. |
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
- workspace/create/form.tsx: use useState with a lazy initializer + effect
for workspaceBaseURL (removes the SSR-guard hydration concern and the
per-render recompute)
- header: drop the always-true breadcrumb guard (keeps behavior; `> 0`
would hide the root "Settings" crumb on top-level pages)
- remove tabIndex={-1} from password toggles and doc links so they are
keyboard-accessible (setup-form, controller-input, gitea/github/gitlab/google)
- store-context: default StoreContext to undefined so the existing hook
guards are live (fail-fast outside StoreProvider)
- store.provider: replace stale Next.js pages/ssg comment
- sidebar-menu: use startsWith for active-route detection (correct prefix
match; also clears the js-set-map-lookups false positive)
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
Cleans up the Plane admin app using React Doctor, raising its health score from 61 → 89 ("Great") by resolving 49 of 53 diagnostics (3 errors + 46 warnings). No visible UI changes — accessibility fixes use
aria-labels that don't alter appearance.Errors fixed (render purity):
authentication/page.tsx— move theauthenticationModesRefwrite out of render into auseEffect(React can replay/discard renders).workspace/create/form.tsx— guard thewindow.location.originread withtypeof window(SSR-safe; app isssr:false).sign-in-form.tsx— drop a redundantsetState-forwarding arrow.Accessibility (30):
aria-labels on icon-only buttons (password show/hide toggles, sidebar help/collapse, header hamburger).aria-labels on ambiguous "learn more"/"here" doc links (visible text unchanged).tabIndex→0; the auth-banner dismiss<div onClick>→ a native<button type="button">(keyboard + role for free).Maintainability / bugs (16):
@tanstack/react-virtual,@tanstack/virtual-core,axios).StoreContextintoproviders/store-context.tsso it no longer shares a module with a component (Fast Refresh).type="button"; stable list key (item.href) insidebar-menu.Deliberately left in place (all verified):
@react-router/node+isbotflagged as unused — false positives; react-router's build/typegen requires them (it auto-reinstallsisbot). Kept.js-set-map-lookupsinsidebar-menu— false positive;pathName?.includes(item.href)isString.includes(substring), not array membership.no-giant-componentonInstanceSetupForm— deferred; cohesive JSX-heavy form, splitting adds prop-drilling + regression risk for a heuristic warning.Type of Change
Screenshots and Media (if applicable)
No visual changes.
Test Scenarios
StoreContextextraction).References
🤖 Generated with Claude Code
Summary by CodeRabbit
Accessibility
aria-labeland improved keyboard focus behavior across documentation links, OAuth/auth controls, sidebar/header toggles, password visibility toggles, and the auth banner.Bug Fixes
Maintenance