Skip to content

Conversation

@FireAndIceFrog
Copy link
Owner

No description provided.

Copilot AI review requested due to automatic review settings September 18, 2025 06:02
@FireAndIceFrog FireAndIceFrog merged commit 27ffaf3 into develop Sep 18, 2025
2 checks passed
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request represents a significant refactoring and enhancement of the string art generation project. The main changes include restructuring the frontend to use React with Redux state management and Material-UI components, while also refactoring the Rust backend with improved architecture and modularity.

  • Complete frontend rewrite using React, Redux, and Material-UI with a stepper interface
  • Rust backend refactoring with better separation of concerns and modular architecture
  • Addition of internationalization support and configuration improvements

Reviewed Changes

Copilot reviewed 68 out of 95 changed files in this pull request and generated 6 comments.

File Description
string-art-website/* New React frontend with Redux state management and Material-UI components
StringArtRustImpl/src/* Refactored Rust backend with traits, factories, and modular architecture
build-wasm.sh Updated build script to target the new frontend directory
.github/workflows/build.yml Modified CI/CD pipeline for the new project structure
Files not reviewed (1)
  • string-art-website/package-lock.json: Language not supported

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

import i18n from './i18n.tsx';

(async () => {
await i18n.init ();
Copy link

Copilot AI Sep 18, 2025

Choose a reason for hiding this comment

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

Remove the extra space before the parentheses. Should be i18n.init() instead of i18n.init ().

Suggested change
await i18n.init ();
await i18n.init();

Copilot uses AI. Check for mistakes.
settings: StringArtConfig;
}

interface StrinArtThunkProperties { imageData: Uint8Array; settings: StringArtConfig; }
Copy link

Copilot AI Sep 18, 2025

Choose a reason for hiding this comment

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

Fix the typo in the interface name. Should be StringArtThunkProperties instead of StrinArtThunkProperties.

Copilot uses AI. Check for mistakes.
</div>
</div>

<div className="canvas-wrapper">
Copy link

Copilot AI Sep 18, 2025

Choose a reason for hiding this comment

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

The CSS class canvas-wrapper is referenced but not defined in the imported CSS module. It should be className={style['canvas-wrapper']} to use the CSS module.

Suggested change
<div className="canvas-wrapper">
<div className={style['canvas-wrapper']}>

Copilot uses AI. Check for mistakes.
Comment on lines +10 to +19
let inited = false;
init().then(() => {
inited = true;
});

self.onmessage = async (event: MessageEvent<WorkerMessage>) => {
const { imageData, config } = event.data;
if (!inited) {
throw new Error("WASM module not initialized");
}

Copy link

Copilot AI Sep 18, 2025

Choose a reason for hiding this comment

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

This check is race condition prone. The worker might receive a message before the WASM module finishes initializing. Consider awaiting the initialization promise instead of using a boolean flag.

Copilot uses AI. Check for mistakes.
Comment on lines +294 to 305
match path {
Ok(path) => {
console::log_1(&format!("✅ WASM: Path generation complete with {} nails", path.len()).into());
// Store the current path
Ok(path)
}
Err(e) => {
console::log_1(&format!("❌ WASM: Path generation failed: {}", e).into());
Err(e)
}
}
}
Copy link

Copilot AI Sep 18, 2025

Choose a reason for hiding this comment

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

This match expression is unnecessary. You can directly return the path result instead of matching and re-wrapping it.

Suggested change
match path {
Ok(path) => {
console::log_1(&format!("✅ WASM: Path generation complete with {} nails", path.len()).into());
// Store the current path
Ok(path)
}
Err(e) => {
console::log_1(&format!("❌ WASM: Path generation failed: {}", e).into());
Err(e)
}
}
}
path
.inspect(|path| {
console::log_1(&format!("✅ WASM: Path generation complete with {} nails", path.len()).into());
})
.inspect_err(|e| {
console::log_1(&format!("❌ WASM: Path generation failed: {}", e).into());
})
}
}

Copilot uses AI. Check for mistakes.
Comment on lines +3 to +14
export type StringArtConfig = Pick<WasmStringArtConfig, keyof {
num_nails: number;
image_size: number;
preserve_eyes: boolean;
preserve_negative_space: boolean;
negative_space_penalty: number;
negative_space_threshold: number;
}> & {
max_lines: number,
line_darkness: number,
min_improvement_score: number,
progress_frequency: number,
Copy link

Copilot AI Sep 18, 2025

Choose a reason for hiding this comment

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

[nitpick] This type definition is complex and fragile. Consider defining a proper interface instead of using Pick with a keyof trick, which makes the code harder to understand and maintain.

Suggested change
export type StringArtConfig = Pick<WasmStringArtConfig, keyof {
num_nails: number;
image_size: number;
preserve_eyes: boolean;
preserve_negative_space: boolean;
negative_space_penalty: number;
negative_space_threshold: number;
}> & {
max_lines: number,
line_darkness: number,
min_improvement_score: number,
progress_frequency: number,
export interface StringArtConfig {
num_nails: number;
image_size: number;
preserve_eyes: boolean;
preserve_negative_space: boolean;
negative_space_penalty: number;
negative_space_threshold: number;
max_lines: number;
line_darkness: number;
min_improvement_score: number;
progress_frequency: number;

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants