-
Notifications
You must be signed in to change notification settings - Fork 0
Fix#377 UnauthorizedException 외의 예외에 대해 동일한 리다이렉트 처리 추가 #378
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
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughServer-side error handling in getServerSideProps for the notice creation page was refactored to branch redirects by error type: Unauthorized redirects to /signin, Forbidden redirects to /mypage/profile/register, and all other errors redirect to /signin. No exported/public signatures changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Next.js as Next.js (SSR)
participant GSSP as getServerSideProps
participant Auth as Auth Service
User->>Next.js: Request /notice/new
Next.js->>GSSP: Invoke
GSSP->>Auth: Validate session / permissions
Auth-->>GSSP: Error or OK
alt Unauthorized error
GSSP-->>Next.js: redirect -> /signin
Next.js-->>User: 302 /signin
else Forbidden error
GSSP-->>Next.js: redirect -> /mypage/profile/register
Next.js-->>User: 302 /mypage/profile/register
else Other error
GSSP-->>Next.js: redirect -> /signin
Next.js-->>User: 302 /signin
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
📝 추가 및 변경된 파일총 1개 파일 변경 |
📚 Storybook이 Chromatic에 배포되었습니다!
|
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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
mosu-app/src/pages/notice/new.tsx (2)
32-36: Preserve and encode returnUrl (avoid hardcoded path, keep queries).Use context.resolvedUrl to round-trip the original URL.
- return { - redirect: { - destination: "/auth/member/signin?returnUrl=/notice/new", - permanent: false, - }, - }; + return { + redirect: { + destination: `/auth/member/signin?returnUrl=${encodeURIComponent( + context.resolvedUrl ?? "/notice/new", + )}`, + permanent: false, + }, + };
37-38: Harden 403 detection beyond instanceof.Ensure Forbidden redirects even if a plain/axios error is thrown.
- } else if (error instanceof ForbiddenException) { + } else if ( + error instanceof ForbiddenException || + (error as any)?.status === 403 || + (error as any)?.response?.status === 403 + ) {
🧹 Nitpick comments (1)
mosu-app/src/pages/notice/new.tsx (1)
44-51: Catch-all redirect: add minimal logging and preserve returnUrl.Logs unexpected failures server-side and keeps the original URL.
- } else { - return { - redirect: { - destination: "/auth/member/signin?returnUrl=/notice/new", - permanent: false, - }, - }; + } else { + // Server-side log for observability (no user impact) + console.error("[notice/new] getServerSideProps unexpected error:", error); + return { + redirect: { + destination: `/auth/member/signin?returnUrl=${encodeURIComponent( + context.resolvedUrl ?? "/notice/new", + )}`, + permanent: false, + }, + };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
mosu-app/src/pages/notice/new.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
mosu-app/src/pages/notice/new.tsx (1)
mosu-app/src/shared/errors/HTTPException.ts (1)
UnauthorizedException(74-80)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build & Publish Storybook
🔇 Additional comments (1)
mosu-app/src/pages/notice/new.tsx (1)
30-51: LGTM on redirect coverage—this will prevent the 500 for unauthenticated access.The added else ensures safe fallback instead of crashing.
| if (error instanceof UnauthorizedException) { | ||
| return { |
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.
🛠️ Refactor suggestion
Harden 401 detection beyond instanceof.
Cross-bundle instanceof checks can fail. Also check status codes.
- if (error instanceof UnauthorizedException) {
+ if (
+ error instanceof UnauthorizedException ||
+ (error as any)?.status === 401 ||
+ (error as any)?.response?.status === 401
+ ) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (error instanceof UnauthorizedException) { | |
| return { | |
| if ( | |
| error instanceof UnauthorizedException || | |
| (error as any)?.status === 401 || | |
| (error as any)?.response?.status === 401 | |
| ) { | |
| return { | |
| // … |
🤖 Prompt for AI Agents
In mosu-app/src/pages/notice/new.tsx around lines 30-31, the code only checks
error instanceof UnauthorizedException which can fail across bundles; change the
conditional to also detect a 401 by checking common status properties (e.g.
error.status === 401 || error.response?.status === 401 || error.statusCode ===
401) and keep the existing UnauthorizedException branch behavior when any of
these checks are true; ensure the combined condition is used wherever
UnauthorizedException was checked so 401 responses are reliably handled
regardless of instanceof mismatches.
✅ Linked Issue
🔍 What I did
Summary by CodeRabbit