Skip to content

Modernize category modal and improve subcategory management#23583

Open
Pickysaurus wants to merge 14 commits into
Nexus-Mods:masterfrom
Pickysaurus:categories-modal
Open

Modernize category modal and improve subcategory management#23583
Pickysaurus wants to merge 14 commits into
Nexus-Mods:masterfrom
Pickysaurus:categories-modal

Conversation

@Pickysaurus

Copy link
Copy Markdown
Contributor

This PR replaces the old categories modal with a new version built on the latest components.

OldCategories NewCategories

Changes for the user

  • New Modal style, including a close button (this was missing from the old design)
  • The sub-category count (as well as nest mod counts) can be seen from the subtitle
  • All interactions are contained in the modal (no more pop-ups over pop-ups)
  • The category top-levels will be open for the first open of the modal
  • Custom "No results" or "Error" views

Technical Changes

  • Re-work the modal using new Tailwind components
  • Removed redundant session reducer for determining if the modal is open. Use the same method as other modals.
  • Removed react-sortable-tree
  • Moved the "business logic" into custom hooks
  • Re-added category fetching from the V1 API (this may need swapping to the internal Nexus class, but I couldn't figure that out)
  • Added missing types to various places in the code

@Pickysaurus Pickysaurus requested a review from a team as a code owner June 29, 2026 08:13

@IDCs IDCs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @Pickysaurus, we're really thankful you're trying to help us out and modernising existing functionality.

One thing we've been pushing for during Vortex 2.x modernisation is unit-tests - Vortex is an intertwined headache and we just can't guarantee that changes to one component don't break something else, and we simply do not have the time to manually test every bit of code manually.

This PR may work just fine right now (haven't tested it yet, need to reserve a time slot); but can break 2 weeks down the road due to some change we made somewhere else.

For this reason, I don't think any of us app devs feel comfortable to merge this in without added unit-tests.

@Pickysaurus

Copy link
Copy Markdown
Contributor Author

Could you give an example of a part of the app with good unit tests so I can see what you're looking for?

All the existing tests within the module still pass (relating to the redux state), but I couldn't find any other tests in the area of the codebase I looked at as a reference for the standard you'd want

@IDCs

IDCs commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

First of all have a look at src/renderer/src/test-utils which defines a bunch of builders and fixtures (e.g. vortex-api) which you can use/extend as needed for your tests (I don't think you need a fixture - just a new builder).

For this PR the one I'd really like covered is buildCategoryTree - it's recursive and easy to break.

  • top-level categories sorted by order
  • children nested under the correct parentId
  • directModCount vs nestedModCount roll-up (parent whose mods live in its grandchildren)
  • customSort overriding the default sort
  • empty input/a category with no mods (?? 0 path)
  • getCategoriesFromNexusMods is worth a couple too.

Have a look at findModByRef.test.ts for a really basic one, deterministicReferenceTag.test.ts is using the builders so that's a preferred pattern.

TL;DR: the bar is "cover the pure logic you extracted" and optionally lean on the Modal.test.tsx style if you want to render-test any of the smaller pieces.

Also - if you find it hard to test something because vitest is pulling transient dependencies - it might be helpful to extract further (but I don't think it's the case here)

@Pickysaurus

Copy link
Copy Markdown
Contributor Author

I've added tests to the areas you suggested, and to some of the React components. Is that suitable?

@Pickysaurus

Pickysaurus commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

I've added tests to the areas you suggested, and to some of the React components. Is that suitable?

N.B. I did start writing tests for the hooks, too but the version of the test library Vortex uses doesn't seem to have renderHook so I'd have to build a little harness for it (according to Copilot) which wasn't something I wanted to bloat the code with if we'll eventually update Vortex to the version with hook tests (plus I don't see any other hook with tests at the moment)

@Pickysaurus Pickysaurus requested a review from IDCs July 7, 2026 18:28
@Pickysaurus

Pickysaurus commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

PR feedback from @IDCs and Claude Code:

Correctness issues

  • "Expand All" only expands top-level categories. In CategoryToolbarActionsHook.ts, the click handler does expandAll(treeData.flatMap((t) => t.categoryId)). treeData is only the root array, and flatMap over a string-returning callback yields just the root ids — nested descendants are never added to expanded, so grandchildren stay collapsed. You need to recursively flatten the whole tree, e.g. reuse the flatten helper already written in CategoryTreeActionsHook.moveCategory.
  • Reducer optional-chaining regression can throw. The diff changes setCategoryOrder's read to const oldOrder = state[gameId]?.[id].order;. The ?. guards state[gameId] but not [id] — if an id in categoryIds isn't present under state[gameId], .order throws a TypeError. The previous getSafe(newState, [gameId, id, "order"], undefined) was null-safe. In practice ids come from the tree (built from categories) so it's usually consistent, but this is a real robustness regression. Use state[gameId]?.[id]?.order. (Also note it now reads from state rather than the accumulating newState; harmless here since ids are distinct, but worth a comment.)
  • Invalid Tailwind arbitrary values silently drop the height constraints. CategoryList.tsx uses max-h-[calc(dvh0.75)] and min-h-[calc(dvh0.5)]. dvh is a unit, not a value — calc(dvh*0.75) is invalid CSS and the browser discards the declaration, so the intended viewport-relative sizing never applies. Use max-h-[75dvh] / min-h-[50dvh].
  • Leftover debug logging. CateogryTreeSelectionHook.ts has console.log("Dispatching remove", gameId, categoryId);. Remove it (and note the project uses the log helper, not console).

Conventions & style

  • Localization regression. The old code wrapped user-facing strings in t(). Several new leaf components hardcode English: toolbar labels in CategoryToolbarActionsHook ("Expand All", "Show Empty", "Fetch from Nexus Mods"), CategoryAddParent ("Create"/"Cancel"/"Add category name…"), CategoryListNoResults ("No categories", "Create Category", "Sync with Nexus Mods"), CategoryListFetchError ("Try again"), and the fetch-error titles built in CategoryTreeActionsHook. Given the project ships translations, these should go through t().
  • Two misspelled filenames. hooks/CategroyTreeStateHook.ts ("Categroy") and hooks/CateogryTreeSelectionHook.ts ("Cateogry"). Worth fixing now while the files are new — imports in CategoryTreeHook.ts reference the typos.
  • Duplicated create logic. createCategory exists in CategoryTreeActionsHook and again as onCreateCategory in CateogryTreeSelectionHook (the latter, plus onShowDialog/onShowError, appears unused now). Drop the dead selection-hook members or consolidate.
  • Inconsistent min-length gating. CategoryAddParent disables Create at newName.length < 2, while CategoryListItem disables subcategory save at newSubcategoryName.length <= 2 (i.e. requires 3+). Pick one rule.
  • Subtitle pluralization. CategorySubtitle renders "1 sub-categories with 10 mods" — no singular/plural handling. Since it's already inside t(), consider i18n count interpolation.

Tests

  • Good, targeted coverage:buildCategoryTree(sort, nesting, direct/nested roll-up, empty, nonexistent-category mods),getCategoriesFromNexusMods(ok/non-ok/network), and component tests for the dialog/list/item/subtitle.
  • buildCategoryTree "respects a provided customSort" is a weak test(buildCategoryTree.test.ts:1222): the custom comparator produces["b","a"], which is identical to the default order-based result, so it wouldn't catch a regression wherecustomSortis ignored. Use inputs where custom vs. default ordering differ.
  • The author notedrenderHookisn't available for testing the hooks directly — reasonable to defer, and the pure logic (buildCategoryTree) is the highest-risk piece and is covered.

- Add i18n (translation) support to category management hooks and views
- Extract tree flattening logic into reusable `flattenTreeToIDs` utility with tests
- Fix filename typos: CateogryTreeSelectionHook → CategoryTreeSelectionHook, CategroyTreeStateHook → CategoryTreeStateHook
- Fix optional chaining bug in category reducer
- Fix sort order logic and validation thresholds
- Clean up unused imports and functions
- Update test expectations to match new i18n string formats
@Pickysaurus

Copy link
Copy Markdown
Contributor Author

Subtitle pluralization. CategorySubtitle renders "1 sub-categories with 10 mods" — no singular/plural handling. Since it's already inside t(), consider i18n count interpolation.

I couldn't seem to get i18n count interpolation working but everything else on the list is fixed.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants