Skip to content

chore: sweep up small items recorded in recent reviews - #180

Merged
Zaldaryon merged 3 commits into
devfrom
chore/review-follow-ups
Aug 22, 2026
Merged

chore: sweep up small items recorded in recent reviews#180
Zaldaryon merged 3 commits into
devfrom
chore/review-follow-ups

Conversation

@Pixnop

@Pixnop Pixnop commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Four loose ends that came up in recent reviews and never got filed anywhere. Each one was checked against the code on dev before anything changed, and one of them turned out to need no change at all.

The mods grid memoization suite. PR #169's review called it a test that reassures without pinning anything, because it mocks ModListCard and wraps its own spy in memo(), so the memo() on the real component never gets exercised. That holds up under measurement: deleting the wrapper from the real ModListCard.tsx leaves the suite green, while modListCardMemo.test.tsx (which imports the unmocked component and counts prop reads through a Proxy) fails the way it should. The suite is not worthless though. Because the spy is memo-wrapped, its assertion catches any prop that loses referential stability on the way from ListMods down to the cards, and adding an unstable dependency to onSelectMod's useCallback does fail it. That is a real property, and a different one from what the file's name promised, so instead of deleting the file I renamed it to modsGridCardPropIdentity.test.tsx, renamed the suite and its one test to match, and rewrote the header comments in both files so neither claims coverage it does not have.

The worker pool idle limits in pathsHandlers.ts. PR #169's review measured the comment about the combined idle count staying inside the shared archive limit as false, with EXTRACT_ON_PATH at 2 and COMPRESS_ON_PATH at 1 against a two-slot limiter. On dev today both constants are 1. Extraction and compression use different worker scripts and WorkerPool keys its idle lists by script path, so the two counts really are separate and really do add up to 2, which is exactly ARCHIVE_CONCURRENCY_LIMIT. Downloads sit at 3 idle against their own limit of 3, and CHANGE_PERMS and RUN_INSTALLER never pool at all. The comment matches the code, so there is no commit for this one and the file is untouched.

The skip-offscreen-render comment in styles.css. It credited Grid.tsx and Table.tsx jointly with AnimatePresence and useInView. Table.tsx imports AnimatePresence and motion and nothing else from motion/react; only Grid.tsx imports useInView. Reworded so the useInView reason for keeping items mounted is attributed to Grid.tsx alone.

The untrimmed manual version in useLookForAVersion. PR #164's review noted that versionFound goes into the guard untrimmed, and it still does on dev: a version typed as nothing but spaces is falsy to a person and truthy to !versionFound, so it slid past the missing-folder-or-version check and got written to the config with its whitespace intact, where it matched nothing else. The trim now happens before the guard, so the duplicate check and the stored value both see the clean string and a padded version stops being stored padded. The new row in versionsLookForAVersion.test.tsx types three spaces and expects the error notification with no config write, and it fails against the old hook.

Typecheck, lint:ci (0 errors, the same 17 pre-existing hook-dependency warnings as dev), format:check and test:coverage are all green locally: 107 files, 1221 passed and 2 skipped, coverage at 90.81 statements, 87.87 branches, 89 functions and 92.48 lines, all clear of the floors in vitest.config.ts.

Pixnop added 3 commits August 21, 2026 19:43
The suite mocks ModListCard and wraps its own spy in memo(), so removing memo()
from the real component leaves it green. What it does catch is a prop losing
referential stability between ListMods and the cards: adding an unstable
dependency to onSelectMod's useCallback fails it. Renamed the file and the
assertions accordingly, and pointed the comments at modListCardMemo.test.tsx
for the memo pin on the real export.
Table.tsx uses AnimatePresence but never useInView; only Grid.tsx imports it.
The comment credited both files with both, so it named a reason for keeping
items mounted that Table.tsx does not have.
A version made only of spaces is falsy to a person but truthy to the guard, so
it passed the missing-folder-or-version check and got written to the config with
its whitespace intact, where it matched nothing. Trimming before the guard also
keeps a padded version from being stored padded.
@Pixnop
Pixnop requested a review from Zaldaryon August 21, 2026 17:45

@Zaldaryon Zaldaryon left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Checked out 252775a and ran the local gates directly: typecheck, lint:ci (0 errors, 17 warnings, the same count this branch's base carries), format:check, and the full test:coverage suite, 107 files, 1221 passed, 2 skipped, matching the description exactly.

The useLookForAVersion trim is correct: versionFound is a plain useState, never undefined, so trimming it before the guard is safe, and the new whitespace-only test fails against the old hook as claimed. The two test-suite renames read honestly now: modListCardMemo.test.tsx pins the real component's memo, modsGridCardPropIdentity.test.tsx pins prop identity on the mocked spy, and neither file's comments overclaim what the other proves. The styles.css correction matches the actual imports in Grid.tsx and Table.tsx.

No blocking findings. Approving.

@Zaldaryon
Zaldaryon merged commit 38511ca into dev Aug 22, 2026
7 checks passed
@Zaldaryon
Zaldaryon deleted the chore/review-follow-ups branch August 22, 2026 00:38
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