feat: update layout calculations to account for dynamic header height#10
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughIntroduces a ResizeObserver-based header measurement via a new reatom utility and integrates it into AppShell to expose a CSS header-height variable; updates pages to consume the variable for layout. Adds 15 Article mock entries and small config/setup changes (CI env and VITE_CONNECT_LOGGER check). Changes
Sequence Diagram(s)sequenceDiagram
participant Header as Header Element
participant RO as ResizeObserver
participant Atom as measureRefAtom
participant CSSVar as CSS Variable (--app-header-h)
participant Pages as Page Components
Header->>RO: mount / resize events
RO->>Atom: emit ResizeObserverEntry for node
Atom->>Atom: compute and expose sizeEntry
Atom->>CSSVar: set --app-header-h to measured height
CSSVar->>Pages: pages use calc(100dvh - var(--app-header-h, 0px))
Pages->>Pages: recalc layout and adjust top offsets
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 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: 1
🤖 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/shared/reatom/reatomMeasureRef.ts`:
- Around line 22-24: The code unconditionally instantiates ResizeObserver (new
ResizeObserver(wrap(observeAction))) which throws in SSR/non-DOM environments;
guard availability by checking typeof ResizeObserver !== "undefined" before
creating it, and return or no-op if unavailable. Specifically, in the block that
creates observer and calls observer.observe(node) (using wrap, observeAction,
and abortVar.subscribe), only construct observer when the guard passes,
otherwise skip observe() and ensure abortVar.subscribe still runs safely (or
subscribe returns a no-op) so disconnect() isn't called on an undefined
observer.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/entities/article/mocks/data.tssrc/pages/calculator/ui/CalculatorPage.tsxsrc/pages/chat/ui/ChatPageLoading.tsxsrc/pages/chat/ui/thread/MessageThread.tsxsrc/pages/chat/ui/thread/MessageThreadLoadingState.tsxsrc/pages/timer/ui/TimerPage.tsxsrc/shared/reatom/index.tssrc/shared/reatom/reatomMeasureRef.tssrc/widgets/app-shell/ui/AppShell.tsxsrc/widgets/master-details/ui/MasterDetails.tsx
There was a problem hiding this comment.
Pull request overview
This PR updates layout/sticky positioning calculations to account for a dynamically-sized app header by measuring the header height and exposing it via a CSS custom property for descendants to use.
Changes:
- Measure the AppShell header height at runtime and expose it as
--app-header-h. - Replace hard-coded viewport-height and sticky offsets (
100dvh,3.5rem,top="0") withcalc(... - var(--app-header-h))/top="var(--app-header-h)"across multiple pages. - Add a shared
reatomMeasureRefutility for ResizeObserver-based element measurement.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/widgets/master-details/ui/MasterDetails.tsx | Offsets sticky master pane by --app-header-h and adjusts height; also constrains detail width. |
| src/widgets/app-shell/ui/AppShell.tsx | Introduces measurement of header height and sets --app-header-h on the main content container. |
| src/shared/reatom/reatomMeasureRef.ts | New helper atom that measures an element via ResizeObserver and exposes ref + latest size entry. |
| src/shared/reatom/index.ts | Re-exports reatomMeasureRef. |
| src/pages/timer/ui/TimerPage.tsx | Uses minH="calc(100dvh - var(--app-header-h))" for correct vertical centering under header. |
| src/pages/chat/ui/thread/MessageThreadLoadingState.tsx | Replaces hard-coded header subtraction with --app-header-h. |
| src/pages/chat/ui/thread/MessageThread.tsx | Replaces hard-coded header subtraction with --app-header-h. |
| src/pages/chat/ui/ChatPageLoading.tsx | Updates sticky list height/top to account for --app-header-h. |
| src/pages/calculator/ui/CalculatorPage.tsx | Uses minH="calc(100dvh - var(--app-header-h))" for correct vertical centering under header. |
| src/entities/article/mocks/data.ts | Adds many new mock articles (unrelated to header-height/layout change). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ebaf1aa565
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/shared/reatom/reatomMeasureRef.ts (1)
12-17:⚠️ Potential issue | 🟠 MajorGuard
ResizeObserverbefore instantiation.This still unconditionally constructs
ResizeObserver; non-DOM runtimes can throw here and break layout state initialization.Proposed fix
return reatomObservable<DOMRect | null>( (set) => { + if (typeof ResizeObserver === 'undefined') { + set(node.getBoundingClientRect()) + return () => {} + } const observer = new ResizeObserver((entries) => { const entry = entries.find((e) => e.target === node) set(entry?.contentRect ?? null) }) observer.observe(node) return () => observer.disconnect() },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/reatom/reatomMeasureRef.ts` around lines 12 - 17, The code unconditionally constructs ResizeObserver in the block that creates observer, which can throw in non-DOM runtimes; guard the instantiation in the function/closure that uses observer (the const observer = new ResizeObserver(...) / observer.observe(node) / return () => observer.disconnect() code) by checking that the global DOM APIs exist (e.g. typeof window !== 'undefined' && 'ResizeObserver' in window) before creating the observer; if unavailable, skip creating/observing and return a no-op cleanup, and ensure set(entry?.contentRect ?? null) still initializes state appropriately (e.g. set(null) or reading getBoundingClientRect when available).
🤖 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/widgets/app-shell/ui/AppShell.tsx`:
- Around line 161-168: The IconButton rendering the Search icon (component
IconButton with aria-label m.topbar_search_placeholder and child Search) is an
interactive control with no handler—create a click/keyboard handler to make it
functional: implement or call an existing method such as openSearchDialog or
focusSearchInput from AppShell (or add a new handler like handleOpenSearch) and
wire it to the IconButton's onClick/onKeyPress to open the search UI or focus
the search input; ensure the handler is imported/defined in the same component
and preserves the aria-label for accessibility.
---
Duplicate comments:
In `@src/shared/reatom/reatomMeasureRef.ts`:
- Around line 12-17: The code unconditionally constructs ResizeObserver in the
block that creates observer, which can throw in non-DOM runtimes; guard the
instantiation in the function/closure that uses observer (the const observer =
new ResizeObserver(...) / observer.observe(node) / return () =>
observer.disconnect() code) by checking that the global DOM APIs exist (e.g.
typeof window !== 'undefined' && 'ResizeObserver' in window) before creating the
observer; if unavailable, skip creating/observing and return a no-op cleanup,
and ensure set(entry?.contentRect ?? null) still initializes state appropriately
(e.g. set(null) or reading getBoundingClientRect when available).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/entities/article/mocks/data.tssrc/pages/calculator/ui/CalculatorPage.tsxsrc/pages/chat/ui/ChatPageLoading.tsxsrc/pages/chat/ui/thread/MessageThread.tsxsrc/pages/chat/ui/thread/MessageThreadLoadingState.tsxsrc/pages/timer/ui/TimerPage.tsxsrc/shared/reatom/index.tssrc/shared/reatom/reatomMeasureRef.tssrc/widgets/app-shell/ui/AppShell.tsxsrc/widgets/master-details/ui/MasterDetails.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- src/pages/chat/ui/thread/MessageThread.tsx
- src/pages/chat/ui/ChatPageLoading.tsx
- src/shared/reatom/index.ts
- src/pages/timer/ui/TimerPage.tsx
- src/pages/calculator/ui/CalculatorPage.tsx
- src/widgets/master-details/ui/MasterDetails.tsx
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/widgets/app-shell/ui/AppShell.tsx (1)
169-176:⚠️ Potential issue | 🟡 MinorSearch IconButton is still non-functional on md/lg.
This control is rendered as interactive but has no action wired, so it behaves like a dead button.
Suggested fix pattern
type Props = { appName: string sidebarContent: ReactNode sidebarFooter: ReactNode mobileHeader: ReactNode breadcrumbs: ReactNode children: ReactNode + onSearchClick?: () => void } export const AppShell = reatomComponent( - ({ appName, sidebarContent, sidebarFooter, mobileHeader, breadcrumbs, children }: Props) => { + ({ appName, sidebarContent, sidebarFooter, mobileHeader, breadcrumbs, children, onSearchClick }: Props) => { @@ <IconButton variant="plain" size="sm" display={{ base: 'none', md: 'inline-flex', xl: 'none' }} aria-label={m.topbar_search_placeholder()} + onClick={onSearchClick} + disabled={!onSearchClick} > <Search /> </IconButton>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/widgets/app-shell/ui/AppShell.tsx` around lines 169 - 176, The Search IconButton is rendered without any action; wire it to the app's search-open handler so it becomes interactive: add an onClick that calls the existing search-opening function (e.g., openSearch, toggleSearch, or a prop like onOpenSearch) or implement a local handler (e.g., handleOpenSearch) that opens the search modal/panel and sets focus to the search input; ensure the IconButton (component IconButton, aria-label m.topbar_search_placeholder(), and Search icon) also includes accessibility hints (aria-haspopup or role if needed) and keyboard activation so the control works on md/lg.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/widgets/app-shell/ui/AppShell.tsx`:
- Around line 169-176: The Search IconButton is rendered without any action;
wire it to the app's search-open handler so it becomes interactive: add an
onClick that calls the existing search-opening function (e.g., openSearch,
toggleSearch, or a prop like onOpenSearch) or implement a local handler (e.g.,
handleOpenSearch) that opens the search modal/panel and sets focus to the search
input; ensure the IconButton (component IconButton, aria-label
m.topbar_search_placeholder(), and Search icon) also includes accessibility
hints (aria-haspopup or role if needed) and keyboard activation so the control
works on md/lg.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/entities/article/mocks/data.tssrc/pages/calculator/ui/CalculatorPage.tsxsrc/pages/chat/ui/ChatPageLoading.tsxsrc/pages/chat/ui/thread/MessageThread.tsxsrc/pages/chat/ui/thread/MessageThreadLoadingState.tsxsrc/pages/timer/ui/TimerPage.tsxsrc/shared/reatom/index.tssrc/shared/reatom/resize-observer.tssrc/widgets/app-shell/ui/AppShell.tsxsrc/widgets/master-details/ui/MasterDetails.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- src/pages/chat/ui/thread/MessageThread.tsx
- src/pages/chat/ui/thread/MessageThreadLoadingState.tsx
- src/pages/chat/ui/ChatPageLoading.tsx
- src/pages/timer/ui/TimerPage.tsx
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/widgets/app-shell/ui/AppShell.tsx (1)
169-176:⚠️ Potential issue | 🟡 MinorSearch IconButton is still a dead interactive control.
On Line 173 this control is rendered as actionable but has no handler/target, so users can focus/click it without any outcome. Please wire an action or disable/hide it until implemented.
Possible fix pattern
<IconButton variant="plain" size="sm" display={{ base: 'none', md: 'inline-flex', xl: 'none' }} aria-label={m.topbar_search_placeholder()} + onClick={onSearchClick} + disabled={!onSearchClick} > <Search /> </IconButton>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/widgets/app-shell/ui/AppShell.tsx` around lines 169 - 176, The Search IconButton in AppShell.tsx is rendered as an actionable control but has no handler; either wire it to the search action or make it non-interactive until implemented: add an onClick handler (e.g., call openSearch or dispatch setSearchOpen in the AppShell component) and/or pass a clear prop like isDisabled or aria-disabled="true" and remove interactive tabindex when not available; locate the IconButton element rendering the <Search /> icon and update it to call the appropriate search-opening function (or mark disabled/hidden) and keep the existing aria-label m.topbar_search_placeholder().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/widgets/app-shell/ui/AppShell.tsx`:
- Around line 169-176: The Search IconButton in AppShell.tsx is rendered as an
actionable control but has no handler; either wire it to the search action or
make it non-interactive until implemented: add an onClick handler (e.g., call
openSearch or dispatch setSearchOpen in the AppShell component) and/or pass a
clear prop like isDisabled or aria-disabled="true" and remove interactive
tabindex when not available; locate the IconButton element rendering the <Search
/> icon and update it to call the appropriate search-opening function (or mark
disabled/hidden) and keep the existing aria-label m.topbar_search_placeholder().
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
sonar-project.propertiessrc/entities/article/mocks/data.tssrc/pages/calculator/ui/CalculatorPage.tsxsrc/pages/chat/ui/ChatPageLoading.tsxsrc/pages/chat/ui/thread/MessageThread.tsxsrc/pages/chat/ui/thread/MessageThreadLoadingState.tsxsrc/pages/timer/ui/TimerPage.tsxsrc/shared/reatom/index.tssrc/shared/reatom/resize-observer.tssrc/widgets/app-shell/ui/AppShell.tsxsrc/widgets/master-details/ui/MasterDetails.tsx
✅ Files skipped from review due to trivial changes (1)
- sonar-project.properties
🚧 Files skipped from review as they are similar to previous changes (8)
- src/shared/reatom/index.ts
- src/pages/chat/ui/ChatPageLoading.tsx
- src/pages/chat/ui/thread/MessageThreadLoadingState.tsx
- src/pages/calculator/ui/CalculatorPage.tsx
- src/widgets/master-details/ui/MasterDetails.tsx
- src/pages/chat/ui/thread/MessageThread.tsx
- src/pages/timer/ui/TimerPage.tsx
- src/entities/article/mocks/data.ts
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
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.
Summary by CodeRabbit
New Content
New Features
Bug Fixes
Refactor
Chores