Skip to content

[OC-77] 折り目をつける機能の実装 - #28

Open
Riku-0801 wants to merge 2 commits into
mainfrom
OC-77
Open

Riku-0801 wants to merge 2 commits into
mainfrom
OC-77

Conversation

@Riku-0801

Copy link
Copy Markdown
Contributor

折り目をつける機能の実装

概要

手順に「折り目をつける」を追加し、アニメーションを作成した。

変更内容

  • foldingAngleにデフォルト値を追加
  • Stepに「折り目をつける(crease)」を追加
  • crease時のアニメーション処理を追加

スクリーンショット

default.webm

備考

@notion-workspace

Copy link
Copy Markdown

@vercel

vercel Bot commented Oct 19, 2025

Copy link
Copy Markdown
Contributor

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
ori-cube Ready Ready Preview Comment Oct 19, 2025 9:25am

💡 Enable Vercel Agent with $100 free credit for automated AI reviews

@Riku-0801 Riku-0801 self-assigned this Oct 19, 2025
@Riku-0801 Riku-0801 added the enhancement New feature or request label Oct 19, 2025
@Riku-0801 Riku-0801 changed the title ✨ feat: 折り目をつける機能の実装 [OC-77] 折り目をつける機能の実装 Oct 19, 2025

@YutaK1026 YutaK1026 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ロジックは動いていそうだったので,コード上の粗さがしになってしまいましたm
確認お願いします!

Comment thread src/app/api/data/route.ts
Comment on lines +13 to +29
// フロント側でprocedureの各stepにfoldingAngleが必ず存在する形に正規化する(初期値は180度)
const normalizeProcedure = (
procedure: Procedure | null | undefined
): Procedure => {
if (!procedure) return {};

return Object.entries(procedure).reduce((acc, [key, step]) => {
if (!step) return acc;
if (step.foldingAngle === undefined) {
acc[key] = { ...step, foldingAngle: 180 } as Step;
} else {
acc[key] = step;
}
return acc;
}, {} as Procedure);
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[imo]
責任分離の観点から,ここら辺はAPI側に任せてもいいのかなと思いましたm
ただ時間的制約もあるので,このままで動くならいいかなとなっています

Comment on lines +101 to +105
useEffect(() => {
const prevValue = prevValueRef.current;
const currentValue = props.value;
const midpoint = props.maxArg / 2;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[imo]
後から見返したときにこのuseEffectが何を担当しているのかわからなさそうなので,コメントで補足が欲しいです!

[nits]
viewファイルにロジックを書くと読みにくくなる観点から,カスタムフックはhooks.tsに書いてほしいです!

Comment on lines +146 to +148
useEffect(() => {
return () => {
const activeInterval = intervalRef.current;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[imo]
同上でコメントが欲しいですm

[nits]
viewファイルにロジックを書くと読みにくくなる観点から,カスタムフックはhooks.tsに書いてほしいです!

@yutteee yutteee left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

commented

Comment thread prisma/mock/トラック.json Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

memo
crease:折り目

Comment thread src/app/api/data/route.ts
Comment on lines +13 to +29
// フロント側でprocedureの各stepにfoldingAngleが必ず存在する形に正規化する(初期値は180度)
const normalizeProcedure = (
procedure: Procedure | null | undefined
): Procedure => {
if (!procedure) return {};

return Object.entries(procedure).reduce((acc, [key, step]) => {
if (!step) return acc;
if (step.foldingAngle === undefined) {
acc[key] = { ...step, foldingAngle: 180 } as Step;
} else {
acc[key] = step;
}
return acc;
}, {} as Procedure);
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Procedure | null | undefinedになるのか
型定義がそもそも間違ってそう...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

この辺のコード整理したいっすね
分岐とuseEffectが多くなってて難しい感じがします

分岐はネスト減らせないか、処理自体をプライベート関数的なものに切り出せないか
useEffectは命名がないからコメント書くか、カスタムフックに切り出すかをしたいみがあります

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

時間ある時に詳しく見ます!

@Riku-0801
Riku-0801 requested a review from Copilot October 19, 2025 12:18

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull Request Overview

Adds "crease" (折り目) support with a configurable foldingAngle and corresponding animation/controls updates.

  • Introduces CreaseStep type and adds foldingAngle to steps
  • Implements crease animation that reverses direction after halfway
  • Normalizes API response to ensure foldingAngle is always present; updates control panel to pause at midpoint for crease

Reviewed Changes

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

Show a summary per file
File Description
src/types/model.ts Adds CreaseStep and foldingAngle fields; extends Step union
src/components/OrigamiDetail/presenter.tsx Computes foldingAngle/maxArg and currentStepType; passes to control panel UI
src/components/OrigamiDetail/Three/index.tsx Handles crease rotation by flipping after midpoint; reuses Base branch
src/components/OrigamiDetail/ControlPanel/index.tsx Adds midpoint pause/resume logic for crease; introduces refs for timers
src/app/api/data/route.ts Normalizes procedure to inject foldingAngle default (180°) in each step
prisma/mock/トラック.json Marks steps as type "crease" with foldingAngle 360°
Comments suppressed due to low confidence (1)

src/components/OrigamiDetail/ControlPanel/index.tsx:78

  • [nitpick] Avoid the non-null assertion on intervalId. Either pass the active interval via intervalRef or update useOnSliderMax to accept possibly undefined and branch internally; this prevents accidental undefined from flowing into the hook.
  useOnSliderMax(
    props,
    isLoop,
    setIsPlaying,
    intervalId!,
    isPlaying,
    isLoopStandby,
    setIsLoopStandby
  );

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

procedureIndex={procedureIndex}
setProcedureIndex={setProcedureIndex}
procedureLength={procedureLength}
currentStepType={currentStepType}

Copilot AI Oct 19, 2025

Copy link

Choose a reason for hiding this comment

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

currentStepType is handled in ControlPanel (container), but here it is being passed to ControlPanelPresenter. This bypasses the new midpoint pause logic implemented in ControlPanel and is likely not consumed (and may cause a prop type error). Replace ControlPanelPresenter with ControlPanel so the logic runs.

Suggested change
currentStepType={currentStepType}

Copilot uses AI. Check for mistakes.
// 折り目をつける動作の場合、設定角度の半分を超えたら反対方向に折る
const maxAngle = stepObject.foldingAngle * (Math.PI / 180);
if (stepObject.type === "crease" && theta > maxAngle / 2) {
theta = 2 * Math.PI - theta;

Copilot AI Oct 19, 2025

Copy link

Choose a reason for hiding this comment

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

The reflection uses 2π - θ, which only behaves correctly when foldingAngle is 360°. For other foldingAngle values it over-rotates. Reflect around the configured maxAngle instead: replace with theta = maxAngle - theta;.

Suggested change
theta = 2 * Math.PI - theta;
theta = maxAngle - theta;

Copilot uses AI. Check for mistakes.
Comment on lines +24 to +29
const [intervalId, setIntervalId] = useState<NodeJS.Timeout | undefined>(undefined);
const intervalRef = useRef<NodeJS.Timeout | null>(null);
const midpointTimeoutRef = useRef<NodeJS.Timeout | null>(null);
const midpointHandledRef = useRef(false);
const prevValueRef = useRef(props.value);
const isPlayingRef = useRef(isPlaying);

Copilot AI Oct 19, 2025

Copy link

Choose a reason for hiding this comment

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

[nitpick] Both intervalId state and intervalRef track the same timer; maintaining two sources of truth increases complexity and the risk of divergence. Prefer a single ref for the interval handle (no state), and update useOnSliderMax to accept the ref or handle an undefined timer.

Copilot uses AI. Check for mistakes.
Comment on lines +30 to +32
const currentStep = modelData.procedure[procedureIndex];
const currentStepType = (currentStep?.type ?? "Base") as string;
const foldingAngle = currentStep?.foldingAngle ?? 180;

Copilot AI Oct 19, 2025

Copy link

Choose a reason for hiding this comment

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

[nitpick] Narrow the type instead of casting to string to retain safety and autocomplete. Consider const currentStepType: Step[\"type\"] = currentStep?.type ?? \"Base\"; (import Step), which aligns with your union.

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

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants