Release/v1.4.0#251
Conversation
backmerge
backmerge
(cherry picked from commit 958cdad)
hotfix: layout fixのfix
misc: remove @h-takeyeah from CODEOWNERS
## チケットへのリンク 無し ## やったこと - @chakra-ui/reactの削除 - Chakra UI依存箇所 (LogoutButton、Toast) を独自の実装へ変更 ## やらないこと 見て分かるレベルのデザイン変更 (Toast除く) ## できるようになること(ユーザ目線) 無し ## できなくなること(ユーザ目線) 無し ## 動作確認 全機能を目視で確認していますが、レビューされる方はお手元の環境でダブルチェックをお願いします (特に生協指定PCをお持ちの方) ## その他 無し <!-- devin-review-badge-begin --> --- <a href="https://app.devin.ai/review/su-its/typing/pull/244" target="_blank"> <picture> <source media="(prefers-color-scheme: dark)" srcset="https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1"> <img src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1" alt="Open with Devin"> </picture> </a> <!-- devin-review-badge-end -->
back merge
## チケットへのリンク 無し ## やったこと UIデザインの軽微な改善 (特にGamePreとGameResult) ## やらないこと 挙動及び大規模なデザインの変更 ## できるようになること(ユーザ目線) 無し ## できなくなること(ユーザ目線) 無し ## 動作確認 全機能を目視で確認していますが、レビューされる方はお手元の環境でダブルチェックをお願いします (特に生協指定PCをお持ちの方) ## その他 無し <!-- devin-review-badge-begin --> --- <a href="https://app.devin.ai/review/su-its/typing/pull/246" target="_blank"> <picture> <source media="(prefers-color-scheme: dark)" srcset="https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1"> <img src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1" alt="Open with Devin"> </picture> </a> <!-- devin-review-badge-end --> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: h-takeyeah <61489178+h-takeyeah@users.noreply.github.com>
1.0.0当時にやるべき作業を1.4.0リリース時にやった
| import Separator from "../atoms/Separater"; | ||
| import styles from "@/assets/sass/organism/Footer.module.scss"; | ||
|
|
||
| const date = new Date(); |
There was a problem hiding this comment.
🟡 Footer copyright year is frozen at module-load/build time due to module-level new Date()
const date = new Date() is evaluated once at module-import time, not at render time. Since Footer is a server component used in layout.tsx, this value is captured either at build time (for statically generated pages) or at server-process start time. If the server process runs across a year boundary (e.g., started in 2025, still running in 2026), the footer will display a stale year. The fix is to call new Date().getFullYear() inside the component function body so it's evaluated fresh on each render.
Prompt for agents
In typing-app/src/components/organism/Footer.tsx, the `const date = new Date()` on line 6 is at module scope, outside the component function. This means the Date object is created once when the module is first imported (which could be at build time or server start time), and its year value never updates. Move the date creation inside the Footer component body, e.g. replace `{date.getFullYear()}` on line 14 with `{new Date().getFullYear()}` and remove line 6 entirely.
Was this helpful? React with 👍 or 👎 to provide feedback.
| @media (max-width: 1024px) { | ||
| width: 90%; | ||
| left: 5%; | ||
| } |
There was a problem hiding this comment.
🟡 Duplicate @media (max-width: 1024px) block in GamePre.module.scss
The .game-pre rule contains two identical @media (max-width: 1024px) blocks: one at lines 14-17 (pre-existing) and another at lines 22-25 (newly added). The second block has the exact same properties (width: 90%; left: 5%;), making it redundant. While CSS applies the last matching rule and the values happen to be the same so there's no visual impact, this appears to be an unintentional copy-paste where the new media queries (max-width: 1024px and max-height: 800px) should have replaced the old one rather than being appended alongside it.
| @media (max-width: 1024px) { | |
| width: 90%; | |
| left: 5%; | |
| } | |
| @media (max-height: 800px) { | |
Was this helpful? React with 👍 or 👎 to provide feedback.
| useEffect(() => { | ||
| if (!pending && state.error) { | ||
| showWarningToast(state.error); | ||
| } | ||
| }, [pending, state.error]); |
There was a problem hiding this comment.
📝 Info: LoginModal useEffect correctly fixes stale closure bug in old form action
The old code had a subtle bug: the form action callback captured state.error in a closure at render time, then called await dispatchAction(formData) followed by state.error && showWarningToast(state.error). Because state is from React state and dispatchAction triggers a state update asynchronously, the state.error read after await still referred to the previous render's error, not the newly returned error. The new useEffect approach at LoginModal.tsx:17-21 correctly reacts to state changes after useActionState updates pending and state.error, ensuring the toast displays the actual current error.
Was this helpful? React with 👍 or 👎 to provide feedback.
| {new Intl.NumberFormat("en-US", { | ||
| style: "percent", | ||
| maximumFractionDigits: 2, | ||
| }).format(score.accuracy)} | ||
| }).format(score.accuracy * 100)} | ||
| <span>%</span> |
There was a problem hiding this comment.
📝 Info: GameResult accuracy formatting change is mathematically equivalent
The old code used Intl.NumberFormat with style: 'percent' which internally multiplies by 100 and appends '%'. The new code at line 57 manually does score.accuracy * 100 and appends <span>%</span> separately. Since score.accuracy is computed as correctKeystrokes / totalKeystrokes (a 0-1 decimal) in GameTyping.tsx:62, both approaches yield identical numeric output. Note that RankingTabs.tsx:34-38 still uses style: 'percent' for the same accuracy field from the API — this is consistent since the API stores accuracy as a 0-1 float per the OpenAPI spec (typing-server/openapi.yaml:282-284).
Was this helpful? React with 👍 or 👎 to provide feedback.
| display: block; | ||
| justify-content: center; | ||
| align-items: center; |
There was a problem hiding this comment.
📝 Info: Inert CSS properties: justify-content and align-items on display: block elements
Both GamePre.module.scss:41-43 and GameResult.module.scss:39-42 set display: block alongside justify-content: center and align-items: center. These flexbox/grid properties have no effect on block-level elements. This appears to be a leftover from the old code where .header used display: flex. While not causing incorrect behavior, it's misleading dead CSS that could confuse future maintainers.
Was this helpful? React with 👍 or 👎 to provide feedback.
| padding: 0; | ||
| font-family: "メイリオ", "Meiryo", "Hiragino Kaku Gothic Pro", "ヒラギノ角ゴ Pro W3", "MS Pゴシック", sans-serif; | ||
| box-sizing: border-box; | ||
| user-select: none; |
There was a problem hiding this comment.
🚩 Global user-select: none may affect developer experience and copy-paste scenarios
The user-select: none on * at globals.css:12 prevents all text selection across the entire application. While this makes sense for a typing game (preventing users from selecting/copying the subject text to cheat), it also prevents selecting text in the ranking table, result screen, footer copyright, and any other text on the page. This is likely intentional for the game context but worth confirming that stakeholders are aware of the broad scope.
Was this helpful? React with 👍 or 👎 to provide feedback.
| useEffect(() => { | ||
| const handler = (event: CustomEvent<ToastData>) => { | ||
| if (closingTimerRef.current) clearTimeout(closingTimerRef.current); | ||
| if (removeTimerRef.current) clearTimeout(removeTimerRef.current); | ||
| setIsClosing(false); | ||
| setToast(event.detail); | ||
|
|
||
| closingTimerRef.current = setTimeout(() => { | ||
| setIsClosing(true); | ||
| removeTimerRef.current = setTimeout(() => { | ||
| setToast(null); | ||
| }, 500); | ||
| }, 3000); | ||
| }; | ||
| window.addEventListener("app-toast", handler as EventListener); | ||
| return () => { | ||
| window.removeEventListener("app-toast", handler as EventListener); | ||
| if (closingTimerRef.current) clearTimeout(closingTimerRef.current); | ||
| if (removeTimerRef.current) clearTimeout(removeTimerRef.current); | ||
| }; | ||
| }, []); |
There was a problem hiding this comment.
📝 Info: Toast component timer cleanup is correctly implemented
The Toast component at Toast.tsx:27-47 uses refs for both closingTimerRef and removeTimerRef, and properly clears them both when a new toast arrives (preventing stale timers from previous toasts) and on component unmount. The pattern of clearing existing timers before setting new ones on line 29-30 correctly handles rapid successive toast events, ensuring only the latest toast's timers are active.
Was this helpful? React with 👍 or 👎 to provide feedback.
| export default function DisableTab() { | ||
| useEffect(() => { | ||
| const handleKeyDown = (e: KeyboardEvent) => { | ||
| if (e.key === "Tab") { | ||
| e.preventDefault(); | ||
| } | ||
| }; | ||
| window.addEventListener("keydown", handleKeyDown); | ||
| return () => { | ||
| window.removeEventListener("keydown", handleKeyDown); | ||
| }; | ||
| }, []); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🚩 DisableTab globally prevents Tab key, breaking keyboard accessibility
The DisableTab component at DisableTab.tsx:3-16 adds a global keydown listener that calls preventDefault() on all Tab key presses. This completely disables keyboard navigation (Tab is the primary way keyboard users move between focusable elements). While this is likely intentional for the typing game to prevent accidental focus changes during gameplay, it's applied at the root layout level (layout.tsx:20), meaning it affects ALL pages including the home page and ranking view where keyboard navigation would be useful.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
本当はPRの途中でdevelopからマージするのはダメなんだけどやってしまった.すみません. |
| import Separator from "../atoms/Separater"; | ||
| import styles from "@/assets/sass/organism/Footer.module.scss"; | ||
|
|
||
| const date = new Date(); |
There was a problem hiding this comment.
📝 Info: Footer copyright year is computed at module scope, cached for server lifetime
const date = new Date() at module scope (Footer.tsx:6) is evaluated once when the module is first imported. In production, this means date.getFullYear() returns the year at server start (or build time for static pages), not the current year at render time. This would only become stale if the server runs across a year boundary (Dec 31 → Jan 1) without restart. Extremely unlikely to matter in practice, but moving new Date().getFullYear() inside the component function would be strictly more correct.
Was this helpful? React with 👍 or 👎 to provide feedback.
| useEffect(() => { | ||
| const handler = (event: CustomEvent<ToastData>) => { | ||
| if (closingTimerRef.current) clearTimeout(closingTimerRef.current); | ||
| if (removeTimerRef.current) clearTimeout(removeTimerRef.current); | ||
| setIsClosing(false); | ||
| setToast(event.detail); | ||
|
|
||
| closingTimerRef.current = setTimeout(() => { | ||
| setIsClosing(true); | ||
| removeTimerRef.current = setTimeout(() => { | ||
| setToast(null); | ||
| }, 500); | ||
| }, 3000); | ||
| }; | ||
| window.addEventListener("app-toast", handler as EventListener); | ||
| return () => { | ||
| window.removeEventListener("app-toast", handler as EventListener); | ||
| if (closingTimerRef.current) clearTimeout(closingTimerRef.current); | ||
| if (removeTimerRef.current) clearTimeout(removeTimerRef.current); | ||
| }; | ||
| }, []); |
There was a problem hiding this comment.
📝 Info: Toast component relies on CustomEvent timing with sibling mount order
The Toast component registers its window.addEventListener("app-toast", ...) in a useEffect. Since Toast is rendered as the last child of <body> in layout.tsx:24, its effect fires after sibling children's effects. If any child component dispatched a toast synchronously in its useEffect, the event could be lost. In practice, all current toast call sites (Game.tsx:36, LoginModal.tsx:17, RankingTabs.tsx:59, GameTyping.tsx:49) trigger toasts asynchronously (after awaiting network calls), so the timing is safe today — but adding a synchronous toast call in a child component's effect would silently fail.
Was this helpful? React with 👍 or 👎 to provide feedback.
openapiのバージョンを1.0.0にしていることの説明: openapi.yamlに対する変更が1.0.0リリース時までのものを最後に今まで無いので,1.4.0ではなく1.0.0にしています.