fix(frontend): overlay main content with inbox when active#601
Merged
Conversation
- keep sidebar view/layout untouched when opening inbox - overlay main content on the right with the inbox centered max-width feed - back link on inbox feed removes overlay and returns to prior view
|
Thank you for contributing to Dembrane ECHO! Before we consider your Pull Request, we ask that you sign our Contributor License Agreement (CLA). This is only required for your first Pull Request. Please review the CLA, and sign it by adding your GitHub username to the contributors.yml file. Thanks! |
Contributor
WalkthroughInbox rendering shifts from AppSidebar's view-routing system to an overlay mode: ChangesInbox Overlay Mode
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
LGTM. 🚥 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)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@echo/frontend/src/components/layout/BaseLayout.tsx`:
- Around line 56-63: Wrap the overlay container for the InboxView with proper
ARIA attributes to make it a announced modal: add role="dialog" and
aria-modal="true" to the div, and provide either aria-labelledby pointing to a
heading ID inside InboxView (create a unique id in InboxView and reference it)
or an aria-label like "Inbox" on the wrapper; also ensure the wrapper is
focusable (e.g., tabIndex={-1}) so you can move focus into it when opened.
Reference the overlay variable and the InboxView component inside BaseLayout to
locate where to add these attributes.
In `@echo/frontend/src/features/sidebar/types.ts`:
- Line 26: Remove the redundant "| null" from the overlay property type so it
reads overlay?: "inbox" | "help"; — update the declaration that currently shows
overlay?: "inbox" | "help" | null; (in
echo/frontend/src/features/sidebar/types.ts) and ensure any code that may rely
on explicit null handling instead treats absent values as undefined.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a038dbcf-aa2b-48fc-9236-6f692f1f334d
📒 Files selected for processing (6)
echo/frontend/src/components/layout/BaseLayout.tsxecho/frontend/src/features/sidebar/AppSidebar.tsxecho/frontend/src/features/sidebar/blocks/InboxBlock.tsxecho/frontend/src/features/sidebar/hooks/useSidebarView.tsecho/frontend/src/features/sidebar/types.tsecho/frontend/src/features/sidebar/views/InboxView.tsx
💤 Files with no reviewable changes (1)
- echo/frontend/src/features/sidebar/AppSidebar.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
echo/frontend/src/components/layout/BaseLayout.tsx (2)
56-67:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove focus into the overlay when it opens.
You've got
tabIndex={-1}on the dialog but nothing actually shifts focus into it. Keyboard users and screen reader users get stranded in the background content. Track the overlay state and imperatively focus the container when it mounts.♿ Proposed focus management
+import { useEffect, useRef } from "react"; + export const BaseLayout = ({ children }: PropsWithChildren) => { const { isAuthenticated } = useAuthenticated(); const { overlay } = useSidebarView(); + const overlayRef = useRef<HTMLDivElement>(null); + + useEffect(() => { + if (overlay === "inbox" && overlayRef.current) { + overlayRef.current.focus(); + } + }, [overlay]);{overlay === "inbox" && ( <div + ref={overlayRef} role="dialog"🤖 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 `@echo/frontend/src/components/layout/BaseLayout.tsx` around lines 56 - 67, The overlay div in BaseLayout (the element rendered when overlay === "inbox") has tabIndex={-1} but never receives focus; add a ref (e.g., inboxRef) to that container and, in a useEffect that watches overlay, imperatively call inboxRef.current?.focus() when overlay becomes "inbox" so keyboard/screen-reader users are moved into the dialog; keep the role="dialog" and tabIndex={-1} on the same element and ensure cleanup/no-op if ref is null.
56-67: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd ESC key handler to close the overlay.
Standard dialog UX: ESC dismisses. Users expect it. Hook into the back navigation or param clearing logic from
useSidebarViewwhen ESC fires.⌨️ Proposed ESC handler
+import { useEffect, useRef } from "react"; +import { useNavigate } from "react-router"; + export const BaseLayout = ({ children }: PropsWithChildren) => { const { isAuthenticated } = useAuthenticated(); const { overlay } = useSidebarView(); + const navigate = useNavigate(); const overlayRef = useRef<HTMLDivElement>(null); useEffect(() => { if (overlay === "inbox" && overlayRef.current) { overlayRef.current.focus(); } }, [overlay]); + + useEffect(() => { + if (overlay !== "inbox") return; + const handleEscape = (e: KeyboardEvent) => { + if (e.key === "Escape") { + const params = new URLSearchParams(window.location.search); + params.delete("sidebar"); + navigate({ search: params.toString() }, { replace: true }); + } + }; + document.addEventListener("keydown", handleEscape); + return () => document.removeEventListener("keydown", handleEscape); + }, [overlay, navigate]);🤖 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 `@echo/frontend/src/components/layout/BaseLayout.tsx` around lines 56 - 67, The inbox overlay should close on ESC: import and use the existing useSidebarView hook in BaseLayout and, when overlay === "inbox", register a keydown listener (useEffect) that listens for the Escape key and calls the sidebar-close/back navigation function returned by useSidebarView (the hook that currently controls overlay/params). Ensure the listener is only active while the dialog is mounted, properly cleans up on unmount, and does not interfere with other keys or inputs (ignore events when focus is inside an input/textarea). Also keep the existing accessibility attributes (role="dialog", aria-modal) and tabIndex handling.
🤖 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 `@echo/frontend/src/components/layout/BaseLayout.tsx`:
- Line 60: Replace the hardcoded aria-label="Inbox" in BaseLayout.tsx with a
localized string using your i18n utilities: locate the JSX element with
aria-label="Inbox" and change it to use the Trans component or the t`...`
template literal (per project conventions) so the value is translated (e.g.,
aria-label={<Trans>Inbox</Trans>} or aria-label={t`Inbox`}); ensure you import
the corresponding Trans or t from your i18n module and run a build/lint to pick
up missing translation keys.
---
Outside diff comments:
In `@echo/frontend/src/components/layout/BaseLayout.tsx`:
- Around line 56-67: The overlay div in BaseLayout (the element rendered when
overlay === "inbox") has tabIndex={-1} but never receives focus; add a ref
(e.g., inboxRef) to that container and, in a useEffect that watches overlay,
imperatively call inboxRef.current?.focus() when overlay becomes "inbox" so
keyboard/screen-reader users are moved into the dialog; keep the role="dialog"
and tabIndex={-1} on the same element and ensure cleanup/no-op if ref is null.
- Around line 56-67: The inbox overlay should close on ESC: import and use the
existing useSidebarView hook in BaseLayout and, when overlay === "inbox",
register a keydown listener (useEffect) that listens for the Escape key and
calls the sidebar-close/back navigation function returned by useSidebarView (the
hook that currently controls overlay/params). Ensure the listener is only active
while the dialog is mounted, properly cleans up on unmount, and does not
interfere with other keys or inputs (ignore events when focus is inside an
input/textarea). Also keep the existing accessibility attributes (role="dialog",
aria-modal) and tabIndex handling.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 554c4ac2-a5ad-487f-83b3-013d38929823
📒 Files selected for processing (2)
echo/frontend/src/components/layout/BaseLayout.tsxecho/frontend/src/features/sidebar/types.ts
| <div | ||
| role="dialog" | ||
| aria-modal="true" | ||
| aria-label="Inbox" |
Contributor
There was a problem hiding this comment.
Internationalize the aria-label.
That hardcoded "Inbox" string breaks i18n. Wrap it in a translation call so it ships properly localized.
🌐 Proposed i18n fix
+import { useTranslation } from "react-i18next";
+
export const BaseLayout = ({ children }: PropsWithChildren) => {
+ const { t } = useTranslation();
const { isAuthenticated } = useAuthenticated();
const { overlay } = useSidebarView(); <div
role="dialog"
aria-modal="true"
- aria-label="Inbox"
+ aria-label={t("inbox.title")}
tabIndex={-1}As per coding guidelines: "Use <Trans> component or t` template literal for translations in React components"
🤖 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 `@echo/frontend/src/components/layout/BaseLayout.tsx` at line 60, Replace the
hardcoded aria-label="Inbox" in BaseLayout.tsx with a localized string using
your i18n utilities: locate the JSX element with aria-label="Inbox" and change
it to use the Trans component or the t`...` template literal (per project
conventions) so the value is translated (e.g., aria-label={<Trans>Inbox</Trans>}
or aria-label={t`Inbox`}); ensure you import the corresponding Trans or t from
your i18n module and run a build/lint to pick up missing translation keys.
ussaama
approved these changes
Jun 2, 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
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.
What this changes
?sidebar=inboxremains deep-linkable and shareable — useSidebarView.ts?sidebar=inboxfrom the URL query parameters, automatically removing the overlay — useSidebarView.ts, BaseLayout.tsxConfidence
Confidence: high. Built exactly according to Plan B + Sameer's overlay requirement. Verified the git diff and structural/indentation cleanliness.
Summary by CodeRabbit
New Features
Refactor