Conversation
FullWidthLayout으로 9개 페이지 전환하여 뷰포트 핵 제거. SubPageHeader/FormPageHeader 공유 컴포넌트로 뒤로가기 헤더 통일. useScrollShadow 훅 추출로 스크롤 shadow 로직 DRY 개선. shadow 값을 shadow-drop-bottom으로 전체 통일. 회고 페이지 CSS 변수 버그(--gnb-height) 수정 및 콘텐츠 래퍼 추가. 라우트 중복 경로 제거.
Closed
23 tasks
Walkthrough여러 페이지의 레이아웃을 재구성하고, FormPageHeader와 SubPageHeader를 도입하여 일관된 헤더 처리를 구현했습니다. 레이아웃 컨테이너(mx-auto, max-w-layout-max, px-layout-padding)를 추가하고, 새로운 useScrollShadow 훅으로 스크롤 기반 그림자 처리를 통합하며, 라우트 트리를 재조정하여 페이지를 적절한 레이아웃 아래로 이동시켰습니다. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 사유: 라우트 트리 재구성(레이아웃 할당 변경), 다수 페이지 간 일관성 있는 헤더/컨테이너 적용, 신규 훅 및 컴포넌트 추가, 변경 사항이 12개 이상 파일에 분산되어 있어 상호의존성 검증 필요. Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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.
Actionable comments posted: 4
🧹 Nitpick comments (6)
src/features/book/components/BookLogList.tsx (1)
177-178: CSS 핵 제거 방향 LGTM.
w-screen / left-1/2 / -translate-x-1/2조합을 제거하고bg-grey-100+ 내부 layout-constrained<section>패턴으로 바꾼 것은 명확하고 일관성 있는 개선입니다.한 가지 사소한 점: 컴포넌트 루트가 이미
<section>(line 112)인데 line 178에도<section>이 중첩됩니다. 의미 구분이 명확하다면 무방하지만, 단순 레이아웃 컨테이너라면<div>로 바꾸는 것도 고려해볼 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/book/components/BookLogList.tsx` around lines 177 - 178, In BookLogList, the root is already a semantic <section> but you’ve added a nested <section> with className="bg-grey-100" and an inner layout-constrained <section>; if the inner element is only a layout container (no new semantic grouping), replace the inner nested <section> with a <div> (the element that currently has className="bg-grey-100" or the immediate child layout container) to avoid semantic nesting; update the element around the "mx-auto max-w-layout-max px-layout-padding py-xlarge" container accordingly while keeping the same classNames and structure.src/shared/components/FormPageHeader.tsx (1)
72-73: 뒤로가기 레이블 하드코딩 —SubPageHeader와의 불일치
SubPageHeader는labelprop으로 뒤로가기 텍스트를 커스터마이징할 수 있지만,FormPageHeader는"뒤로가기"가 고정되어 있습니다. 현재 사용처에선 문제없으나, 향후 다른 텍스트가 필요한 페이지에 쓸 경우 변경이 필요합니다.♻️ backLabel prop 추가 제안
export interface FormPageHeaderProps { + /** 뒤로가기 버튼 텍스트 (기본값: '뒤로가기') */ + backLabel?: string title: string ... } export default function FormPageHeader({ + backLabel = '뒤로가기', title, ... }: FormPageHeaderProps) { ... - <TextButton size="medium" icon={ChevronLeft} onClick={handleBack} className="py-small"> - 뒤로가기 + <TextButton size="medium" icon={ChevronLeft} onClick={handleBack} className="py-small"> + {backLabel} </TextButton>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/components/FormPageHeader.tsx` around lines 72 - 73, FormPageHeader currently hardcodes the back button text ("뒤로가기") which creates inconsistency with SubPageHeader's customizable label; update FormPageHeader to accept an optional prop (e.g., backLabel) and pass it to the TextButton instead of the literal string, defaulting to the existing "뒤로가기" when backLabel is undefined, and ensure the prop is documented/typed in the FormPageHeader props interface (reference FormPageHeader, TextButton, handleBack, and SubPageHeader for consistent behavior).src/shared/hooks/useScrollShadow.ts (1)
3-13: 훅 인스턴스마다window스크롤 리스너 중복 등록현재 구조에서는 이 훅을 사용하는 컴포넌트가 동시에 마운트될 때마다 개별
scroll리스너가window에 추가됩니다. 예를 들어SubPageHeader,FormPageHeader,LandingLayout이 모두 같은 컨텍스트에서 마운트될 경우 3개의 동일한 리스너가 등록됩니다.
passive옵션 덕분에 현재 심각한 성능 문제는 아니지만, 훅 사용처가 늘어날수록 누적됩니다. 모듈 수준 싱글톤 구독 또는 React Context로 공유하는 방식을 고려해보세요.♻️ 모듈 레벨 싱글톤 패턴 예시
-import { useEffect, useState } from 'react' +import { useEffect, useState, useSyncExternalStore } from 'react' + +let listeners: (() => void)[] = [] +let isScrolled = false + +function getSnapshot() { return isScrolled } +function subscribe(cb: () => void) { + const handler = () => { + const next = window.scrollY > 0 + if (next !== isScrolled) { isScrolled = next; listeners.forEach((l) => l()) } + } + if (listeners.length === 0) window.addEventListener('scroll', handler, { passive: true }) + listeners.push(cb) + return () => { + listeners = listeners.filter((l) => l !== cb) + if (listeners.length === 0) window.removeEventListener('scroll', handler) + } +} export function useScrollShadow() { - const [isScrolled, setIsScrolled] = useState(false) - - useEffect(() => { - const handleScroll = () => setIsScrolled(window.scrollY > 0) - handleScroll() - window.addEventListener('scroll', handleScroll, { passive: true }) - return () => window.removeEventListener('scroll', handleScroll) - }, []) - - return isScrolled + return useSyncExternalStore(subscribe, getSnapshot) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/hooks/useScrollShadow.ts` around lines 3 - 13, The hook useScrollShadow currently adds a separate window 'scroll' listener per instance (handleScroll) causing duplicate listeners; refactor to a module-level singleton subscription that maintains the current scroll state and a set of subscriber callbacks so only one window listener is ever attached. Implement a module-scoped store (e.g., currentIsScrolled boolean and a Set of subscriber functions) and a single handleScroll function that updates currentIsScrolled and notifies subscribers; then change useScrollShadow to subscribe its setIsScrolled on mount and unsubscribe on unmount, initializing the listener only when the first subscriber registers and removing it when the last unsubscribes.src/routes/index.tsx (1)
97-100: 라우트 패턴 정의 방식이 일관되지 않습니다.
ROUTES.PRE_OPINIONS(':gatheringId', ':meetingId')는 네비게이션용 헬퍼 함수를 경로 패턴 정의에 사용하고 있습니다. 동일 블록 내 다른 라우트들은 모두 인라인 템플릿 리터럴을 사용 중입니다.→ 동작에는 문제없으나, 패턴이 섞이면 유지보수 시 혼란을 줄 수 있습니다.
제안
- { - path: ROUTES.PRE_OPINIONS(':gatheringId', ':meetingId'), - element: <PreOpinionListPage />, - }, + { + path: `${ROUTES.GATHERINGS}/:gatheringId/meetings/:meetingId/pre-opinions`, + element: <PreOpinionListPage />, + },Based on learnings: "In React Router route definitions, define path patterns inline instead of using ROUTES.() helper functions. ROUTES.() are intended for generating actual URLs during navigation."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/index.tsx` around lines 97 - 100, The route path uses the navigation helper ROUTES.PRE_OPINIONS(':gatheringId', ':meetingId') instead of an inline route pattern, which mixes conventions; update the route definition for PreOpinionListPage to use an inline path pattern (i.e. the literal route string with :gatheringId and :meetingId parameters) so it matches the other routes' style and keep ROUTES.*() reserved for URL generation during navigation.src/pages/Meetings/MeetingDetailPage.tsx (1)
89-114: 좌측 컬럼w-[300px]이 모바일에서 레이아웃을 깨뜨릴 수 있습니다.
flex justify-between gap-[36px]와w-[300px] flex-none조합은 데스크탑에선 문제없지만, 뷰포트가 좁아지면 우측 영역이 극도로 압축됩니다. 반응형 breakpoint(md:flex-row flex-col등)를 검토해보세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Meetings/MeetingDetailPage.tsx` around lines 89 - 114, The left column's fixed width ("w-[300px] flex-none") inside the container div ("flex justify-between gap-[36px]") breaks narrow viewports; update the parent layout to be responsive (e.g., switch to column on small screens and row on md+) and make the left column responsive (e.g., md:w-[300px] w-full and md:flex-none flex-auto or similar) so the right content can shrink naturally; adjust gap classes to responsive variants as well. Edit the container div around the columns and the left column div that renders MeetingDetailHeader, MeetingDetailInfo, and MeetingDetailButton to apply these responsive class changes.src/pages/Retrospectives/MeetingRetrospectiveCreatePage.tsx (1)
44-62: 두 패널 레이아웃이 모바일 반응형을 고려하지 않고 있습니다.
flex gap-medium으로 두 패널을 나란히 배치하는데, 작은 화면에서 패널이 너무 좁아질 수 있습니다.md:flex-row flex-col등의 반응형 처리를 추가하면 좋겠습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Retrospectives/MeetingRetrospectiveCreatePage.tsx` around lines 44 - 62, The two-panel layout currently uses a single non-responsive flex container (the outer div with class "mx-auto max-w-layout-max px-layout-padding flex gap-medium mt-base") and fixed side panels (the two child divs with "flex flex-1 ..."); update the outer container to switch to column layout on small screens and row on medium+ (e.g., use responsive utility ordering like "flex flex-col md:flex-row" or "md:flex-row flex-col") so the panels stack on mobile, and keep the child panels as flexible ("flex-1") so they fill available width; optionally add responsive spacing (e.g., "md:gap-medium" vs "gap-medium") to control gaps per breakpoint.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/features/book/components/BookLogList.tsx`:
- Line 114: The sticky container in BookLogList uses a hardcoded top value
(`top-[108px]`) which should be changed to reference the project CSS variable
pattern used elsewhere; update the class on the div in BookLogList to use
`top-[calc(var(--spacing-gnb-height)+59px)]` (matching MeetingRetrospectivePage
pattern) so GNB height changes are respected, and consider extracting the 44px
SubPageHeader constant into a design token/variable if that value is a shared
component height.
In `@src/pages/Meetings/MeetingDetailPage.tsx`:
- Around line 84-87: The useParams generic is incorrect and the non-null
assertion on gatheringId is unsafe; change useParams to use the default (no
generic) so params are typed as string | undefined, then add an explicit guard
like in MeetingSettingPage that returns/redirects (or shows a loader) when
gatheringId or meetingId is undefined before rendering SubPageHeader; remove the
trailing ! from ROUTES.GATHERING_DETAIL(gatheringId!) and only call
ROUTES.GATHERING_DETAIL with the validated gatheringId, and ensure any code that
uses meeting (e.g., meeting?.gathering.gatheringName) still handles meeting
being undefined.
In `@src/pages/Retrospectives/MeetingRetrospectiveCreatePage.tsx`:
- Line 21: The hardcoded 59px magic number in the sticky container top calc
inside MeetingRetrospectiveCreatePage.tsx should be replaced with a CSS variable
(e.g. --spacing-subpage-header-height) or moved into the SubPageHeader
component; update the div with className "sticky
top-[calc(var(--spacing-gnb-height)+59px)] ..." to use
"top-[calc(var(--spacing-gnb-height)+var(--spacing-subpage-header-height))]" (or
relocate the second sticky into the SubPageHeader component), and then
add/declare --spacing-subpage-header-height in theme.css (or your theme token
file) so the offset is driven by the SubPageHeader height instead of a hardcoded
value.
In `@src/pages/Retrospectives/MeetingRetrospectivePage.tsx`:
- Around line 65-66: The sticky header uses a hardcoded "59px" magic number in
the top calc (top-[calc(var(--spacing-gnb-height)+59px)]) that implicitly
depends on SubPageHeader height; change this to use a shared CSS variable or an
explicit height class on SubPageHeader so the value is maintained in one place.
Update SubPageHeader (component name: SubPageHeader) to either define/emit a CSS
variable like --spacing-sub-header-height or add a fixed h-* Tailwind class, and
replace the 59px in MeetingRetrospectivePage's sticky container with
var(--spacing-sub-header-height) (i.e. top calc should reference
var(--spacing-sub-header-height) instead of 59px). Ensure both places are
updated so layout stays consistent when padding/font sizes change.
---
Nitpick comments:
In `@src/features/book/components/BookLogList.tsx`:
- Around line 177-178: In BookLogList, the root is already a semantic <section>
but you’ve added a nested <section> with className="bg-grey-100" and an inner
layout-constrained <section>; if the inner element is only a layout container
(no new semantic grouping), replace the inner nested <section> with a <div> (the
element that currently has className="bg-grey-100" or the immediate child layout
container) to avoid semantic nesting; update the element around the "mx-auto
max-w-layout-max px-layout-padding py-xlarge" container accordingly while
keeping the same classNames and structure.
In `@src/pages/Meetings/MeetingDetailPage.tsx`:
- Around line 89-114: The left column's fixed width ("w-[300px] flex-none")
inside the container div ("flex justify-between gap-[36px]") breaks narrow
viewports; update the parent layout to be responsive (e.g., switch to column on
small screens and row on md+) and make the left column responsive (e.g.,
md:w-[300px] w-full and md:flex-none flex-auto or similar) so the right content
can shrink naturally; adjust gap classes to responsive variants as well. Edit
the container div around the columns and the left column div that renders
MeetingDetailHeader, MeetingDetailInfo, and MeetingDetailButton to apply these
responsive class changes.
In `@src/pages/Retrospectives/MeetingRetrospectiveCreatePage.tsx`:
- Around line 44-62: The two-panel layout currently uses a single non-responsive
flex container (the outer div with class "mx-auto max-w-layout-max
px-layout-padding flex gap-medium mt-base") and fixed side panels (the two child
divs with "flex flex-1 ..."); update the outer container to switch to column
layout on small screens and row on medium+ (e.g., use responsive utility
ordering like "flex flex-col md:flex-row" or "md:flex-row flex-col") so the
panels stack on mobile, and keep the child panels as flexible ("flex-1") so they
fill available width; optionally add responsive spacing (e.g., "md:gap-medium"
vs "gap-medium") to control gaps per breakpoint.
In `@src/routes/index.tsx`:
- Around line 97-100: The route path uses the navigation helper
ROUTES.PRE_OPINIONS(':gatheringId', ':meetingId') instead of an inline route
pattern, which mixes conventions; update the route definition for
PreOpinionListPage to use an inline path pattern (i.e. the literal route string
with :gatheringId and :meetingId parameters) so it matches the other routes'
style and keep ROUTES.*() reserved for URL generation during navigation.
In `@src/shared/components/FormPageHeader.tsx`:
- Around line 72-73: FormPageHeader currently hardcodes the back button text
("뒤로가기") which creates inconsistency with SubPageHeader's customizable label;
update FormPageHeader to accept an optional prop (e.g., backLabel) and pass it
to the TextButton instead of the literal string, defaulting to the existing
"뒤로가기" when backLabel is undefined, and ensure the prop is documented/typed in
the FormPageHeader props interface (reference FormPageHeader, TextButton,
handleBack, and SubPageHeader for consistent behavior).
In `@src/shared/hooks/useScrollShadow.ts`:
- Around line 3-13: The hook useScrollShadow currently adds a separate window
'scroll' listener per instance (handleScroll) causing duplicate listeners;
refactor to a module-level singleton subscription that maintains the current
scroll state and a set of subscriber callbacks so only one window listener is
ever attached. Implement a module-scoped store (e.g., currentIsScrolled boolean
and a Set of subscriber functions) and a single handleScroll function that
updates currentIsScrolled and notifies subscribers; then change useScrollShadow
to subscribe its setIsScrolled on mount and unsubscribe on unmount, initializing
the listener only when the first subscriber registers and removing it when the
last unsubscribes.
haruyam15
approved these changes
Feb 19, 2026
choiyoungae
approved these changes
Feb 19, 2026
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
3 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.
🚀 풀 리퀘스트 제안
📋 작업 내용
CSS viewport 핵(
100dvh강제 지정 등)을 제거하고, FullWidthLayout 기반으로 서브 페이지들의 레이아웃 구조를 리팩토링했습니다. 공통 헤더 컴포넌트를 추출하고 스크롤 shadow 로직을 커스텀 훅으로 통합했습니다.🔧 변경 사항
SubPageHeader/FormPageHeader공통 컴포넌트 추출 및 각 페이지에 적용useScrollShadow커스텀 훅으로 SubPageHeader, FormPageHeader, LandingLayout의 중복 스크롤 shadow 로직 통합shadow-drop-bottom으로 통일 (BookLogList, LandingLayout 등)--gnb-height→--spacing-gnb-height)📄 기타
관련 이슈: #83
Summary by CodeRabbit
릴리스 노트
New Features
Style
Chores