polish(categories): honest doc + extract/test picker-options glue (#246 review)
All checks were successful
PR Check / rust (pull_request) Successful in 22m33s
PR Check / frontend (pull_request) Successful in 2m26s

Address the two non-blocking review notes on PR #252:
- The re-injected non-leaf parent (e.g. #1710) is technically clickable, not
  'never offered'; correct the MappingRow comment to state selecting it is a
  no-op (only leaves resolve).
- Extract the per-row option-building glue into a pure, unit-tested helper
  comboboxCategoriesForTarget (4 cases: null/leaf pass-through by reference,
  non-leaf append, unresolvable stale id).
This commit is contained in:
le king fu 2026-07-05 16:53:27 -04:00
parent ce2793fc2c
commit 4f87ce329c
3 changed files with 85 additions and 9 deletions

View file

@ -1,6 +1,7 @@
import { useMemo } from "react"; import { useMemo } from "react";
import { useTranslation } from "react-i18next"; import { useTranslation } from "react-i18next";
import CategoryCombobox from "../shared/CategoryCombobox"; import CategoryCombobox from "../shared/CategoryCombobox";
import { comboboxCategoriesForTarget } from "./migrationTargets";
import type { Category } from "../../shared/types"; import type { Category } from "../../shared/types";
import type { import type {
MappingRow as MappingRowType, MappingRow as MappingRowType,
@ -64,15 +65,13 @@ export default function MappingRow({
// The picker lists leaves only. If this row's current target is a non-leaf // The picker lists leaves only. If this row's current target is a non-leaf
// parent (a low-confidence default like "Divertissement" #1710), it is absent // parent (a low-confidence default like "Divertissement" #1710), it is absent
// from that list and would blank the combobox input, so re-inject it for this // from that list and would blank the combobox input, so re-inject it for this
// row only — shown as the current value, never offered as a new pick. // row so it stays visible. It remains clickable, but selecting it is a no-op —
const comboboxCategories = useMemo(() => { // handleTargetChange only resolves ids present in `targetCategories` (leaves) —
const id = row.v1TargetId; // so any pick that takes effect is a leaf. See comboboxCategoriesForTarget.
if (id == null || targetCategories.some((c) => c.id === id)) { const comboboxCategories = useMemo(
return targetCategories; () => comboboxCategoriesForTarget(targetCategories, row.v1TargetId ?? null, resolveTarget),
} [targetCategories, row.v1TargetId, resolveTarget],
const current = resolveTarget(id); );
return current ? [...targetCategories, current] : targetCategories;
}, [targetCategories, row.v1TargetId, resolveTarget]);
const badgeLabel = t( const badgeLabel = t(
`categoriesSeed.migration.simulate.confidence.${row.confidence}`, `categoriesSeed.migration.simulate.confidence.${row.confidence}`,

View file

@ -2,7 +2,9 @@ import { describe, it, expect } from "vitest";
import { import {
taxonomyToComboboxCategories, taxonomyToComboboxCategories,
findTaxonomyCategory, findTaxonomyCategory,
comboboxCategoriesForTarget,
} from "./migrationTargets"; } from "./migrationTargets";
import type { Category } from "../../shared/types";
import { import {
getTaxonomyV1, getTaxonomyV1,
type TaxonomyNode, type TaxonomyNode,
@ -127,3 +129,49 @@ describe("findTaxonomyCategory", () => {
expect(c).toMatchObject({ id: 1710, is_inputable: false }); expect(c).toMatchObject({ id: 1710, is_inputable: false });
}); });
}); });
describe("comboboxCategoriesForTarget", () => {
const leaf = (id: number): Category => ({
id,
name: `Leaf ${id}`,
parent_id: undefined,
type: "expense",
is_active: true,
is_inputable: true,
sort_order: id,
created_at: "",
});
const leaves = [leaf(1111), leaf(1121), leaf(1131)];
const parent1710: Category = {
id: 1710,
name: "Divertissement",
parent_id: undefined,
type: "expense",
is_active: true,
is_inputable: false,
sort_order: Number.MAX_SAFE_INTEGER,
created_at: "",
};
const resolveTarget = (id: number) => (id === 1710 ? parent1710 : null);
it("returns the leaves list unchanged (same reference) when target is null", () => {
expect(comboboxCategoriesForTarget(leaves, null, resolveTarget)).toBe(leaves);
});
it("returns the leaves list unchanged (same reference) when target is already a leaf", () => {
expect(comboboxCategoriesForTarget(leaves, 1121, resolveTarget)).toBe(leaves);
});
it("appends the resolved non-leaf parent when the target is a parent absent from leaves", () => {
const out = comboboxCategoriesForTarget(leaves, 1710, resolveTarget);
expect(out).toHaveLength(leaves.length + 1);
expect(out[out.length - 1]).toMatchObject({ id: 1710, is_inputable: false });
// originals preserved, in order
expect(out.slice(0, 3).map((c) => c.id)).toEqual([1111, 1121, 1131]);
});
it("returns the leaves list unchanged when the non-leaf target cannot be resolved", () => {
// targetId absent from leaves AND resolver returns null (stale id) -> no injection
expect(comboboxCategoriesForTarget(leaves, 9999, resolveTarget)).toBe(leaves);
});
});

View file

@ -79,3 +79,32 @@ export function findTaxonomyCategory(
} }
return null; return null;
} }
/**
* Build the target-picker option list for one migration row.
*
* Returns the leaves-only `leafOptions` unchanged when the row's current target
* is null or already a leaf (the common case same array reference, so the
* combobox never re-renders needlessly).
*
* When the target is a NON-leaf parent absent from the leaves list (a
* low-confidence default like "Divertissement" #1710), the resolved parent is
* appended so the combobox can DISPLAY it as the current value. It stays visible
* (and, since `CategoryCombobox` renders every option, clickable) but selecting
* it is a no-op: the row's resolve handler only accepts ids present in the
* leaves list, so re-picking the parent changes nothing and the user is steered
* to a leaf. New picks that take effect are therefore always leaves.
*
* Pure helper no React, no i18n, no DB.
*/
export function comboboxCategoriesForTarget(
leafOptions: Category[],
targetId: number | null,
resolveTarget: (id: number) => Category | null,
): Category[] {
if (targetId == null || leafOptions.some((c) => c.id === targetId)) {
return leafOptions;
}
const current = resolveTarget(targetId);
return current ? [...leafOptions, current] : leafOptions;
}