-
Notifications
You must be signed in to change notification settings - Fork 0
[WIP] ReSummaryModal 디자인 수정 및 api 연결 #300
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
base: main
Are you sure you want to change the base?
Conversation
Walkthrough이 PR은 요약 재생성 모달의 디자인을 재구성하고 API 연결을 추가합니다. 새로운 Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. 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.
Actionable comments posted: 2
🧹 Nitpick comments (7)
src/components/wrappers/ReSummaryModal/PrevSummary.tsx (1)
1-10: LGTM - 다만 NewSummary와의 코드 중복을 고려해보세요.컴포넌트 구현은 깔끔합니다.
NewSummary.tsx와 거의 동일한 구조를 가지고 있어, 라벨만 다른 공통SummaryBox컴포넌트로 추출하는 것을 고려해볼 수 있습니다.🔎 공통 컴포넌트 추출 예시
// SummaryBox.tsx interface SummaryBoxProps { label: string; content: string; } export default function SummaryBox({ label, content }: SummaryBoxProps) { return ( <div className="flex flex-1 flex-col gap-2"> <span className="font-label-sm">{label}</span> <div className="rounded-lg bg-white p-2"> <div className="custom-scrollbar font-body-md h-45 overflow-auto pr-1">{content}</div> </div> </div> ); }src/apis/summary.ts (1)
24-27: GET 요청에 Content-Type 헤더는 일반적으로 불필요합니다.GET 요청은 요청 본문이 없으므로
Content-Type헤더가 의미가 없습니다. 제거해도 무방합니다.🔎 수정 제안
const body = await safeFetch<SummaryRes>(url.toString(), { method: 'GET', headers: { - 'Content-Type': 'application/json', Authorization: `Bearer ${API_TOKEN}`, }, timeout: 15_000, jsonContentTypeCheck: true, });src/components/wrappers/ReSummaryModal/ReSummaryModal.tsx (2)
17-19: 주석 처리된 인터페이스는 제거하거나 구현하세요.사용되지 않는 주석 코드는 코드베이스를 혼란스럽게 만들 수 있습니다. API 연결 시 필요하다면 해당 시점에 추가하는 것이 좋습니다.
🔎 제거 제안
-// interface ReSummaryProps { -// id: number; -// }
24-26: API 연결 TODO가 남아있습니다.PR 목표에 API 연결이 포함되어 있는데, 현재 하드코딩된 플레이스홀더 데이터만 사용 중입니다.
fetchNewSummaryAPI가 이미 구현되어 있으니, 향후 이를 연결해야 합니다.API 연결 구현을 도와드릴까요, 아니면 별도 이슈로 추적할까요?
src/components/wrappers/ReSummaryModal/PostReSummaryButton.tsx (3)
2-2: 사용하지 않는 주석 코드를 제거하세요.주석 처리된 import 문은 더 이상 필요하지 않으므로 제거하는 것이 좋습니다.
🔎 제거 제안
import Button from '@/components/basics/Button/Button'; -// import Toast from '@/components/basics/Toast/Toast'; import { useModalStore } from '@/stores/modalStore';
14-14: 사용하지 않는 주석 코드를 제거하세요.🔎 제거 제안
const { close } = useModalStore(); - // const [toastVisible, setToastVisible] = useState(false);
16-33: onClick 콜백의 비동기 처리 및 에러 핸들링을 고려하세요.현재
onClick()이 호출된 후 즉시 모달을 닫고 성공 토스트를 표시합니다. 만약onClick이 비동기 저장 작업이라면, 작업 완료 전에 성공 메시지가 표시될 수 있습니다. API 연결 시 이 부분을 개선해야 할 수 있습니다.🔎 비동기 처리 예시
- const handleClick = () => { - onClick(); + const handleClick = async () => { + try { + await onClick(); + } catch (error) { + showToast({ + id: 'save-error', + message: '저장 중 오류가 발생했습니다.', + variant: 'error', + }); + return; + } if (type === 'prev') { close();Props 타입도 업데이트 필요:
interface Props { disabled: boolean; type: 'prev' | 'new'; - onClick: () => void; + onClick: () => void | Promise<void>; }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/apis/summary.tssrc/components/wrappers/ReSummaryModal/NewSummary.tsxsrc/components/wrappers/ReSummaryModal/PostReSummaryButton.tsxsrc/components/wrappers/ReSummaryModal/PrevSummary.tsxsrc/components/wrappers/ReSummaryModal/ReSummaryModal.tsxsrc/stories/ReSummaryModal.stories.tsxsrc/types/api/summaryApi.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Bangdayeon
Repo: Team-SoFa/linkiving PR: 97
File: src/components/basics/LinkCard/LinkCard.tsx:12-19
Timestamp: 2025-11-23T12:03:33.890Z
Learning: In src/components/basics/LinkCard/LinkCard.tsx, the summary prop should remain required (string type) because the backend always provides it as a string value. The isHaveSummary flag controls whether to display the summary text or show the AddSummaryButton, not whether the data exists.
📚 Learning: 2025-11-23T12:03:33.890Z
Learnt from: Bangdayeon
Repo: Team-SoFa/linkiving PR: 97
File: src/components/basics/LinkCard/LinkCard.tsx:12-19
Timestamp: 2025-11-23T12:03:33.890Z
Learning: In src/components/basics/LinkCard/LinkCard.tsx, the summary prop should remain required (string type) because the backend always provides it as a string value. The isHaveSummary flag controls whether to display the summary text or show the AddSummaryButton, not whether the data exists.
Applied to files:
src/components/wrappers/ReSummaryModal/NewSummary.tsxsrc/components/wrappers/ReSummaryModal/PrevSummary.tsxsrc/types/api/summaryApi.tssrc/components/wrappers/ReSummaryModal/ReSummaryModal.tsxsrc/components/wrappers/ReSummaryModal/PostReSummaryButton.tsxsrc/apis/summary.tssrc/stories/ReSummaryModal.stories.tsx
🧬 Code graph analysis (4)
src/components/wrappers/ReSummaryModal/NewSummary.tsx (3)
src/components/wrappers/ReSummaryModal/hooks/useReSummary.ts (3)
() => {(11-28)reSummary(10-30)useReSummary(5-32)src/components/basics/LinkCard/components/AddSummaryButton.tsx (2)
onSummaryGenerate(3-18)console(4-6)src/components/wrappers/LinkCardDetailPanel/LinkCardDetailPanel.tsx (1)
console(201-201)
src/components/wrappers/ReSummaryModal/ReSummaryModal.tsx (5)
src/components/wrappers/ReSummaryModal/hooks/useReSummary.ts (1)
useReSummary(5-32)src/components/basics/Badge/Badge.tsx (1)
Badge(16-32)src/components/wrappers/ReSummaryModal/PrevSummary.tsx (1)
PrevSummary(1-10)src/components/wrappers/ReSummaryModal/NewSummary.tsx (1)
NewSummary(1-10)src/components/wrappers/ReSummaryModal/PostReSummaryButton.tsx (1)
PostReSummaryButton(12-46)
src/components/wrappers/ReSummaryModal/PostReSummaryButton.tsx (2)
src/stores/modalStore.ts (1)
useModalStore(18-23)src/stores/toastStore.ts (1)
showToast(42-43)
src/apis/summary.ts (2)
src/types/api/summaryApi.ts (2)
SummaryData(1-5)SummaryRes(7-13)src/hooks/util/server/safeFetch.ts (1)
safeFetch(37-125)
🔇 Additional comments (3)
src/stories/ReSummaryModal.stories.tsx (1)
7-7: LGTM!메타 타이틀이 컴포넌트 이름과 일치하도록 적절히 업데이트되었습니다.
src/components/wrappers/ReSummaryModal/NewSummary.tsx (1)
1-10: LGTM!PrevSummary와 동일한 구조로 일관성 있게 구현되었습니다.
src/types/api/summaryApi.ts (1)
1-13: LGTM!API 응답 타입이 명확하게 정의되었습니다.
fetchNewSummary에서body.success체크 후body.data에 접근하므로 현재 구조가 적합합니다.
| const API_URL = process.env.NEXT_PUBLIC_BASE_API_URL; | ||
| const API_TOKEN = process.env.NEXT_PUBLIC_API_TOKEN; | ||
|
|
||
| if (!API_URL) throw new Error('Missing environment variable: NEXT_PUBLIC_BASE_API_URL'); | ||
| if (!API_TOKEN) throw new Error('Missing environment variable: NEXT_PUBLIC_API_TOKEN'); |
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.
모듈 로드 시점의 환경 변수 검증은 런타임 오류를 유발할 수 있습니다.
모듈이 import되는 시점에 환경 변수가 없으면 즉시 에러가 발생합니다. 테스트 환경이나 특정 빌드 시나리오에서 문제가 될 수 있습니다. 또한 NEXT_PUBLIC_API_TOKEN은 클라이언트에 노출되므로, 민감한 토큰인 경우 서버 사이드에서만 사용하는 것이 좋습니다.
🔎 개선 제안
-const API_URL = process.env.NEXT_PUBLIC_BASE_API_URL;
-const API_TOKEN = process.env.NEXT_PUBLIC_API_TOKEN;
-
-if (!API_URL) throw new Error('Missing environment variable: NEXT_PUBLIC_BASE_API_URL');
-if (!API_TOKEN) throw new Error('Missing environment variable: NEXT_PUBLIC_API_TOKEN');
+const getApiConfig = () => {
+ const API_URL = process.env.NEXT_PUBLIC_BASE_API_URL;
+ const API_TOKEN = process.env.NEXT_PUBLIC_API_TOKEN;
+
+ if (!API_URL) throw new Error('Missing environment variable: NEXT_PUBLIC_BASE_API_URL');
+ if (!API_TOKEN) throw new Error('Missing environment variable: NEXT_PUBLIC_API_TOKEN');
+
+ return { API_URL, API_TOKEN };
+};Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/apis/summary.ts around lines 4-8 the code performs environment-variable
validation at module load time and throws immediately; change this to perform
checks at runtime (e.g., inside an initialization function or the function that
actually calls the API) instead of during import, and avoid exposing sensitive
tokens to the client by removing NEXT_PUBLIC_API_TOKEN use on client-side
code—read the token from a server-only env var (no NEXT_PUBLIC prefix) and
validate it when the server-side handler starts; if you need lazy validation,
create a getter/init function that reads process.env, throws a descriptive error
if missing, and use that from server-only code so imports never cause immediate
runtime exceptions.
| <IconButton | ||
| icon="IC_Regenerate" | ||
| size="sm" | ||
| variant="tertiary_subtle" | ||
| ariaLabel="요약 재생성 재시도" | ||
| /> |
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.
재시도 버튼에 onClick 핸들러가 없습니다.
IconButton에 onClick 핸들러가 누락되어 있어 사용자가 재시도 버튼을 클릭해도 아무 동작이 발생하지 않습니다. 재시도 기능을 구현하거나, API 연결 전까지 버튼을 비활성화해야 합니다.
🔎 수정 제안
<IconButton
icon="IC_Regenerate"
size="sm"
variant="tertiary_subtle"
ariaLabel="요약 재생성 재시도"
+ onClick={() => {
+ // TODO: 재시도 로직 구현
+ }}
/>📝 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.
| <IconButton | |
| icon="IC_Regenerate" | |
| size="sm" | |
| variant="tertiary_subtle" | |
| ariaLabel="요약 재생성 재시도" | |
| /> | |
| <IconButton | |
| icon="IC_Regenerate" | |
| size="sm" | |
| variant="tertiary_subtle" | |
| ariaLabel="요약 재생성 재시도" | |
| onClick={() => { | |
| // TODO: 재시도 로직 구현 | |
| }} | |
| /> |
관련 이슈
PR 설명