Document file-local sub-component pattern as preferred over render*() helpers

[nulmete]

Modified patterns.md to document that we'll prefer plain JSX components over render*() closure functions for inner components.

Testing

• QA'd all new/changed functionality manually

Summary by CodeRabbit

• Chores

  • Updated linting rules to add an override for JSX/TSX files that allows using functions and variables before declaration while retaining error severity.

• Refactor

  • Extracted the targets counting UI/logic into a dedicated component to improve structure and props-driven rendering.

[codecov]

Codecov Report

❌ Patch coverage is 83.33333% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.99%. Comparing base (e2df64b) to head (76a6359).
⚠️ Report is 575 commits behind head on main.

Files with missing lines	Patch %	Lines
frontend/components/LiveQuery/SelectTargets.tsx	83.33%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #46213      +/-   ##
==========================================
+ Coverage   66.83%   66.99%   +0.15%     
==========================================
  Files        2755     3254     +499     
  Lines      220204   223041    +2837     
  Branches    10916    11895     +979     
==========================================
+ Hits       147176   149423    +2247     
- Misses      59735    60319     +584     
- Partials    13293    13299       +6     

Flag	Coverage Δ
frontend	57.81% <83.33%> (+2.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[nulmete]

perhaps we can avoid passing baseClass and just have this component have its own top/base class

[sgress454]

It looks like it's passed in to the main component, so I think we have to have it here if we want to stick with the pattern of namespacing classes by appending __my-class to the base.

[sgress454]

I don't think we need the explanation of why this pattern is preferred. At most I would add a sentence like "This improves readability and makes it easier to extract that component into a separate file if needed" to the first paragraph.

The note of

render*() helpers are still acceptable, especially in existing code — don't
refactor an entire page just to migrate. New code should prefer the
sub-component pattern.

would be more appropriate in a Claude rules file, but even then I would only add it if we find it doing unnecessary refactors.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: f49d64b8-ac90-4bfb-b49b-21ea779408a8

📥 Commits

Reviewing files that changed from the base of the PR and between ca68f37 and 76a6359.

📒 Files selected for processing (1)

• .eslintrc.js

---

Walkthrough

This PR relaxes the TypeScript ESLint rule by adding an override for *.tsx and *.jsx that sets @typescript-eslint/no-use-before-define to ["error", { functions: false, variables: false }]. It also refactors SelectTargets.tsx by replacing the inline renderTargetsCount() helper with a new typed TargetsCount component and updating the parent to render it with baseClass, isFetchingCounts, errorCounts, and counts.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description is incomplete. It mentions only testing completion but omits the required 'Related issue' field and lacks details about why changes to .eslintrc.js and SelectTargets.tsx were made.	Add the 'Related issue' field (e.g., 'Resolves #46213'), explain the relationship between the eslint configuration and component pattern changes, and provide details about the code refactoring in SelectTargets.tsx.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: documenting a preference for file-local JSX components over render*() helpers, which aligns with the actual changes made to patterns.md and demonstrated in the code changes.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch components-in-same-file-instead-of-closures

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[Copilot]

Pull request overview

This PR documents a preferred frontend pattern (“file-local sub-components” under the main component) and updates an existing LiveQuery component to follow that pattern, alongside an ESLint configuration change intended to support it.

Changes:

• Add a new documentation section recommending file-local sub-components over render*() helper closures.
• Refactor SelectTargets to replace a renderTargetsCount() helper with a file-local TargetsCount sub-component.
• Relax @typescript-eslint/no-use-before-define to permit referencing bottom-of-file components.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
frontend/docs/patterns.md	Documents the preferred file-local sub-component pattern.
frontend/components/LiveQuery/SelectTargets.tsx	Refactors a JSX helper closure into a file-local sub-component.
.eslintrc.js	Adjusts linting rules to allow referencing declarations before they appear in the file.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[RachelElysia]

Thanks for getting this going! I put this in draft since I think we need to chat about it more as a team.

I really think extracting TargetsCount does read better than the 60-line closure it replaced and a real solution about how we handle the long tail of render helpers. I ran a quick audit across frontend/ to ground the discussion, and I'd love a group discussion before we land the documented pattern.

Audit of render*() helpers in frontend (464 functions):

Stat	Lines
Mean	35.7
Median	22
StDev	50.6
Min/Max	3 / 631

Distribution:
0-5 lines: 2.6%
5-10 lines: 10.3%
10-20 lines: 29.7%
20-40 lines: 31.7%
40-80 lines: 17.2%
80-160 lines: 6.7%
160+ lines: 1.7%

A few things I wanted to share:

~72% of our render* helpers are under 40 lines, which feels like the zone where they exist precisely because sub-component extraction would cost more than it saves — closing over baseClass, isFetchingCounts, counts, etc. is the whole appeal. I wonder if recommending the sub-component pattern as the default might push the boilerplate onto a lot of small helpers where the tradeoff isn't quite there. I don't think I'm quite on board with that. The PR's example (~62 lines) sits in the long tail rather than the typical case, so it's a great extraction candidate — but it might not be the strongest argument for "always prefer this."

Re: placement - The rest of the codebase leans on declarations-before-use as the reading order, and putting sub-components below their usage means readers have to scroll past to see what it does which is a anti-pattern. Would it work to place the extracted sub-component above the main component instead? That would keep the helpers-above convention consistent and let us drop the ESLint override entirely, which I think is the part I'm most cautious about, it opts the whole JSX/TSX surface out of forward-reference protection, which feels like a wide change to enable one ordering choice.

A possible middle-ground for our patterns:

• <40 lines: keep as a const renderFoo = () => (...) closure, placed above return. Matches ~72% of current usage and what the codebase already does. Doesn't need extra props/etc extraction and likely will never be a true subcomponent moved to it's own file
• 40+ lines, or accepts explicit props: extract to a sub-component above the main component (no ESLint override needed).
• Reused, or pushes the file past a reasonable length: extract to its own file — at that point we're already paying the interface + props cost, so we may as well get real discoverability for it.

Here's the top 10 candidates Claude found if we want a follow-up cleanup pass (these are the ones the data really argues for componentizing or extracting to their own file):

Lines	Function	File
631	renderContent	pages/hosts/details/HostDetailsPage/HostDetailsPage.tsx
501	renderVitalsAlphabetically	pages/hosts/details/cards/Vitals/Vitals.tsx
347	renderDeviceUserPage	pages/hosts/details/DeviceUserPage/DeviceUserPage.tsx
292	renderEditableQueryForm	pages/queries/edit/components/EditQueryForm/EditQueryForm.tsx
244	renderPanel	components/AddHostsModal/PlatformWrapper/PlatformWrapper.tsx
227	renderForm	pages/admin/IntegrationsPage/cards/Calendars/Calendars.tsx
196	renderTable	pages/hosts/ManageHostsPage/ManageHostsPage.tsx
188	renderPolicyForm	pages/policies/edit/components/PolicyForm/PolicyForm.tsx
141	renderProfileAssignmentList	pages/hosts/details/modals/MDMStatusModal/MDMStatusModal.tsx
140	renderForm	pages/admin/ManageFleetsPage/TeamDetailsWrapper/TeamSettings/TeamSettings.tsx

Those are all well past the "should be a real component (or its own file)" line, and they'd be a great proving ground for an updated convention we settle on here.

Super appreciate you kicking this off and looking forward to another group chat on it!

[nulmete]

@RachelElysia thanks for your review!

For context, these were my main arguments/thoughts when I put this up:

• Ease of testing: if we want to test a sub-component in isolation we can just render it alone, with no need to render the entire tree. As you said, we don't want to extract every little piece to its own sub-component because it creates higher cognitive load, and that's not what we want. But if you look at HostDetailsPage for example:

  • it's not tested
  • if you wanted to introduce a test:
    • you'd probably have a ton of mocks and repeated code, especially if you wanted to write up different test cases for different combinations of variables
    • you wouldn't be able to do shallow rendering which might be a good fit for this particular page (so that you can test it on a high level, and then have more specific tests for those shallow-rendered sub-components)

• There are some specific cases where these render functions are wrapped with useCallback which is a code smell to me: components should already be stable references by definition. For example:

I don't know what specifically required useCallback in this case (it also makes the closure read more like a regular callback than UI code, which adds a small cognitive cost).

• This is more of a nitpick: I'm already familiar with the codebase but maybe if a new engineer gets onboarded they might mistake a render*() for something that performs a calculation due to its name not being PascalCase as a React component usually is.

~72% of our render* helpers are under 40 lines, which feels like the zone where they exist precisely because sub-component extraction would cost more than it saves — closing over baseClass, isFetchingCounts, counts, etc. is the whole appeal. I wonder if recommending the sub-component pattern as the default might push the boilerplate onto a lot of small helpers where the tradeoff isn't quite there.

I agree 👍 , and I'm not saying that we should choose one or the other just because one method produces less/more LOC than the other. For example, as it was pointed out in your comment above, those top 10 candidates that Claude brought up might deserve a quick look to determine how readable they are for us, and maybe split some of them up.

Would it work to place the extracted sub-component above the main component instead?

I think either approach makes sense. I wouldn't mind having it at the top -- IMHO that's even more readable than a render function within the main component.
My preference is to keep the main component at the top (i.e. the one the filename represents) - that's what you're looking for when Cmd+clicking on it, or doing a search, right?
I think of the child components (whether declared as render*() functions or actual <Component />s) as private functions/implementation details that you can inspect if you need to see how certain UI is implemented. Still, if you look at the main component you get an idea of what it displays.

drop the ESLint override entirely

Sounds good 👍

Reused, or pushes the file past a reasonable length: extract to its own file — at that point we're already paying the interface + props cost, so we may as well get real discoverability for it.

Agreed 👍

Here's the top 10 candidates Claude found if we want a follow-up cleanup pass

Yeah, this is the main concern for me. These render*() functions should be fine if we're talking about <100 LOC, let's say. For example, renderVitalsAlphabetically is huge as you pointed out, and I don't think it needs to be a function -- all of it could just be part of Vitals.tsx. The fact that they're rendered alphabetically is an implementation detail.

TL;DR:

• I'm fine with keeping whatever eslint rule we have.
• I'm also fine with declaring sub-components in the same file (and them being at the top rather than at the bottom) whenever it's not justified to move it to another file due to increased cognitive load / file still not huge enough that justifies that move OR testing it is not a pain in the neck yet.
• I think HostDetailsPage deserves a little bit of love and we could probably split it into smaller pieces that would make it easier to test.

[nulmete]

Closing this since we agreed on doing what I mentioned in the TL;DR

cc @RachelElysia