Skip to content

Fold the repeated renderer shells into one component each #489

Description

@Pixnop

Summary

Five shells in the renderer are written out once per place that needs them instead of once: the button, the confirm dialog, the single-select dropdown, the multi-select filter dropdown, and the menu chrome that two controls still spell out by hand. Same shape, copied, and in three of the five the copies have already drifted apart in ways a player can see.

Total estimated saving: about 500 lines, and five look-and-behaviour decisions that stop being able to differ per control.

1. Buttons.tsx and FormButtons.tsx are one button implementation written twice

src/renderer/src/components/ui/Buttons.tsx:51 and src/renderer/src/components/ui/FormComponents/FormButtons.tsx:97

NormalButton and FormButton have the same body: forwardRef, useActionBusy(onClick, busy, disabled), the same HButton with type/disabled/onClick/title/aria-label/aria-busy/aria-pressed, the same clsx of BUTTON_BASE_STYLES plus size plus variant, then renderActionContent. They differ in the default variant (ghost versus secondary), one extra prop each (style versus ariaExpanded) and an overflow-hidden class. LinkButton:89 and FormLinkButton:134 are the same story over <Link>.

Replace with: one Button and one ButtonLink holding the body, with NormalButton / FormButton / LinkButton / FormLinkButton kept as presets that set the default variant, and FormButtons.tsx re-exporting the two form presets so the barrel and every import path are untouched. No call site changes.

Savings: verified by building it: the two files go from 278 to 166 lines, 112 saved.

Risk and test: the rest-props spread is load-bearing. Headless UI's MenuItem with as={Fragment} clones its child and merges role/tabIndex/data-* onto it, so the merged component must keep the same Omit<ComponentPropsWithoutRef<"button">, ...> rest slot. FormButton's FormButtonAction union (submit may omit onClick, everything else may not) must survive, and it is exercised by four real nativeType="submit" call sites (ListVersions.tsx, ModProfilesPopup.tsx twice, SessionButton.tsx). One trap found while verifying: a preset that spreads Omit<ButtonProps, "variant"> does not typecheck, because Omit collapses the discriminated union; each preset needs its own restated prop type. Pinned by tests/renderer-dom/actionBusyState.test.tsx and selectableItemA11y.test.tsx across roughly 400 call sites; both pass, as does the full typecheck.

Worth: high.

2. Six inline confirm dialogs repeat a shell three components already own

src/renderer/src/features/installations/pages/ManageInstallationBackups.tsx:189 and :209, features/installations/pages/ListInstallations.tsx:250, features/versions/pages/ListVersions.tsx:198 and :240, features/servers/pages/ManageInstallationServers.tsx:180

Each inlines the same block: PopupDialogPanel plus a question paragraph plus a text-zinc-400 consequence paragraph plus <ButtonsWrapper className="text-base" bgDark={false} equalWidth flush> with a secondary Cancel (PiXCircleDuotone) first and a destructive confirm second. DeleteModDialog.tsx:55 and RemoveServerModsDialog.tsx:32 already wrap exactly that shape as components, so the shell exists twice over as a component and six more times inline.

Replace with: one ConfirmDialog({ title, isOpen, close, question, consequence, confirmLabel, confirmIcon, onConfirm, children }), with the two existing dialogs reduced to presets over it and the six inline copies replaced by a call.

Savings: about 94 lines of inline JSX down to about 35, plus one component of about 30 lines. Eight confirm surfaces with one button order instead of eight.

Risk and test: Cancel comes first on purpose and it is load-bearing (Headless UI's focus trap focuses the first focusable child, so Enter on a fresh prompt cancels); the generic version must keep that DOM order. tests/renderer-dom/launchPlayGame.test.tsx:498 pins buttons[0] as Cancel by DOM order. ListInstallations also carries a "delete data" checkbox inside its dialog (:247) and ListVersions picks title, question, consequence and label off versionToDelete?.linked (:190-201), so the component needs a children slot and must not own the copy. tests/renderer-dom/installationsDelete.test.tsx, installationsRestoreBackup.test.tsx and versionsListVersions.test.tsx are the check.

Correction to the original write-up: LaunchBackupPrompt.tsx was listed as a third existing wrapper. It is not the same shape (no ButtonsWrapper, no consequence paragraph, icon-only children), so leave it out of the first pass.

Worth: high.

3. Four hand-written copies of the same single-select Listbox

src/renderer/src/features/mods/components/SideFilter.tsx:11, features/mods/components/InstalledFilter.tsx, components/ui/LanguagesMenu.tsx, and UIScale in features/config/pages/ConfigPage.tsx:440

The same component four times: a Listbox whose trigger renders through the same OPTIONS.filter(o => o.key === value).map(...) idiom, then an identical AnimatePresence plus ListboxOptions static anchor="bottom" plus motion.ul with DROPDOWN_MENU_WRAPPER_VARIANTS plus ListboxOption as={motion.li} block. SideFilter and InstalledFilter differ only by which array of {key, value} they close over.

Replace with: one SelectMenu<T>({ value, onChange, options, renderOption, size }) in components/ui, reached by all four. InstalledModsSelectFilter (value/onChange/options/label over bare strings) becomes a thin preset over it, once item 5 has put it on the shared styles.

Savings: about 270 lines down to about 130; the caret, the anchor and the option styling stop being able to drift per control.

Risk and test: LanguagesMenu and the UI scale control have no DOM test at all, so do them last and check by hand that the language switch still persists to localStorage and that data-uiscale still updates. SideFilter and InstalledFilter are covered by tests/renderer-dom/listModsFilterBar.test.tsx and modsListModsFilterUpdate.test.tsx. PR #348 already favoured exactly this kind of Listbox-scaffold fold and carved out only the ModDB Combobox filters, which are not in this list.

Worth: high.

4. TagsFilter and VersionsFilter are the same component twice

src/renderer/src/features/mods/components/TagsFilter.tsx:12 and features/mods/components/VersionsFilter.tsx (77 lines each)

Line for line identical apart from the lookup hook (useTagsLookup versus useGameVersionsLookup), the placeholder key, and a before:content-['#'] on the tag chips. Same Listbox multiple, same trigger with the overflow-x-scroll chip row, same lookupFailed row, same ListboxOption block with the check icon. Both operate on types that already share a {tagid, name, color} shape.

Replace with: one MultiSelectFilter<T extends { tagid: string | number; name: string }>({ value, onChange, options, placeholder, failed, chipClassName, size }), with the two filters reduced to a call each (drafted: about a 75-line generic plus two 18-line wrappers, against 154 today).

Savings: about 44 lines, and one copy of the chip row and the failure row.

Risk and test: the failed-lookup row is what #411 added (an empty list and a dead request looked identical), so it must render in the generic version for both. tests/renderer-dom/modsFilterLookupFailure.test.tsx pins it, listModsFilterBar.test.tsx pins the selection behaviour. One more cost the original write-up missed: tests/text-contrast.test.ts reads these files by exact path and anchors on the literal tagsFilter.length / versionsFilter.length names, so moving the placeholder-colour logic into a shared component means updating that pinned test in the same change. The floors it enforces do not move, only where it reads them from.

Do not fold in InstalledTagsFilter in the same pass. It was deliberately left out of PR #348, so its trigger and option classes diverge, it has no lookup-failure state (its tags are a local prop, not a ModDB fetch) and it works on plain strings. Folding it in either changes the Manage Mods page's look without being asked or pushes override props back into the generic component.

Worth: medium.

5. Two dropdowns still inline the chrome the shared button system owns

src/renderer/src/features/mods/components/InstalledModsSelectFilter.tsx:38 and features/mods/components/InstalledTagsFilter.tsx

Both carry the ListboxButton class string literally ("px-2 flex items-center justify-between gap-2 rounded-sm overflow-hidden border border-zinc-400/5 bg-zinc-950/50 ...") and the ListboxOption string literally, while the 14 other files that render a menu import MENU_TRIGGER_STYLES and MENU_OPTION_STYLES from components/ui/buttonStyles.ts. They are the only two left: both landed in PR #348, about ninety minutes before PR #350 introduced the shared constants, and were never swept in afterwards. The drift is visible already, the shared trigger gives the caret a caret-optical class and the focus-visible outline BUTTON_BASE_STYLES carries, and these two have neither.

Replace with: className={clsx(MENU_TRIGGER_STYLES, size)} and className={clsx(MENU_OPTION_STYLES, ...)}, the way AuthorFilter, VersionsFilter, SideFilter, OrderFilter, InstalledFilter and LanguagesMenu already do. TagsFilter proves the shape fits a variable-width filter trigger, not just fixed-option menus.

Savings: verified by applying it: 9 insertions, 19 deletions, net 10 lines, and two controls rejoin the one place a menu's look is decided.

Risk and test: MENU_TRIGGER_STYLES deliberately carries no width (its own comment says why), so the size prop must still be appended, and MENU_OPTION_STYLES uses min-h-10 where these use h-8, so the option row height changes. That is a visual change on Manage Mods and should be in the PR description. tests/text-contrast.test.ts reads the shared constants, so the measured surface does not change; typecheck, eslint and the full suite (4091 tests) pass on the applied change.

Worth: medium.

Suggested order

  1. Item 5 first: ten lines, already verified, and it puts both Manage Mods dropdowns on the shared styles so items 3 and 4 have one styling source to generalise from.
  2. Item 1 next: it is self-contained, has the widest blast radius (about 400 call sites) and is best landed alone.
  3. Item 2, which depends on nothing but is the largest JSX diff.
  4. Item 3, then item 4. Both are dropdown generalisations and item 3's SelectMenu sets the prop-naming convention item 4's MultiSelectFilter should echo. Leave LanguagesMenu and the UI scale control for last inside item 3, since neither has a DOM test.

Out of scope

The accessibility work stays and is not what any of this trims: Cancel-first DOM order in the confirm shells, aria-busy / aria-pressed / ariaExpanded on the buttons, the focus-visible outline, and the contrast floors tests/text-contrast.test.ts measures all survive the merge, and item 4 updates that test's reading path without touching its thresholds. tests/security-boundaries.test.ts, tests/log-provenance.test.ts and tests/i18n/i18n-parity.test.ts keep their rules (no HTML sinks, log provenance, locale parity). The hexagonal split (pure src/domain, src/ipc and src/main as host, the renderer through window.api and feature adapters), the path policy, the IPC validation at the boundary and the mutation-tested guards are deliberate and untouched here. The ModDB Combobox filters that PR #348 deliberately kept separate stay separate.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    area: rendererReact UI, contexts, feature adapterstech debtInherited debt, tracked to be paid down

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions