fix: Address Copilot review feedback

- Simplify StepCircle cursor logic to use isComplete || isCurrent
- Fix UploadButton to use boundsFits prop instead of !!boundsError
- Remove XSS vulnerability by parsing markdown safely without dangerouslySetInnerHTML
- Move ColorBlock type to shared types.ts file to reduce coupling
- Rename useDisplayFilename to getDisplayFilename and move to utils (not a hook)
- Improve threadMetadata JSDoc with detailed examples
- Make WorkflowStep interface properties readonly for full immutability
- Fix PyodideProgress redundant negation logic

All issues from Copilot review resolved.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This commit is contained in:
Jan-Henrik Bruhn 2025-12-27 16:41:58 +01:00
parent 681ce223c3
commit 095c879ea3
12 changed files with 96 additions and 38 deletions

View file

@ -20,7 +20,7 @@ import {
usePatternRotationUpload,
usePatternValidation,
} from "@/hooks";
import { useDisplayFilename } from "../../hooks/domain/useDisplayFilename";
import { getDisplayFilename } from "../../utils/displayFilename";
import { PatternInfoSkeleton } from "../SkeletonLoader";
import { PatternInfo } from "../PatternInfo";
import { DocumentTextIcon } from "@heroicons/react/24/solid";
@ -105,7 +105,7 @@ export function FileUpload() {
// Use prop pesData if available (from cached pattern), otherwise use local state
const pesData = pesDataProp || localPesData;
// Use currentFileName from App state, or local fileName, or resumeFileName for display
const displayFileName = useDisplayFilename({
const displayFileName = getDisplayFilename({
currentFileName,
localFileName: fileName,
resumeFileName,
@ -229,6 +229,7 @@ export function FileUpload() {
isConnected={isConnected}
isUploading={isUploading}
uploadProgress={uploadProgress}
boundsFits={boundsCheck.fits}
boundsError={boundsCheck.error}
onUpload={handleUpload}
patternUploaded={patternUploaded}

View file

@ -25,7 +25,7 @@ export function PyodideProgress({
<div className="mb-3">
<div className="flex justify-between items-center mb-1.5">
<span className="text-xs font-medium text-gray-600 dark:text-gray-400">
{isFileLoading && !pyodideReady
{isFileLoading
? "Please wait - initializing Python environment..."
: pyodideLoadingStep || "Initializing Python environment..."}
</span>
@ -35,7 +35,7 @@ export function PyodideProgress({
</div>
<Progress value={pyodideProgress} className="h-2.5" />
<p className="text-xs text-gray-500 dark:text-gray-400 mt-1.5 italic">
{isFileLoading && !pyodideReady
{isFileLoading
? "File dialog will open automatically when ready"
: "This only happens once on first use"}
</p>

View file

@ -17,6 +17,7 @@ interface UploadButtonProps {
isConnected: boolean;
isUploading: boolean;
uploadProgress: number;
boundsFits: boolean;
boundsError: string | null;
onUpload: () => Promise<void>;
patternUploaded: boolean;
@ -28,6 +29,7 @@ export function UploadButton({
isConnected,
isUploading,
uploadProgress,
boundsFits,
boundsError,
onUpload,
patternUploaded,
@ -43,7 +45,7 @@ export function UploadButton({
return (
<Button
onClick={onUpload}
disabled={!isConnected || isUploading || !!boundsError}
disabled={!isConnected || isUploading || !boundsFits}
className="flex-1"
aria-label={
isUploading

View file

@ -12,18 +12,7 @@ import {
} from "@heroicons/react/24/solid";
import { Progress } from "@/components/ui/progress";
import { formatThreadMetadata } from "../../utils/threadMetadata";
export interface ColorBlock {
colorIndex: number;
threadHex: string;
startStitch: number;
endStitch: number;
stitchCount: number;
threadCatalogNumber: string | null;
threadBrand: string | null;
threadDescription: string | null;
threadChart: string | null;
}
import type { ColorBlock } from "./types";
interface ColorBlockItemProps {
block: ColorBlock;

View file

@ -5,7 +5,8 @@
*/
import { ScrollArea } from "@/components/ui/scroll-area";
import { ColorBlockItem, type ColorBlock } from "./ColorBlockItem";
import { ColorBlockItem } from "./ColorBlockItem";
import type { ColorBlock } from "./types";
interface ColorBlockListProps {
colorBlocks: ColorBlock[];

View file

@ -23,7 +23,7 @@ import { ProgressStats } from "./ProgressStats";
import { ProgressSection } from "./ProgressSection";
import { ColorBlockList } from "./ColorBlockList";
import { ProgressActions } from "./ProgressActions";
import type { ColorBlock } from "./ColorBlockItem";
import type { ColorBlock } from "./types";
export function ProgressMonitor() {
// Machine store

View file

@ -0,0 +1,15 @@
/**
* Shared types for ProgressMonitor components
*/
export interface ColorBlock {
colorIndex: number;
threadHex: string;
startStitch: number;
endStitch: number;
stitchCount: number;
threadCatalogNumber: string | null;
threadBrand: string | null;
threadDescription: string | null;
threadChart: string | null;
}

View file

@ -28,7 +28,7 @@ export const StepCircle = forwardRef<HTMLDivElement, StepCircleProps>(
onClick={onClick}
className={`
w-8 h-8 lg:w-10 lg:h-10 rounded-full flex items-center justify-center font-bold text-xs transition-all duration-300 border-2 shadow-md
${stepId <= (isCurrent ? stepId : isComplete ? stepId : stepId - 1) ? "cursor-pointer hover:scale-110" : "cursor-not-allowed"}
${isComplete || isCurrent ? "cursor-pointer hover:scale-110" : "cursor-not-allowed"}
${isComplete ? "bg-success-500 dark:bg-success-600 border-success-400 dark:border-success-500 text-white shadow-success-500/30 dark:shadow-success-600/30" : ""}
${isCurrent ? "bg-primary-600 dark:bg-primary-700 border-primary-500 dark:border-primary-600 text-white scale-105 lg:scale-110 shadow-primary-600/40 dark:shadow-primary-700/40 ring-2 ring-primary-300 dark:ring-primary-500 ring-offset-2 dark:ring-offset-gray-900" : ""}
${isUpcoming ? "bg-primary-700 dark:bg-primary-800 border-primary-500/30 dark:border-primary-600/30 text-primary-200/70 dark:text-primary-300/70" : ""}

View file

@ -98,18 +98,20 @@ export const StepPopover = forwardRef<HTMLDivElement, StepPopoverProps>(
<ul
className={`list-disc list-inside text-sm ${listColorClasses[content.type]} space-y-1`}
>
{content.items.map((item, index) => (
<li
key={index}
className="pl-2"
dangerouslySetInnerHTML={{
__html: item.replace(
/\*\*(.*?)\*\*/g,
"<strong>$1</strong>",
),
}}
/>
))}
{content.items.map((item, index) => {
// Parse **text** markdown syntax into React elements safely
const parts = item.split(/(\*\*.*?\*\*)/);
return (
<li key={index} className="pl-2">
{parts.map((part, i) => {
if (part.startsWith("**") && part.endsWith("**")) {
return <strong key={i}>{part.slice(2, -2)}</strong>;
}
return part;
})}
</li>
);
})}
</ul>
)}
</div>

View file

@ -3,9 +3,9 @@
*/
export interface WorkflowStep {
id: number;
label: string;
description: string;
readonly id: number;
readonly label: string;
readonly description: string;
}
export const WORKFLOW_STEPS: readonly WorkflowStep[] = [

View file

@ -1,5 +1,5 @@
/**
* useDisplayFilename Hook
* getDisplayFilename Utility
*
* Determines which filename to display based on priority:
* 1. currentFileName (from pattern store)
@ -8,7 +8,7 @@
* 4. Empty string
*/
export function useDisplayFilename(options: {
export function getDisplayFilename(options: {
currentFileName: string | null;
localFileName: string;
resumeFileName: string | null;

View file

@ -1,6 +1,54 @@
/**
* Format thread metadata for display
* Format thread metadata for display.
*
* Combines brand, catalog number, chart, and description into a readable string
* using the following rules:
*
* - The primary part consists of the brand and catalog number:
* - The brand (if present) appears first.
* - The catalog number (if present) is prefixed with `#` and appended after
* the brand, separated by a single space (e.g. `"DMC #310"`).
* - The secondary part consists of the chart and description:
* - The chart is omitted if it is `null`/empty or exactly equal to
* `threadCatalogNumber`.
* - The chart (when shown) and the description are joined with a single
* space (e.g. `"Anchor 24-colour Black"`).
* - The primary and secondary parts are joined with `" • "` (space, bullet,
* space). If either part is empty, only the non-empty part is returned.
*
* Examples:
*
* - Brand and catalog only:
* - Input:
* - `threadBrand: "DMC"`
* - `threadCatalogNumber: "310"`
* - `threadChart: null`
* - `threadDescription: null`
* - Output: `"DMC #310"`
*
* - Brand, catalog, and description:
* - Input:
* - `threadBrand: "DMC"`
* - `threadCatalogNumber: "310"`
* - `threadChart: null`
* - `threadDescription: "Black"`
* - Output: `"DMC #310 • Black"`
*
* - Brand, catalog, chart (different from catalog), and description:
* - Input:
* - `threadBrand: "Anchor"`
* - `threadCatalogNumber: "403"`
* - `threadChart: "24-colour"`
* - `threadDescription: "Black"`
* - Output: `"Anchor #403 • 24-colour Black"`
*
* - Chart equal to catalog number (chart omitted):
* - Input:
* - `threadBrand: "DMC"`
* - `threadCatalogNumber: "310"`
* - `threadChart: "310"`
* - `threadDescription: "Black"`
* - Output: `"DMC #310 • Black"`
*/
interface ThreadMetadata {