fix(frontend): overlay main content with inbox when active#601
Open
dembrane-sam-bot wants to merge 1 commit into
Open
fix(frontend): overlay main content with inbox when active#601dembrane-sam-bot wants to merge 1 commit into
dembrane-sam-bot wants to merge 1 commit into
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 ChangesInbox Overlay Mode
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
🚥 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
Comment on lines
+56
to
+63
| {overlay === "inbox" && ( | ||
| <div | ||
| className="absolute inset-0 z-50 flex flex-col overflow-hidden" | ||
| style={{ backgroundColor: "var(--app-background)" }} | ||
| > | ||
| <InboxView /> | ||
| </div> | ||
| )} |
Contributor
There was a problem hiding this comment.
Add ARIA attributes to the overlay wrapper.
Screen readers need semantic hints to announce the overlay properly.
♿ Suggested a11y improvement
{overlay === "inbox" && (
<div
className="absolute inset-0 z-50 flex flex-col overflow-hidden"
style={{ backgroundColor: "var(--app-background)" }}
+ role="dialog"
+ aria-label="Inbox"
>
<InboxView />
</div>
)}📝 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.
Suggested change
| {overlay === "inbox" && ( | |
| <div | |
| className="absolute inset-0 z-50 flex flex-col overflow-hidden" | |
| style={{ backgroundColor: "var(--app-background)" }} | |
| > | |
| <InboxView /> | |
| </div> | |
| )} | |
| {overlay === "inbox" && ( | |
| <div | |
| className="absolute inset-0 z-50 flex flex-col overflow-hidden" | |
| style={{ backgroundColor: "var(--app-background)" }} | |
| role="dialog" | |
| aria-label="Inbox" | |
| > | |
| <InboxView /> | |
| </div> | |
| )} |
🤖 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 - 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.
| projectId?: string; | ||
| section?: string; | ||
| }; | ||
| overlay?: "inbox" | "help" | null; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Optional: | null is redundant in the type union.
Since the field is already optional, it can be undefined. The codebase never explicitly sets overlay to null—it's either "inbox", "help", or omitted. Cleaner type:
- overlay?: "inbox" | "help" | null;
+ overlay?: "inbox" | "help";📝 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.
Suggested change
| overlay?: "inbox" | "help" | null; | |
| overlay?: "inbox" | "help"; |
🤖 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/features/sidebar/types.ts` at 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.
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.
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