refactor: consolidate menu snippets and strengthen type safety

Replaces duplicated dropdown/context menu code with parameterized snippet, eliminates unsafe type casts, adds error handling for clipboard and API calls, and improves accessibility annotations.
2026-01-31 12:23:33 -06:00 · 2026-01-29 11:40:55 -06:00
parent b5eaedc9bc
commit 61f8bd9de7
10 changed files with 148 additions and 120 deletions
@@ -0,0 +1,8 @@
+export type { CodeDescription } from "./CodeDescription";
+export type { CourseResponse } from "./CourseResponse";
+export type { DbMeetingTime } from "./DbMeetingTime";
+export type { InstructorResponse } from "./InstructorResponse";
+export type { SearchResponse } from "./SearchResponse";
+export type { ServiceInfo } from "./ServiceInfo";
+export type { ServiceStatus } from "./ServiceStatus";
+export type { StatusResponse } from "./StatusResponse";
@@ -18,11 +18,15 @@ let copiedEmail: string | null = $state(null);

 async function copyEmail(email: string, event: MouseEvent) {
  event.stopPropagation();
-  await navigator.clipboard.writeText(email);
-  copiedEmail = email;
-  setTimeout(() => {
-    copiedEmail = null;
-  }, 2000);
+  try {
+    await navigator.clipboard.writeText(email);
+    copiedEmail = email;
+    setTimeout(() => {
+      copiedEmail = null;
+    }, 2000);
+  } catch (err) {
+    console.error("Failed to copy email:", err);
+  }
 }
 </script>

@@ -42,8 +46,8 @@ async function copyEmail(email: string, event: MouseEvent) {
                  class="inline-flex items-center gap-1.5 text-sm font-medium bg-card border border-border rounded-md px-2.5 py-1 text-foreground hover:border-foreground/20 hover:bg-card/80 transition-colors"
                >
                  {instructor.displayName}
-                  {#if 'rmpRating' in instructor && instructor.rmpRating}
-                    {@const rating = instructor.rmpRating as number}
+                  {#if instructor.rmpRating != null}
+                    {@const rating = instructor.rmpRating}
                    <span
                      class="text-[10px] font-semibold {rating >= 4.0 ? 'text-status-green' : rating >= 3.0 ? 'text-yellow-500' : 'text-status-red'}"
                    >{rating.toFixed(1)}★</span>
@@ -59,9 +63,9 @@ async function copyEmail(email: string, event: MouseEvent) {
                  {#if instructor.isPrimary}
                    <div class="text-muted-foreground">Primary instructor</div>
                  {/if}
-                  {#if 'rmpRating' in instructor && instructor.rmpRating}
+                  {#if instructor.rmpRating != null}
                    <div class="text-muted-foreground">
-                      {(instructor.rmpRating as number).toFixed(1)}/5 ({(instructor as any).rmpNumRatings ?? 0} ratings)
+                      {instructor.rmpRating.toFixed(1)}/5 ({instructor.rmpNumRatings ?? 0} ratings)
                    </div>
                  {/if}
                  {#if instructor.email}
@@ -262,97 +262,56 @@ const table = createSvelteTable({
 });
 </script>

-{#snippet columnVisibilityItems(variant: "dropdown" | "context")}
-    {#if variant === "dropdown"}
-        <DropdownMenu.Group>
-            <DropdownMenu.GroupHeading
-                class="px-2 py-1.5 text-xs font-medium text-muted-foreground"
+{#snippet columnVisibilityGroup(
+    Group: typeof DropdownMenu.Group,
+    GroupHeading: typeof DropdownMenu.GroupHeading,
+    CheckboxItem: typeof DropdownMenu.CheckboxItem,
+    Separator: typeof DropdownMenu.Separator,
+    Item: typeof DropdownMenu.Item,
+)}
+    <Group>
+        <GroupHeading
+            class="px-2 py-1.5 text-xs font-medium text-muted-foreground"
+        >
+            Toggle columns
+        </GroupHeading>
+        {#each columns as col}
+            {@const id = col.id!}
+            {@const label =
+                typeof col.header === "string" ? col.header : id}
+            <CheckboxItem
+                checked={columnVisibility[id] !== false}
+                closeOnSelect={false}
+                onCheckedChange={(checked) => {
+                    columnVisibility = {
+                        ...columnVisibility,
+                        [id]: checked,
+                    };
+                }}
+                class="relative flex items-center gap-2 rounded-sm px-2 py-1.5 text-sm cursor-pointer select-none outline-none data-highlighted:bg-accent data-highlighted:text-accent-foreground"
            >
-                Toggle columns
-            </DropdownMenu.GroupHeading>
-            {#each columns as col}
-                {@const id = col.id!}
-                {@const label =
-                    typeof col.header === "string" ? col.header : id}
-                <DropdownMenu.CheckboxItem
-                    checked={columnVisibility[id] !== false}
-                    closeOnSelect={false}
-                    onCheckedChange={(checked) => {
-                        columnVisibility = {
-                            ...columnVisibility,
-                            [id]: checked,
-                        };
-                    }}
-                    class="relative flex items-center gap-2 rounded-sm px-2 py-1.5 text-sm cursor-pointer select-none outline-none data-highlighted:bg-accent data-highlighted:text-accent-foreground"
-                >
-                    {#snippet children({ checked })}
-                        <span
-                            class="flex size-4 items-center justify-center rounded-sm border border-border"
-                        >
-                            {#if checked}
-                                <Check class="size-3" />
-                            {/if}
-                        </span>
-                        {label}
-                    {/snippet}
-                </DropdownMenu.CheckboxItem>
-            {/each}
-        </DropdownMenu.Group>
-        {#if hasCustomVisibility}
-            <DropdownMenu.Separator class="mx-1 my-1 h-px bg-border" />
-            <DropdownMenu.Item
-                class="flex items-center gap-2 rounded-sm px-2 py-1.5 text-sm cursor-pointer select-none outline-none data-highlighted:bg-accent data-highlighted:text-accent-foreground"
-                onSelect={resetColumnVisibility}
-            >
-                <RotateCcw class="size-3.5" />
-                Reset to default
-            </DropdownMenu.Item>
-        {/if}
-    {:else}
-        <ContextMenu.Group>
-            <ContextMenu.GroupHeading
-                class="px-2 py-1.5 text-xs font-medium text-muted-foreground"
-            >
-                Toggle columns
-            </ContextMenu.GroupHeading>
-            {#each columns as col}
-                {@const id = col.id!}
-                {@const label =
-                    typeof col.header === "string" ? col.header : id}
-                <ContextMenu.CheckboxItem
-                    checked={columnVisibility[id] !== false}
-                    closeOnSelect={false}
-                    onCheckedChange={(checked) => {
-                        columnVisibility = {
-                            ...columnVisibility,
-                            [id]: checked,
-                        };
-                    }}
-                    class="relative flex items-center gap-2 rounded-sm px-2 py-1.5 text-sm cursor-pointer select-none outline-none data-highlighted:bg-accent data-highlighted:text-accent-foreground"
-                >
-                    {#snippet children({ checked })}
-                        <span
-                            class="flex size-4 items-center justify-center rounded-sm border border-border"
-                        >
-                            {#if checked}
-                                <Check class="size-3" />
-                            {/if}
-                        </span>
-                        {label}
-                    {/snippet}
-                </ContextMenu.CheckboxItem>
-            {/each}
-        </ContextMenu.Group>
-        {#if hasCustomVisibility}
-            <ContextMenu.Separator class="mx-1 my-1 h-px bg-border" />
-            <ContextMenu.Item
-                class="flex items-center gap-2 rounded-sm px-2 py-1.5 text-sm cursor-pointer select-none outline-none data-highlighted:bg-accent data-highlighted:text-accent-foreground"
-                onSelect={resetColumnVisibility}
-            >
-                <RotateCcw class="size-3.5" />
-                Reset to default
-            </ContextMenu.Item>
-        {/if}
+                {#snippet children({ checked })}
+                    <span
+                        class="flex size-4 items-center justify-center rounded-sm border border-border"
+                    >
+                        {#if checked}
+                            <Check class="size-3" />
+                        {/if}
+                    </span>
+                    {label}
+                {/snippet}
+            </CheckboxItem>
+        {/each}
+    </Group>
+    {#if hasCustomVisibility}
+        <Separator class="mx-1 my-1 h-px bg-border" />
+        <Item
+            class="flex items-center gap-2 rounded-sm px-2 py-1.5 text-sm cursor-pointer select-none outline-none data-highlighted:bg-accent data-highlighted:text-accent-foreground"
+            onSelect={resetColumnVisibility}
+        >
+            <RotateCcw class="size-3.5" />
+            Reset to default
+        </Item>
    {/if}
 {/snippet}

@@ -379,7 +338,13 @@ const table = createSvelteTable({
                                {...props}
                                transition:fly={{ duration: 150, y: -10 }}
                            >
-                                {@render columnVisibilityItems("dropdown")}
+                                {@render columnVisibilityGroup(
+                    DropdownMenu.Group,
+                    DropdownMenu.GroupHeading,
+                    DropdownMenu.CheckboxItem,
+                    DropdownMenu.Separator,
+                    DropdownMenu.Item,
+                )}
                            </div>
                        </div>
                    {/if}
@@ -574,6 +539,7 @@ const table = createSvelteTable({
                                        )}
                                        {@const display = primaryInstructorDisplay(course)}
                                        {@const commaIdx = display.indexOf(", ")}
+                                        {@const ratingData = primaryRating(course)}
                                        <td class="py-2 px-2 whitespace-nowrap">
                                            {#if display === "Staff"}
                                                <span
@@ -597,22 +563,20 @@ const table = createSvelteTable({
                                                    {/if}
                                                </SimpleTooltip>
                                            {/if}
-                                            {#if primaryRating(course)}
-                                                {@const r =
-                                                    primaryRating(course)!}
+                                            {#if ratingData}
                                                <SimpleTooltip
-                                                    text="{r.rating.toFixed(
+                                                    text="{ratingData.rating.toFixed(
                                                        1,
-                                                    )}/5 ({r.count} ratings on RateMyProfessors)"
+                                                    )}/5 ({ratingData.count} ratings on RateMyProfessors)"
                                                    delay={150}
                                                    side="bottom"
                                                    passthrough
                                                >
                                                    <span
                                                        class="ml-1 text-xs font-medium {ratingColor(
-                                                            r.rating,
+                                                            ratingData.rating,
                                                        )}"
-                                                        >{r.rating.toFixed(
+                                                        >{ratingData.rating.toFixed(
                                                            1,
                                                        )}★</span
                                                    >
@@ -772,7 +736,13 @@ const table = createSvelteTable({
                                in:fade={{ duration: 100 }}
                                out:fade={{ duration: 100 }}
                            >
-                                {@render columnVisibilityItems("context")}
+                                {@render columnVisibilityGroup(
+                    ContextMenu.Group,
+                    ContextMenu.GroupHeading,
+                    ContextMenu.CheckboxItem,
+                    ContextMenu.Separator,
+                    ContextMenu.Item,
+                )}
                            </div>
                        </div>
                    {/if}
@@ -29,6 +29,7 @@ let {
  <input
    type="text"
    placeholder="Search courses..."
+    aria-label="Search courses"
    bind:value={query}
    class="h-9 border border-border bg-card text-foreground rounded-md px-3 text-sm flex-1 min-w-[200px]
           focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-offset-2 focus-visible:ring-offset-background
@@ -58,12 +58,15 @@ $effect(() => {
    if (!o) searchValue = "";
  }}
 >
+  <!-- svelte-ignore a11y_no_static_element_interactions -->
  <div
    class="relative h-9 rounded-md border border-border bg-card
           flex items-center w-40 cursor-pointer
           has-[:focus-visible]:ring-2 has-[:focus-visible]:ring-ring has-[:focus-visible]:ring-offset-2 has-[:focus-visible]:ring-offset-background"
+    role="presentation"
    bind:this={containerEl}
    onclick={() => { containerEl?.querySelector('input')?.focus(); }}
+    onkeydown={() => { containerEl?.querySelector('input')?.focus(); }}
  >
    <Combobox.Input
      oninput={(e) => (searchValue = e.currentTarget.value)}
@@ -186,14 +186,35 @@ describe("abbreviateInstructor", () => {
 describe("getPrimaryInstructor", () => {
  it("returns primary instructor", () => {
    const instructors: InstructorResponse[] = [
-      { bannerId: "1", displayName: "A", email: null, isPrimary: false },
-      { bannerId: "2", displayName: "B", email: null, isPrimary: true },
+      {
+        bannerId: "1",
+        displayName: "A",
+        email: null,
+        isPrimary: false,
+        rmpRating: null,
+        rmpNumRatings: null,
+      },
+      {
+        bannerId: "2",
+        displayName: "B",
+        email: null,
+        isPrimary: true,
+        rmpRating: null,
+        rmpNumRatings: null,
+      },
    ];
    expect(getPrimaryInstructor(instructors)?.displayName).toBe("B");
  });
  it("returns first instructor when no primary", () => {
    const instructors: InstructorResponse[] = [
-      { bannerId: "1", displayName: "A", email: null, isPrimary: false },
+      {
+        bannerId: "1",
+        displayName: "A",
+        email: null,
+        isPrimary: false,
+        rmpRating: null,
+        rmpNumRatings: null,
+      },
    ];
    expect(getPrimaryInstructor(instructors)?.displayName).toBe("A");
  });
@@ -26,8 +26,19 @@ onMount(() => {
    },
  });

+  const unwatch = $effect.root(() => {
+    $effect(() => {
+      osInstance.options({
+        scrollbars: {
+          theme: themeStore.isDark ? "os-theme-dark" : "os-theme-light",
+        },
+      });
+    });
+  });
+
  return () => {
-    osInstance?.destroy();
+    unwatch();
+    osInstance.destroy();
  };
 });
 </script>
@@ -62,11 +62,16 @@ let error = $state<string | null>(null);
 $effect(() => {
  const term = selectedTerm;
  if (!term) return;
-  client.getSubjects(term).then((s) => {
-    subjects = s;
-    const validCodes = new Set(s.map((sub) => sub.code));
-    selectedSubjects = selectedSubjects.filter((code) => validCodes.has(code));
-  });
+  client
+    .getSubjects(term)
+    .then((s) => {
+      subjects = s;
+      const validCodes = new Set(s.map((sub) => sub.code));
+      selectedSubjects = selectedSubjects.filter((code) => validCodes.has(code));
+    })
+    .catch((e) => {
+      console.error("Failed to fetch subjects:", e);
+    });
 });

 // Centralized throttle configuration - maps trigger source to throttle delay (ms)
@@ -3,6 +3,11 @@ import { BannerApiClient } from "$lib/api";

 export const load: PageLoad = async ({ url, fetch }) => {
  const client = new BannerApiClient(undefined, fetch);
-  const terms = await client.getTerms();
-  return { terms, url };
+  try {
+    const terms = await client.getTerms();
+    return { terms, url };
+  } catch (e) {
+    console.error("Failed to load terms:", e);
+    return { terms: [], url };
+  }
 };