Skip to content

wip #39

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
benhalverson merged 1 commit into from
Mar 7, 2026
Merged

wip #39

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions src/components/ColorPicker.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,17 @@ import userEvent from "@testing-library/user-event";
import { vi } from "vitest";
import ColorPicker from "../components/ColorPicker";
import { ColorProvider } from "../context/ColorContext";
import type { ColorsResponse } from "../interfaces";

vi.mock("../config", () => ({ BASE_URL: "https://example.test" }));

// Mock data for color options
const mockColors: ColorsResponse[] = [
// v1 /colors mock response shape
type V1ColorResponse = {
filament: string;
hexColor: string;
colorTag: string;
};

const mockColors: V1ColorResponse[] = [
{ filament: "PLA", hexColor: "FF5733", colorTag: "Red" },
{ filament: "PLA", hexColor: "33FF57", colorTag: "Green" },
{ filament: "PLA", hexColor: "3357FF", colorTag: "Blue" },
Expand Down
33 changes: 27 additions & 6 deletions src/components/ColorPicker.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,29 @@ const ColorPicker: React.FC<Props> = ({ filamentType }) => {
if (filamentType) url.searchParams.set("filamentType", filamentType);

const response = await fetch(url.toString());
const colors = (await response.json()) as ColorsResponse[];
dispatch({ type: "SET_COLOR_OPTIONS", payload: colors });

// v1 /colors response shape
type V1ColorResponse = {
filament: string;
hexColor: string;
colorTag: string;
};

const rawColors = (await response.json()) as V1ColorResponse[];

const mappedColors: ColorsResponse[] = rawColors.map((c) => ({
name: c.filament,
provider: "", // not provided by v1 API
public: true,
available: true,
profile: c.colorTag,
color: c.filament,
Copy link

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

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

The v1→ColorsResponse mapping sets color (used for the option's accessible label) to c.filament, which will make every radio option label identical (e.g. all "PLA") and loses the actual color name (likely colorTag). Map the display/label field(s) from colorTag instead so options remain distinguishable and accessible.

Suggested change
color: c.filament,
color: c.colorTag,

Copilot uses AI. Check for mistakes.
hexValue: `#${c.hexColor.replace(/^#/, "")}`,
publicId: c.colorTag,
}));

console.log("Fetched colors (v1 mapped):", mappedColors);
dispatch({ type: "SET_COLOR_OPTIONS", payload: mappedColors });
} catch (error) {
console.error("Failed to fetch colors:", error);
Comment on lines +40 to 43
Copy link

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

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

Avoid leaving console.log in the shipping UI component; it will spam browser logs and can leak potentially sensitive data. Prefer removing it or gating behind a debug flag.

Suggested change
console.log("Fetched colors (v1 mapped):", mappedColors);
dispatch({ type: "SET_COLOR_OPTIONS", payload: mappedColors });
} catch (error) {
console.error("Failed to fetch colors:", error);
dispatch({ type: "SET_COLOR_OPTIONS", payload: mappedColors });
} catch (error) {
console.error("Failed to fetch colors:", error);
console.error("Failed to fetch colors:", error);

Copilot uses AI. Check for mistakes.
} finally {
Expand All @@ -40,8 +61,8 @@ const ColorPicker: React.FC<Props> = ({ filamentType }) => {
className="flex items-center space-x-3">
{colorOptions?.map((colorOption, index) => (
<Radio
key={`${colorOption.hexColor}-${index}`}
value={colorOption.hexColor}
key={`${colorOption.hexValue}-${index}`}
value={colorOption.hexValue}
className={({ checked }) =>
`relative -m-0.5 flex cursor-pointer items-center justify-center rounded-full p-0.5 focus:outline-none ${
checked ? "ring-2 ring-offset-1 ring-blue-500" : ""
Expand All @@ -52,9 +73,9 @@ const ColorPicker: React.FC<Props> = ({ filamentType }) => {
<span
aria-hidden="true"
className="h-8 w-8 rounded-full border border-black border-opacity-10"
style={{ backgroundColor: `#${colorOption.hexColor}` }}
style={{ backgroundColor: `${colorOption.hexValue}` }}
/>
<span className="sr-only">{colorOption.colorTag}</span>
<span className="sr-only">{colorOption.color}</span>
{checked && (
<span
className="absolute inset-0 rounded-full ring-2 ring-offset-2"
Expand Down
89 changes: 89 additions & 0 deletions src/components/ColorPickerV2.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
import { Radio, RadioGroup } from "@headlessui/react";
import { useEffect } from "react";
import { BASE_URL } from "../config";
import { useColorContext } from "../context/ColorContext";
import type { ColorsResponse } from "../interfaces";

interface Props {
filamentType: string;
}

// v2 ColorPicker: consumes the new /v2/colors API which already
// returns data in the ColorsResponse shape.
const ColorPickerV2: React.FC<Props> = ({ filamentType }) => {
const { state, dispatch } = useColorContext();
const { colorOptions, isLoading, color } = state;

useEffect(() => {
const fetchColors = async () => {
dispatch({ type: "SET_IS_LOADING", payload: true });
try {
const url = new URL(`${BASE_URL}/v2/colors`);
if (filamentType) url.searchParams.set("profile", filamentType);

const response = await fetch(url.toString());

type V2ColorsEnvelope = {
success: boolean;
message: string;
data: ColorsResponse[];
};

const json = (await response.json()) as V2ColorsEnvelope;
const colors = json.data ?? [];

Comment on lines +32 to +34
Copy link

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

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

fetch response handling doesn’t check response.ok and assumes the body matches V2ColorsEnvelope. If the endpoint returns non-2xx or an error shape, this will either throw or silently set colorOptions incorrectly. Add an ok/status check and validate json.success/json.data before dispatching.

Suggested change
const json = (await response.json()) as V2ColorsEnvelope;
const colors = json.data ?? [];
if (!response.ok) {
console.error(
`Failed to fetch v2 colors: ${response.status} ${response.statusText}`
);
dispatch({ type: "SET_COLOR_OPTIONS", payload: [] });
return;
}
const json = (await response.json()) as Partial<V2ColorsEnvelope>;
if (!json || json.success !== true || !Array.isArray(json.data)) {
console.error(
"Unexpected v2 colors response shape or unsuccessful response:",
json
);
dispatch({ type: "SET_COLOR_OPTIONS", payload: [] });
return;
}
const colors = json.data;

Copilot uses AI. Check for mistakes.
console.log("Fetched colors (v2):", colors);
dispatch({ type: "SET_COLOR_OPTIONS", payload: colors });
} catch (error) {
console.error("Failed to fetch v2 colors:", error);
Comment on lines +35 to +38
Copy link

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

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

Avoid leaving console.log statements in the component; it will clutter production logs for every product page visit. Remove it or guard it behind a debug flag.

Suggested change
console.log("Fetched colors (v2):", colors);
dispatch({ type: "SET_COLOR_OPTIONS", payload: colors });
} catch (error) {
console.error("Failed to fetch v2 colors:", error);
dispatch({ type: "SET_COLOR_OPTIONS", payload: colors });
} catch (error) {
console.error("Failed to fetch v2 colors:", error);
console.error("Failed to fetch v2 colors:", error);

Copilot uses AI. Check for mistakes.
} finally {
dispatch({ type: "SET_IS_LOADING", payload: false });
}
};

if (filamentType) fetchColors();
}, [filamentType, dispatch]);

if (isLoading) return <div>Loading...</div>;

return (
<fieldset aria-label="Choose a color" className="mt-2">
<RadioGroup
value={color}
onChange={(newColor) =>
dispatch({ type: "SET_COLOR", payload: newColor })
}
className="flex items-center space-x-3">
{colorOptions?.map((colorOption, index) => (
<Radio
key={`${colorOption.hexValue}-${index}`}
value={colorOption.hexValue}
className={({ checked }) =>
`relative -m-0.5 flex cursor-pointer items-center justify-center rounded-full p-0.5 focus:outline-none ${
checked ? "ring-2 ring-offset-1 ring-blue-500" : ""
}`
}>
{({ checked }) => (
<>
<span
aria-hidden="true"
className="h-8 w-8 rounded-full border border-black border-opacity-10"
style={{ backgroundColor: `${colorOption.hexValue}` }}
/>
<span className="sr-only">{colorOption.color}</span>
{checked && (
<span
className="absolute inset-0 rounded-full ring-2 ring-offset-2"
aria-hidden="true"
/>
)}
</>
)}
</Radio>
))}
</RadioGroup>
</fieldset>
);
};

export default ColorPickerV2;
19 changes: 19 additions & 0 deletions src/components/ColorPickerWrapper.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import type React from "react";
import { COLOR_PICKER_VERSION } from "../config";
import ColorPicker from "./ColorPicker";
import ColorPickerV2 from "./ColorPickerV2";

interface Props {
filamentType: string;
}

const ColorPickerWrapper: React.FC<Props> = ({ filamentType }) => {
if (COLOR_PICKER_VERSION === "v2") {
return <ColorPickerV2 filamentType={filamentType} />;
}

// Default to v1 implementation
return <ColorPicker filamentType={filamentType} />;
Comment on lines +2 to +16
Copy link

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

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

ColorPickerWrapper imports both v1 and v2 implementations eagerly, so the v2 code ships even when COLOR_PICKER_VERSION is "v1". If bundle size matters, consider lazy-loading the inactive implementation (e.g., via React.lazy or dynamic import()), especially since the wrapper is on a core page.

Suggested change
import { COLOR_PICKER_VERSION } from "../config";
import ColorPicker from "./ColorPicker";
import ColorPickerV2 from "./ColorPickerV2";
interface Props {
filamentType: string;
}
const ColorPickerWrapper: React.FC<Props> = ({ filamentType }) => {
if (COLOR_PICKER_VERSION === "v2") {
return <ColorPickerV2 filamentType={filamentType} />;
}
// Default to v1 implementation
return <ColorPicker filamentType={filamentType} />;
import { lazy, Suspense } from "react";
import { COLOR_PICKER_VERSION } from "../config";
const ColorPicker = lazy(() => import("./ColorPicker"));
const ColorPickerV2 = lazy(() => import("./ColorPickerV2"));
interface Props {
filamentType: string;
}
const ColorPickerWrapper: React.FC<Props> = ({ filamentType }) => {
if (COLOR_PICKER_VERSION === "v2") {
return (
<Suspense fallback={null}>
<ColorPickerV2 filamentType={filamentType} />
</Suspense>
);
}
// Default to v1 implementation
return (
<Suspense fallback={null}>
<ColorPicker filamentType={filamentType} />;
</Suspense>
);

Copilot uses AI. Check for mistakes.
};
Comment on lines +10 to +17
Copy link

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

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

New ColorPicker version selection logic is introduced, but there’s no test coverage ensuring the wrapper renders v1 vs v2 based on COLOR_PICKER_VERSION. Adding a small unit test for ColorPickerWrapper would help prevent accidental regressions when toggling versions.

Copilot uses AI. Check for mistakes.

export default ColorPickerWrapper;
2 changes: 2 additions & 0 deletions src/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,11 @@ import { z } from "zod";
const zEnv = z.object({
VITE_BASE_URL: z.string().url(),
VITE_DOMAIN: z.string().min(1).max(100),
VITE_COLOR_PICKER_VERSION: z.enum(["v1", "v2"]).optional(),
});

const parsed = zEnv.parse(import.meta.env);

export const BASE_URL = parsed.VITE_BASE_URL;
export const DOMAIN = parsed.VITE_DOMAIN;
Copy link

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

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

A new env var (VITE_COLOR_PICKER_VERSION) is introduced here, but it isn't present in .env.sample (and may not be documented elsewhere). Consider updating the sample env/README so local/dev deployments discover the flag and valid values (v1/v2).

Suggested change
export const DOMAIN = parsed.VITE_DOMAIN;
export const DOMAIN = parsed.VITE_DOMAIN;
/**
* Color picker implementation version selector.
*
* Backed by the `VITE_COLOR_PICKER_VERSION` env var.
* Valid values:
* - "v1" (default)
* - "v2"
*
* When `VITE_COLOR_PICKER_VERSION` is not set, "v1" is used by default.
*/

Copilot uses AI. Check for mistakes.
export const COLOR_PICKER_VERSION = parsed.VITE_COLOR_PICKER_VERSION ?? "v1";
2 changes: 1 addition & 1 deletion src/context/ColorContext.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ const colorReducer = (state: ColorState, action: ColorAction): ColorState => {
colorOptions: action.payload,
color:
!state.hasInitialized && action.payload.length > 0
? action.payload[0].hexColor
? action.payload[0].hexValue
: state.color,
hasInitialized: true,
};
Expand Down
11 changes: 8 additions & 3 deletions src/interfaces/colors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,12 @@
* The response for the /colors endpoint
*/
export interface ColorsResponse {
filament: string;
hexColor: string;
colorTag: string;
name: string;
provider: string;
public: boolean;
available: boolean;
profile: string;
color: string;
hexValue: string;
publicId: string;
}
1 change: 1 addition & 0 deletions src/pages/Product.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import ProductPage from "./Product";
vi.mock("../config", () => ({
BASE_URL: "http://test.local",
DOMAIN: "http://test.local",
COLOR_PICKER_VERSION: "v1",
}));
Comment on lines 15 to 19
Copy link

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

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

ProductPage now renders ColorPickerWrapper, but this test file only mocks ../components/ColorPicker. Depending on Vitest module ID resolution, the wrapper may still render the real picker and trigger an extra /colors fetch (conflicting with the product fetch mock). Consider mocking ../components/ColorPickerWrapper directly (or making the fetch mock branch on URL) to keep the page tests isolated.

Copilot uses AI. Check for mistakes.

vi.mock("../components/PreviewComponent", () => ({
Expand Down
4 changes: 2 additions & 2 deletions src/pages/Product.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { ShoppingBagIcon, UserIcon } from "@heroicons/react/24/outline";
import { lazy, Suspense, useEffect, useState } from "react";
import { Link, useParams } from "react-router-dom";
import ColorPicker from "../components/ColorPicker";
import ColorPickerWrapper from "../components/ColorPickerWrapper";
import FilamentDropdown from "../components/FilamentDropdown";
import Gallery from "../components/Gallery";
import { BASE_URL } from "../config";
Expand Down Expand Up @@ -146,7 +146,7 @@ export default function ProductPage() {
</div>
<div>
<h2 className="text-sm font-medium text-gray-900">Color</h2>
<ColorPicker filamentType={selectedFilament} />
<ColorPickerWrapper filamentType={selectedFilament} />
<div className="mt-4">
<h2 className="text-sm font-medium text-gray-900">
Quantity
Expand Down