Codified from recurring code-review feedback on the admin-facing "Order Issues V2" React frontend (PRs #31657, #31665, #31709, #31719, reviewer: Carolina Martes). The goal is simple: a coding agent that reads this file should never trigger the same review comment twice.
Apply these proactively when writing or changing React/TypeScript/frontend code in this repo.
- The admin-tools frontend is Flow-typed (
// @flow,.jsfiles, exact object types{| |}), but every rule here applies identically to TypeScript/React — treatanyand Flow weak types (flowtype/no-weak-types) as the same thing. - "Styled components" in this repo means named plain style objects (e.g.
const headingStyle = { ... }), not thestyled-componentslibrary. Never introduce that library. - Admin-tools has no
LocalisationProvider, so currency symbols/formatting are derived client-side. - i18n keys live in
config/locales/en.yml(neveren.json); user-facing text usesuseTranslation.
Styling & tokens
- All styles are named style objects — no inline
style={{ ... }}literals. - Colours come from
BRAND_COLOURSdirectly — no localCOLORSmap or alias. - Spacing and font sizes use admin design tokens — no hardcoded
px. - Shared styles live in one feature
thememodule — not re-declared per component. - No raw
<style>tags or per-file@keyframes— animations live in a shared module.
Reuse before building
- Searched for an existing component (incl. the main web app) before hand-rolling one.
- Admin/feature-specific components are namespaced under
admin/<feature>/, notshared/. - Searched for an existing helper/util before writing one; generic non-domain logic is extracted out of components/hooks.
- No new package added for a trivial need (use built-ins like
React.useId).
Structure & cohesion
- Domain logic (validation, serialization, selection) lives outside UI components.
- No "god hook" — overloaded hooks are decomposed by responsibility.
- Prop/type surfaces are small; related state is grouped into one object; state is separated from UI.
- One top-level page + one React mount per feature.
Hooks & effects
- No complicated or unnecessary
useEffect. - Typing-driven API calls are debounced, and the debounce lives in the data hook (with cleanup).
- No
setTimeouthacks to manage focus/blur/dropdown state.
State correctness
- Each independent UI consumer owns its own stateful-hook instance — no shared instance racing.
- Dependent/derived flags reset whenever their source changes (incl. programmatic setters).
- A toggle that changes a value's meaning resets/converts + re-validates it — never silently reinterprets.
- Form fields use plain keys + one generic update helper — no per-field get/set closures.
Typing, data, i18n
- No
any/ Flow weak types unless the type model genuinely can't express it (then a localized, intentional disable). - Values are typed to the API wire format (API date strings stay
string). - No
global.fetch; admin data-fetching follows theRequestResultunion + thunk convention. - Currency is localised for display; user-facing copy is not hardcoded (routed through i18n).
Analytics, navigation, comments, tests
- Analytics is not disabled on interactive elements without a stated reason.
- Navigation uses a real
<a href>— neveronClick+window.location.assign. - No AI-narration comments; no comments describing structure that doesn't exist yet.
- New frontend functionality ships with at least base Jest/RTL coverage; oversized PRs are split.
Extract every set of styles into a named const …Style = { ... } object declared at module top, and reference it (style={fieldsContainerStyle}). Compose them ({ ...inputStyle, width: '100%' }) rather than repeating literals. Don't mix inline literals and named objects in the same component — pick the one standard (named objects).
"following standard from other pr can we use styled components throughout please?" · "we have a mix on inline styles and styled components. would be good to have 1 standard"
// ❌ inline literals (and mixing the two approaches)
<div style={{ marginTop: '12px', display: 'flex', gap: '16px' }}>
<span style={{ color: BRAND_COLOURS.brandBrown500 }}>{label}</span>
<button style={dangerButtonStyle}>Remove</button>
</div>
// ✅ all styles as named objects
const fieldContainerStyle = { marginTop: '12px', display: 'flex', gap: '16px' }
const itemLabelStyle = { color: BRAND_COLOURS.brandBrown500 }
<div style={fieldContainerStyle}>
<span style={itemLabelStyle}>{label}</span>
<button style={dangerButtonStyle}>Remove</button>
</div>Import the shared tokens and reference them by their real names (BRAND_COLOURS.brandBrown500). Do not hand-copy hex values into a local COLORS object, and do not alias them (const COLORS = BRAND_COLOURS). Aliases add indirection with zero benefit; local maps drift from the design system.
"we should use already available admin theme" · "why do we need an alias at all?" · "prob dont need this"
// ❌ local hex map, or an alias
const COLORS = { brown500: '#522a10', pink500: '#80254a' }
const COLORS = BRAND_COLOURS // also flagged
// ✅ import shared tokens, use real names
import BRAND_COLOURS from '@/constants/BrandColours'
const labelStyle = { color: BRAND_COLOURS.brandBrown500 }Just as colours come from BRAND_COLOURS, spacing and font sizes should come from the admin design tokens/variables. Hardcoded magic px values bypass the design system and drift.
"late mention but dont we have spacing variables in admin to use?" · "same for font size"
// ❌ hardcoded spacing/font size
const sectionStyle = { marginTop: '20px' }
const labelStyle = { marginBottom: '8px', fontSize: '16px' }
// ✅ design tokens
const sectionStyle = { marginTop: SPACING.lg }
const labelStyle = { marginBottom: SPACING.sm, fontSize: FONT_SIZES.body }If several components/steps in a feature need the same style objects, define them once in a central theme module and import them. Re-declaring headingStyle/sectionStyle/cardStyle in every file produces long, duplicated style lists that drift apart. If a whole piece of UI repeats, abstract it into a shared component.
"we are not sharing any styles across this feature so we're rewriting them all each time. might be worth an audit pr"
// ❌ each step re-declares the same objects
// EditingStep.js, ConfirmedStep.js, ErrorStep.js each define:
const headingStyle = { margin: '0 0 20px 0', color: BRAND_COLOURS.brandBrown500, fontSize: '24px' }
// ✅ define once, import everywhere
// theme.js
export const headingStyle = { ... }
// EditingStep.js
import { headingStyle, sectionStyle, cardStyle } from '../theme'Don't render literal <style>...</style> elements or declare per-component @keyframes/KEYFRAMES constants. Define animations once in a shared styling module and reference them.
"I dont think we usually use style tags" · "might be worth putting animations in a shared space so we can reuse"
// ❌ inline <style> tag + per-file keyframes
<style>{ '@keyframes spin { to { transform: rotate(360deg); } }' }</style>
const spinnerStyle = { animation: 'spin 0.9s linear infinite' }
// ✅ shared animation module
import { spinAnimation } from '@/styles/animations'
const spinnerStyle = { animation: spinAnimation }Before hand-rolling a UI primitive (button, error banner, input, notification), search for an existing equivalent — including the main web app — and reuse it. Reuse must be substantive: recolouring a re-implementation of the same widget doesn't count. If a component is genuinely admin/feature-specific, put it under admin/<feature>/ — not in the generic shared/ tree.
"is there no equivalent of this already? I would also place it under some admin namespace" · "might be worth looking at web app ones to not reinvent the wheel" · "no error banner we can reuse?" · "I only see color changes in that link"
// ❌ hand-rolled Button/banner, placed in shared/, or a "reuse" that only swaps colours
app/javascript/components/shared/elements/Button/Button.js.flow
const errorBannerStyle = { padding: '12px', backgroundColor: '#f9dfe1' }
// ✅ import the existing component; namespace admin-only variants
import { Button } from '<existing shared/web-app Button path>'
import ErrorBanner from '@/components/admin/.../ErrorBanner'Before adding a local utility (date parsing/formatting, currency conversion, clipboard copy, DOM helpers), search utils/ and surrounding admin code and prefer what exists. Conversely, generic non-domain behaviour inlined in a component/hook belongs in a shared util/helper module. Keep a bespoke helper only when nothing fits and you can say why (e.g. you need the raw number, not a formatted string).
"have we checked if any of these helpers already exist?" · "I think we have currency helpers we can use" · "is this maybe a util?"
// ❌ clipboard logic inlined in a hook; new local date helper without checking utils
const handleCopy = useCallback(() => navigator.clipboard.writeText(text).then(...), [text])
// ✅ extract generic behaviour to a shared helper and call it
import { copyToClipboard } from './helpers'
const handleCopy = useCallback(() => copyToClipboard(text).then(...), [text])Don't introduce a new package (e.g. uuid) for something small, especially when it isn't already a dependency. For throwaway client-side IDs (React keys, local row identity), use React.useId or a minimal inline helper.
"why not uuid package? I assume we use this or some other standard solution" → "yeh if we dont have it agree we shouldnt import it, its just a basic pattern. could be even react useId hook?"
// ❌ new dependency for a temporary client-side id
import { v4 as uuid } from 'uuid'
const id = uuid()
// ✅ built-in, no new dependency
const id = React.useId()For text, prefer the shared typography/text abstraction (the web app's <Type ...>-style component) over raw <p> with ad-hoc inline text styles. Raised as non-blocking, so treat it as a preference — but lean toward it in new code.
"in the web app they dont use ps really they use an abstraction ... something like
<Type"
A component file renders UI — it should not own domain logic. Pull recurring concerns (product selection, (de)serialization to/from API input types, validation, immutable-update helpers) into dedicated modules and import them. When the same shape of logic repeats across variants, factor it out instead of copying it.
"this feels a bit bloated atm, could we split out domain logic outside of ui components and extract shared patterns like product selection, validation, and serialization" · "there seems to be a lot of repetition in this structure"
// ❌ one component file also defines serialization + product mutators + validation
const withProductAdded = (resolution, product) => { /* … */ }
const addressToInput = (address) => ({ /* … */ })
const NonCoreFields = (props) => { /* 250 lines of UI */ }
// ✅ shared logic in reusable modules; component just composes
import { withItemAdded, withItemRemoved, withItemQuantity } from './productItems'
import ProductSearchField from '../components/ProductSearchField'
const NonCoreFields = (props) => { /* thin UI */ }A hook/file that owns many unrelated concerns is a "god hook" — split it. Extract each cohesive concern into its own hook and let the parent compose them. Group state by what changes together (e.g. all submission state — errors, applied resolutions, payload building, and the submitting → confirmed/editing transitions — in one submission hook). Keep the public return type stable so consumers don't change.
"this file also looks pretty overloaded." · "could we extract the handleSubmit path into a useOrderIssueSubmission hook that owns submitErrors, appliedResolutions, the validation/payload-building, and the submitting -> confirmed/editing transitions, so this one is mostly coordinating…"
// ❌ one hook owns recommendation loading AND submission AND product search AND messaging
const useOrderIssueSuggestion = (input) => { /* everything */ }
// ✅ cohesive concern in its own hook; parent composes
const useOrderIssueSubmission = (deps) => { /* submitErrors, appliedResolutions, handleSubmit */ }
const useOrderIssueSuggestion = (input) => {
const submission = useOrderIssueSubmission({ /* … */ })
// parent coordinates recommendation loading + local edit state only
}Don't scatter validation as ad-hoc free functions (isComplete, customAddressErrors) inside a component. Encapsulate validation in a hook (e.g. useResolutionValidation(resolution) → { errors, isComplete }) so it's reusable across variants and testable in isolation.
"can we abstract a hook to handle validations?"
When a prop list balloons (many data fields interleaved with many callbacks) or a type sprouts large unions and boolean flags, restructure: pass a single cohesive state/controller object (or context) and split UI from state rather than threading a dozen props. Large prop/type blocks signal a component doing too much.
"would be nice if we can prevent these huge types by separating state and ui" · "overall not a fan of a lot of props here"
// ❌ huge prop type mixing state + UI (14 fields, 6 callbacks)
type EditingStepProps = {| submitErrors, boxes, selectedBoxId, onBoxChange, summary,
orderDetails, affectedItems, resolutions, productSearchContext,
onUpdateAffectedItemQuantity, onRemoveAffectedItem, onCancel, onSubmit |}
// ✅ cohesive controller object; minimal UI props
type EditingStepProps = {| flow: EditingFlow, onCancel: () => void, onSubmit: () => void |}A feature has a single top-level page component and a single React mount/entrypoint. Don't add a new ReactDOM.render mount (or a new top-level page) for each sub-view — switch sub-views/steps inside the one page.
"is this the top level page? I would have imagined we would only have one order issue top level page"
// ❌ a separate mount per sub-view
mountIfPresent('admin_order_issue_suggestion', <OrderIssueSuggestionPage {...data} />)
// ...later another mount for admin_order_issue_edit
// ✅ one mount; the page renders the right step
mountIfPresent('admin_order_issue', <OrderIssuePage {...data} />)Treat useEffect as a last resort — it's a side effect to the render flow. Don't reach for one for logic that can run in an event handler, be derived during render, or be computed where the data already flows. A large multi-purpose effect (coordinating many setters, fragile dep lists) is a refactor signal.
"not great practice to have such a complicated useEffect, I normally try to avoid use effects unless necessary since they are side effects to the flow."
// ❌ a large effect that orchestrates the whole flow
useEffect(() => {
recommendFromDescription(boxId, description).then((rec) => {
setIssueType(rec.issueType); setSummary(rec.summary)
setAffectedItems(rec.affectedItems.map(toState)); loadFromRecommended(/* …many args… */)
})
}, [/* long, fragile dep list */])
// ✅ derive/compute in a callback, invoked where the data arrives
const applyRecommendation = useCallback((rec) => { /* focused mapping + setters */ }, [/* … */])Never fire a network request on every keystroke. Debounce input-driven fetches (~250ms), clearing the pending timer on the next keystroke and on unmount. Put the debounce + fetch inside the data hook (e.g. useProductSearch), keeping the presentational input component dumb. (Reusing a shared debounce util is a soft preference — fine to inline the timer if the shared util is type-incompatible with the file.)
"why do we need a timeout? can we avoid?" → "or at least move to the useproduct search hook"
// ❌ hand-rolled setTimeout inside the presentational field component
const ProductSearchField = ({ onSearch }) => {
useEffect(() => { const id = setTimeout(() => onSearch(search), 250); return () => clearTimeout(id) }, [search])
}
// ✅ debounce owned by the data hook, with cleanup
const SEARCH_DEBOUNCE_MS = 250
const useProductSearch = (ctx) => {
const timer = useRef(null)
const cancel = useCallback(() => { if (timer.current) { clearTimeout(timer.current); timer.current = null } }, [])
const scheduleSearch = useCallback((q) => { cancel(); timer.current = setTimeout(() => loadProducts(q), SEARCH_DEBOUNCE_MS) }, [cancel, loadProducts])
useEffect(() => cancel, [cancel]) // cleanup on unmount
return { search: scheduleSearch }
}Don't paper over focus/blur ordering with a magic-millisecond delay (e.g. delaying a dropdown close so a click registers). Handle it deterministically: onMouseDown + preventDefault (fires before blur), check e.relatedTarget in onBlur, or use a click-outside ref handler.
"can we avoid this? not great to have a window set timeout"
// ❌ arbitrary delay to keep the dropdown open through a click
const handleBlur = useCallback(() => { window.setTimeout(() => setFocused(false), 120) }, [])
// ✅ deterministic: relatedTarget check
const handleBlur = useCallback((e) => {
if (!containerRef.current?.contains(e.relatedTarget)) setFocused(false)
}, [])When a hook holds instance-specific state (query results, loading, error flags, in-flight tracking, debounce timers), each independent UI consumer must instantiate its own copy. Don't create one controller high up and hand the same instance to two sibling inputs — they'll race and overwrite each other's results/loading/error state. Thread the plain inputs down (memoized) and let each consumer call the hook itself. Also guard async results against out-of-order responses (e.g. a request-id ref) so a stale response can't overwrite fresh state.
"creates a single productSearch controller and both resolution UIs consume that same instance … typing in one search box overwrites the other box's results and error/loading state, so the two inputs race each other and can show the wrong catalogue results." (her highest-priority finding)
// ❌ one shared controller instance handed to multiple inputs
const productSearch = useProductSearch({ selectedBoxId, csrfToken })
// <AddProductField productSearch={productSearch} /> // both read/write the SAME state → race
// <NonCoreField productSearch={productSearch} />
// ✅ thread inputs (memoized) down; each consumer owns its own hook instance
const productSearchContext = useMemo(() => ({ selectedBoxId, csrfToken }), [selectedBoxId, csrfToken])
const AddProductField = ({ productSearchContext }) => {
const productSearch = useProductSearch(productSearchContext) // isolated state + request-id guard
}If a piece of UI state is tied to another value (a copied badge tied to a text field, a "saved" badge tied to a form), reset it in every path that mutates the source — including programmatic setters, not just user-edit handlers. Never let a "done" flag persist after the thing it described has changed.
"updates the message text without clearing
copied, andapplyRecommendation()calls that setter whenever a fresh recommendation arrives … the UI will still say the message is copied even though the clipboard still contains the old text."
// ❌ copied not cleared when the message changes programmatically
const setMessage = useCallback((m) => setCustomerMessage(m), [])
// ✅ every path that changes the source clears the dependent flag
const setMessage = useCallback((m) => { setCustomerMessage(m); setCopied(false) }, [])
const handleChange = useCallback((e) => { setCustomerMessage(e.currentTarget.value); setCopied(false) }, [])5.3 When a toggle changes a value's meaning, reset/convert + re-validate — never silently reinterpret
If a toggle changes how an existing input is interpreted (currency amount ↔ percentage, minor ↔ major units), changing the toggle must not leave the raw value to be silently re-parsed under the new meaning. On toggle, either reset the value and force deliberate re-entry, or explicitly convert it — and re-validate. Keep the stored value and its interpretation consistent at all times. (This class of bug is financially dangerous: 10.00 silently becoming 10%.)
"still silently reinterprets the entered refund when the admin changes basis … So 10.00 can become 10%, and 50 can become a fixed £50 refund, without the UI forcing a re-entry."
// ❌ toggling basis leaves the amount to be reinterpreted downstream
const handleBasisChange = (basis) => onChange({ ...resolution, basis })
// ✅ reset (or convert) + revalidate so meaning stays consistent
const handleBasisChange = (basis) => onChange({ ...resolution, basis, amount: '' })Don't model form fields with config objects carrying per-field get/set closures. List fields as plain descriptors ({ key, label, optional }) and update via one generic helper that keys into the object. This makes the form read like normal controlled inputs and cuts boilerplate.
"odd to see getter setter patterns could we replace the get/set functions with plain field keys plus a generic updateAddressField helper so the form reads more like normal controlled inputs … seems like a lot of config"
// ❌ getter/setter closures per field
const ADDRESS_FIELDS = [
{ key: 'recipientName', label: 'Recipient name', get: (a) => a.recipientName, set: (a, v) => ({ ...a, recipientName: v }) },
// …repeated for every field
]
onAddressChange(field.set(address, value))
// ✅ plain keys + one generic updater
const ADDRESS_FIELDS = [{ key: 'recipientName', label: 'Recipient name', optional: false } /* … */]
const updateAddressField = (address, key, value) => ({ ...address, [key]: value })
onAddressChange(updateAddressField(address, field.key, value))A function that builds the starting value of a piece of state represents its default state — name it defaultState / defaultResolution / initialState, not blank.
"v v nit but I dont love the name blank, this would be default state?"
Avoid any / weak types. If you feel you need one, first assume it points to a fixable type-model problem (a missing generic, a discriminated union that isn't wired up, an interface that should be parameterised) and fix that. Only if there's genuinely no clean type do you fall back to a localized, intentional // eslint-disable-next-line flowtype/no-weak-types (or @typescript-eslint/no-explicit-any) with a reason. Equally, don't satisfy the lint rule by contorting call sites into repetitive boilerplate (e.g. re-narrowing a union in every function) — pick the lesser evil consciously.
"can we avoid any please?" → "normally having to use any points to a wider type problem, that being said … if nothing, yeah prefer disable."
// ❌ reaching for any without investigating the type model
type AnyResolutionKind = ResolutionKind<any>
// ✅ preferred — model it properly if you can
type AnyResolutionKind =
| ResolutionKind<DiscountResolution>
| ResolutionKind<RefundResolution>
| ResolutionKind<NonCoreResolution>
// ✅ acceptable fallback — only if no clean type exists; keep it localized + intentional
// eslint-disable-next-line flowtype/no-weak-types -- registry is heterogeneous; per-kind narrowing happens at the boundary
type AnyResolutionKind = ResolutionKind<any>Type a field to match what the API actually sends. GraphQL dates arrive as ISO YYYY-MM-DD strings — keep them typed as string through state and props, and parse to a Date only transiently at the point of display formatting. Don't eagerly convert API strings into richer objects in state.
"dates are string?" → confirmed: the API returns ISO strings; parse to
Dateonly transiently for display.
Never call global.fetch (use plain fetch, or the app's request utilities). More broadly, match the admin frontend's data-fetching contract: the query file returns a discriminated RequestResult = Success | ServerError | UnknownError union and a thunk handles the branching (mirror single_customer_view/rails_queries/getShippingDetails.js + single_customer_view/thunks/getShippingDetails.js). Don't collapse non-200s and GraphQL errors into a single generic message, and don't build ad-hoc result unions inside the API file.
"I dont think we normally use global like this?" · "point 2 makes sense to keep as this is the current pattern across admin"
// ❌ global.fetch + ad-hoc union collapsing all failures to one message
const res = await global.fetch('/graphql', { ... })
type RecommendResult = {| type: 'Recommendation', ... |} | {| type: 'RequestError', message: string |}
// ✅ query returns the standard union; a thunk branches on it
// rails_queries/…: type RequestResult = Success | ServerError | UnknownError
// thunks/…: switch on result.type and dispatch accordinglyAny currency value rendered to a user must go through a localised formatter — decimal separator (period vs comma), grouping, and symbol placement all vary by country. Don't hardcode £, assume a . decimal, or build display strings by hand. (Converting between minor and major units for a raw, symbol-less form input value is fine — that's a number, not localised display.)
"currency is something we always localise (different countries will have period vs comma or different place to put symbol)"
// ❌ hardcoded symbol + assumed '.' decimal for display
const priceLabel = `£${(minorUnits / 100).toFixed(2)}`
// ✅ localised formatter for display; raw number is fine for a form value
const priceLabel = formatCurrencyWithDecimal(minorUnits, currencyCode)
const majorValue = (minorUnits / 100).toFixed(2) // form input value, not displayUser-facing strings (button labels, empty/error states, placeholders) shouldn't be hardcoded English literals. Use useTranslation with keys in config/locales/en.yml. If a screen is genuinely intended to be English-only, flag it and confirm rather than silently hardcoding.
"everything here is hardcoded to english copy … could we check with CL if cool to have in english only or should we consider localisation"
// ❌ hardcoded English literals
<button>{'Remove'}</button>
<p>{'Could not load the products for this order.'}</p>
// ✅ translated via i18n
const { t } = useTranslation('orderIssues')
<button>{t('productSearch.remove')}</button>
<p>{t('productSearch.loadError')}</p>Don't pass disableAnalytics (or any opt-out of default tracking) on buttons/interactive controls by default. Buttons are tracked for a reason; silently disabling it loses product signal. Only opt out with a concrete, documented reason.
"why disable analytics?" · "not sure this belongs here?"
// ❌ disabling analytics with no rationale
<Button disableAnalytics onClick={onCopyMessage} typography={{ text: 'Copy message' }} />
// ✅ keep default tracking (add disableAnalytics only with a comment explaining why)
<Button onClick={onCopyMessage} typography={{ text: 'Copy message' }} />For navigation (especially "back" actions), render a real anchor/link instead of an onClick that calls window.location.assign(...). A fake-navigation button breaks cmd/ctrl-click and middle-click (open in new tab), right-click "copy link", and history/back behaviour. Reserve imperative window.location for cases with no URL to link to.
"can we avoid this to not mess up browser behaviour"
// ❌ a button that fakes navigation
const handleBackToSoc = useCallback(() => { window.location.assign(socPath) }, [socPath])
<Button onClick={handleBackToSoc} typography={{ text: 'Back to SOC' }} />
// ✅ a real link preserves browser behaviour
<a href={socPath} style={backLinkStyle}>{'Back to SOC'}</a>Don't add narration comments that restate what the code/types already say, and never write comments describing structure that doesn't exist yet in this PR (e.g. "add a descriptor in resolutionKinds/" when there's no such directory). If a comment is confusing or describes a future plan, delete it. Rely on clear naming and types. (This repo avoids doc-style class/method comments generally.)
"a lot of these generated comments are unnecessary" · "not sure I understand this comment"
// ❌ narration + a reference to structure that doesn't exist
// Editable form of a non-core delivery address. Optional lines stay as ''
// New resolution types require a descriptor in resolutionKinds/
type NonCoreAddressState = {| recipientName: string, addressLineOne: string |}
// ✅ self-describing types/names, no comment
type NonCoreAddressState = {| recipientName: string, addressLineOne: string |}A PR adding meaningful frontend functionality (a new registry, new components, new state logic) must include at least base Jest/React Testing Library coverage for the main paths. Keep PRs reviewable by splitting large ones along natural seams (e.g. skeleton/infrastructure vs individual feature files).
"this pr is already quite big and has no testing coverage, we want at least base coverage for main functionality" · "it would make sense to split this out into skeleton and resolution type files"