Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Walkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/web/src/app/university/search/PageContent.tsx (1)
37-37:defaultHomeUniversitySlug상수를 컴포넌트 바깥으로 이동하는 것을 고려해보세요.
현재 위치 문제
defaultHomeUniversitySlug는 props나 state에 의존하지 않으며, 모든 렌더링에서 동일한 값("inha")을 반환합니다.- 컴포넌트 내부에 선언하면 매 렌더마다 불필요하게 재평가됩니다.
추가로 확인할 점
SearchBar.tsx에도 동일한const defaultHomeUniversitySlug = HOME_UNIVERSITY_TO_SLUG_MAP[HomeUniversity.INHA];가 선언되어 있습니다.- 같은 폴더 내 두 파일에서 동일한 상수가 중복되고 있으니, 모듈 스코프로 올리거나 공유 상수로 추출하는 것을 권장합니다.
♻️ 개선 예시
import { CountryCode, HomeUniversity, LanguageTestType } from "@/types/university"; import CustomDropdown from "../CustomDropdown"; +const defaultHomeUniversitySlug = HOME_UNIVERSITY_TO_SLUG_MAP[HomeUniversity.INHA]; const SchoolSearchForm = () => { const router = useRouter(); - const defaultHomeUniversitySlug = HOME_UNIVERSITY_TO_SLUG_MAP[HomeUniversity.INHA];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/university/search/PageContent.tsx` at line 37, Move the constant defaultHomeUniversitySlug = HOME_UNIVERSITY_TO_SLUG_MAP[HomeUniversity.INHA] out of the component scope and into module scope (or a shared constants module) so it is not re-evaluated on every render; update the component to import/use that module-level/shared constant instead of redeclaring it, and do the same for the duplicate declaration found in SearchBar (consolidate into one exported constant to avoid duplication).apps/web/src/app/university/search/SearchBar.tsx (1)
32-32:defaultHomeUniversitySlug가PageContent.tsx와 중복됩니다. 모듈 스코프로 이동을 권장합니다.
중복 선언
PageContent.tsx37번 줄과 완전히 동일한 상수가 다시 선언되어 있습니다.- 같은 폴더 내에서 동일한 값을 두 번 정의하는 것은 DRY 원칙에 어긋납니다.
렌더링 최적화
- props·state와 무관한 정적 상수이므로 컴포넌트 내부에 둘 이유가 없습니다.
- 모듈 스코프로 올리거나, 별도의 상수 파일로 추출하면 양쪽 컴포넌트가 함께 참조할 수 있습니다.
♻️ 개선 예시 (모듈 스코프로 이동)
import { HOME_UNIVERSITY_TO_SLUG_MAP } from "@/constants/university"; import { HomeUniversity } from "@/types/university"; +const defaultHomeUniversitySlug = HOME_UNIVERSITY_TO_SLUG_MAP[HomeUniversity.INHA]; const SearchBar = ({ initText }: SearchBarProps) => { const router = useRouter(); - const defaultHomeUniversitySlug = HOME_UNIVERSITY_TO_SLUG_MAP[HomeUniversity.INHA];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/university/search/SearchBar.tsx` at line 32, The constant defaultHomeUniversitySlug in SearchBar (derived from HOME_UNIVERSITY_TO_SLUG_MAP and HomeUniversity) is duplicated with PageContent.tsx; extract that constant to module scope or a shared constants module (e.g., export DEFAULT_HOME_UNIVERSITY_SLUG from a new or existing constants file) and update both SearchBar.tsx and PageContent.tsx to import it instead of declaring it locally; remove the local declaration in SearchBar (and in PageContent if duplicated) so the single exported symbol is reused across components.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/web/src/app/university/search/PageContent.tsx`:
- Line 37: Move the constant defaultHomeUniversitySlug =
HOME_UNIVERSITY_TO_SLUG_MAP[HomeUniversity.INHA] out of the component scope and
into module scope (or a shared constants module) so it is not re-evaluated on
every render; update the component to import/use that module-level/shared
constant instead of redeclaring it, and do the same for the duplicate
declaration found in SearchBar (consolidate into one exported constant to avoid
duplication).
In `@apps/web/src/app/university/search/SearchBar.tsx`:
- Line 32: The constant defaultHomeUniversitySlug in SearchBar (derived from
HOME_UNIVERSITY_TO_SLUG_MAP and HomeUniversity) is duplicated with
PageContent.tsx; extract that constant to module scope or a shared constants
module (e.g., export DEFAULT_HOME_UNIVERSITY_SLUG from a new or existing
constants file) and update both SearchBar.tsx and PageContent.tsx to import it
instead of declaring it locally; remove the local declaration in SearchBar (and
in PageContent if duplicated) so the single exported symbol is reused across
components.
vercel
bot
temporarily deployed
to
Preview – solid-connect-web-admin
vercel
bot
temporarily deployed
to
Preview – solid-connect-web-admin
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apps/web/src/app/university/[homeUniversity]/page.tsx (1)
16-16:revalidate와searchParams동시 사용 시 ISR 캐시 동작을 검토해보세요.
searchParams는 Dynamic API로, 이를 접근하면 페이지가 요청 시점에 동적 렌더링으로 opt-in됩니다. 따라서export const revalidate = 15552000이 설정되어 있어도,searchParams의 각 고유 조합마다 별도의 캐시 엔트리가 생성될 수 있습니다.searchText,languageTestType,countryCode조합이 많아질수록 캐시 스토리지가 과도하게 커질 수 있습니다.운영 환경에서 캐시 크기가 우려된다면:
revalidate를 제거하고export const dynamic = 'force-dynamic'으로 명시적으로 동적 렌더링을 선언하거나,- 자주 사용되는 필터 조합에 대해서만
generateStaticParams를 확장하는 방안을 검토해보세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/university/`[homeUniversity]/page.tsx at line 16, The page exports a static ISR revalidate value while also reading searchParams (searchText, languageTestType, countryCode), which causes per-query dynamic cache entries and potentially unbounded cache growth; either remove export const revalidate = 15552000 and add export const dynamic = 'force-dynamic' to opt into fully dynamic rendering, or keep ISR but implement generateStaticParams to prebuild only the common filter combinations (or sanitize/limit which searchParams are read) so cache entries remain bounded; update the page component's exports (revalidate or dynamic) and/or add a generateStaticParams implementation accordingly.apps/web/src/app/university/search/SearchClientContent.tsx (1)
1-49: 새 컴포넌트 구조 LGTM! 대학 선택 → 라우팅 흐름이 명확합니다. 👍
초기값 설정 (Lines 11-13)
HOME_UNIVERSITY_TO_SLUG_MAP[HomeUniversity.INHA]를 사용해 하드코딩을 피한 점이 좋습니다.pill 선택 → slug 전파 (Lines 18-44)
selectedHomeUniversitySlug가SearchBar와SchoolSearchForm양쪽에 일관되게 전달됩니다.
[Optional]
SchoolSearchForm임포트 별칭이 파일명과 불일치합니다.
PageContent.tsx의 default export는SchoolSearchForm이지만, 모듈 파일명이 컴포넌트명과 다르기 때문에 다음 개발자가 파일을 찾을 때 약간 혼란스러울 수 있습니다.✨ 가독성 개선 제안 (선택 사항)
파일명을 컴포넌트명에 맞게 일치시키거나, export 이름을 파일명 기준으로 통일하는 것을 검토해보세요:
-import SchoolSearchForm from "./PageContent"; +import SchoolSearchForm from "./SchoolSearchForm"; // 파일명 변경 시또는 현재 파일명(
PageContent.tsx)을 유지한다면 임포트 alias를 파일명 기반으로 사용:-import SchoolSearchForm from "./PageContent"; +import PageContent from "./PageContent"; // ... -<SchoolSearchForm homeUniversitySlug={selectedHomeUniversitySlug} /> +<PageContent homeUniversitySlug={selectedHomeUniversitySlug} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/university/search/SearchClientContent.tsx` around lines 1 - 49, The import alias for the default export doesn't match the filename: SearchClientContent imports SchoolSearchForm from "./PageContent" which can confuse future readers; either rename the module file PageContent.tsx to match the component (SchoolSearchForm.tsx) or change the export/default name in PageContent.tsx to match the filename (e.g., export default PageContent) and update usages accordingly (references: SchoolSearchForm in SearchClientContent and the default export inside PageContent.tsx).apps/web/src/app/university/[homeUniversity]/_ui/UniversityListContent.tsx (1)
31-37: [Optional] props→state 동기화에useEffect대신keyprop 사용을 고려해보세요.현재 두
useEffect는initialSearchText/initialRegion이 변경될 때 state를 동기화합니다. 이 패턴은 "render 후 한 프레임 지연"이 발생할 수 있습니다. 부모에서key를 교체하면 컴포넌트를 완전히 리셋하여 더 예측 가능하게 동작합니다.✨ 대안: key prop 기반 리셋 (선택 사항)
부모 컴포넌트(page.tsx)에서
key를 지정하면 두useEffect를 모두 제거할 수 있습니다:// page.tsx <UniversityListContent + key={`${initialSearchText}-${initialRegion}`} universities={filteredUniversities} homeUniversity={universityInfo} homeUniversitySlug={homeUniversity} initialSearchText={initialSearchText} initialRegion={initialRegion} />// UniversityListContent.tsx -import { useEffect, useMemo, useState } from "react"; +import { useMemo, useState } from "react"; - useEffect(() => { - setSearchText(initialSearchText.trim()); - }, [initialSearchText]); - - useEffect(() => { - setSelectedRegion(initialRegion); - }, [initialRegion]);단, 이 방식은 같은 값으로 재방문할 때도 컴포넌트를 리마운트하므로 현재 UX에 적합한지 확인이 필요합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/university/`[homeUniversity]/_ui/UniversityListContent.tsx around lines 31 - 37, The two useEffect hooks in UniversityListContent that call setSearchText(initialSearchText.trim()) and setSelectedRegion(initialRegion) cause a render-after-mount sync and should be replaced by resetting the component via a key from the parent: remove those useEffect blocks (referencing setSearchText, initialSearchText, setSelectedRegion, initialRegion in UniversityListContent) and instead have the parent (page component that renders <UniversityListContent ... />) pass a unique key derived from initialSearchText and/or initialRegion so the component remounts with fresh state; ensure this change is acceptable for UX because remounting will reset internal state on revisit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/web/src/app/university/`[homeUniversity]/_ui/UniversityListContent.tsx:
- Around line 31-37: The two useEffect hooks in UniversityListContent that call
setSearchText(initialSearchText.trim()) and setSelectedRegion(initialRegion)
cause a render-after-mount sync and should be replaced by resetting the
component via a key from the parent: remove those useEffect blocks (referencing
setSearchText, initialSearchText, setSelectedRegion, initialRegion in
UniversityListContent) and instead have the parent (page component that renders
<UniversityListContent ... />) pass a unique key derived from initialSearchText
and/or initialRegion so the component remounts with fresh state; ensure this
change is acceptable for UX because remounting will reset internal state on
revisit.
In `@apps/web/src/app/university/`[homeUniversity]/page.tsx:
- Line 16: The page exports a static ISR revalidate value while also reading
searchParams (searchText, languageTestType, countryCode), which causes per-query
dynamic cache entries and potentially unbounded cache growth; either remove
export const revalidate = 15552000 and add export const dynamic =
'force-dynamic' to opt into fully dynamic rendering, or keep ISR but implement
generateStaticParams to prebuild only the common filter combinations (or
sanitize/limit which searchParams are read) so cache entries remain bounded;
update the page component's exports (revalidate or dynamic) and/or add a
generateStaticParams implementation accordingly.
In `@apps/web/src/app/university/search/SearchClientContent.tsx`:
- Around line 1-49: The import alias for the default export doesn't match the
filename: SearchClientContent imports SchoolSearchForm from "./PageContent"
which can confuse future readers; either rename the module file PageContent.tsx
to match the component (SchoolSearchForm.tsx) or change the export/default name
in PageContent.tsx to match the filename (e.g., export default PageContent) and
update usages accordingly (references: SchoolSearchForm in SearchClientContent
and the default export inside PageContent.tsx).
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
1 participant
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.
요약
/university/search페이지에서 검색 시 이동 경로를 기존/university?...에서/university/inha?...로 변경했습니다.SearchBar)과 조건 검색 폼(PageContent) 모두 동일하게 인하대 기본 경로를 사용하도록 맞췄습니다.HOME_UNIVERSITY_TO_SLUG_MAP[HomeUniversity.INHA]를 사용해 기존 상수 체계를 따르도록 적용했습니다.변경 파일
apps/web/src/app/university/search/SearchBar.tsxapps/web/src/app/university/search/PageContent.tsx검증
pnpm --filter @solid-connect/web run lint:checkpnpm --filter @solid-connect/web run typecheckpnpm --filter @solid-connect/web run build