Fix: #78 리다이렉트 로직 수정#79
Open
yiyoonseo wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughThis PR introduces a centralized API client module that standardizes response parsing and unauthorized (401) error handling across the application. It then refactors five existing API modules to remove duplicate logic and import shared utilities, eliminating code duplication in ChangesAPI Response Handling Centralization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
jobdri/src/lib/api/questions.ts (1)
131-135: 💤 Low valueNon-null assertion is inconsistent with other usages in this file.
Lines 50 and 99 use defensive handling (
result ?? [],result?.questions ?? []), but this line uses a non-null assertion. IfparseApiResponsecan legitimately returnnull, this could cause a runtime error. If it guarantees non-null for valid responses, the assertion is unnecessary.Consider either removing the assertion if the base function guarantees non-null, or adding explicit handling:
♻️ Suggested fix
const result = await parseApiResponse<SaveApplyResult>( response, "답변 제출에 실패했습니다.", ); - return result!; + if (!result) { + throw new Error("답변 제출에 실패했습니다."); + } + return result;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jobdri/src/lib/api/questions.ts` around lines 131 - 135, The code uses a non-null assertion on result from parseApiResponse<SaveApplyResult> which is inconsistent with other defensive patterns in this file; update the save/apply handling in questions.ts by removing the `!` and explicitly handle a null/undefined result from parseApiResponse (for example, return a safe default, throw a descriptive error, or map to `{ success: false }`), referencing the result variable and the parseApiResponse<SaveApplyResult> call so the fix is applied where the response is processed.jobdri/src/lib/api/credit.ts (1)
22-34: ⚡ Quick winConsider using
parseApiResponsefor consistency with other modules.The other refactored modules (
jobPostings.ts,mockApplies.ts,questions.ts) all delegate toparseApiResponsefrom the shared client, which handles 401 checks, JSON parsing, and error handling uniformly. This file introduces a separatecheckResponse+ manual JSON parsing pattern that diverges from the established convention.Unifying to
parseApiResponsewould reduce maintenance burden and ensure consistent error handling behavior across all API modules.♻️ Suggested refactor to align with other modules
-import { getAuthHeaders, handleUnauthorized } from "`@/lib/api/client`"; +import { getAuthHeaders, parseApiResponse } from "`@/lib/api/client`"; const BASE_URL = process.env.NEXT_PUBLIC_API_BASE_URL; // ... types remain the same ... -function checkResponse(response: Response, fallbackMessage: string): void { - if (response.status === 401) handleUnauthorized(); - if (!response.ok) throw new Error(fallbackMessage); -} - export async function fetchCreditBalance(): Promise<number> { const response = await fetch(`${BASE_URL}/api/payments/credits/me`, { headers: getAuthHeaders(), }); - checkResponse(response, "크레딧 잔액 조회에 실패했습니다."); - const { result }: ApiResponse<{ creditBalance: number }> = - await response.json(); - return result.creditBalance; + const result = await parseApiResponse<{ creditBalance: number }>( + response, + "크레딧 잔액 조회에 실패했습니다.", + ); + return result.creditBalance; }Apply the same pattern to
fetchCreditTransactions,fetchCreditPlans,preparePurchase, andconfirmPurchase.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jobdri/src/lib/api/credit.ts` around lines 22 - 34, The current checkResponse + manual JSON parsing in fetchCreditBalance (and sibling functions fetchCreditTransactions, fetchCreditPlans, preparePurchase, confirmPurchase) diverges from other modules; replace those patterns to call parseApiResponse from the shared client instead: remove checkResponse usage and direct response.json() calls in the listed functions and delegate to parseApiResponse<ResponseType>(response, "fallback message") so 401 handling, parsing, and error messages are consistent; ensure you import parseApiResponse and keep the same return types (e.g., ApiResponse<{ creditBalance: number }>) and use the returned .result field as other modules do.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@jobdri/src/lib/api/client.ts`:
- Around line 81-82: The conditional can throw if data is null (JSON.parse can
return null); update the guard to use optional chaining — change the check from
if (!response.ok || !data.isSuccess) to if (!response.ok || !data?.isSuccess)
and keep using data?.error || data?.message || fallbackMessage when throwing so
the code safely handles data === null; locate this in client.ts around the
response/data handling (same area as parseApiResponse logic) and apply the
optional chaining fix.
- Around line 48-49: The current validation wrongly treats legitimate falsy
results as failures and can throw when data is null; update the check in the
response handling so it uses safe null checks and explicit undefined/null
checks: replace the condition `if (!response.ok || !data.isSuccess ||
!data.result)` with something like `if (!response.ok || data == null ||
data.isSuccess === false || data.result == null)` (use optional chaining where
appropriate, e.g., `data == null` and `data.isSuccess === false`, and check
`data.result == null` instead of `!data.result`) so that `response`, `data`,
`isSuccess`, and `result` are validated correctly in the function that parses
`response.json()` in client.ts.
- Around line 21-30: handleUnauthorized currently builds redirectPath using
window.location.pathname which drops ?query and `#hash`; change how redirectPath
is composed in the handleUnauthorized function to include window.location.search
and window.location.hash (e.g. const redirectPath =
encodeURIComponent(window.location.pathname + window.location.search +
window.location.hash)) before calling
window.location.replace(`${ROUTES.LOGIN}?redirect=${redirectPath}`) so the full
original URL (path, query, and hash) is preserved when redirecting to
ROUTES.LOGIN.
---
Nitpick comments:
In `@jobdri/src/lib/api/credit.ts`:
- Around line 22-34: The current checkResponse + manual JSON parsing in
fetchCreditBalance (and sibling functions fetchCreditTransactions,
fetchCreditPlans, preparePurchase, confirmPurchase) diverges from other modules;
replace those patterns to call parseApiResponse from the shared client instead:
remove checkResponse usage and direct response.json() calls in the listed
functions and delegate to parseApiResponse<ResponseType>(response, "fallback
message") so 401 handling, parsing, and error messages are consistent; ensure
you import parseApiResponse and keep the same return types (e.g., ApiResponse<{
creditBalance: number }>) and use the returned .result field as other modules
do.
In `@jobdri/src/lib/api/questions.ts`:
- Around line 131-135: The code uses a non-null assertion on result from
parseApiResponse<SaveApplyResult> which is inconsistent with other defensive
patterns in this file; update the save/apply handling in questions.ts by
removing the `!` and explicitly handle a null/undefined result from
parseApiResponse (for example, return a safe default, throw a descriptive error,
or map to `{ success: false }`), referencing the result variable and the
parseApiResponse<SaveApplyResult> call so the fix is applied where the response
is processed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 34dad631-610e-4012-b5f5-e485620fba82
📒 Files selected for processing (6)
jobdri/src/lib/api/client.tsjobdri/src/lib/api/credit.tsjobdri/src/lib/api/jobPostings.tsjobdri/src/lib/api/mockApplies.tsjobdri/src/lib/api/questions.tsjobdri/src/lib/api/result.ts
Comment on lines
+21
to
+30
| export function handleUnauthorized(): never { | ||
| clearAuthTokens(); | ||
|
|
||
| if (typeof window !== "undefined") { | ||
| const redirectPath = encodeURIComponent(window.location.pathname); | ||
| window.location.replace(`${ROUTES.LOGIN}?redirect=${redirectPath}`); | ||
| } | ||
|
|
||
| throw new UnauthorizedError(); | ||
| } |
There was a problem hiding this comment.
Preserve query string and hash in the redirect path.
window.location.pathname drops the current ?query and #hash. Since the goal is to return the user to the exact page after login, pages that rely on query params will lose state on redirect-back.
🔧 Proposed fix
if (typeof window !== "undefined") {
- const redirectPath = encodeURIComponent(window.location.pathname);
+ const redirectPath = encodeURIComponent(
+ window.location.pathname + window.location.search + window.location.hash,
+ );
window.location.replace(`${ROUTES.LOGIN}?redirect=${redirectPath}`);
}📝 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.
Suggested change
| export function handleUnauthorized(): never { | |
| clearAuthTokens(); | |
| if (typeof window !== "undefined") { | |
| const redirectPath = encodeURIComponent(window.location.pathname); | |
| window.location.replace(`${ROUTES.LOGIN}?redirect=${redirectPath}`); | |
| } | |
| throw new UnauthorizedError(); | |
| } | |
| export function handleUnauthorized(): never { | |
| clearAuthTokens(); | |
| if (typeof window !== "undefined") { | |
| const redirectPath = encodeURIComponent( | |
| window.location.pathname + window.location.search + window.location.hash, | |
| ); | |
| window.location.replace(`${ROUTES.LOGIN}?redirect=${redirectPath}`); | |
| } | |
| throw new UnauthorizedError(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@jobdri/src/lib/api/client.ts` around lines 21 - 30, handleUnauthorized
currently builds redirectPath using window.location.pathname which drops ?query
and `#hash`; change how redirectPath is composed in the handleUnauthorized
function to include window.location.search and window.location.hash (e.g. const
redirectPath = encodeURIComponent(window.location.pathname +
window.location.search + window.location.hash)) before calling
window.location.replace(`${ROUTES.LOGIN}?redirect=${redirectPath}`) so the full
original URL (path, query, and hash) is preserved when redirecting to
ROUTES.LOGIN.
Comment on lines
+48
to
+49
| if (!response.ok || !data.isSuccess || !data.result) { | ||
| throw new Error(data?.error || data?.message || fallbackMessage); |
There was a problem hiding this comment.
!data.result rejects valid falsy results, and data may be null here.
Two concerns on the validation check:
!data.resulttreats legitimate falsy values (0,false,"") as failures and throws even whenisSuccessistrue. Use an explicit null/undefined check instead.response.json()can resolve tonull(literal JSONnullbody), sodatamay benull;!data.isSuccesswould then throw aTypeError. The optional chaining on line 49 (data?.error) suggests this guard was intended but is missing on line 48.
🔧 Proposed fix
- if (!response.ok || !data.isSuccess || !data.result) {
+ if (!response.ok || !data?.isSuccess || data.result == null) {
throw new Error(data?.error || data?.message || fallbackMessage);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@jobdri/src/lib/api/client.ts` around lines 48 - 49, The current validation
wrongly treats legitimate falsy results as failures and can throw when data is
null; update the check in the response handling so it uses safe null checks and
explicit undefined/null checks: replace the condition `if (!response.ok ||
!data.isSuccess || !data.result)` with something like `if (!response.ok || data
== null || data.isSuccess === false || data.result == null)` (use optional
chaining where appropriate, e.g., `data == null` and `data.isSuccess === false`,
and check `data.result == null` instead of `!data.result`) so that `response`,
`data`, `isSuccess`, and `result` are validated correctly in the function that
parses `response.json()` in client.ts.
Comment on lines
+81
to
+82
| if (!response.ok || !data.isSuccess) { | ||
| throw new Error(data?.error || data?.message || fallbackMessage); |
There was a problem hiding this comment.
Same potential null dereference as parseApiResponse.
JSON.parse("null") returns null, so data can be null here while responseText is non-empty. !data.isSuccess on line 81 would throw a TypeError, despite line 82 using data?.. Apply the same optional-chaining guard.
🔧 Proposed fix
- if (!response.ok || !data.isSuccess) {
+ if (!response.ok || !data?.isSuccess) {
throw new Error(data?.error || data?.message || fallbackMessage);
}📝 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.
Suggested change
| if (!response.ok || !data.isSuccess) { | |
| throw new Error(data?.error || data?.message || fallbackMessage); | |
| if (!response.ok || !data?.isSuccess) { | |
| throw new Error(data?.error || data?.message || fallbackMessage); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@jobdri/src/lib/api/client.ts` around lines 81 - 82, The conditional can throw
if data is null (JSON.parse can return null); update the guard to use optional
chaining — change the check from if (!response.ok || !data.isSuccess) to if
(!response.ok || !data?.isSuccess) and keep using data?.error || data?.message
|| fallbackMessage when throwing so the code safely handles data === null;
locate this in client.ts around the response/data handling (same area as
parseApiResponse logic) and apply the optional chaining fix.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔗 관련 이슈
#78
📝 개요
⌨️ 작업 상세 내용
기존에는 401에러 인터셉터가 없어서 추가하였습니다.
src/lib/api/client.ts생성 — 공통 API 클라이언트 (401 인터셉터,parseApiResponse,parseApiResponseAllowNull)clearAuthTokens()호출 후/login?redirect=<현재경로>로 리다이렉트mockApplies,jobPostings,result,questions,credit)에서 중복 코드 제거 후 공통 클라이언트로 교체💡 코드 설명 및 참고사항
📸 스크린샷 (UI 변경 시)
🔍 리뷰 요구사항 (Reviewers)
Summary by CodeRabbit
Bug Fixes
Refactor