Add Tilely Next.js studio for collage creation #1
Conversation
Summary of ChangesHello @Sunwood-ai-labs, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request establishes the initial scaffolding and core features for 'Tilely,' a new Next.js 14-based web studio designed for creating dynamic video and image collages. It provides a robust platform for users to manage their media assets, arrange them on a customizable canvas with various layout and style options, and fine-tune individual media properties and timelines. The new studio aims to offer a seamless and intuitive experience for collage creation, from asset import to simulated export, leveraging a modern and efficient frontend architecture. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
There is a problem with the Gemini CLI PR review. Please check the action logs for details. |
There was a problem hiding this comment.
Code Review
このプルリクエストは、多機能なコラージュエディタである "Tilely" Next.js アプリケーションを導入するものです。UI、Zustand を用いた状態管理、様々なエディタ機能など、実装は包括的です。コードはよく構造化されており、モダンな React のプラクティスに従っています。
レビューを通して、いくつかの改善点を特定しました：
editor-shell.tsxにおいて、クリアされないタイムアウトによる潜在的なメモリリーク。top-bar.tsxにおいて、より堅牢にできるハードコードされた値や壊れやすいロジック。canvas-preview.tsxにおけるパフォーマンス最適化の機会。lib/store.tsにおいて、Promise ベースのファイル処理でエラーがハンドルされておらず、UIがハングする可能性がある重大な問題。
全体として、これは Tilely アプリケーションの素晴らしい基盤です。これらの点を修正することで、アプリケーションの安定性と保守性がさらに向上するでしょう。
| export async function fileToAsset(file: File, type: AssetType): Promise<Asset> { | ||
| const url = URL.createObjectURL(file); | ||
| const baseAsset: Asset = { | ||
| id: uuid(), | ||
| name: file.name, | ||
| type, | ||
| url, | ||
| size: file.size, | ||
| createdAt: Date.now() | ||
| }; | ||
|
|
||
| if (type === "image" || type === "logo") { | ||
| const dimensions = await new Promise<{ width: number; height: number }>((resolve) => { | ||
| const image = new Image(); | ||
| image.onload = () => resolve({ width: image.width, height: image.height }); | ||
| image.src = url; | ||
| }); | ||
| return { ...baseAsset, ...dimensions }; | ||
| } | ||
|
|
||
| if (type === "video" || type === "audio") { | ||
| const metadata = await new Promise<{ duration: number; width?: number; height?: number }>((resolve) => { | ||
| const element = document.createElement(type === "audio" ? "audio" : "video"); | ||
| element.preload = "metadata"; | ||
| element.onloadedmetadata = () => | ||
| resolve({ | ||
| duration: element.duration, | ||
| width: element instanceof HTMLVideoElement ? element.videoWidth : undefined, | ||
| height: element instanceof HTMLVideoElement ? element.videoHeight : undefined | ||
| }); | ||
| element.src = url; | ||
| }); | ||
| return { ...baseAsset, ...metadata }; | ||
| } | ||
|
|
||
| return baseAsset; | ||
| } |
There was a problem hiding this comment.
fileToAsset 関数内の new Promise で、アセットの読み込みに失敗した場合のエラーが処理されていません。例えば、ファイルが壊れている場合、onerror が呼ばれますが、Promise は reject されずに永遠に pending 状態になります。これにより、UIが応答しなくなる可能性があります。
image.onerror と element.onerror ハンドラを追加して Promise を reject するように修正してください。
export async function fileToAsset(file: File, type: AssetType): Promise<Asset> {
const url = URL.createObjectURL(file);
const baseAsset: Asset = {
id: uuid(),
name: file.name,
type,
url,
size: file.size,
createdAt: Date.now()
};
if (type === "image" || type === "logo") {
const dimensions = await new Promise<{ width: number; height: number }>((resolve, reject) => {
const image = new Image();
image.onload = () => resolve({ width: image.width, height: image.height });
image.onerror = reject;
image.src = url;
});
return { ...baseAsset, ...dimensions };
}
if (type === "video" || type === "audio") {
const metadata = await new Promise<{ duration: number; width?: number; height?: number }>((resolve, reject) => {
const element = document.createElement(type === "audio" ? "audio" : "video");
element.preload = "metadata";
element.onloadedmetadata = () =>
resolve({
duration: element.duration,
width: element instanceof HTMLVideoElement ? element.videoWidth : undefined,
height: element instanceof HTMLVideoElement ? element.videoHeight : undefined
});
element.onerror = reject;
element.src = url;
});
return { ...baseAsset, ...metadata };
}
return baseAsset;
}| useEffect(() => { | ||
| if (!renderJob || (renderJob.status !== "queued" && renderJob.status !== "processing")) { | ||
| return; | ||
| } | ||
| let progress = renderJob.progress; | ||
| let frame = 0; | ||
| updateRenderProgress(progress, "processing"); | ||
| const interval = window.setInterval(() => { | ||
| frame += 1; | ||
| progress = Math.min(100, progress + 12); | ||
| if (progress >= 100) { | ||
| const blob = new Blob(["Tilely export placeholder"], { type: "text/plain" }); | ||
| const url = URL.createObjectURL(blob); | ||
| updateRenderProgress(100, "succeeded", url); | ||
| window.setTimeout(() => URL.revokeObjectURL(url), 10_000); | ||
| window.clearInterval(interval); | ||
| } else { | ||
| updateRenderProgress(progress, "processing"); | ||
| } | ||
| if (frame > 20) { | ||
| window.clearInterval(interval); | ||
| } | ||
| }, 500); | ||
| return () => window.clearInterval(interval); | ||
| }, [renderJob?.id]); |
There was a problem hiding this comment.
useEffect 内で window.setTimeout を使用していますが、クリーンアップ関数で clearTimeout が呼ばれていません。コンポーネントがアンマウントされた場合にメモリリークや意図しない動作を引き起こす可能性があります。
timeoutId を変数に保持し、クリーンアップ関数でクリアすることをお勧めします。また、useEffect の依存配列に updateRenderProgress を追加すると、より堅牢になります。
useEffect(() => {
if (!renderJob || (renderJob.status !== "queued" && renderJob.status !== "processing")) {
return;
}
let progress = renderJob.progress;
let frame = 0;
updateRenderProgress(progress, "processing");
let timeoutId: ReturnType<typeof setTimeout>;
const interval = window.setInterval(() => {
frame += 1;
progress = Math.min(100, progress + 12);
if (progress >= 100) {
const blob = new Blob(["Tilely export placeholder"], { type: "text/plain" });
const url = URL.createObjectURL(blob);
updateRenderProgress(100, "succeeded", url);
timeoutId = window.setTimeout(() => URL.revokeObjectURL(url), 10_000);
window.clearInterval(interval);
} else {
updateRenderProgress(progress, "processing");
}
if (frame > 20) {
window.clearInterval(interval);
}
}, 500);
return () => {
window.clearInterval(interval);
if (timeoutId) {
window.clearTimeout(timeoutId);
}
};
}, [renderJob?.id, updateRenderProgress]);
| <Button | ||
| className="w-full" | ||
| onClick={() => { | ||
| queueRender("custom", customResolution * customFps > 40000 ? "server" : "browser"); |
There was a problem hiding this comment.
マジックナンバー 40000 が使われています。これはコードの可読性と保守性を低下させる可能性があります。
この値が何を表すのかを示す名前付き定数を定義することをお勧めします。
const BROWSER_RENDER_PIXEL_LIMIT = 40000; // 例: 解像度 * FPS の上限
// ...
queueRender("custom", customResolution * customFps > BROWSER_RENDER_PIXEL_LIMIT ? "server" : "browser");| target: "browser" | "server", | ||
| queueRender: (presetId: string, target: "browser" | "server") => void | ||
| ) { | ||
| const preset = exportPresets[target === "browser" ? 0 : 1]; |
There was a problem hiding this comment.
exportPresets 配列のインデックス (0 or 1) に直接依存してプリセットを選択しています。これは将来 exportPresets の順序が変更された場合に壊れやすい実装です。
id や他のプロパティでプリセットを検索する方法がより堅牢です。
// 例
const presetId = target === "browser" ? "social-1080p" : "hq-2160p";
const preset = exportPresets.find(p => p.id === presetId);
if (!preset) {
toast.error("書き出しプリセットが見つかりませんでした。");
return;
}
queueRender(preset.id, target);
toast.success(`${preset.label} でレンダリング開始したよ〜！`);| {cells.map((cell, index) => { | ||
| const track = project.tracks.find((item) => item.cellIndex === index); | ||
| const asset = track ? project.assets.find((item) => item.id === track.assetId) : undefined; | ||
| const isActive = activeCell === index; | ||
| return ( | ||
| <button | ||
| key={cell.id} | ||
| type="button" | ||
| onClick={() => setActiveCell(index)} | ||
| className={cn( | ||
| "relative flex h-full w-full items-center justify-center overflow-hidden rounded-[inherit] border border-transparent transition", | ||
| isActive ? "border-indigo-400 shadow-[0_0_0_2px_rgba(129,140,248,0.6)]" : "border-white/5" | ||
| )} | ||
| > | ||
| {asset ? ( | ||
| <AssetPreview trackId={track!.id} /> | ||
| ) : ( | ||
| <div className="flex flex-col items-center gap-1 text-xs text-muted-foreground"> | ||
| <ImageOff className="h-5 w-5" /> | ||
| セル {index + 1} | ||
| </div> | ||
| )} | ||
| </button> | ||
| ); | ||
| })} |
There was a problem hiding this comment.
cells.map の中で project.tracks.find を使用しているため、セルごとにトラック配列全体を走査しています。セルの数やトラック数が多くなるとパフォーマンスに影響を与える可能性があります。
useMemo を使って cellIndex からトラックへのマップを事前に作成し、O(1) でアクセスできるようにすることで、この処理を最適化できます。
import { type Track } from "@/lib/types";
export function CanvasPreview() {
// ...
const tracksByCellIndex = useMemo(() => {
return project.tracks.reduce((map, track) => {
map.set(track.cellIndex, track);
return map;
}, new Map<number, Track>());
}, [project.tracks]);
// ...
// .map() の中で
const track = tracksByCellIndex.get(index);
// ...
}
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68fe18bff25c832caddc0ac336d635cd