fix: Make "Insight" button visible on mobile#2691
Conversation
📝 WalkthroughWalkthroughThe mobile top navigation centers the logo link and conditionally displays an Insights icon link for families, with Turbo prefetch disabled and an unread-count badge capped at ChangesMobile navigation
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 2
🧹 Nitpick comments (1)
app/views/layouts/application.html.erb (1)
62-63: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid counting active insights twice per request.
The mobile block performs
Current.family.insights.active.count, while the desktop block repeats the same query at Lines 179-180 even though both navs are rendered in the same response. Compute the count once before the markup and reuse it in both links.🤖 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 `@app/views/layouts/application.html.erb` around lines 62 - 63, Compute Current.family.insights.active.count once before the navigation markup, store it in a shared local, and reuse that value in both the mobile and desktop navigation blocks instead of querying again in the desktop block.
🤖 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 `@app/views/layouts/application.html.erb`:
- Around line 62-77: Update the application layout header around the Insights
action group so the mobile logo remains centered independently of the right-side
actions. Replace the width-dependent justify-between positioning with a
three-column layout or absolute centering, while preserving the existing
Insights link and responsive desktop alignment.
- Around line 67-74: Update the insights link generated by the link_to block to
expose unread_insights_count to assistive technology. Use a localized accessible
label that includes the displayed count, or associate the link with the
notification badge through aria-describedby, while preserving the existing
“Insights” label and capped “9+” display behavior.
---
Nitpick comments:
In `@app/views/layouts/application.html.erb`:
- Around line 62-63: Compute Current.family.insights.active.count once before
the navigation markup, store it in a shared local, and reuse that value in both
the mobile and desktop navigation blocks instead of querying again in the
desktop block.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 041895e1-1b2b-4106-bd31-6dfb7ff443f9
📒 Files selected for processing (1)
app/views/layouts/application.html.erb
| <%= link_to insights_path, | ||
| class: "relative inline-flex items-center justify-center w-9 h-9 pointer-coarse:w-11 pointer-coarse:h-11 rounded-lg text-secondary hover:text-primary hover:bg-container-inset-hover transition-colors focus-ring", | ||
| title: t("layouts.application.insights"), | ||
| aria: { label: t("layouts.application.insights") }, | ||
| data: { turbo_prefetch: false } do %> | ||
| <%= icon("lightbulb", size: "sm") %> | ||
| <% if unread_insights_count.positive? %> | ||
| <span class="absolute top-1 right-1 min-w-4 h-4 px-1 rounded-full bg-inverse text-inverse text-[10px] font-medium flex items-center justify-center pointer-events-none"><%= unread_insights_count > 9 ? "9+" : unread_insights_count %></span> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Expose the unread count to assistive technology.
The static aria-label replaces the link’s descendant text, so screen readers announce “Insights” but not the unread count. Include the count in a localized accessible label or associate it with the link via aria-describedby.
🤖 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 `@app/views/layouts/application.html.erb` around lines 67 - 74, Update the
insights link generated by the link_to block to expose unread_insights_count to
assistive technology. Use a localized accessible label that includes the
displayed count, or associate the link with the notification badge through
aria-describedby, while preserving the existing “Insights” label and capped “9+”
display behavior.
![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: 80835e3737
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| <% end %> | ||
|
|
||
| <div class="flex items-center gap-1"> | ||
| <% if Current.family %> |
There was a problem hiding this comment.
For guest users in the intro layout, this mobile header still renders because the new guard only checks Current.family; the desktop insights button is inside the lower unless intro_mode header, and intro users intentionally get the reduced assistant-first navigation. On any mobile intro/profile page for a guest with a family, this adds a direct /insights shortcut and unread badge into the restricted intro workflow, so gate this block with !intro_mode as well.
Useful? React with 👍 / 👎.
| <%= link_to insights_path, | ||
| class: "relative inline-flex items-center justify-center w-9 h-9 pointer-coarse:w-11 pointer-coarse:h-11 rounded-lg text-secondary hover:text-primary hover:bg-container-inset-hover transition-colors focus-ring", | ||
| title: t("layouts.application.insights"), | ||
| aria: { label: t("layouts.application.insights") }, | ||
| data: { turbo_prefetch: false } do %> |
There was a problem hiding this comment.
AGENTS.md Design System Hygiene says repeated hand-rolled shapes should be lifted to a DS::* primitive before the second copy lands. This pastes the same insights icon link and unread badge markup that already exists in the desktop header, so changes to the count query, prefetch guard, accessibility label, or badge styling now have to be kept in sync in two places; extract a shared/DS primitive instead of adding the duplicate.
Useful? React with 👍 / 👎.
jjmata
left a comment
jjmata
left a comment
There was a problem hiding this comment.
Thanks for fixing the mobile visibility of the Insights button — the fix itself works and follows most of the repo's conventions well: it reuses the existing layouts.application.insights i18n key (no new hardcoded strings), uses the icon helper (not lucide_icon), and has both title and aria-label on the icon-only link for accessibility.
Two things worth addressing before merge, left as inline comments:
- Containing-block bug: the logo link now uses
absolute left-1/2 -translate-x-1/2, but the parent<nav>isn'trelative(and nothing further up the tree is positioned either), so it centers against the viewport rather than the nav bar itself. Minor visually here, but fragile/incorrect CSS. - Duplication: the new mobile insights button + "9+" badge block is a verbatim copy-paste of the existing desktop insights button block elsewhere in this same file, including the code comment. Given the codebase already has a pattern for sharing nav markup (
layouts/shared/_nav_item.html.erb), extracting this into a shared partial would avoid two copies of the same badge-capping logic drifting apart over time.
No i18n or ViewComponent-placement concerns otherwise — this is a reasonable place to add the markup, just should be de-duplicated.
Generated by Claude Code
| <% end %> | ||
|
|
||
| <%= link_to home_path, class: "block" do %> | ||
| <%= link_to home_path, class: "absolute left-1/2 -translate-x-1/2" do %> |
There was a problem hiding this comment.
The logo link switches from class: "block" to class: "absolute left-1/2 -translate-x-1/2", but its parent <nav class="lg:hidden flex justify-between items-center p-3 ..."> (line 51) isn't given relative (or any position other than static). Since there's no positioned ancestor between this <nav> and <html> either (checked _htmldoc.html.erb and the wrapping <div> on line 24 — neither sets position), the containing block for this absolute element resolves to the initial containing block (viewport) rather than the nav bar. In practice left: 50% will center against the full viewport width instead of the nav's padded content box (the nav has p-3, so there's a ~12px discrepancy), and the behavior is fragile — it only "works" because nothing currently establishes a positioning context up the tree; adding position: relative anywhere in that ancestor chain later would silently break this centering.
Suggest adding relative to the <nav> class here so the logo is explicitly centered within the nav bar itself.
Generated by Claude Code
| <% if Current.family %> | ||
| <% unread_insights_count = Current.family.insights.active.count %> | ||
| <%# Prefetch disabled: /insights marks insights read on real visits | ||
| and deliberately skips prefetch requests, so a prefetched | ||
| response served on click would never clear the badge. %> | ||
| <%= link_to insights_path, | ||
| class: "relative inline-flex items-center justify-center w-9 h-9 pointer-coarse:w-11 pointer-coarse:h-11 rounded-lg text-secondary hover:text-primary hover:bg-container-inset-hover transition-colors focus-ring", | ||
| title: t("layouts.application.insights"), | ||
| aria: { label: t("layouts.application.insights") }, | ||
| data: { turbo_prefetch: false } do %> | ||
| <%= icon("lightbulb", size: "sm") %> | ||
| <% if unread_insights_count.positive? %> | ||
| <span class="absolute top-1 right-1 min-w-4 h-4 px-1 rounded-full bg-inverse text-inverse text-[10px] font-medium flex items-center justify-center pointer-events-none"><%= unread_insights_count > 9 ? "9+" : unread_insights_count %></span> | ||
| <% end %> | ||
| <% end %> | ||
| <% end %> |
There was a problem hiding this comment.
This entire block — the Current.family guard, unread_insights_count calculation, the prefetch-disabled comment, the link_to insights_path with its classes/title/aria, the icon("lightbulb", ...), and the 9+-capped badge <span> — is a byte-for-byte duplicate of the existing desktop insights button further down in this same file (the block under <%# SHARED - Main content %> → DESKTOP - Left navbar's sticky header, unread_insights_count = Current.family.insights.active.count / link_to insights_path / same badge markup). Even the inline comment is copy-pasted verbatim.
Given this codebase already extracts shared nav markup into partials (e.g. layouts/shared/_nav_item.html.erb, rendered from both the mobile bottom nav and desktop left navbar), it'd be more consistent to pull this into a layouts/shared/_insights_button.html.erb partial and render it from both the mobile top nav and the desktop sticky header, rather than maintaining two copies of the same badge-capping logic (unread_insights_count > 9 ? "9+" : unread_insights_count) that can now drift out of sync.
Generated by Claude Code
DS Drift Patrol — 1 findingRule 1 — Bypassing DS components
<span class="absolute top-1 right-1 min-w-4 h-4 px-1 rounded-full bg-inverse text-inverse text-[10px] font-medium flex items-center justify-center pointer-events-none"><%= unread_insights_count > 9 ? "9+" : unread_insights_count %></span>Hand-rolled rounded-full count badge instead of Everything else in this PR is clean — the new insights link uses Scanned by DS Drift Patrol — diff-scope only ( Generated by Claude Code |
DS Drift Patrol — 2 findings0 of 1 previously-flagged issue(s) resolved since the last run. Rule 1 — Bypassing DS components
Scanned by DS Drift Patrol — diff-scope only ( Generated by Claude Code |
jjmata
left a comment
jjmata
left a comment
There was a problem hiding this comment.
Thanks for always keeping an eye on PWA regressions/UX, @alessiocappa!
Can you address the PR review comments?
DS Drift Patrol — 2 findings (update)0 of 2 previously flagged issues resolved. Bypassing DS components (rule 1)
Scanned by DS Drift Patrol — diff-scope only ( Generated by Claude Code |
3 participants
The new “Insight” button is not visible on mobile devices. This PR adds it to the mobile navigation bar as well.
Summary by CodeRabbit
9+.