-
Notifications
You must be signed in to change notification settings - Fork 0
/api/me のアクセスに伴う手動リダイレクト #173
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Preview (prod) → https://173-prod.rucq-ui-preview.trapti.tech/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements manual redirect handling for the /api/me endpoint to properly handle HTTP 307 (Temporary Redirect) responses. The implementation disables automatic redirects globally and adds custom logic to detect and manually perform redirects when a 307 status is returned.
- Disabled automatic redirects globally in the API client configuration
- Added manual redirect detection and handling for 307 status responses in user initialization
- Modified query caching behavior to always fetch fresh user data
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/api/apiClient.ts |
Configures the API client to disable automatic redirect handling by setting redirect: 'manual' |
src/store.ts |
Implements manual 307 redirect detection and handling in initUser, changes from ensureQueryData to fetchQuery, and modifies staleTime configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| const initUser = async () => { | ||
| const data = await queryClient.ensureQueryData({ | ||
| const data = await queryClient.fetchQuery({ |
Copilot
AI
Jan 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change from ensureQueryData to fetchQuery alters the caching behavior. ensureQueryData returns cached data if available and only fetches if the cache is empty, while fetchQuery always executes the query function regardless of cached data. This means initUser will now make an API call every time it's called, even if valid cached data exists. If this is intentional due to the redirect handling requirements, it should be documented. Otherwise, consider keeping ensureQueryData for better performance.
| const data = await queryClient.fetchQuery({ | |
| const data = await queryClient.ensureQueryData({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
仮に ensureQueryData を維持した場合、/api/me の gcTime は Infinity なので queryFn は初回以降発動しないことが想定されます。このままではログアウトした状態を検知することができず、別の場所、たとえば /api/camps へのリクエストに伴うリダイレクトでエラーが発生して情報が正しく表示されなくなり、問題を解決できないように思われます
If this is intentional due to the redirect handling requirements, it should be documented.
については、直後の手動リダイレクト処理がほぼその説明になっていると考えて省いています
src/api/apiClient.ts
Outdated
| import type { paths } from '@/api/schema' | ||
|
|
||
| export const apiClient = createClient<paths>() | ||
| export const apiClient = createClient<paths>({ redirect: 'manual' }) |
Copilot
AI
Jan 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting redirect: 'manual' globally affects all API calls throughout the application, but redirect handling is only implemented for the /api/me endpoint. This means other API endpoints that might return redirect responses (like 301, 302, 307, 308) will not follow those redirects automatically and will fail silently. Consider either:
- Implementing redirect handling for all affected endpoints
- Using a more targeted approach where redirect handling is configured per-request rather than globally
| export const apiClient = createClient<paths>({ redirect: 'manual' }) | |
| export const apiClient = createClient<paths>() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
フロントエンドとしてはそもそもリダイレクトの発生自体をほぼ想定しておらず、この変更によって壊れる既存機能は思い当たりません。リダイレクトという特異な事態は明示的に制御されるべきであり、その意図を設定に反映させるため apiClient 自体に変更を加えることにしました
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
長期的に見て何かあるかもしれないので、api/meのfetchqueryだけmanualにするのはどうですか?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
そんなにこだわりないので変えちゃうか
外部リソースへのリダイレクトは基本 CORS でダメだけど、内部同士でリダイレクトみたいなことをする可能性は将来的に無きにしも非ず?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
かなーと思って言ってみたけど僕もどっちでもいいよ。apiclientにまとめたほうがきれいではあるしね
src/store.ts
Outdated
| if (error || !data) { | ||
| // Temporary Redirect の場合、手動でリダイレクト処理を行う | ||
| if (response?.status === 307) { | ||
| const location = response.headers.get('Location') | ||
| if (location) { | ||
| window.location.href = location | ||
| return new Promise<never>(() => {}) | ||
| // ユーザーにエラー表示をさせないよう、解決しない Promise を返す | ||
| } | ||
| } |
Copilot
AI
Jan 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The redirect check is performed after verifying error || !data, which means the 307 status check only happens when there's an error. However, if the redirect is handled correctly by the fetch API with redirect: 'manual', the response object should be available regardless of error state. The logic should check response?.status === 307 before the error check to properly handle redirects.
| if (error || !data) { | |
| // Temporary Redirect の場合、手動でリダイレクト処理を行う | |
| if (response?.status === 307) { | |
| const location = response.headers.get('Location') | |
| if (location) { | |
| window.location.href = location | |
| return new Promise<never>(() => {}) | |
| // ユーザーにエラー表示をさせないよう、解決しない Promise を返す | |
| } | |
| } | |
| // Temporary Redirect の場合、手動でリダイレクト処理を行う | |
| if (response?.status === 307) { | |
| const location = response.headers.get('Location') | |
| if (location) { | |
| window.location.href = location | |
| // ユーザーにエラー表示をさせないよう、解決しない Promise を返す | |
| return new Promise<never>(() => {}) | |
| } | |
| } | |
| if (error || !data) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
確かにそうかもしれません
src/store.ts
Outdated
| return new Promise<never>(() => {}) | ||
| // ユーザーにエラー表示をさせないよう、解決しない Promise を返す |
Copilot
AI
Jan 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning a never-resolving Promise can lead to memory leaks and prevents proper cleanup. The Promise remains in memory indefinitely with its closure and any captured variables. After triggering the redirect with window.location.href, the page will navigate away, but until then this Promise keeps the function context alive unnecessarily. Consider simply returning after the redirect or throwing a specific error that can be caught and handled appropriately.
| return new Promise<never>(() => {}) | |
| // ユーザーにエラー表示をさせないよう、解決しない Promise を返す | |
| // リダイレクト後は処理を中断し、エラーとして扱うことで Promise を解決させる | |
| throw new Error('Redirecting to alternate location') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
リダイレクトをエラーとして扱うのは意味論的に自然ではないように思われます
結局はページの移動によって基本的なものはクリーンアップされるはずなので、それまで画面に不必要な変更を生じないための遅延として「解決しないプロミス」を置いています
| throw new Error(`ユーザー情報を取得できません: ${error}`) | ||
| } | ||
| return data | ||
| }, |
Copilot
AI
Jan 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing staleTime from Infinity to 0 means the user data will be considered stale immediately after being fetched. This can cause unnecessary refetches whenever the query is accessed again. If the reason for this change is to force a fresh fetch on every initUser call, consider documenting why this is necessary. If the user data doesn't change frequently, this could lead to unnecessary API requests.
| }, | |
| }, | |
| // ユーザー情報は initUser 呼び出しごとに最新状態を取得したいため、 | |
| // キャッシュを信頼せず常に再フェッチするよう staleTime を 0 に設定する。 | |
| // (頻繁な変更を許容した設計であることに注意) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ユーザー情報はもちろんそう頻繁に変わるものではありません。ここで /api/me に頻繁に(具体的には、ページにアクセスするたびに)リクエストを送信するのは、専らログアウト状態を早くに検知することでバックエンドからの正常なレスポンスを得られないまま画面が表示されてしまうことを未然に防ぐためです
mumumu6
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
リダイレクト処理はよさそうです
ありがとうございます!
src/api/apiClient.ts
Outdated
| import type { paths } from '@/api/schema' | ||
|
|
||
| export const apiClient = createClient<paths>() | ||
| export const apiClient = createClient<paths>({ redirect: 'manual' }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
長期的に見て何かあるかもしれないので、api/meのfetchqueryだけmanualにするのはどうですか?
mumumu6
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okです!
No description provided.