Feat/refacter#39
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can disable sequence diagrams in the walkthrough.Disable the |
Deploying splatoon-random-weapon with
|
| Latest commit: |
d5ad3a0
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://598956cb.splatoon-random-weapon.pages.dev |
| Branch Preview URL: | https://feat-refacter.splatoon-random-weapon.pages.dev |
Summary of ChangesHello, 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 introduces significant refactoring to enhance the "Splatoon Random Weapon" application. The core changes revolve around implementing a persistent history for weapon draw results, improving the robustness of the random weapon selection API with input validation, and overhauling the frontend user interface to display this new history and provide a more interactive experience. Additionally, the project's development environment has been streamlined with updated dependencies and the adoption of pre-commit hooks for better code quality. A new developer guide has also been added to facilitate onboarding and maintenance. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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. Footnotes
|
There was a problem hiding this comment.
Pull request overview
This PR refactors the Splatoon Random Weapon app to use real backend-backed “result history” (KV) instead of hardcoded sample cards, improves the UI/UX around submission/error states, and tightens backend request validation.
Changes:
- Frontend: add submission/error UI state, wire history list to
/api/results, and support subtitles/controlled select/disabled button. - Backend: validate
/weapons/randomcount, and add/resultsGET + POST with KV persistence (latest 10). - Tooling/docs: update Vite version, update app title, and add repository guidance in
SKILL.md.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| yarn.lock | Lockfile updates reflecting dependency/tooling changes (e.g., Vite/esbuild/rollup). |
| package.json | Removes wrangler/miniflare deps; downgrades Vite to ^4.5.0. |
| index.html | Updates document title to “Splatoon Random Weapon”. |
| src/app.tsx | Wires UI to results API, adds loading/error/submitting states, persists results, renders history from SWR. |
| src/components/SelectBox.tsx | Adds value prop to support controlled selection. |
| src/components/Button.tsx | Adds disabled prop + disabled styling. |
| src/components/Card.tsx | Adds optional subtitle display. |
| src/components/CardList.tsx | Adds optional id and passes subtitle through; changes key strategy. |
| functions/api/routes/weapon.ts | Sanitizes/validates count and prevents unsafe LIMIT interpolation. |
| functions/api/routes/result.ts | Adds result history types and implements GET/POST persistence in KV. |
| README.md | Updates backend/tooling bullet points (Wrangler/Husky/lint-staged). |
| SKILL.md | Adds repo “skill” doc, but currently includes a couple outdated statements (see comments). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const body = (await c.req.json()) as CreateResultPayload | ||
| const weaponList = body.weaponList?.filter(Boolean) ?? [] | ||
|
|
There was a problem hiding this comment.
POST /results assumes body.weaponList is an array and calls .filter on it. If the client sends a non-array value (or invalid JSON that still parses), this will throw and return a 500. Please validate the request body (e.g., ensure weaponList is an array) and return a 400 for invalid payloads.
| const body = (await c.req.json()) as CreateResultPayload | |
| const weaponList = body.weaponList?.filter(Boolean) ?? [] | |
| let body: CreateResultPayload | |
| try { | |
| body = (await c.req.json()) as CreateResultPayload | |
| } catch { | |
| return c.json({ message: 'Invalid JSON payload' }, 400) | |
| } | |
| if (!body || !Array.isArray(body.weaponList)) { | |
| return c.json({ message: 'weaponList must be an array' }, 400) | |
| } | |
| const weaponList = body.weaponList.filter(Boolean) |
| const body = (await c.req.json()) as CreateResultPayload | ||
| const weaponList = body.weaponList?.filter(Boolean) ?? [] | ||
|
|
||
| if (weaponList.length === 0) { | ||
| return c.json({ message: 'weaponList is required' }, 400) | ||
| } | ||
|
|
||
| const currentResults = | ||
| (await c.env.RANDOM_WEAPONS.get<ResultHistoryItem[]>('results', 'json')) ?? | ||
| [] | ||
| const nextResult: ResultHistoryItem = { | ||
| id: crypto.randomUUID(), | ||
| title: `結果 ${currentResults.length + 1}`, | ||
| weaponList, | ||
| createdAt: new Date().toISOString(), | ||
| } | ||
| const nextResults = [nextResult, ...currentResults].slice(0, 10) | ||
|
|
||
| await c.env.RANDOM_WEAPONS.put('results', JSON.stringify(nextResults)) |
There was a problem hiding this comment.
weaponList is currently accepted with no upper bound and without validating element types. Since this is persisted into KV, a malicious/accidental large payload could exceed KV limits or bloat storage; non-string items could also break frontend assumptions. Consider enforcing a max length (e.g., 4) and ensuring each entry is a non-empty string before storing.
|
|
||
| import { AppType } from '../functions/api/[[route]]' | ||
| import { ResultHistoryItem } from '../functions/api/routes/result' | ||
| import { Weapon } from '../functions/api/routes/weapon' |
There was a problem hiding this comment.
This import is used only as a type. Prefer import type here to guarantee it’s erased from the client bundle (and to avoid accidentally pulling server-only modules into the browser build if it later becomes value-used).
| import { Weapon } from '../functions/api/routes/weapon' | |
| import type { Weapon } from '../functions/api/routes/weapon' |
| type CardListProps = { | ||
| cards: { | ||
| id?: string | ||
| title: string | ||
| subtitle?: string | ||
| weaponList: Array<string> | ||
| }[] |
There was a problem hiding this comment.
cards items may be prepended/inserted over time (history list). Falling back to index for the React/Preact key will cause unstable keys when order changes, which can lead to unnecessary remounts and lost component state. Since history items now have an id, consider making id required and using it as the key unconditionally.
| - Be careful with the boundary between real data and placeholders: | ||
| - `useSWR('results', ...)` fetches live result data from KV. | ||
| - `cards` in `src/app.tsx` are currently hardcoded sample history entries. | ||
| - Do not assume the rendered history UI is already wired to backend data. | ||
| - `src/constants/weapon.ts` appears to be legacy or unused. Confirm usage before editing it. | ||
| - `tsconfig.json` includes only `src`, even though the frontend imports types from `functions/`. If type changes behave strangely, inspect this include boundary first. | ||
| - `README.md` mentions `lefthook`, while `package.json` is configured for `husky` and `lint-staged`. Avoid assuming hooks are consistently installed. |
There was a problem hiding this comment.
This section is now out of sync with the repo after the changes in this PR: src/app.tsx no longer has hardcoded cards, and README.md no longer mentions lefthook. Please update/remove these bullets to reflect the current state so this doc doesn’t mislead future work.
| - Be careful with the boundary between real data and placeholders: | |
| - `useSWR('results', ...)` fetches live result data from KV. | |
| - `cards` in `src/app.tsx` are currently hardcoded sample history entries. | |
| - Do not assume the rendered history UI is already wired to backend data. | |
| - `src/constants/weapon.ts` appears to be legacy or unused. Confirm usage before editing it. | |
| - `tsconfig.json` includes only `src`, even though the frontend imports types from `functions/`. If type changes behave strangely, inspect this include boundary first. | |
| - `README.md` mentions `lefthook`, while `package.json` is configured for `husky` and `lint-staged`. Avoid assuming hooks are consistently installed. | |
| - `src/constants/weapon.ts` appears to be legacy or unused. Confirm usage before editing it. | |
| - `tsconfig.json` includes only `src`, even though the frontend imports types from `functions/`. If type changes behave strangely, inspect this include boundary first. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
| let { results }: Weapons = await c.env.DB.prepare( | ||
| `SELECT * FROM Weapons ORDER BY RANDOM() LIMIT ${count};` | ||
| `SELECT * FROM Weapons ORDER BY RANDOM() LIMIT ${parsedCount};` | ||
| ).all() |
There was a problem hiding this comment.
SQLインジェクションのリスクを完全に排除するため、D1のプリペアドステートメントでは、クエリ文字列に直接変数を埋め込むのではなく、bind()メソッドを使用したパラメータバインディングを利用することが強く推奨されます。
現在はparsedCountが整数であることが検証されていますが、パラメータバインディングを用いることで、より安全で標準的な実装になります。
let { results }: Weapons = await c.env.DB.prepare(
`SELECT * FROM Weapons ORDER BY RANDOM() LIMIT ?`
)
.bind(parsedCount)
.all()| return c.json(results) | ||
| }) | ||
| .post('/', async (c) => { | ||
| const body = (await c.req.json()) as CreateResultPayload |
There was a problem hiding this comment.
リクエストボディを as CreateResultPayload で型アサーションしていますが、これでは型安全性が保証されず、予期しない形式のデータが送られてきた場合にランタイムエラーが発生する可能性があります。
Honoには zodValidator などのバリデーションミドルウェアが用意されています。これらを利用してリクエストボディの検証と型付けを安全に行うことをお勧めします。これにより、コードの堅牢性が向上し、意図しないデータに対するエラーハンドリングも容易になります。
これは、スタイルガイドの「any の回避」(19行目)の原則にも合致する改善です。
References
anyの使用は極力避け、具体的な型を指定します。 (link)
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
No description provided.