web-next: Replace AppSidebar with responsive Navigation#194
web-next: Replace AppSidebar with responsive Navigation#194yihyunjoon wants to merge 10 commits intohackers-pub:mainfrom
Conversation
Replace FloatingComposeButton with a full Navigation component that provides iOS-style tab bar navigation for mobile users. The navigation includes Feed, Notifications, Create, Search, and Profile tabs with the user's avatar displayed for the profile link.
Remove AppSidebar component and enhance Navigation to serve as the primary navigation. Navigation now displays as a bottom bar on mobile and a sticky left sidebar on sm+ screens with logo.
- lg: 200px width with text labels next to icons - Dark/light mode logo switching using picture element - Hover states for menu items
Move Create Note button to a prominent position in sidebar with primary button styling, adjust logo size, and use rounded-full for hover states.
Add reusable Footer component with Code of Conduct link and license information, integrate it into Navigation sidebar.
# Conflicts: # web-next/src/components/AppSidebar.tsx # web-next/src/components/FloatingComposeButton.tsx # web-next/src/locales/en-US/messages.po # web-next/src/locales/ja-JP/messages.po # web-next/src/locales/ko-KR/messages.po # web-next/src/locales/zh-CN/messages.po # web-next/src/locales/zh-TW/messages.po # web-next/src/routes/(root).tsx
Add left padding to nav list at lg breakpoint and move compose button outside the ul element for proper sizing and semantic correctness.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThis PR refactors the app's navigation layout by removing AppSidebar and FloatingComposeButton components and replacing them with new Navigation and Footer components in the root layout, with corresponding translation file updates to relocate or deprecate affected strings. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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
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 |
Summary of ChangesHello @yihyunjoon, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the application's primary navigation system to enhance responsiveness and user experience across various devices. It consolidates previous sidebar and floating action button functionalities into a new, unified Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a great step forward in improving the application's responsiveness by replacing the AppSidebar and FloatingComposeButton with a new, unified Navigation component. The implementation uses responsive design principles effectively. I've identified a potential functional regression and a minor code cleanup opportunity that would further enhance the quality of these changes. Overall, this is a solid and well-executed refactoring.
| <div class="hidden sm:flex w-full px-2 mt-5"> | ||
| <button | ||
| type="button" | ||
| onClick={openNoteCompose} | ||
| class="flex lg:flex-1 items-center justify-center p-3 rounded-full bg-primary text-primary-foreground transition-colors hover:bg-primary/90" | ||
| aria-label={t`Create Note`} | ||
| > | ||
| <IconSquarePen class="size-5" aria-hidden="true" /> | ||
| </button> | ||
| </div> |
There was a problem hiding this comment.
The new unified navigation is a significant improvement. However, it seems the "Create Article" functionality, which was available in both the previous sidebar and the floating action button, has been omitted. Both the mobile and desktop versions of the new compose button only trigger the note composition modal.
If this omission was unintentional, you might consider reintroducing this feature. For the desktop view, a dropdown menu on the compose button could be an effective way to offer both "Create Note" and "Create Article" options, similar to the previous implementation.
| <SidebarProvider> | ||
| <AppSidebar | ||
| $signedAccount={signedAccount()?.viewer} | ||
| signedAccountLoaded={!signedAccount.pending} | ||
| /> | ||
| <RootLayoutContent signedAccount={signedAccount}> | ||
| {props.children} | ||
| </RootLayoutContent> | ||
| </SidebarProvider> |
There was a problem hiding this comment.
The SidebarProvider appears to be a remnant from the old AppSidebar component. Since the new Navigation component does not utilize the useSidebar context, this provider is likely no longer necessary. Removing it, along with its import, will help clean up the code and improve maintainability.
<RootLayoutContent signedAccount={signedAccount}>
{props.children}
</RootLayoutContent>
yihyunjoon
force-pushed
the
navigation
branch
from
0b5ad4b to
eaee1e8
Compare
![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
🤖 Fix all issues with AI agents
In `@web-next/src/components/Footer.tsx`:
- Around line 23-35: The external anchor elements in Footer.tsx that render the
"GitHub repository" and the "AGPL-3.0" links use target="_blank" but lack rel
attributes, creating a tabnabbing risk; update the anchor JSX (the <a> elements
that produce the "GitHub repository" and the link for "AGPL-3.0") to include
rel="noopener noreferrer" alongside target="_blank" so external links open
safely in a new tab.
- Around line 19-40: The Trans message placeholders do not match the locale's
numeric placeholders, so update the usage in Footer.tsx to use the same numeric
placeholders or update the values keys accordingly: change the message prop to
use {0} and {1} instead of ${"GITHUB_REPOSITORY"} and ${"AGPL-3.0"}, or
alternatively replace the values keys GITHUB_REPOSITORY and "AGPL-3.0" with
numeric keys (0 and 1) so the Trans component injects the anchor elements into
the placeholders; modify the Trans call and its values object (referencing the
Trans component, the message prop, and the values entries) to ensure placeholder
names match the locale format.
In `@web-next/src/components/Navigation.tsx`:
- Around line 123-136: The Profile nav item only checks for exact match against
location.pathname, so subroutes like /@username/notes aren't treated as active;
update the logic in Navigation.tsx (the A element's aria-current and the Avatar
classList check around account.username and location.pathname) to use a prefix
match (e.g., location.pathname.startsWith(`/@${account.username}`)) or compute
an isProfileRoute variable and reuse it for both aria-current and the "ring-2
ring-foreground" class so the profile tab stays active on subroutes.
In `@web-next/src/locales/zh-CN/messages.po`:
- Around line 1207-1212: The translation swaps the placeholder order: msgid uses
{0} for repository and {1} for license but msgstr in
web-next/src/locales/zh-CN/messages.po places them opposite; update the msgstr
so that {0} corresponds to the repository and {1} to the license (as used by
src/components/Footer.tsx) to preserve correct semantics.
🧹 Nitpick comments (2)
web-next/src/components/Footer.tsx (1)
8-14: Destructure props in the component signature.This keeps component style consistent with project conventions.
♻️ Suggested refactor
-export function Footer(props: FooterProps) { +export function Footer({ class: className }: FooterProps) { const { t } = useLingui(); return ( <footer - class={`p-4 text-xs text-muted-foreground ${props.class ?? ""}`} + class={`p-4 text-xs text-muted-foreground ${className ?? ""}`} >As per coding guidelines, use functional components with props destructuring.
web-next/src/components/Navigation.tsx (1)
19-32: Destructure props in the component signature.Keeps component style consistent across the codebase.
♻️ Suggested refactor
-export function Navigation(props: NavigationProps) { +export function Navigation({ $signedAccount }: NavigationProps) { @@ - () => props.$signedAccount, + () => $signedAccount,As per coding guidelines, use functional components with props destructuring.
| <Trans | ||
| message={t`The source code of this website is available on ${"GITHUB_REPOSITORY"} under the ${"AGPL-3.0"} license.`} | ||
| values={{ | ||
| GITHUB_REPOSITORY: () => ( | ||
| <a | ||
| href="https://github.com/hackers-pub/hackerspub" | ||
| target="_blank" | ||
| class="underline" | ||
| > | ||
| {t`GitHub repository`} | ||
| </a> | ||
| ), | ||
| "AGPL-3.0": () => ( | ||
| <a | ||
| href="https://www.gnu.org/licenses/agpl-3.0.en.html" | ||
| target="_blank" | ||
| class="underline" | ||
| > | ||
| AGPL 3.0 | ||
| </a> | ||
| ), | ||
| }} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all .po files and search for the message
fd -e po . web-next/src/locales --exec grep -l "The source code of this website is available on" {} \;Repository: hackers-pub/hackerspub
Length of output: 260
🏁 Script executed:
#!/bin/bash
# Get details of the message in locale files
rg -A 3 "The source code of this website is available on" web-next/src/localesRepository: hackers-pub/hackerspub
Length of output: 1898
Align Trans component placeholders with locale file format.
The locale entries use {0} and {1} placeholders, but the Trans component uses named placeholders (GITHUB_REPOSITORY and "AGPL-3.0"). This mismatch prevents the values object from properly injecting the anchor elements.
✅ Correct placeholder names
values={{
- GITHUB_REPOSITORY: () => (
+ "{0}": () => (
<a
href="https://github.com/hackers-pub/hackerspub"
target="_blank"
class="underline"
>
{t`GitHub repository`}
</a>
),
- "AGPL-3.0": () => (
+ "{1}": () => (
<a
href="https://www.gnu.org/licenses/agpl-3.0.en.html"
target="_blank"
class="underline"
>
AGPL 3.0
</a>
),
}}🧰 Tools
🪛 Biome (2.1.2)
[error] 25-25: Avoid using target="_blank" without rel="noopener" or rel="noreferrer".
Opening external links in new tabs without rel="noopener" is a security risk. See the explanation for more details.
Safe fix: Add the rel="noopener" attribute.
(lint/security/noBlankTarget)
[error] 34-34: Avoid using target="_blank" without rel="noopener" or rel="noreferrer".
Opening external links in new tabs without rel="noopener" is a security risk. See the explanation for more details.
Safe fix: Add the rel="noopener" attribute.
(lint/security/noBlankTarget)
🤖 Prompt for AI Agents
In `@web-next/src/components/Footer.tsx` around lines 19 - 40, The Trans message
placeholders do not match the locale's numeric placeholders, so update the usage
in Footer.tsx to use the same numeric placeholders or update the values keys
accordingly: change the message prop to use {0} and {1} instead of
${"GITHUB_REPOSITORY"} and ${"AGPL-3.0"}, or alternatively replace the values
keys GITHUB_REPOSITORY and "AGPL-3.0" with numeric keys (0 and 1) so the Trans
component injects the anchor elements into the placeholders; modify the Trans
call and its values object (referencing the Trans component, the message prop,
and the values entries) to ensure placeholder names match the locale format.
| <a | ||
| href="https://github.com/hackers-pub/hackerspub" | ||
| target="_blank" | ||
| class="underline" | ||
| > | ||
| {t`GitHub repository`} | ||
| </a> | ||
| ), | ||
| "AGPL-3.0": () => ( | ||
| <a | ||
| href="https://www.gnu.org/licenses/agpl-3.0.en.html" | ||
| target="_blank" | ||
| class="underline" |
There was a problem hiding this comment.
Add rel="noopener noreferrer" to external links opened in a new tab.
Both external anchors open in a new tab without rel, which is a tabnabbing risk.
🛡️ Proposed fix
<a
href="https://github.com/hackers-pub/hackerspub"
target="_blank"
+ rel="noopener noreferrer"
class="underline"
>
...
<a
href="https://www.gnu.org/licenses/agpl-3.0.en.html"
target="_blank"
+ rel="noopener noreferrer"
class="underline"
>📝 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.
| <a | |
| href="https://github.com/hackers-pub/hackerspub" | |
| target="_blank" | |
| class="underline" | |
| > | |
| {t`GitHub repository`} | |
| </a> | |
| ), | |
| "AGPL-3.0": () => ( | |
| <a | |
| href="https://www.gnu.org/licenses/agpl-3.0.en.html" | |
| target="_blank" | |
| class="underline" | |
| <a | |
| href="https://github.com/hackers-pub/hackerspub" | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| class="underline" | |
| > | |
| {t`GitHub repository`} | |
| </a> | |
| ), | |
| "AGPL-3.0": () => ( | |
| <a | |
| href="https://www.gnu.org/licenses/agpl-3.0.en.html" | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| class="underline" |
🧰 Tools
🪛 Biome (2.1.2)
[error] 25-25: Avoid using target="_blank" without rel="noopener" or rel="noreferrer".
Opening external links in new tabs without rel="noopener" is a security risk. See the explanation for more details.
Safe fix: Add the rel="noopener" attribute.
(lint/security/noBlankTarget)
[error] 34-34: Avoid using target="_blank" without rel="noopener" or rel="noreferrer".
Opening external links in new tabs without rel="noopener" is a security risk. See the explanation for more details.
Safe fix: Add the rel="noopener" attribute.
(lint/security/noBlankTarget)
🤖 Prompt for AI Agents
In `@web-next/src/components/Footer.tsx` around lines 23 - 35, The external anchor
elements in Footer.tsx that render the "GitHub repository" and the "AGPL-3.0"
links use target="_blank" but lack rel attributes, creating a tabnabbing risk;
update the anchor JSX (the <a> elements that produce the "GitHub repository" and
the link for "AGPL-3.0") to include rel="noopener noreferrer" alongside
target="_blank" so external links open safely in a new tab.
| <A | ||
| href={`/@${account.username}`} | ||
| class="flex items-center justify-center py-3 sm:p-3 lg:justify-start lg:gap-3 lg:rounded-full lg:hover:bg-accent" | ||
| aria-label={t`Profile`} | ||
| aria-current={location.pathname === `/@${account.username}` | ||
| ? "page" | ||
| : undefined} | ||
| > | ||
| <Avatar | ||
| class="size-6 shrink-0" | ||
| classList={{ | ||
| "ring-2 ring-foreground": location.pathname === | ||
| `/@${account.username}`, | ||
| }} |
There was a problem hiding this comment.
Keep the Profile nav item active on subroutes.
Currently the profile item is only active on the exact /@username path. Subpages like /@username/notes won’t highlight the profile tab.
✅ Proposed fix
- {(account) => (
+ {(account) => {
+ const profilePath = `/@${account.username}`;
+ const profileActive = isActive(profilePath);
+ return (
<nav
class="fixed sm:flex sm:flex-col z-50 border-border bg-background/95 bottom-0 left-0 right-0 border-t sm:sticky sm:top-0 sm:h-screen sm:w-16 sm:shrink-0 sm:border-t-0 sm:border-r lg:w-50 sm:py-3"
role="navigation"
aria-label={t`Main navigation`}
>
@@
<li class="flex-1 sm:flex-none sm:w-full">
<A
- href={`/@${account.username}`}
+ href={profilePath}
class="flex items-center justify-center py-3 sm:p-3 lg:justify-start lg:gap-3 lg:rounded-full lg:hover:bg-accent"
aria-label={t`Profile`}
- aria-current={location.pathname === `/@${account.username}`
- ? "page"
- : undefined}
+ aria-current={profileActive ? "page" : undefined}
>
<Avatar
class="size-6 shrink-0"
classList={{
- "ring-2 ring-foreground": location.pathname ===
- `/@${account.username}`,
+ "ring-2 ring-foreground": profileActive,
}}
>
@@
- </nav>
- )}
+ </nav>
+ );}}🤖 Prompt for AI Agents
In `@web-next/src/components/Navigation.tsx` around lines 123 - 136, The Profile
nav item only checks for exact match against location.pathname, so subroutes
like /@username/notes aren't treated as active; update the logic in
Navigation.tsx (the A element's aria-current and the Avatar classList check
around account.username and location.pathname) to use a prefix match (e.g.,
location.pathname.startsWith(`/@${account.username}`)) or compute an
isProfileRoute variable and reuse it for both aria-current and the "ring-2
ring-foreground" class so the profile tab stays active on subroutes.
| #. placeholder {0}: "GITHUB_REPOSITORY" | ||
| #. placeholder {1}: "AGPL-3.0" | ||
| #: src/components/AppSidebar.tsx:536 | ||
| #: src/components/Footer.tsx:20 | ||
| msgid "The source code of this website is available on {0} under the {1} license." | ||
| msgstr "可在 {1} 上以 {0} 许可获取该网站的源代码。" | ||
|
|
There was a problem hiding this comment.
Swap the placeholder order to keep meaning correct.
{0} is the repository and {1} is the license, but the translation currently places them in the opposite semantic roles.
📝 Proposed fix
-msgstr "可在 {1} 上以 {0} 许可获取该网站的源代码。"
+msgstr "可在 {0} 上以 {1} 许可获取该网站的源代码。"🤖 Prompt for AI Agents
In `@web-next/src/locales/zh-CN/messages.po` around lines 1207 - 1212, The
translation swaps the placeholder order: msgid uses {0} for repository and {1}
for license but msgstr in web-next/src/locales/zh-CN/messages.po places them
opposite; update the msgstr so that {0} corresponds to the repository and {1} to
the license (as used by src/components/Footer.tsx) to preserve correct
semantics.
1 participant
Summary
AppSidebarandFloatingComposeButtonwith a unifiedNavigationcomponentsm, full sidebar with labels atlgFooterinto a standalone componentlgfor better visual alignmentScreenshots
Summary by CodeRabbit
New Features
Refactor
Localization
✏️ Tip: You can customize this high-level summary in your review settings.