Feature/translationProgress #38
Conversation
|
以下は、重複を排除し、関連する変更をグループ化した更新後の変更セットです。 ウォークスルー新しい翻訳ページの機能追加、認証ロジックの変更、リトライロジックの追加、データベーススキーマの更新、およびJSON設定ファイルのフォーマット調整が行われました。 変更点
Uplevel your code reviews with CodeRabbit ProCodeRabbit ProIf you like this project, please support us by purchasing the Pro version. The Pro version has advanced context, superior noise reduction and several proprietary improvements compared to the open source version. Moreover, CodeRabbit Pro is free for open source projects. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| export async function getOrCreatePageVersionTranslationInfo( | ||
| pageVersionId: number, | ||
| targetLanguage: string, | ||
| translationTitle: string, | ||
| ) { | ||
| return await prisma.pageVersionTranslationInfo.upsert({ | ||
| where: { | ||
| pageVersionId_targetLanguage: { | ||
| pageVersionId, | ||
| targetLanguage, | ||
| }, | ||
| }, | ||
| update: {}, // 既存のレコードがある場合は更新しない | ||
| create: { | ||
| pageVersionId, | ||
| targetLanguage, | ||
| translationTitle, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
関数名 getOrCreatePageVersionTranslationInfo は、既存のレコードがある場合に更新しないことを示していますが、upsert メソッドの update フィールドが空であるため、実際には何も更新されません。これは誤解を招く可能性があります。関数名を変更するか、コメントを追加して意図を明確にすることをお勧めします。
- update: {}, // 既存のレコードがある場合は更新しない
+ update: {}, // 既存のレコードがある場合は何も更新しない
| try { | ||
| const userAITranslationInfo = await prisma.userAITranslationInfo.upsert({ | ||
| where: { | ||
| userId_pageVersionId_targetLanguage: { | ||
| userId, | ||
| pageVersionId, | ||
| targetLanguage, | ||
| }, | ||
| }, | ||
| update: { | ||
| aiTranslationStatus: "pending", | ||
| aiTranslationProgress: 0, | ||
| }, | ||
| create: { | ||
| userId, | ||
| pageVersionId, | ||
| targetLanguage, | ||
| aiTranslationStatus: "pending", | ||
| aiTranslationProgress: 0, | ||
| }, | ||
| }); | ||
| return userAITranslationInfo; | ||
| } catch (error) { | ||
| console.error("Error in getOrCreateUserAITranslationInfo:", error); | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
エラーハンドリングを改善するために、errorオブジェクトの詳細を含めると良いでしょう。これにより、デバッグが容易になります。
- console.error("Error in getOrCreateUserAITranslationInfo:", error);
+ console.error("Error in getOrCreateUserAITranslationInfo:", {
+ message: error.message,
+ stack: error.stack,
+ context: { userId, pageVersionId, targetLanguage }
+ });
| import { createHash } from "node:crypto"; | ||
| import { prisma } from "./prisma"; | ||
| import { prisma } from "../utils/prisma"; | ||
|
|
||
| export async function getOrCreatePageVersionId( | ||
| url: string, |
There was a problem hiding this comment.
Note: This review was outside of the patch, but no patch was found that overlapped with it. Original lines [8-9]
requireUserId関数はユーザー認証を行いますが、エラーハンドリングが不足しています。ユーザーが認証されていない場合の処理を追加する必要があります。
+ try {
+ const userId = await requireUserId(request);
+ } catch (error) {
+ return redirect("/login");
+ }
| import { createHash } from "node:crypto"; | ||
| import { prisma } from "./prisma"; | ||
| import { prisma } from "../utils/prisma"; | ||
|
|
||
| export async function getOrCreatePageVersionId( | ||
| url: string, |
There was a problem hiding this comment.
Note: This review was outside of the patch, but no patch was found that overlapped with it. Original lines [24-26]
findUniqueメソッドの結果がnullの場合に404エラーを返すのは良いですが、セキュリティ上の理由から、存在しないページバージョンIDについての詳細な情報を提供しない方が良いです。
- throw new Response("Page version not found", { status: 404 });
+ throw new Response("Not Found", { status: 404 });
| import { createHash } from "node:crypto"; | ||
| import { prisma } from "./prisma"; | ||
| import { prisma } from "../utils/prisma"; | ||
|
|
||
| export async function getOrCreatePageVersionId( | ||
| url: string, |
There was a problem hiding this comment.
Note: This review was outside of the patch, but no patch was found that overlapped with it. Original lines [38-39]
フォームデータの取得時に型チェックが不足しています。textとtargetLanguageがnullである可能性を考慮する必要があります。
- const text = formData.get("text");
- const targetLanguage = formData.get("targetLanguage");
+ const text = formData.get("text")?.toString() || "";
+ const targetLanguage = formData.get("targetLanguage")?.toString() || "";Changes made to web/app/utils/gemini.ts for your review
---new_hunk---
import axios from "axios";
export async function getGeminiModelResponse(input: string): Promise<any> {
let attempts = 0;
const maxAttempts = 3;
const delay = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms));
while (attempts < maxAttempts) {
try {
const response = await axios.post("https://api.gemini.com/v1/model", { input });
return response.data;
} catch (error) {
attempts++;
if (attempts >= maxAttempts) {
throw new Error("Failed to get response from Gemini model after multiple attempts");
}
await delay(1000); // Wait for 1 second before retrying
}
}
}---comment_chains---
Please review this change.
---end_change_section---
Response
| import { createHash } from "node:crypto"; | ||
| import { prisma } from "./prisma"; | ||
| import { prisma } from "../utils/prisma"; | ||
|
|
||
| export async function getOrCreatePageVersionId( | ||
| url: string, |
There was a problem hiding this comment.
Note: This review was outside of the patch, but no patch was found that overlapped with it. Original lines [7-7]
delay関数は毎回新しく作成されていますが、ループ外に移動して再利用可能にすることでパフォーマンスを向上させることができます。
+ const delay = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms));
while (attempts < maxAttempts) {
| restart: always | ||
| ports: | ||
| - '6379:6379' | ||
| command: redis-server --save 20 1 --loglevel warning --requirepass eYVX7EwVmmxKPCDmwMtyKVge8oLd2t81 | ||
| command: redis-server --save 20 1 --loglevel warning | ||
| volumes: | ||
| - redis:/data | ||
|
|
There was a problem hiding this comment.
Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [1-19]
エラーメッセージの詳細を追加し、ユーザーにとってわかりやすくするために、エラーハンドリングを改善できます。
- throw new Response("Page version ID is required", { status: 400 });
+ throw new Response("Bad Request: Page version ID is required in the query parameters.", { status: 400 });
- throw new Response("Page version not found", { status: 404 });
+ throw new Response("Not Found: The requested page version does not exist.", { status: 404 });
| restart: always | ||
| ports: | ||
| - '6379:6379' | ||
| command: redis-server --save 20 1 --loglevel warning --requirepass eYVX7EwVmmxKPCDmwMtyKVge8oLd2t81 | ||
| command: redis-server --save 20 1 --loglevel warning | ||
| volumes: | ||
| - redis:/data | ||
|
|
There was a problem hiding this comment.
Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [1-17]
@@foreignKeyは正しくありません。代わりに@relation属性を使用してください。
- @@foreignKey([userId], references: [id], onDelete: Cascade)
- @@foreignKey([pageVersionId], references: [id], onDelete: Cascade)
+ user User @relation(fields: [userId], references: [id], onDelete: Cascade)
+ pageVersion PageVersion @relation(fields: [pageVersionId], references: [id], onDelete: Cascade)
| import { createHash } from "node:crypto"; | ||
| import { prisma } from "./prisma"; | ||
| import { prisma } from "../utils/prisma"; | ||
|
|
||
| export async function getOrCreateSourceTextId( | ||
| text: string, |
There was a problem hiding this comment.
Note: This review was outside of the patch, but no patch was found that overlapped with it. Original lines [16-18]
requireUserId関数の呼び出しが不要です。ユーザーIDは使用されていないため、削除することをお勧めします。
- const userId = await requireUserId(request);
| @@ -1,4 +1,4 @@ | |||
| import { prisma } from "./prisma"; | |||
| import { prisma } from "../utils/prisma"; | |||
There was a problem hiding this comment.
インポートの順序を整理し、標準ライブラリ、サードパーティライブラリ、ローカルモジュールの順に並べると良いでしょう。
+ import { json, LoaderFunction, ActionFunction } from "@remix-run/node";
+ import { useLoaderData, useActionData } from "@remix-run/react";
+ import { z } from "zod";
+ import { db } from "~/utils/db.server";
+ import { requireUserId } from "~/utils/session.server";
+ import { TranslatePage } from "~/components/TranslatePage";
| import { prisma } from "../utils/prisma"; | ||
|
|
||
| export async function updateUserReadHistory( | ||
| userId: number, |
There was a problem hiding this comment.
Note: This review was outside of the patch, but no patch was found that overlapped with it. Original lines [14-16]
エラーハンドリングが不足しています。データベースからデータを取得する際にエラーが発生した場合に備えて、try-catchブロックを追加しましょう。
export const loader: LoaderFunction = async ({ request }) => {
const userId = await requireUserId(request);
- const data = await db.translation.findMany({
- where: { userId },
- });
+ let data;
+ try {
+ data = await db.translation.findMany({
+ where: { userId },
+ });
+ } catch (error) {
+ console.error("Failed to load translations:", error);
+ throw new Response("Failed to load translations", { status: 500 });
+ }
return json(data);
};
| import { prisma } from "../utils/prisma"; | ||
|
|
||
| export async function updateUserReadHistory( | ||
| userId: number, |
There was a problem hiding this comment.
Note: This review was outside of the patch, but no patch was found that overlapped with it. Original lines [24-26]
フォームデータの処理中にエラーが発生した場合に備えて、try-catchブロックを追加しましょう。
export const action: ActionFunction = async ({ request }) => {
const userId = await requireUserId(request);
- const formData = await request.formData();
- const result = translationSchema.safeParse(Object.fromEntries(formData));
+ let formData;
+ try {
+ formData = await request.formData();
+ } catch (error) {
+ console.error("Failed to parse form data:", error);
+ return json({ error: "Invalid form data" }, { status: 400 });
+ }
+
+ const result = translationSchema.safeParse(Object.fromEntries(formData));
| import { prisma } from "../utils/prisma"; | ||
|
|
||
| export async function updateUserReadHistory( | ||
| userId: number, |
There was a problem hiding this comment.
Note: This review was outside of the patch, but no patch was found that overlapped with it. Original lines [28-28]
翻訳API呼び出し時のエラーハンドリングが不足しています。try-catchブロックを追加して、エラーが発生した場合に適切なレスポンスを返すようにしましょう。
const { text, targetLanguage } = result.data;
// Call the translation API here
- const translatedText = await translateText(text, targetLanguage);
+ let translatedText;
+ try {
+ translatedText = await translateText(text, targetLanguage);
+ } catch (error) {
+
<!-- This is an auto-generated comment by OSS CodeRabbit -->
| import { redirect } from "@remix-run/node"; | ||
| import { typedjson, useTypedLoaderData } from "remix-typedjson"; | ||
| import { z } from "zod"; | ||
| import { Header } from "~/components/Header"; | ||
| import { json } from "@remix-run/node"; |
There was a problem hiding this comment.
redirectとjsonのインポート順序が逆になっています。コードの一貫性を保つために、アルファベット順に並べ替えることをお勧めします。
import { redirect } from "@remix-run/node";
import { json } from "@remix-run/node";
| import { z } from "zod"; | ||
| import { Header } from "~/components/Header"; | ||
| import { json } from "@remix-run/node"; | ||
| import { useLoaderData } from "@remix-run/react"; |
There was a problem hiding this comment.
useLoaderDataはreactからインポートされていますが、以前のコードではremix-typedjsonのuseTypedLoaderDataを使用していました。型安全性を確保するために、useTypedLoaderDataを引き続き使用することを検討してください。
- import { useLoaderData } from "@remix-run/react";
+ import { typedjson, useTypedLoaderData } from "remix-typedjson";
| import { authenticator } from "~/utils/auth.server"; | ||
| import { getSession } from "~/utils/session.server"; | ||
| import { extractNumberedElements } from "../../utils/extractNumberedElements"; | ||
| import { prisma } from "../../utils/prisma"; | ||
| import { validateGeminiApiKey } from "~/utils/gemini"; |
There was a problem hiding this comment.
authenticatorとvalidateGeminiApiKeyのインポート順序が逆になっています。コードの一貫性を保つために、アルファベット順に並べ替えることをお勧めします。
import { authenticator } from "~/utils/auth.server";
import { validateGeminiApiKey } from "~/utils/gemini";
| import { GoogleSignInAndGeminiApiKeyForm } from "./components/GoogleSignInAndGeminiApiKeyForm"; | ||
| import { | ||
| URLTranslationForm, | ||
| urlTranslationSchema, | ||
| } from "./components/URLTranslationForm"; | ||
| import { addNumbersToContent } from "./utils/addNumbersToContent"; | ||
| import { extractArticle } from "./utils/articleUtils"; | ||
| import { fetchWithRetry } from "./utils/fetchWithRetry"; | ||
| import { validateGeminiApiKey } from "./utils/gemini"; | ||
| import { translate } from "./utils/translation"; | ||
| import { geminiApiKeySchema } from "./types"; |
There was a problem hiding this comment.
GoogleSignInAndGeminiApiKeyFormとgeminiApiKeySchemaのインポート順序が逆になっています。コードの一貫性を保つために、アルファベット順に並べ替えることをお勧めします。
import { GoogleSignInAndGeminiApiKeyForm } from "./components/GoogleSignInAndGeminiApiKeyForm";
import { geminiApiKeySchema } from "./types";
| @@ -5,7 +5,7 @@ export const loader = ({ request }: LoaderFunctionArgs) => { | |||
| try { | |||
| return authenticator.authenticate("google", request, { | |||
| successRedirect: "/", | |||
| failureRedirect: "/auth/login", | |||
| failureRedirect: "/", | |||
There was a problem hiding this comment.
エラーメッセージが具体的でないため、ユーザーにとってわかりやすいメッセージに変更することをお勧めします。
- if (!pageId) throw new Response("Page ID is required", { status: 400 });
+ if (!pageId) throw new Response("ページIDが必要です", { status: 400 });
| try { | ||
| return authenticator.authenticate("google", request, { | ||
| successRedirect: "/", | ||
| failureRedirect: "/auth/login", | ||
| failureRedirect: "/", | ||
| }); | ||
| } catch (error) { | ||
| console.error("Google authentication error:", error); |
There was a problem hiding this comment.
Note: This review was outside of the patch, but no patch was found that overlapped with it. Original lines [16-16]
エラーメッセージが具体的でないため、ユーザーにとってわかりやすいメッセージに変更することをお勧めします。
- if (!text) throw new Response("Text is required", { status: 400 });
+ if (!text) throw new Response("テキストが必要です", { status: 400 });
| import { prisma } from "../utils/prisma"; | ||
|
|
||
| export async function getOrCreateAIUser(name: string): Promise<number> { | ||
| const user = await prisma.user.upsert({ | ||
| where: { email: `${name}@ai.com` }, | ||
| update: {}, | ||
| create: { name, email: `${name}@ai.com`, isAI: true }, | ||
| create: { name, email: `${name}@ai.com`, isAI: true, image: "" }, | ||
| }); | ||
|
|
||
| return user.id; |
There was a problem hiding this comment.
Note: This review was outside of the patch, but no patch was found that overlapped with it. Original lines [32-32]
requireUserId関数はユーザーIDを取得するために使用されていますが、ユーザーIDが存在しない場合のエラーハンドリングが不足しています。以下のように修正してください。
- const userId = await requireUserId(request);
+ let userId;
+ try {
+ userId = await requireUserId(request);
+ } catch (error
<!-- This is an auto-generated comment by OSS CodeRabbit -->
| const createUserTranslationQueue = (userId: number) => | ||
| new Queue(`translation-user-${userId}`, REDIS_URL, { | ||
| defaultJobOptions: { | ||
| removeOnComplete: true, | ||
| removeOnFail: true | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Queueのインスタンスを作成する際に、defaultJobOptionsの設定が適切ですが、ジョブの再試行回数を指定することで、失敗したジョブの自動再試行を追加することをお勧めします。これにより、一時的なエラーによるジョブの失敗を減らすことができます。
new Queue(`translation-user-${userId}`, REDIS_URL, {
defaultJobOptions: {
removeOnComplete: true,
removeOnFail: true,
+ attempts: 3,
+ backoff: {
+ type: 'exponential',
+ delay: 5000,
+ },
},
});
| export const geminiApiKeySchema = z.object({ | ||
| geminiApiKey: z.string().min(1, "API key is required"), | ||
| }); |
There was a problem hiding this comment.
geminiApiKeyのスキーマ定義で、APIキーが空文字列でないことを確認していますが、APIキーの形式や長さに関する追加のバリデーションがあるとより安全です。
- geminiApiKey: z.string().min(1, "API key is required"),
+ geminiApiKey: z.string().min(32, "API key must be at least 32 characters long").regex(/^[A-Za-z0-9_-]+$/, "Invalid API key format"),
| targetLanguage: z.string(), | ||
| aiTranslationStatus: z.string(), | ||
| aiTranslationProgress: z.number(), | ||
| lastTranslatedAt: z.string().or(z.date()), |
There was a problem hiding this comment.
lastTranslatedAtフィールドは文字列または日付型を受け入れますが、一貫性を保つためにISO 8601形式の文字列のみを許可する方が良いでしょう。
- lastTranslatedAt: z.string().or(z.date()),
+ lastTranslatedAt: z.string().refine((val) => !isNaN(Date.parse(val)), {
+ message: "Invalid date format",
+ }),
| export const UserAITranslationInfoSchema = z.object({ | ||
| id: z.number(), | ||
| userId: z.number(), | ||
| pageVersionId: z.number(), | ||
| targetLanguage: z.string(), | ||
| aiTranslationStatus: z.string(), | ||
| aiTranslationProgress: z.number(), | ||
| lastTranslatedAt: z.string().or(z.date()), | ||
| pageVersion: z.object({ | ||
| title: z.string(), | ||
| page: z.object({ | ||
| url: z.string(), | ||
| }), | ||
| pageVersionTranslationInfo: z | ||
| .array(PageVersionTranslationInfoSchema) | ||
| .optional(), | ||
| }), | ||
| }); |
There was a problem hiding this comment.
lastTranslatedAtのスキーマ定義において、.or(z.date())は冗長です。z.string().datetime()を使用することで、ISO 8601形式の日付文字列を検証できます。
- lastTranslatedAt: z.string().or(z.date()),
+ lastTranslatedAt: z.string().datetime(),
| import { z } from "zod"; | ||
|
|
||
| export type TranslationStatus = "pending" | "in_progress" | "completed" | "failed"; | ||
|
|
||
| export interface TranslationStatusRecord { | ||
| id: number; | ||
| pageVersionId: number; | ||
| language: string; | ||
| status: TranslationStatus; | ||
| } | ||
|
|
||
| export const geminiApiKeySchema = z.object({ | ||
| geminiApiKey: z.string().min(1, "API key is required"), | ||
| }); |
There was a problem hiding this comment.
Note: This review was outside of the patch, but no patch was found that overlapped with it. Original lines [8-27]
loader関数内で、pageVersionIdを取得する際にNumber関数を使用していますが、これは潜在的なエラーの原因となる可能性があります。parseIntを使用して明示的に整数に変換する方が安全です。
- const pageVersion = await db.pageVersion.findUnique({
- where: { id: Number(pageVersionId) },
+ const pageVersion = await db.pageVersion.findUnique({
+ where: { id: parseInt(pageVersionId, 10) },
| import { z } from "zod"; | ||
|
|
||
| export type TranslationStatus = "pending" | "in_progress" | "completed" | "failed"; | ||
|
|
||
| export interface TranslationStatusRecord { | ||
| id: number; | ||
| pageVersionId: number; | ||
| language: string; | ||
| status: TranslationStatus; | ||
| } | ||
|
|
||
| export const geminiApiKeySchema = z.object({ | ||
| geminiApiKey: z.string().min(1, "API key is required"), | ||
| }); |
There was a problem hiding this comment.
Note: This review was outside of the patch, but no patch was found that overlapped with it. Original lines [35-56]
action関数内で、フォームデータの処理後にデータベース操作を行っていますが、トランザクションを使用して一貫性を保つことを検討してください。また、エラーハンドリングを強化するためにtry-catchブロックを追加することをお勧めします。
- export const action = async ({ request }) => {
- const userId = await requireUserId(request);
- const formData = await request.formData();
- const result = schema.safeParse(Object.fromEntries(formData));
-
- if (!result.success) {
- return json(result.error.format(), { status: 400 });
- }
-
- const { pageVersionId, targetLanguage, translationText } = result.data;
-
- const translation = await db.translation.create({
- data: {
- pageVersionId,
- targetLanguage,
- translationText,
- userId,
- },
- });
-
- return redirect(`/translate/${translation.id}`);
- };
+ export const action = async ({ request }) => {
+ try {
+
<!-- This is an auto-generated comment by OSS CodeRabbit -->
| let lastError: Error | null = null; | ||
|
|
||
| for (let retryCount = 0; retryCount < MAX_RETRIES; retryCount++) { | ||
| try { | ||
| const result = await modelConfig.generateContent( | ||
| generateSystemMessage(title, source_text, target_language), | ||
| ); | ||
| console.log("result", result.response.text()); | ||
| return result.response.text(); | ||
| } catch (error: unknown) { | ||
| const typedError = error as Error; | ||
| console.error( | ||
| `Translation attempt ${retryCount + 1} failed:`, | ||
| typedError, | ||
| ); | ||
| lastError = typedError; | ||
|
|
||
| if (retryCount < MAX_RETRIES - 1) { | ||
| const delay = 1000 * (retryCount + 1); | ||
| console.log(`Retrying in ${delay / 100} seconds...`); | ||
| await new Promise((resolve) => setTimeout(resolve, delay)); | ||
| } | ||
| } | ||
| } | ||
| console.error("Max retries reached. Translation failed."); | ||
| throw lastError || new Error("Translation failed after max retries"); |
There was a problem hiding this comment.
リトライロジックの遅延計算に誤りがあります。遅延時間を秒単位でログ出力していますが、実際にはミリ秒単位で計算されています。
- console.log(`Retrying in ${delay / 100} seconds...`);
+ console.log(`Retrying in ${delay / 1000} seconds...`);
| - You are about to drop the column `password` on the `users` table. All the data in the column will be lost. | ||
| - Added the required column `image` to the `users` table without a default value. This is not possible if the table is not empty. |
There was a problem hiding this comment.
この変更は、既存のデータベースにデータがある場合に問題を引き起こす可能性があります。特に、imageカラムにデフォルト値が設定されていないため、既存のレコードに対してNULL制約違反が発生する可能性があります。以下のようにデフォルト値を設定することを検討してください。
- ADD COLUMN "image" TEXT NOT NULL;
+ ADD COLUMN "image" TEXT NOT NULL DEFAULT 'default_image_url';
| import { createRemixStub } from "@remix-run/testing"; | ||
| import { test, vi } from "vitest"; | ||
| import { action } from "../app/routes/_index/utils/translation"; | ||
| import { action } from "../app/routes/_index/libs/translation"; | ||
|
|
||
| // モックの translateAndDisplayContent 関数 | ||
| vi.mock("./translation", () => ({ |
There was a problem hiding this comment.
Note: This review was outside of the patch, but no patch was found that overlapped with it. Original lines [7-7]
pageIdがnullの場合にエラーメッセージを返す前に、return文を追加することで処理を終了させるべきです。
- throw new Response("Page ID is required", { status: 400 });
+ return new Response("Page ID is required", { status: 400 });
キューにした
Summary by CodeRabbit