仮デプロイ - #33
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds session validation functionality and UI improvements to the Pay Crew2 application. The title "仮デプロイ" (Temporary Deployment) suggests this is a staging deployment for testing purposes.
Changes:
- Added session check endpoint (
/api/session) for validating user sessions - Introduced new routing structure with
Sharelayout andSessionCheckwrapper components - Created reusable UI components (Title, SubTitle, MarkdownContent, Loading, LinkButton, Error)
- Added Terms and Privacy policy pages with markdown rendering support
- Updated HTML metadata, favicons, and global styling
Reviewed changes
Copilot reviewed 41 out of 59 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| products/validator/src/response/check.ts | New validator schema for session check response |
| products/backend/src/presentation/routes/check.ts | New backend endpoint for session validation |
| products/frontend/src/routes/Share/index.tsx | New layout component with header and footer |
| products/frontend/src/routes/SessionCheck/index.tsx | Session validation wrapper for protected routes |
| products/frontend/src/share/*.tsx | Reusable UI components (Title, SubTitle, MarkdownContent) |
| products/frontend/src/routes/Root/index.tsx | Refactored to use session check and new components |
| products/frontend/index.html | Added comprehensive metadata, OGP tags, and favicon references |
| package.json | Added react-markdown and remark-gfm dependencies |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }; | ||
|
|
||
| const SubTitle: FC<Props> = (props: Props) => { | ||
| return <h1 className={styles.subTitle}>{props.subTitle}</h1>; |
There was a problem hiding this comment.
SubTitle component uses <h1> which is semantically incorrect for a subtitle. Subtitles should use <h2> or lower heading levels. Using multiple <h1> tags on a page can harm SEO and accessibility.
| }; | ||
|
|
||
| const Loading: FC<Props> = (props: Props) => { | ||
| return <h1 className={styles.title}>{props.content}</h1>; |
There was a problem hiding this comment.
Loading component uses <h1> for displaying loading text, which is semantically incorrect. Loading messages should use <p> or <div> elements instead of heading tags.
| const LinkButton: FC<Props> = (props: Props) => { | ||
| return <h1 className={styles.title}>{props.content}</h1>; | ||
| }; |
There was a problem hiding this comment.
LinkButton component renders an <h1> element but doesn't use the path prop to create an actual link. This component should render a link element (<Link> or <a>) using the path prop to be functional as a button.
| }; | ||
|
|
||
| const Error: FC<Props> = (props: Props) => { | ||
| return <h1 className={styles.title}>{props.content}</h1>; |
There was a problem hiding this comment.
Error component uses <h1> for displaying error messages, which is semantically incorrect. Error messages should use appropriate ARIA roles or <p> elements with proper styling instead of heading tags.
| useEffect(() => { | ||
| sessionCheckMutation.mutate({ credentials: 'include' }); | ||
| }, []); |
There was a problem hiding this comment.
The useEffect dependency array is empty but uses sessionCheckMutation. This should include sessionCheckMutation in the dependencies or use sessionCheckMutation.mutate directly if it's stable. This could cause issues with React's exhaustive-deps rule.
| useEffect(() => { | ||
| sessionCheckMutation.mutate({ credentials: 'include' }); | ||
| }, []); |
There was a problem hiding this comment.
The useEffect dependency array is empty but uses sessionCheckMutation. This should include sessionCheckMutation in the dependencies or use sessionCheckMutation.mutate directly if it's stable. This could cause issues with React's exhaustive-deps rule.
| useEffect(() => { | ||
| sessionCheckMutation.mutate({ credentials: 'include' }); | ||
| }, []); |
There was a problem hiding this comment.
The useEffect dependency array is empty but uses sessionCheckMutation. This should include sessionCheckMutation in the dependencies or use sessionCheckMutation.mutate directly if it's stable. This could cause issues with React's exhaustive-deps rule.
| {receivables.map((t) => ( | ||
| <li key={t.counterparty_id}> |
There was a problem hiding this comment.
The key uses t.counterparty_id which could cause issues if the same counterparty appears multiple times. Consider using a more unique identifier if available, or combining with additional fields to ensure uniqueness.
| <link rel="icon" type="image/svg+xml" href="/vite.svg" /> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0" /> | ||
| <title>pay-crew2</title> | ||
| <meta property="description" content="台帳管理サービス「Pay Crew2」です。" /> |
There was a problem hiding this comment.
The description meta tag should use name attribute instead of property attribute. The property attribute is for Open Graph protocol, while standard HTML meta tags use name.
| <meta property="description" content="台帳管理サービス「Pay Crew2」です。" /> | |
| <meta name="description" content="台帳管理サービス「Pay Crew2」です。" /> |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8befd5bae7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const sessionCheckMutation = $api.useMutation('get', '/api/session', { | ||
| onError: () => { | ||
| navigate('/login', { replace: true }); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Because the Header runs a session check on mount and unconditionally redirects on any error, unauthenticated users visiting public routes like /terms or /privacy (which are outside SessionCheck in App routing) will still get bounced to /login. That makes those public pages inaccessible whenever /api/session returns 401. Consider limiting this redirect to protected routes only, or handling 401 by showing the login link without navigation.
Useful? React with 👍 / 👎.
No description provided.