apps/desktop/src/components/sidebar/SidebarModal.tsx (1)

18-30: Consider removing unused isFocused tracking.

The isFocused state is tracked (lines 8-16) and included in the effect dependencies (line 30), but it's no longer used after removing the focus check from the Escape key handler (line 20).

Apply this diff to remove the unused state:

 export default function SidebarModal() {
     const { isOpen, content, setOpen } = useSidebarModalStore();
-    const [isFocused, setIsFocused] = useState(false);
-
-    const handleFocus = () => {
-        setIsFocused(true);
-    };
-
-    const handleBlur = () => {
-        setIsFocused(false);
-    };

     useEffect(() => {
         const handleKeyDown = (event: KeyboardEvent) => {
             if (event.key === "Escape") {
                 setOpen(false);
             }
         };

         window.addEventListener("keydown", handleKeyDown);

         return () => {
             window.removeEventListener("keydown", handleKeyDown);
         };
-    }, [setOpen, isFocused]);
+    }, [setOpen]);

     return (
         <div
             tabIndex={0}
-            onFocus={handleFocus}
-            onBlur={handleBlur}
             className={`rounded-6 border-stroke bg-modal shadow-fg-1 backdrop-blur-32 absolute top-0 left-[50px] z-40 h-full min-h-0 max-w-[36rem] overflow-hidden border p-12 outline-hidden ${
                 isOpen ? "animate-scale-in flex" : "hidden"
             }`}
         >
             {content}
         </div>
     );
 }

apps/desktop/src/__mocks__/generators.ts (1)

362-401: Redundant type assertion.

The explicit as DatabaseMarcherPage assertion on line 400 is unnecessary since the function's return type is already declared as DatabaseMarcherPage (line 372). TypeScript will automatically enforce this type constraint.

Apply this diff to remove the redundant assertion:

     return {
         id,
         marcher_id: marcher.id,
         page_id: page.id,
         x,
         y,
         path_data_id: pathDataId,
         path_start_position: pathStartPosition,
         path_end_position: pathEndPosition,
         notes,
         rotation_degrees: rotationDegrees,
         created_at: faker.date.recent().toISOString(),
         updated_at: faker.date.recent().toISOString(),
-    } as DatabaseMarcherPage;
+    };
 };

apps/desktop/src/components/tags/TagsModel.tsx (1)

1-5: Filename typo: "TagsModel.tsx" should likely be "TagsModal.tsx".

The filename contains "Model" but the component is TagsModalLauncher, which suggests this should be TagsModal.tsx for consistency with the component naming.

apps/desktop/src/components/ui/ColorPicker.tsx (2)

36-47: Docstring mentions non-existent size parameter.

Line 44 documents a size parameter ("Compact or default") that doesn't exist in the component's props interface.

Apply this diff:

 * @param defaultColor - The default color of the color picker
 * @param onChange - The function to call when the color changes
+* @param onBlur - The function to call when the color picker loses focus
 * @param className - The class name of the color picker
-* @param size - Compact or default
 * @param doNotUseForm - Whether to use the form field or not (if true, the color picker will not be inside a form field)

---

129-196: Significant code duplication between form and non-form paths.

The popover content (lines 129-196) is nearly identical to DefaultColorPicker (lines 222-290). Since DefaultColorPicker already exists, reuse it in the else branch to eliminate duplication.

Apply this diff to the else branch:

     else
         return (
             <FormField
                 label={label}
                 tooltip={tooltip}
                 ref={pickerRef}
                 className={className}
             >
-                <div className="flex items-center gap-8">
-                    <Popover.Root>
-                        ... (duplicate content)
-                    </Popover.Root>
-
-                    <Button
-                        ... (duplicate content)
-                    </Button>
-                </div>
+                {internalComponent}
             </FormField>
         );

Also applies to: 222-290

apps/desktop/src/global/classes/__test__/MarcherPage.test.ts (1)

65-65: Consider removing Partial<> type wrapper for stronger type safety.

The test data is declared as Partial<DatabaseMarcherPage>[] but then cast to DatabaseMarcherPage[] when calling the function. Since the test data appears to include all required fields, using Partial<> with an explicit cast undermines TypeScript's compile-time type checking and could mask missing fields.

Apply this diff to use the full type:

-        const testMarcherPages: Partial<DatabaseMarcherPage>[] = [
+        const testMarcherPages: DatabaseMarcherPage[] = [
             {
                 id: 1,
                 marcher_id: 1,
                 ...
             },
         ];

         const result = databaseMarcherPagesToMarcherPages(
-            testMarcherPages as DatabaseMarcherPage[],
+            testMarcherPages,
         );

Also applies to: 111-111

apps/desktop/src/hooks/queries/usePages.ts (1)

243-249: Parameter naming inconsistency: tx used for DbConnection.

The getPagesInOrder function is called with { tx: db }, but db is a DbConnection, not a DbTransaction. While the function signature accepts DbConnection | DbTransaction, the parameter name tx (short for transaction) is misleading when passing a connection.

Consider renaming the parameter in getPagesInOrder to something like conn or keeping it generic, or update the call site comment to clarify. This is a minor naming nit.

-        queryFn: async () => await getPagesInOrder({ tx: db }),
+        queryFn: async () => await getPagesInOrder({ tx: db }), // db is DbConnection, tx accepts both

apps/desktop/src/components/inspector/MarcherEditor.tsx (1)

485-493: Redundant Set creation for selectedMarcherIds.

selectedMarcherIds is already computed as a Set via useMemo at line 186-189. Creating a new Set here is redundant.

                            <TagButtons
-                                selectedMarcherIds={
-                                    new Set(
-                                        selectedMarchers.map(
-                                            (marcher) => marcher.id,
-                                        ),
-                                    )
-                                }
+                                selectedMarcherIds={selectedMarcherIds}
                            />

apps/desktop/e2e/utils/marchers.mts (1)

1-24: New e2e helper for creating marchers.

The helper encapsulates the marcher creation flow well. However, some selectors may be fragile:

1. Line 10: { name: "Other" } - this appears to be a fallback/placeholder label. Consider using a more stable selector or adding a data-testid.

2. Lines 19-23: The selector .filter({ hasText: "MarchersMarcher" }).nth(4) is fragile as it depends on specific DOM structure and index positioning. Consider adding a data-testid to the target element for more reliable selection.

-    await page
-        .locator("div")
-        .filter({ hasText: "MarchersMarcher" })
-        .nth(4)
-        .press("Escape");
+    // Consider using a more stable selector like:
+    // await page.getByTestId("marcher-list-container").press("Escape");
+    await page
+        .locator("div")
+        .filter({ hasText: "MarchersMarcher" })
+        .nth(4)
+        .press("Escape");

apps/desktop/src/components/marcher/MarcherList.tsx (1)

318-327: Type inconsistency: RefObject vs MutableRefObject for changesRef.

VirtualizedMarcherRowsProps uses RefObject<{ [key: number | string]: unknown }> (line 326), but MarcherRowProps uses MutableRefObject<{ [key: number | string]: unknown }> (line 403). Since changesRef is being mutated (values are assigned in handleChange), MutableRefObject is the correct type.

 interface VirtualizedMarcherRowsProps {
     localMarchers: Marcher[];
     isEditing: boolean;
     handleChange: (value: string, attribute: string, marcherId: number) => void;
     handleDeleteMarcher: (marcherId: number) => void;
     sectionOptions: SectionOption[];
     sectionLabelMap: Record<string, string>;
     selectPlaceholder: string;
-    changesRef: RefObject<{ [key: number | string]: unknown }>;
+    changesRef: MutableRefObject<{ [key: number | string]: unknown }>;
 }

apps/desktop/src/global/classes/canvasObjects/OpenMarchCanvas.ts (1)

23-23: Type workaround in getRoundedCoordinate is safe but could be tightened

Switching fakeMarcherPage to Partial<MarcherPage> and casting to MarcherPage only at the getRoundCoordinates call avoids compile-time breakage from newly required fields and doesn’t change runtime behavior, since only x and y are used.

You might consider defining a narrower helper type (e.g. type CoordMarcherPage = Pick<MarcherPage, "id" | "marcher_id" | "page_id" | "x" | "y">;) and updating getRoundCoordinates to accept that (or just { x, y }) long-term, to drop the as MarcherPage cast.

Also applies to: 1365-1382

apps/desktop/src/components/exporting/MarcherCoordinateSheet.tsx (1)

58-60: Narrowed marcher-page type and row rendering look correct

Restricting marcherPages to a Pick of only the fields needed for coordinate output is a nice decoupling from the full MarcherPage/appearance surface, and state initialization follows that type correctly. The new <tbody> implementation cleanly joins each marcher page to its Page, uses measureRangeString(page) when includeMeasuresState is true, and renders X/Y via ReadableCoords with the configured rounding and terse/verbose modes. No functional issues spotted here.

If you end up reusing this subset elsewhere, consider extracting a shared alias (e.g. type MarcherPageCoords = Pick<MarcherPage, "id" | "marcher_id" | "page_id" | "x" | "y">;) to avoid repeating the Pick.

Also applies to: 244-247, 311-312, 526-596

apps/desktop/src/components/marcher/appearance/SectionAppearanceList.tsx (1)

62-71: Align availableSections logic and button disabled state with intent

Right now availableSections is all section names (lines 64–67) while the comment says “sections without appearances”, and the “Add section style” trigger is only disabled when availableSections.length === 0 (Line 97), which will never happen as long as SECTIONS is non‑empty. The real constraint is whether there is any section not present in takenSections.

Consider:

• Deriving availableSections from SECTIONS but filtering out takenSections, and
• Using that filtered list both for the “All” tab and for the disabled condition on the trigger.

For example:

-    const defaultShapeType = "circle";
-
-    // Get available sections (sections without appearances)
-    const availableSections = Object.values(SECTIONS).map(
-        (section) => section.name,
-    );
+    const defaultShapeType = "circle";
@@
-    /** A set of all sections that have an appearance */
-    const takenSections = useMemo(() => {
-        return new Set(
-            sectionAppearancesSorted.map((appearance) => appearance.section),
-        );
-    }, [sectionAppearancesSorted]);
+    /** A set of all sections that have an appearance */
+    const takenSections = useMemo(
+        () =>
+            new Set(
+                sectionAppearancesSorted.map(
+                    (appearance) => appearance.section,
+                ),
+            ),
+        [sectionAppearancesSorted],
+    );
+
+    // Sections without existing appearances
+    const availableSections = useMemo(
+        () =>
+            Object.values(SECTIONS)
+                .map((section) => section.name)
+                .filter((name) => !takenSections.has(name)),
+        [takenSections],
+    );

The rest of the code (trigger disabled={availableSections.length === 0} and availableSections.map(...) in the “All” tab) can remain as is and will now reflect the actual availability.

Also applies to: 95-103, 156-179

apps/desktop/src/db-functions/__test__/tag.test.ts (1)

287-425: Consider avoiding fc.sample inside properties for clearer shrinking and reproducibility

The property-based tests under "multiple appearances of same tag" (around Lines 289–425) create additional randomness via fc.sample(...) inside the fc.property body. This works, but:

• It makes failing cases slightly harder to reproduce from a seed.
• It bypasses fast-check’s normal shrinking on those inner generated values.

If you ever need to debug a failing case here, it will be simpler to express these as proper arbitraries instead, for example by:

• Defining an arbitrary for the per-tag appearance start pages (e.g., fc.uniqueArray(fc.integer(...))), and
• Passing that arbitrary as a normal argument to fc.property instead of sampling inside.

This is a style/perf tweak only; the current tests are logically correct.

apps/desktop/src/components/marcher/appearance/AppearanceModal.tsx (1)

9-36: Clarify naming and hard localStorage dependency in AppearanceModal

Two minor points:

1. The default export is still called MetronomeModal (Lines 9–24) even though this file is AppearanceModal.tsx and the contents are all appearance-related. Renaming the component (and call sites) to something like AppearanceModal or AppearanceModalLauncher would make this easier to discover and reason about.

2. AppearanceModalContents reads from localStorage at render time (Lines 33–36). That’s fine in the Electron renderer, but it will throw if this component is ever imported in a non-browser test environment. A small guard keeps it robust:

-    const localStorageMode = localStorage.getItem("marcher-appearance-mode");
+    const localStorageMode =
+        typeof window !== "undefined"
+            ? window.localStorage.getItem("marcher-appearance-mode")
+            : null;

Both are non-breaking cleanups that improve maintainability and testability.

apps/desktop/src/components/canvas/Canvas.tsx (1)

10-11: Avoid duplicating marcher fetching now that marcherWithVisualsQueryOptions exists

The new marcherWithVisualsQueryOptions integration (Lines 10–11, 56–58) looks good, and the additional guards on marcherVisuals == null before rendering are sensible.

One small inefficiency: you still have a separate useQuery(allMarchersQueryOptions()) (Line 53), but marcherWithVisualsQueryOptions itself calls queryClient.fetchQuery(allMarchersQueryOptions()) internally when building the visual groups. That means:

• Marcher data is fetched twice at the query layer (once via useQuery, once via fetchQuery), and
• In several effects (Lines ~220–289, 305–374, 438–447) you only really need marcher IDs, which can likely be derived from marcherVisuals instead.

If you want to tighten this up, you could:

• Drop the top-level const { data: marchers } = useQuery(allMarchersQueryOptions());, and
• Derive IDs from marcherVisuals (e.g., Object.keys(marcherVisuals).map(Number) or via a helper on the visual group structure) where you currently rely on marchers.

This would centralize marcher data loading in the visuals query and avoid redundant fetching, while keeping the current rendering guards intact.

Also applies to: 56-58, 220-289, 305-374, 438-447

apps/desktop/src/components/marcher/appearance/TagAppearanceList.tsx (1)

34-36: Map stores tag.name instead of the derived display name.

tagNamesByTagId stores tag.name which can be null, but consumers call getTagName with this value anyway. This works but is slightly misleading since the map doesn't contain "tag names" but raw name values.

Consider storing the derived name directly for clarity:

     const tagNamesByTagId = useMemo(() => {
-        return new Map(allTags?.map((tag) => [tag.id, tag.name]) ?? []);
+        return new Map(
+            allTags?.map((tag) => [
+                tag.id,
+                getTagName({ tag_id: tag.id, name: tag.name }),
+            ]) ?? [],
+        );
     }, [allTags]);

apps/desktop/src/components/inspector/marcher/TagEditor.tsx (3)

98-98: Non-null assertion on context could cause runtime error.

useSelectedPage()! assumes the context is always available. If this component is rendered outside the provider, it will throw.

Consider adding a guard or using optional chaining:

-    const { selectedPage } = useSelectedPage()!;
+    const selectedPageContext = useSelectedPage();
+    const selectedPage = selectedPageContext?.selectedPage;

---

193-196: Hardcoded UI strings should use i18n (Tolgee).

Several user-facing strings are hardcoded instead of using the <T> component or t() function: "Rename tag", "Edit appearance", "Delete tag".

Replace hardcoded strings with translation keys:

-                            <label className="text-text-subtitle text-sm">
-                                Rename tag
-                            </label>
+                            <label className="text-text-subtitle text-sm">
+                                <T keyName="tags.rename" />
+                            </label>

Also applies to: 215-216, 221-222, 228-229

---

308-313: Another hardcoded string that should be internationalized.

"Define a name for the new tag (optional)" should use a translation key.

apps/desktop/src/components/marcher/appearance/AppearanceEditor.tsx (1)

56-68: Unnecessary async/await on non-async callback.

handleUpdateAppearance returns void per the type signature, so the async keyword and await are unnecessary and may cause confusion.

-    async function handleChange(changes: Partial<AppearanceComponentOptional>) {
-        await handleUpdateAppearance({
+    function handleChange(changes: Partial<AppearanceComponentOptional>) {
+        handleUpdateAppearance({
             visible:
                 changes.visible !== undefined
                     ? changes.visible
                     : appearance.visible,
             label_visible:
                 changes.label_visible !== undefined
                     ? changes.label_visible
                     : appearance.label_visible,
             ...changes,
         });
     }

apps/desktop/src/components/toolbar/tabs/SelectTab.tsx (2)

19-19: Import from index file instead of specific module.

Based on learnings, db-functions should be imported from the index file.

-import { getTagName } from "@/db-functions/tag";
+import { getTagName } from "@/db-functions";

---

45-45: Non-null assertion on context hook.

Similar to TagEditor.tsx, useSelectedMarchers()! assumes the context is always available.

Consider adding a guard:

-    const { selectedMarchers, setSelectedMarchers } = useSelectedMarchers()!;
+    const context = useSelectedMarchers();
+    if (!context) {
+        throw new Error("useHandleSelect must be used within SelectedMarchersProvider");
+    }
+    const { selectedMarchers, setSelectedMarchers } = context;

.cursor/rules/basic-database-interaction.mdc (1)

139-162: Consider returning early without warning for expected empty input.

The console.warn on line 148 may be noisy if empty arrays are a valid use case. Consider whether this should be a silent early return or if the warning is intentional for debugging.

 export async function create{tableName}s({
     newItems,
     db,
 }: {
     newItems: New{tableName}Args[];
     db: DbConnection;
 }): Promise<Database{tableName}[]> {
     if (newItems.length === 0) {
-        console.warn("No new items to create");
         return [];
     }

apps/desktop/src/global/classes/MarcherVisualGroup.ts (1)

125-146: Complex visibility logic may benefit from clarification.

The isHidden method's logic is subtle: when there are multiple appearances and the first (marcherPage) is visible, it checks the second appearance's visibility. The inline comment explains this, but the logic could be clearer.

Consider extracting the visibility resolution to a helper or adding a more explicit comment about why marcherPage visibility is skipped when true:

     static isHidden(
         appearancesInput:
             | AppearanceComponentOptional[]
             | AppearanceComponentOptional,
     ) {
         const appearances = Array.isArray(appearancesInput)
             ? appearancesInput
             : [appearancesInput];

-        let visible: boolean;
-        if (appearances.length > 0)
-            if (appearances.length > 1 && appearances[0].visible) {
-                // If there is more than one appearance, and the marcherPage appearance is true, use the next one
-                // This is because the marcherPage will almost always be visible
-                visible = appearances[1].visible;
-            } else {
-                visible = appearances[0].visible;
-            }
-        else visible = true;
+        if (appearances.length === 0) {
+            return false; // Default to visible when no appearances
+        }
+
+        // Priority: If marcherPage (first) is visible and there's a tag/section appearance,
+        // defer to the tag/section appearance since marcherPage.visible is usually true.
+        const visible =
+            appearances.length > 1 && appearances[0].visible
+                ? appearances[1].visible
+                : appearances[0].visible;

         return !visible;
     }

apps/desktop/src/hooks/queries/useMarchersWithVisuals.ts (1)

147-147: Document rationale for eslint-disable.

The @tanstack/query/exhaustive-deps disable suppresses warnings about queryClient not being in the key. Consider adding a brief comment explaining why this is intentional (e.g., queryClient is stable singleton).

-        // eslint-disable-next-line @tanstack/query/exhaustive-deps
+        // eslint-disable-next-line @tanstack/query/exhaustive-deps -- queryClient is a stable singleton from App.tsx
         queryKey: marcherWithVisualsKeys.byPageId(pageId!),

apps/desktop/electron/database/migrations/schema.ts (1)

27-39: Good pattern: reusable appearance columns.

Exporting appearance_columns as a shared object and spreading it into multiple tables (marcher_pages, section_appearances, tag_appearances) is an excellent pattern that:

• Ensures consistency across appearance-related fields
• Reduces duplication
• Makes schema evolution easier

Consider adding JSDoc comments to document each field in appearance_columns, especially shape_type, equipment_name, and equipment_state, to clarify their purpose and expected values.

apps/desktop/src/entity-components/appearance.ts (1)

17-32: Consider edge case handling in parseRgbaColor.

The regex pattern handles both rgb() and rgba() formats correctly, with a sensible default fallback { r: 0, g: 0, b: 0, a: 1 }.

However, the function silently returns a default black color for invalid input. Consider whether this should:

1. Log a warning for invalid color strings
2. Return null instead of a default color
3. Throw an error for invalid input

Example enhancement:

 function parseRgbaColor(colorStr: string): RgbaColor {
     const match = colorStr.match(
         /rgba?\((\d+),\s*(\d+),\s*(\d+)(?:,\s*(\d*\.?\d+))?\)/,
     );
     if (match) {
         return {
             r: parseInt(match[1], 10),
             g: parseInt(match[2], 10),
             b: parseInt(match[3], 10),
             a: match[4] ? parseFloat(match[4]) : 1,
         };
     }
+    console.warn(`Invalid color string: ${colorStr}, using default black`);
     return { r: 0, g: 0, b: 0, a: 1 };
 }

apps/desktop/src/global/classes/canvasObjects/CanvasMarcher.ts (1)

36-37: Verify if dotObject needs to be public.

The dotObject field has been changed from private to public (no explicit modifier). This makes it accessible outside the class, which could lead to unintended mutations.

While this change appears necessary for the setAppearance method to dynamically replace the marker shape, consider:

1. Whether external code needs to access this field
2. If a getter method would be more appropriate
3. Adding documentation about mutability expectations

apps/desktop/src/db-functions/tag.ts (1)

372-385: Inconsistent naming for inTransaction functions.

The tag appearance inTransaction functions (createTagAppearancesInTransaction, updateTagAppearancesInTransaction, deleteTagAppearancesInTransaction) don't follow the underscore-prefix convention used by marcher tag functions (_createMarcherTagsInTransaction, _updateMarcherTagsInTransaction).

Per coding guidelines, private database operation functions should be prefixed with underscore.

Also applies to: 410-436, 463-476

apps/desktop/src/components/ui/ColorPicker.tsx (1)

61-64: Both onChange and onBlur fire on close - potential double callback.

This was previously flagged but remains unaddressed. When handleClose is called, both callbacks fire sequentially, which could cause duplicate state updates if a consumer provides the same handler to both props.

apps/desktop/src/db-functions/tag.ts (1)

101-137: Missing public wrapper functions for updateTags and deleteTags.

Based on coding guidelines, all database mutations should use transactionWithHistory for automatic undo/redo support. While createTags (lines 69-84) correctly wraps _createTagsInTransaction with transactionWithHistory, the updateTagsInTransaction and deleteTagsInTransaction functions are exported directly without public wrappers.

This forces callers to either use the transaction functions directly (bypassing history tracking) or manually wrap them. For consistency and to ensure proper undo/redo support, add public wrapper functions similar to createTags.

Based on learnings, database mutations must use transactionWithHistory for automatic undo/redo support.

apps/desktop/src/components/ui/ColorPicker.tsx (1)

27-27: Consider renaming doNotUseForm to avoid double-negative.

The flag name doNotUseForm requires mental negation to understand behavior. A positive alternative like useStaticField, standalone, or disableFormWrapper would improve readability.

apps/desktop/src/components/inspector/marcher/TagEditor.tsx (1)

34-38: Consider showing a loading state.

The component returns null while tags are loading. For better UX, consider showing a loading indicator or skeleton, especially if the load takes noticeable time.

apps/desktop/src/components/marcher/appearance/AppearanceEditor.tsx (1)

32-37: Minor cleanup: simplify handleChange and tighten React type imports

If handleUpdateAppearance is intended to be synchronous (as currently typed), you can simplify a bit:

• Drop async/await from handleChange and call it directly from handlers, removing the need for void wrappers.
• Import ReactNode from "react" instead of using the React namespace, which avoids relying on a global React type and is more idiomatic.

Example diff:

+import type { ReactNode } from "react";
 import {
     AppearanceComponent,
     AppearanceComponentOptional,
 } from "@/entity-components/appearance";
@@
-const shapeIcons: Record<ShapeType, React.ReactNode> = {
+const shapeIcons: Record<ShapeType, ReactNode> = {
@@
-export function AppearanceEditor({
+export function AppearanceEditor({
@@
-    async function handleChange(changes: Partial<AppearanceComponentOptional>) {
-        await handleUpdateAppearance({
+    function handleChange(changes: Partial<AppearanceComponentOptional>) {
+        handleUpdateAppearance({
             visible:
                 changes.visible !== undefined
                     ? changes.visible
                     : appearance.visible,
@@
     function onColorChange(
         attribute: "fill_color" | "outline_color",
         color: RgbaColor | null,
     ) {
-        void handleChange({ [attribute]: color });
+        handleChange({ [attribute]: color });
     }
@@
     function onShapeChange(shape: string) {
-        // Convert the special "__none__" value to null
-        void handleChange({ shape_type: shape === "__none__" ? null : shape });
+        // Convert the special "__none__" value to null
+        handleChange({ shape_type: shape === "__none__" ? null : shape });
     }
     const handleVisibilityChange = () => {
-        void handleChange({ visible: !appearance.visible });
+        handleChange({ visible: !appearance.visible });
     };
     const handleLabelVisibilityChange = () => {
-        void handleChange({
-            label_visible: !appearance.label_visible,
-        });
+        handleChange({ label_visible: !appearance.label_visible });
     };

If you do expect handleUpdateAppearance to become async, instead update its type to () => Promise<void> so the current async/await + void pattern stays consistent.

Also applies to: 56-88

apps/desktop/src/db-functions/__test__/page.test.ts (1)

2878-3161: Tighten getPagesInOrder tests for robustness and clarity

The tests broadly look good and exercise ordering, mapping, and edge cases. A few small refinements would make them more future‑proof and self‑documenting:

1. Avoid relying on beat.id === beat.position where not necessary
   Several expectations assume specific positions from hard‑coded start_beat IDs (e.g., expecting positions [0, 2, 5, 8] or [0, 1, 3, 7, 10, 12, 15]). If createBeats ever changes how position is assigned, these tests could fail even when getPagesInOrder is still correct.
   Consider:

   • Asserting only that beatObject.position values are strictly ascending (you already do this in the “many pages” test), and
   • Verifying the first page specifically (FIRST_PAGE_ID / FIRST_BEAT_ID), while deriving expectations for other positions by querying schema.beats or by comparing to sorted result.map(r => r.beatObject.position) instead of hard‑coding specific numeric positions.
     This keeps the tests focused on ordering semantics rather than internal assumptions about ID/position coupling.

2. Prefer typed results over any
   In a few places you use let result: any; (e.g., “ordered by beat position ascending”, “join pages with their start beats”, and the last edge‑case test). Since DatabasePageWithBeat is imported and some tests already use it, you could strengthen typing without much friction, for example:

•            let result: any;
  

•            let result: DatabasePageWithBeat[] = [];
             await transaction(db, async (tx) => {
                 result = await getPagesInOrder({ tx });
             });
  

  This keeps the pattern compatible with the existing `transaction` helper while giving you compile‑time checks on the result shape.
  
  
  

3. Align test description with setup for “identical beat positions”
   The test "should handle pages with identical beat positions (should not happen but test robustness)" currently uses createBeats with monotonically increasing positions and creates only a single non‑first page, so it does not actually exercise identical beat positions.
   Either:
   • Adjust the description to something like “should handle additional pages with valid beat positions”, or
   • If you truly want to cover identical beat.position values, insert beats manually via tx.insert(schema.beats) with the same position and create pages pointing at them, then assert that getPagesInOrder still returns without error and preserves a deterministic order.

Overall behavior is correct; these are maintainability and clarity tweaks you can adopt if you think they’re worthwhile.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: CR
Repo: OpenMarch/OpenMarch PR: 0
File: .cursor/rules/basic-database-interaction.mdc:0-0
Timestamp: 2025-11-27T02:03:46.836Z
Learning: Applies to apps/desktop/src/db-functions/*.ts : Database function files must include data transformation functions (e.g., `realDatabase[TableName]ToDatabase[TableName]`) to convert between database types and application types

Learnt from: CR
Repo: OpenMarch/OpenMarch PR: 0
File: .cursor/rules/basic-database-interaction.mdc:0-0
Timestamp: 2025-11-27T02:03:46.836Z
Learning: Applies to apps/desktop/src/db-functions/index.ts : Update `apps/desktop/src/db-functions/index.ts` to export all new database function files

Learnt from: CR
Repo: OpenMarch/OpenMarch PR: 0
File: .cursor/rules/basic-database-interaction.mdc:0-0
Timestamp: 2025-11-27T02:03:46.836Z
Learning: Applies to apps/desktop/src/db-functions/*.ts : Database function files must include proper TypeScript types with `Database[TableName]`, `New[TableName]Args`, and `Modified[TableName]Args` interfaces

Learnt from: CR
Repo: OpenMarch/OpenMarch PR: 0
File: .cursor/rules/basic-database-interaction.mdc:0-0
Timestamp: 2025-11-27T02:03:46.836Z
Learning: Applies to apps/desktop/src/db-functions/*.ts : Database function files must handle database-specific type transformations (e.g., boolean as 0|1 in SQLite) with explicit conversion functions

Learnt from: CR
Repo: OpenMarch/OpenMarch PR: 0
File: .cursor/rules/basic-database-interaction.mdc:0-0
Timestamp: 2025-11-27T02:03:46.836Z
Learning: Applies to apps/desktop/src/db-functions/*.ts : All database mutations (create, update, delete) must use `transactionWithHistory` for automatic undo/redo support

Learnt from: CR
Repo: OpenMarch/OpenMarch PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T02:03:30.741Z
Learning: Applies to **/*.test.ts : Test internal `*Db` functions expecting raw data arrays, test public wrapper functions expecting `DatabaseResponse` format

Learnt from: CR
Repo: OpenMarch/OpenMarch PR: 0
File: .cursor/rules/basic-database-interaction.mdc:0-0
Timestamp: 2025-11-27T02:03:46.836Z
Learning: Applies to apps/desktop/src/db-functions/*.ts : Database function files must implement comprehensive query functions (getAll, getById, etc.) before implementing mutations

Learnt from: CR
Repo: OpenMarch/OpenMarch PR: 0
File: .cursor/rules/component-database-interaction.mdc:0-0
Timestamp: 2025-11-27T02:04:04.374Z
Learning: Applies to **/*.test.ts : Test business logic functions independently of React components using real database operations (no mocks), and verify transaction rollback scenarios and query invalidation behavior

Learnt from: CR
Repo: OpenMarch/OpenMarch PR: 0
File: .cursor/rules/basic-database-interaction.mdc:0-0
Timestamp: 2025-11-27T02:03:46.836Z
Learning: Applies to apps/desktop/src/db-functions/*.ts : Database mutations should implement separate 'inTransaction' functions (e.g., `create[TableName]sInTransaction`) that accept a `DbTransaction` parameter

Learnt from: CR
Repo: OpenMarch/OpenMarch PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T02:03:30.741Z
Learning: Applies to **/*.test.ts : Create foreign key references before testing dependent entities in database tests

Learnt from: CR
Repo: OpenMarch/OpenMarch PR: 0
File: .cursor/rules/basic-database-interaction.mdc:0-0
Timestamp: 2025-11-27T02:03:46.836Z
Learning: Applies to apps/desktop/src/{db-functions,hooks/queries}/*.ts : Follow naming conventions: Database files as `[tableName].ts` (camelCase), hook files as `use[TableName].ts` (PascalCase), functions as `get[TableNames]`, `create[TableNames]`, `update[TableNames]`, `delete[TableNames]`

Learnt from: CR
Repo: OpenMarch/OpenMarch PR: 0
File: .cursor/rules/multi-query-hooks.mdc:0-0
Timestamp: 2025-11-27T02:04:17.663Z
Learning: Applies to src/hooks/**/*.test.ts?(x) : Test early return conditions (incomplete data) in multi-query hook combine functions

Learnt from: CR
Repo: OpenMarch/OpenMarch PR: 0
File: .cursor/rules/multi-query-hooks.mdc:0-0
Timestamp: 2025-11-27T02:04:17.663Z
Learning: Applies to src/hooks/**/*.test.ts?(x) : Test data transformation logic in multi-query hooks with various data combinations

Learnt from: CR
Repo: OpenMarch/OpenMarch PR: 0
File: .cursor/rules/multi-query-hooks.mdc:0-0
Timestamp: 2025-11-27T02:04:17.663Z
Learning: Applies to src/hooks/**/*.test.ts?(x) : Use real query options, not mocks, for integration testing of multi-query hooks

Learnt from: CR
Repo: OpenMarch/OpenMarch PR: 0
File: .cursor/rules/unit-testing.mdc:0-0
Timestamp: 2025-11-27T02:04:29.206Z
Learning: Applies to **/{test,__tests__,tests}/**/*.{test,spec}.{ts,tsx,js,jsx} : Never use mocks for packages or database operations; all functionality should be testable with real implementations, except for mocking React context providers when rendering components that rely on hooks

Learnt from: CR
Repo: OpenMarch/OpenMarch PR: 0
File: .cursor/rules/component-database-interaction.mdc:0-0
Timestamp: 2025-11-27T02:04:04.374Z
Learning: Applies to **/*.ts : Always await when using `transactionWithHistory()` to ensure the database action finishes in time for query invalidation

Learnt from: CR
Repo: OpenMarch/OpenMarch PR: 0
File: .cursor/rules/multi-query-hooks.mdc:0-0
Timestamp: 2025-11-27T02:04:17.663Z
Learning: Applies to src/hooks/**/*.test.ts?(x) : Test multi-query hooks by mocking query options to return loading and error states

Learnt from: CR
Repo: OpenMarch/OpenMarch PR: 0
File: .cursor/rules/repo.mdc:0-0
Timestamp: 2025-11-27T02:04:23.301Z
Learning: Applies to packages/**/*.{ts,tsx} : Prefer to use components in './packages/ui/src/components/base' folder for common items like buttons and inputs

Learnt from: CR
Repo: OpenMarch/OpenMarch PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T02:03:30.741Z
Learning: Applies to **/*.{ts,tsx} : Always use absolute paths (with `@/` alias for `src/` directory) for file operations, never relative paths

Learnt from: CR
Repo: OpenMarch/OpenMarch PR: 0
File: .cursor/rules/repo.mdc:0-0
Timestamp: 2025-11-27T02:04:23.301Z
Learning: Applies to packages/ui/src/components/**/*.{ts,tsx} : All styles are stored in tailwind.css; match existing style patterns and color usage when making frontend components

Learnt from: CR
Repo: OpenMarch/OpenMarch PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T02:03:30.741Z
Learning: Applies to **/*.{ts,tsx} : Co-locate UI utility helper functions with their components

Learnt from: CR
Repo: OpenMarch/OpenMarch PR: 0
File: .cursor/rules/basic-database-interaction.mdc:0-0
Timestamp: 2025-11-27T02:03:46.836Z
Learning: Applies to apps/desktop/src/db-functions/*.ts : Create a database function file in `apps/desktop/src/db-functions/[tableName].ts` when adding new database table interactions

Learnt from: CR
Repo: OpenMarch/OpenMarch PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T02:03:30.741Z
Learning: Applies to **/*.ts : Locate database functions in `electron/database/tables/` (legacy IPC-based) OR consolidate in `src/global/classes/` (new shared SQL bridge pattern)

Learnt from: CR
Repo: OpenMarch/OpenMarch PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T02:03:30.741Z
Learning: Applies to **/*.ts : Database entity types should use Drizzle inferred types: `typeof table.$inferSelect` and `typeof table.$inferInsert`, never manually defined interfaces that duplicate schema information

Learnt from: CR
Repo: OpenMarch/OpenMarch PR: 0
File: .cursor/rules/component-database-interaction.mdc:0-0
Timestamp: 2025-11-27T02:04:04.374Z
Learning: Applies to **/*[A-Z]*.ts : Keep database operation functions private and prefixed with underscore (_) in FeatureName.ts files, though they can still be exported for testing

Learnt from: CR
Repo: OpenMarch/OpenMarch PR: 0
File: .cursor/rules/component-database-interaction.mdc:0-0
Timestamp: 2025-11-27T02:04:04.374Z
Learning: Applies to **/*[A-Z]*.ts : Always use `transactionWithHistory()` for operations that modify multiple database entities, need undo/redo support, or should be atomic

Learnt from: CR
Repo: OpenMarch/OpenMarch PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T02:03:30.741Z
Learning: Applies to **/*.ts : Internal database functions should use `*Db()` suffix and return raw data types, public wrapper functions should have the same name and apply `DatabaseResponse` wrapper

Learnt from: CR
Repo: OpenMarch/OpenMarch PR: 0
File: .cursor/rules/component-database-interaction.mdc:0-0
Timestamp: 2025-11-27T02:04:04.374Z
Learning: Separate business logic into `.ts` files, React integration into custom hooks, and database operations using transaction functions from `@/db-functions`

apps/desktop/src/components/toolbar/tabs/SelectTab.tsx (3)

223-270: DrillPrefixSelector looks consistent and accessible

The selector correctly derives unique drill prefixes, wires them through to selection, and now uses a specific aria-label="Select drill prefixes", which addresses the earlier accessibility concern for this control. No further changes needed.

---

272-312: FamilySelector grouping and selection logic are clear

Grouping by SectionFamily, filtering sections by family, and then selecting matching marchers is straightforward and easy to follow. The updated aria-label="Select families" also aligns with the control’s purpose.

---

171-221: Fix remaining aria-label and consider distinguishing “no tags” from “not yet loaded”

TagSelector still uses aria-label="Select sections", which is misleading for screen readers and was flagged previously; DrillPrefixSelector and FamilySelector have been corrected, so this one just needs to be brought in line.

Also, as written, when tags haven’t finished loading yet, the UI will show “noTagsToSelect”, which conflates “no data” with “still loading”. If you care about that distinction, you could gate the empty-state message behind a separate “loaded & empty” condition.

Minimal aria-label fix:

-    return (
-        <ToolbarSection aria-label="Select sections">
+    return (
+        <ToolbarSection aria-label="Select tags">

apps/desktop/src/components/toolbar/tabs/SelectTab.tsx (2)

44-71: Use functional state updates in shift-select to avoid stale selection and simplify deps

The shift-select branch closes over selectedMarchers and includes it in the dependency array, which is fine but a bit brittle. Using a functional update removes that dependency and avoids any future stale state surprises if the context becomes more complex.

You could refactor like this:

-    const selectMarcherIds = useCallback(
-        (e: React.MouseEvent<HTMLButtonElement>, ids: number[]) => {
+    const selectMarcherIds = useCallback(
+        (e: React.MouseEvent<HTMLButtonElement>, ids: number[]) => {
             e.preventDefault();
             const newSelectedMarchers = allMarchers.filter((marcher) =>
                 ids.includes(marcher.id),
             );

             // if holding shift, add to selection
             if (e.shiftKey) {
-                const currentlySelectedIds = new Set(
-                    selectedMarchers.map((marcher) => marcher.id),
-                );
-                const newSelectedMarchersToAdd = newSelectedMarchers.filter(
-                    (marcher) => !currentlySelectedIds.has(marcher.id),
-                );
-                setSelectedMarchers([
-                    ...selectedMarchers,
-                    ...newSelectedMarchersToAdd,
-                ]);
+                setSelectedMarchers((prev) => {
+                    const currentlySelectedIds = new Set(
+                        prev.map((marcher) => marcher.id),
+                    );
+                    const newSelectedMarchersToAdd =
+                        newSelectedMarchers.filter(
+                            (marcher) => !currentlySelectedIds.has(marcher.id),
+                        );
+                    return [...prev, ...newSelectedMarchersToAdd];
+                });
             } else {
                 setSelectedMarchers(newSelectedMarchers);
             }
         },
-        [allMarchers, selectedMarchers, setSelectedMarchers],
+        [allMarchers, setSelectedMarchers],
     );

---

314-361: Consider guarding localStorage access and localizing the loading message

Two small polish points in SelectTabContents:

• The initial localStorage.getItem call in the useState initializer assumes a browser-like environment. If this component is ever reused outside the desktop context (e.g., SSR/tests without localStorage), a try/catch or typeof window !== "undefined" guard would make it more robust.
• The "Loading..." string is hard-coded, whereas the rest of the file uses T for i18n. If you already have a shared loading key, wiring it through here would keep localization consistent.

Both are optional, but worth considering for future reuse.

apps/desktop/src/components/marcher/appearance/TagAppearanceList.tsx (1)

95-95: Consider error handling for async operation.

The fire-and-forget pattern (void) could silently swallow errors, leaving the UI in an inconsistent state if the tag appearance creation or scrolling fails.

Consider adding error handling:

-        void checkAndCreateTagAppearance();
+        checkAndCreateTagAppearance().catch((error) => {
+            console.error('Failed to create/scroll to tag appearance:', error);
+            // Optionally show a toast notification
+        });

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: CR
Repo: OpenMarch/OpenMarch PR: 0
File: .cursor/rules/basic-database-interaction.mdc:0-0
Timestamp: 2025-11-27T02:03:46.836Z
Learning: Applies to apps/desktop/src/hooks/queries/index.ts : Update `apps/desktop/src/hooks/queries/index.ts` to export all new React Query hook files

Learnt from: CR
Repo: OpenMarch/OpenMarch PR: 0
File: .cursor/rules/basic-database-interaction.mdc:0-0
Timestamp: 2025-11-27T02:03:46.836Z
Learning: Applies to apps/desktop/src/hooks/queries/use*.ts : React Query mutation options must use selective cache invalidation by invalidating specific query keys rather than blanket invalidations where possible

Learnt from: CR
Repo: OpenMarch/OpenMarch PR: 0
File: .cursor/rules/multi-query-hooks.mdc:0-0
Timestamp: 2025-11-27T02:04:17.663Z
Learning: Applies to src/hooks/use*.ts?(x) : Consider memoization for expensive calculations within data transformations in multi-query hooks

Learnt from: CR
Repo: OpenMarch/OpenMarch PR: 0
File: .cursor/rules/basic-database-interaction.mdc:0-0
Timestamp: 2025-11-27T02:03:46.836Z
Learning: Applies to apps/desktop/src/hooks/queries/use*.ts : React Query mutation options must include `onSuccess` handlers for smart cache invalidation and `onError` handlers with `conToastError` for error notification

Learnt from: CR
Repo: OpenMarch/OpenMarch PR: 0
File: .cursor/rules/basic-database-interaction.mdc:0-0
Timestamp: 2025-11-27T02:03:46.836Z
Learning: Applies to apps/desktop/src/hooks/queries/use*.ts : Create a React Query hook file in `apps/desktop/src/hooks/queries/use[TableName].ts` when adding new database table interactions

Learnt from: CR
Repo: OpenMarch/OpenMarch PR: 0
File: .cursor/rules/multi-query-hooks.mdc:0-0
Timestamp: 2025-11-27T02:04:17.663Z
Learning: Applies to src/hooks/use*.ts?(x) : Avoid inline combine functions in `useQueries` as they cause unnecessary re-renders

Learnt from: CR
Repo: OpenMarch/OpenMarch PR: 0
File: .cursor/rules/basic-database-interaction.mdc:0-0
Timestamp: 2025-11-27T02:03:46.836Z
Learning: Applies to apps/desktop/src/db-functions/index.ts : Update `apps/desktop/src/db-functions/index.ts` to export all new database function files

Learnt from: CR
Repo: OpenMarch/OpenMarch PR: 0
File: .cursor/rules/repo.mdc:0-0
Timestamp: 2025-11-27T02:04:23.301Z
Learning: Applies to packages/**/*.{ts,tsx} : Prefer to use components in './packages/ui/src/components/base' folder for common items like buttons and inputs

apps/desktop/e2e/tests/1-basic-render.spec.mts (3)

1-1: Prefer a type-only import for Page

Since Page is only used as a type, consider switching to a type-only import to keep runtime imports minimal and make intent explicit:

-import { expect, Page } from "@playwright/test";
+import { expect } from "@playwright/test";
+import type { Page } from "@playwright/test";

This keeps the runtime dependency surface a bit cleaner while preserving typings.

---

37-40: Good reuse via canvasIsVisible; consider making the locators more stable

Centralizing the canvas/timeline visibility checks in canvasIsVisible(page) is a nice cleanup and makes the intent of the tests clearer. The only concern is potential flakiness from:

• page.getByText("Timeline") if the label text changes or appears in multiple places.
• page.locator("canvas").nth(1) if the DOM adds/removes/reorders <canvas> elements.

Not a blocker, but if these tests start to flicker, it may be worth switching to a more stable locator (e.g., role, data-testid, or a more specific container for the main canvas) while keeping this helper as the single place to update it.

Also applies to: 51-52, 56-57

---

93-94: nth(1) on the Marchers workspace locator may be brittle

page.locator("#workspace div").filter({ hasText: "Marchers" }).nth(1) assumes a specific ordering/count of matching divs. If the layout or copy changes, this test could start failing even though the UI is still correct.

If possible, consider targeting a more explicit element (role/heading/region or a dedicated test id) so you don't depend on the second matching div:

• e.g., a region/section with accessible name “Marchers”, or
• a specific container element with a stable selector.

This would make the “Sidebars are visible” test more robust to UI tweaks.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: CR
Repo: OpenMarch/OpenMarch PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T02:03:30.741Z
Learning: Use Vitest for unit tests and Playwright for e2e tests