FE: Cleanup lint warnings Part VI - #44741
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #44741 +/- ##
=======================================
Coverage 66.68% 66.68%
=======================================
Files 2652 2652
Lines 213674 213674
Branches 9698 9698
=======================================
Hits 142483 142483
Misses 58228 58228
Partials 12963 12963
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
There was a problem hiding this comment.
Pull request overview
This PR continues the frontend lint/typing cleanup effort by removing unnecessary return false statements, tightening some location prop typings, and improving error handling by avoiding catch (e: any) and using safer helpers (getErrorReason, hasStatusKey) for unknown error shapes.
Changes:
- Narrowed several
location: anyprops to the specific fields used (hash/query) in a few route components. - Replaced
catch (e: any)withcatch (e)and safer error extraction (String(...),getErrorReason,hasStatusKey). - Removed dead/unused returns (
return false) and an unused import.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/router/components/AuthenticatedRoutes/AuthenticatedRoutes.tsx | Narrows location typing to hash. |
| frontend/pages/ResetPasswordPage/ResetPasswordPage.tsx | Narrows location typing to query.token. |
| frontend/pages/RegistrationPage/RegistrationPage.tsx | Removes dead return false from page navigation guard. |
| frontend/pages/queries/live/screens/RunQuery.tsx | Removes dead returns; uses String(...) for safer caught error conversion. |
| frontend/pages/queries/edit/EditQueryPage.tsx | Drops : any in catch blocks; uses getErrorReason with unknown. |
| frontend/pages/policies/ManagePoliciesPage/components/PoliciesTable/PoliciesTableConfig.tsx | Adds TODO context around any header props typing. |
| frontend/pages/policies/live/screens/RunQuery.tsx | Removes dead returns; makes error shape checks safe before "message" in .... |
| frontend/pages/policies/edit/screens/QueryEditor.tsx | Switches to getErrorReason instead of deep property access into error payloads. |
| frontend/pages/ManageControlsPage/Variables/components/AddCustomVariableModal/AddCustomVariableModal.tsx | Uses hasStatusKey guard before checking error.status. |
| frontend/pages/LogoutPage/LogoutPage.tsx | Removes dead return renderFlash(...). |
| frontend/pages/LoginPage/LoginPage.tsx | Removes dead return false from SSO error path. |
| frontend/pages/hosts/ManageHostsPage/ManageHostsPage.tsx | Removes a lint suppression comment; removes dead return false. |
| frontend/pages/hosts/details/cards/Software/HostSoftwareTable/HostSoftwareTable.tsx | Removes unused isAndroid import. |
| frontend/pages/ConfirmSSOInvitePage/ConfirmSSOInvitePage.tsx | Narrows location typing to query.email/name. |
| frontend/pages/ApiOnlyUser/ApiOnlyUser.tsx | Removes dead return false in catch. |
| frontend/pages/admin/UserManagementPage/EditUserPage/EditUserPage.tsx | Narrows location typing to optional query.type. |
| frontend/pages/AccountPage/AccountSidePanel/AccountSidePanel.tsx | Removes dead return false in catch. |
| frontend/interfaces/errors.ts | Improves hasStatusKey typing by avoiding any cast. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| interface IManageHostsProps { | ||
| route: RouteProps; | ||
| router: InjectedRouter; | ||
| params: Params; | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| location: any; // no type in react-router v3 TODO: Improve this type | ||
| } |
| id: "selection", | ||
| // TODO: headerProps is `any` because local IHeaderProps is a simplified | ||
| // subset of react-table's HeaderProps. Fixing requires refactoring | ||
| // IDataColumn/IHeaderProps to align with react-table's actual types. |
WalkthroughThis PR refactors the frontend codebase to improve type safety and consistency. Changes include: replacing Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/pages/policies/live/screens/RunQuery.tsx (1)
165-188:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing fallback flash for non-object errors in catch path.
On Line [173], the generic flash now only runs when
campaignErroris an object withmessage. Primitive errors (e.g., string errors other than"resource already created") can silently teardown with no user feedback.💡 Proposed fix
- } catch (campaignError) { - if (campaignError === "resource already created") { + } catch (campaignError) { + if (campaignError === "resource already created") { renderFlash( "error", "A campaign with the provided query text has already been created" ); - } - - if ( + } else if ( typeof campaignError === "object" && campaignError !== null && "message" in campaignError ) { const { message } = campaignError as { message: string }; @@ } else { renderFlash("error", "Something has gone wrong. Please try again."); } + } else { + renderFlash("error", "Something has gone wrong. Please try again."); } return teardownDistributedQuery(); }🤖 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 `@frontend/pages/policies/live/screens/RunQuery.tsx` around lines 165 - 188, The catch block handling campaignError in RunQuery.tsx only shows a generic flash when campaignError is an object with a message, so primitive errors (e.g., strings other than "resource already created") produce no user feedback; update the catch in the async handler (the block referencing campaignError and renderFlash) to add a final fallback branch that calls renderFlash("error", "Something has gone wrong. Please try again.") for all other cases (including non-object/primitive errors) after the existing checks so any thrown value triggers a user-visible error.
🤖 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 `@frontend/pages/ConfirmSSOInvitePage/ConfirmSSOInvitePage.tsx`:
- Around line 16-18: Change the prop type for location.query to be optional
(location: { query?: { email?: string; name?: string } }) and update the
destructuring/reading of email and name in ConfirmSSOInvitePage (the place where
you currently destructure from location.query) to use optional chaining or a
guarded fallback (e.g., const { email, name } = location.query ?? {} or
reference location.query?.email / location.query?.name) so the code matches the
defensive pattern used elsewhere like EditUserPage.
---
Outside diff comments:
In `@frontend/pages/policies/live/screens/RunQuery.tsx`:
- Around line 165-188: The catch block handling campaignError in RunQuery.tsx
only shows a generic flash when campaignError is an object with a message, so
primitive errors (e.g., strings other than "resource already created") produce
no user feedback; update the catch in the async handler (the block referencing
campaignError and renderFlash) to add a final fallback branch that calls
renderFlash("error", "Something has gone wrong. Please try again.") for all
other cases (including non-object/primitive errors) after the existing checks so
any thrown value triggers a user-visible error.
🪄 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: 86d95b54-62ac-4f37-9712-87b97285e765
📒 Files selected for processing (18)
frontend/interfaces/errors.tsfrontend/pages/AccountPage/AccountSidePanel/AccountSidePanel.tsxfrontend/pages/ApiOnlyUser/ApiOnlyUser.tsxfrontend/pages/ConfirmSSOInvitePage/ConfirmSSOInvitePage.tsxfrontend/pages/LoginPage/LoginPage.tsxfrontend/pages/LogoutPage/LogoutPage.tsxfrontend/pages/ManageControlsPage/Variables/components/AddCustomVariableModal/AddCustomVariableModal.tsxfrontend/pages/RegistrationPage/RegistrationPage.tsxfrontend/pages/ResetPasswordPage/ResetPasswordPage.tsxfrontend/pages/admin/UserManagementPage/EditUserPage/EditUserPage.tsxfrontend/pages/hosts/ManageHostsPage/ManageHostsPage.tsxfrontend/pages/hosts/details/cards/Software/HostSoftwareTable/HostSoftwareTable.tsxfrontend/pages/policies/ManagePoliciesPage/components/PoliciesTable/PoliciesTableConfig.tsxfrontend/pages/policies/edit/screens/QueryEditor.tsxfrontend/pages/policies/live/screens/RunQuery.tsxfrontend/pages/queries/edit/EditQueryPage.tsxfrontend/pages/queries/live/screens/RunQuery.tsxfrontend/router/components/AuthenticatedRoutes/AuthenticatedRoutes.tsx
💤 Files with no reviewable changes (4)
- frontend/pages/AccountPage/AccountSidePanel/AccountSidePanel.tsx
- frontend/pages/ApiOnlyUser/ApiOnlyUser.tsx
- frontend/pages/hosts/details/cards/Software/HostSoftwareTable/HostSoftwareTable.tsx
- frontend/pages/LoginPage/LoginPage.tsx
| location: { | ||
| query: { email?: string; name?: string }; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Cross-check how this route is mounted and whether query is guaranteed.
rg -n -C3 'ConfirmSSOInvitePage|confirm-invite|invite_token' frontend/router frontend/pages
rg -n -C2 '\blocation\.query\b' frontend/pages/ConfirmSSOInvitePage frontend/router
# Expected:
# - If route plumbing always provides query object, current typing is acceptable.
# - Otherwise, make query optional and update reads to optional chaining/defaults.Repository: fleetdm/fleet
Length of output: 7791
🏁 Script executed:
# Check for react-router types/definitions in the codebase
fd -e ts -e tsx | xargs grep -l "react-router" | head -20
# Look at package.json to confirm react-router version
cat package.json | grep -A2 "react-router"
# Check ResetPasswordPage mentioned in the original comment
fd "ResetPasswordPage" -type f
# Look for location.query patterns in the codebase to see how it's commonly handled
rg "location\.query" frontend/ -A2 -B2 | head -40Repository: fleetdm/fleet
Length of output: 4868
🏁 Script executed:
# Find ResetPasswordPage to see the comparison mentioned in the original comment
fd "ResetPasswordPage" -type f
# Check package.json for more react-router type info
cat package.json | grep -A5 -B5 "react-router"
# Look at one of the pages that uses location.query to see how it's typed
cat -n frontend/pages/queries/details/QueryDetailsPage/QueryDetailsPage.tsx | head -50
# Check if there's a type definition for location in the codebase
rg "location:" frontend/ -A3 -B1 | head -60Repository: fleetdm/fleet
Length of output: 6985
🏁 Script executed:
# Get the full location interface definition from QueryDetailsPage
cat -n frontend/pages/queries/details/QueryDetailsPage/QueryDetailsPage.tsx | sed -n '47,60p'
# Find and check ResetPasswordPage
fd "ResetPasswordPage"
# Check a few more pages to see the pattern of how location.query is defined
rg "interface.*Props" frontend/pages/ConfirmSSOInvitePage -A10
# Look for any conditional checks on location.query in the codebase
rg "location\.query\?" frontend/ -B2 -A2 | head -50Repository: fleetdm/fleet
Length of output: 2064
🏁 Script executed:
# Check ResetPasswordPage to see the comparison mentioned in the original comment
cat -n frontend/pages/ResetPasswordPage/ResetPasswordPage.tsx | head -30
# Check the EditUserPage that uses optional chaining on location.query
cat -n frontend/pages/admin/UserManagementPage/EditUserPage/EditUserPage.tsx | head -60
# Check the full interface definition for EditUserPage
rg "interface.*EditUserPageProps" frontend/pages/admin/UserManagementPage/EditUserPage -A15Repository: fleetdm/fleet
Length of output: 5668
Make location.query optional and use optional chaining for defensive consistency.
Lines 16-18 define query as required, but the codebase inconsistently handles this—EditUserPage marks it optional and uses optional chaining (location.query?.type). While react-router v3 always provides the query object, matching the defensive pattern would improve consistency and code safety. Change to query?: { email?: string; name?: string } and update line 30 to guard the destructuring.
🤖 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 `@frontend/pages/ConfirmSSOInvitePage/ConfirmSSOInvitePage.tsx` around lines 16
- 18, Change the prop type for location.query to be optional (location: {
query?: { email?: string; name?: string } }) and update the
destructuring/reading of email and name in ConfirmSSOInvitePage (the place where
you currently destructure from location.query) to use optional chaining or a
guarded fallback (e.g., const { email, name } = location.query ?? {} or
reference location.query?.email / location.query?.name) so the code matches the
defensive pattern used elsewhere like EditUserPage.
Every change is either:
Summary by CodeRabbit