Fix PWA issues #172
Fix PWA issues #172
Conversation
WalkthroughUpdated several view templates to adjust CSS classes: added safe-area inset padding to dialog, fixed-positioned the mobile bottom nav, changed HTML root height/background classes, and modified bottom padding on budget and dashboard pages. No control flow or data changes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/views/layouts/application.html.erb (1)
114-114: Add mobile-specific bottom padding to prevent content overlap.The main content area uses
py-4for vertical padding, but with the fixed bottom navigation on mobile (line 159), content will be obscured by the nav bar. Add mobile-specific bottom padding to account for the navigation height plus safe-area inset.Consider applying this pattern:
-<%= tag.main class: class_names("grow overflow-y-auto px-3 lg:px-10 py-4 w-full mx-auto max-w-5xl"), data: { app_layout_target: "content" } do %> +<%= tag.main class: class_names("grow overflow-y-auto px-3 lg:px-10 pt-4 pb-20 lg:py-4 w-full mx-auto max-w-5xl"), data: { app_layout_target: "content" } do %>Note: Adjust
pb-20(5rem/80px) based on the actual height of your bottom navigation bar. You may need to measure the nav height and use a custom padding value that includes both the nav height andenv(safe-area-inset-bottom).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/components/DS/dialog.html.erb(1 hunks)app/views/layouts/application.html.erb(1 hunks)app/views/layouts/shared/_htmldoc.html.erb(1 hunks)
🧰 Additional context used
📓 Path-based instructions (19)
app/components/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
app/components/**/*: Prefer ViewComponents for reusable or complex UI components; keep domain logic out of view templates.
Logic belongs in component files, not in component template (*.html.erb, *.html.slim) files.
Files:
app/components/DS/dialog.html.erb
**/*.{html,erb,slim,js,jsx,ts,tsx,css,scss}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Always use functional design tokens (e.g., text-primary, bg-container) from the design system; do not use raw colors or ad-hoc classes.
Files:
app/components/DS/dialog.html.erbapp/views/layouts/shared/_htmldoc.html.erbapp/views/layouts/application.html.erb
**/app/**/*.erb
📄 CodeRabbit inference engine (CLAUDE.md)
Always use the
iconhelper for icons in templates; never uselucide_icondirectly.
Files:
app/components/DS/dialog.html.erbapp/views/layouts/shared/_htmldoc.html.erbapp/views/layouts/application.html.erb
**/app/components/**/*
📄 CodeRabbit inference engine (CLAUDE.md)
**/app/components/**/*: Component logic should live in component files; keep domain logic out of view templates.
Prefer components over partials when available.
Files:
app/components/DS/dialog.html.erb
**/app/**/*.{rb,erb,js,css}
📄 CodeRabbit inference engine (CLAUDE.md)
Push Rails to its limits before adding new dependencies; a strong technical/business reason is required for new dependencies.
Files:
app/components/DS/dialog.html.erbapp/views/layouts/shared/_htmldoc.html.erbapp/views/layouts/application.html.erb
**/*.{rb,erb,haml,slim}
📄 CodeRabbit inference engine (.cursor/rules/general-rules.mdc)
**/*.{rb,erb,haml,slim}: UseCurrent.userfor the current user; do not usecurrent_user
UseCurrent.familyfor the current family; do not usecurrent_family
Ignore i18n methods; hardcode strings in English for now (do not useI18n.t,t, or similar)
Files:
app/components/DS/dialog.html.erbapp/views/layouts/shared/_htmldoc.html.erbapp/views/layouts/application.html.erb
{app/javascript/controllers/**/*.{js,ts},app/views/**/*.erb,app/components/**/*.erb}
📄 CodeRabbit inference engine (.cursor/rules/stimulus_conventions.mdc)
Use declarative Stimulus actions in ERB (data-action) instead of imperative event listeners in controller lifecycle (e.g., avoid addEventListener in connect); controllers should just respond to actions
Files:
app/components/DS/dialog.html.erbapp/views/layouts/shared/_htmldoc.html.erbapp/views/layouts/application.html.erb
{app/components/**/*.{js,ts,erb},app/views/**/*.erb}
📄 CodeRabbit inference engine (.cursor/rules/stimulus_conventions.mdc)
Component-scoped Stimulus controllers in app/components must be used only within their component views, not in app/views
Files:
app/components/DS/dialog.html.erbapp/views/layouts/shared/_htmldoc.html.erbapp/views/layouts/application.html.erb
{app/views,app/components}/**/*.html.erb
📄 CodeRabbit inference engine (.cursor/rules/view_conventions.mdc)
{app/views,app/components}/**/*.html.erb: Keep domain logic out of ERB templates; compute values in component/controller code and reference them in the template
Integrate Stimulus declaratively in ERB: templates declare data-controller/data-action; controllers respond to those declarations
Pass data from Rails to Stimulus via data-*-value attributes, not inline JavaScript
Files:
app/components/DS/dialog.html.erbapp/views/layouts/shared/_htmldoc.html.erbapp/views/layouts/application.html.erb
app/views/**/*.erb
📄 CodeRabbit inference engine (AGENTS.md)
app/views/**/*.erb: Keep heavy logic out of ERB views; prefer helpers/components instead
ERB templates are linted by erb-lint per .erb_lint.ymlAlways use the icon helper (icon) for icons; never call lucide_icon directly
Files:
app/views/layouts/shared/_htmldoc.html.erbapp/views/layouts/application.html.erb
app/views/**/*.html.*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
app/views/**/*.html.*: Use partials only for simple, context-specific, mostly static HTML content.
Prefer semantic HTML; use Turbo Frames where possible instead of client-side solutions.
Use query params for state, not localStorage or sessionStorage.
Always perform server-side formatting for currencies, numbers, and dates.
Files:
app/views/layouts/shared/_htmldoc.html.erbapp/views/layouts/application.html.erb
**/app/views/**/*.erb
📄 CodeRabbit inference engine (CLAUDE.md)
**/app/views/**/*.erb: Keep domain logic OUT of view templates; logic belongs in component files, not templates.
Use partials only for primarily static, simple HTML with minimal logic, and only when not likely to be reused.
Prefer native HTML over JavaScript components for UI elements (e.g., use<dialog>,<details><summary>).
Leverage Turbo frames for page sections over client-side JavaScript solutions.
Use query params (not localStorage/sessions) for client state management.
Perform server-side formatting for currencies, numbers, and dates in templates.
Files:
app/views/layouts/shared/_htmldoc.html.erbapp/views/layouts/application.html.erb
app/views/**/*.html.erb
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
app/views/**/*.html.erb: Prefer native HTML elements (e.g., ,) over JS-based components
Leverage Turbo frames to break up pages instead of JS-driven client-side solutions
Prefer native client-side form validation when possible
Files:
app/views/layouts/shared/_htmldoc.html.erbapp/views/layouts/application.html.erb
app/views/layouts/**/*.erb
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Be mindful of performance in global layouts (e.g., avoid loading large data payloads)
Files:
app/views/layouts/shared/_htmldoc.html.erbapp/views/layouts/application.html.erb
app/{models,controllers,views}/**/*.{rb,erb}
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Avoid N+1 queries
Files:
app/views/layouts/shared/_htmldoc.html.erbapp/views/layouts/application.html.erb
{app/views/**,app/helpers/**,app/javascript/controllers/**}
📄 CodeRabbit inference engine (.cursor/rules/ui-ux-design-guidelines.mdc)
{app/views/**,app/helpers/**,app/javascript/controllers/**}: Style UI using TailwindCSS v4.x with the custom design system defined in app/assets/tailwind/maybe-design-system.css
Always start by referencing app/assets/tailwind/maybe-design-system.css to understand available primitives, functional tokens, and component tokens before styling
Prefer functional tokens from the design system over raw Tailwind values (e.g., use text-primary, bg-container, border border-primary instead of text-white, bg-white, border-gray-200)
Files:
app/views/layouts/shared/_htmldoc.html.erbapp/views/layouts/application.html.erb
{app/views/**,app/helpers/**}
📄 CodeRabbit inference engine (.cursor/rules/ui-ux-design-guidelines.mdc)
Always generate semantic HTML
Files:
app/views/layouts/shared/_htmldoc.html.erbapp/views/layouts/application.html.erb
app/views/**/_*.html.erb
📄 CodeRabbit inference engine (.cursor/rules/view_conventions.mdc)
Name partials with a leading underscore (e.g., _trend_change.html.erb, _form_errors.html.erb)
Files:
app/views/layouts/shared/_htmldoc.html.erb
app/views/layouts/application.html.erb
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Use Turbo frames in the application layout to load controller actions as demonstrated
Files:
app/views/layouts/application.html.erb
🧠 Learnings (2)
📚 Learning: 2025-09-23T22:21:06.637Z
Learnt from: CR
PR: we-promise/sure#0
File: .cursor/rules/project-conventions.mdc:0-0
Timestamp: 2025-09-23T22:21:06.637Z
Learning: Applies to app/views/**/*.html.erb : Prefer native HTML elements (e.g., <dialog>, <details><summary>) over JS-based components
Applied to files:
app/components/DS/dialog.html.erbapp/views/layouts/shared/_htmldoc.html.erb
📚 Learning: 2025-09-20T08:42:15.420Z
Learnt from: CR
PR: we-promise/sure#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-20T08:42:15.420Z
Learning: Applies to **/app/views/**/*.erb : Prefer native HTML over JavaScript components for UI elements (e.g., use `<dialog>`, `<details><summary>`).
Applied to files:
app/components/DS/dialog.html.erbapp/views/layouts/shared/_htmldoc.html.erb
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci / test
🔇 Additional comments (2)
app/components/DS/dialog.html.erb (1)
2-2: LGTM! Safe-area insets properly applied for PWA compatibility.The addition of
pt-[env(safe-area-inset-top)]andpb-[env(safe-area-inset-bottom)]ensures dialogs respect device notches and system UI on iOS devices, preventing content from being obscured. This aligns well with the PWA objectives.app/views/layouts/shared/_htmldoc.html.erb (1)
10-10: Unable to locatebg-surface’s color value; confirm it resolves to#000as requested.
|
|
||
| <%# MOBILE - Bottom Nav %> | ||
| <%= tag.nav class: "lg:hidden bg-surface shrink-0 z-10 pb-[env(safe-area-inset-bottom)] border-t border-tertiary flex justify-around" do %> | ||
| <%= tag.nav class: "lg:hidden bg-surface fixed bottom-0 left-0 z-10 w-full shrink-0 pb-[env(safe-area-inset-bottom)] border-t border-tertiary flex justify-around" do %> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify that main content has sufficient bottom padding to prevent overlap with fixed navigation.
The transition to fixed positioning (fixed bottom-0 left-0 w-full) is appropriate for PWA mobile navigation. However, when navigation is fixed, the main content area should have bottom padding equal to the navigation height plus safe-area inset to prevent content from being obscured.
🏁 Script executed:
#!/bin/bash
# Description: Check if the main content area has appropriate bottom padding to account for the fixed bottom navigation.
# Search for the main content area and check for bottom padding
rg -n "app_layout_target.*content" app/views/layouts/application.html.erb -A 5 -B 5
# Also check if there are any mobile-specific bottom padding classes
rg -n "pb-\[|pb-[0-9]" app/views/layouts/application.html.erbLength of output: 933
Add bottom padding to <main> to offset the fixed bottom nav.
In application.html.erb:114, the <main> only uses py-4; include a class like pb-[calc(3rem+env(safe-area-inset-bottom))] (or appropriate value) to prevent content from being obscured on mobile.
🤖 Prompt for AI Agents
In app/views/layouts/application.html.erb around lines 114 and 159, the fixed
bottom nav at line 159 can obscure page content because <main> only has py-4;
add a bottom padding class to <main> such as
pb-[calc(3rem+env(safe-area-inset-bottom))] (or an appropriate value) so the
page content is offset above the fixed nav; update the <main> element’s class
list to include that pb-... utility and verify mobile safe-area inset is
respected.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/views/budgets/show.html.erb(1 hunks)app/views/pages/dashboard.html.erb(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/views/budgets/show.html.erb
🧰 Additional context used
📓 Path-based instructions (14)
app/views/**/*.erb
📄 CodeRabbit inference engine (AGENTS.md)
app/views/**/*.erb: Keep heavy logic out of ERB views; prefer helpers/components instead
ERB templates are linted by erb-lint per .erb_lint.ymlAlways use the icon helper (icon) for icons; never call lucide_icon directly
Files:
app/views/pages/dashboard.html.erb
app/views/**/*.html.*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
app/views/**/*.html.*: Use partials only for simple, context-specific, mostly static HTML content.
Prefer semantic HTML; use Turbo Frames where possible instead of client-side solutions.
Use query params for state, not localStorage or sessionStorage.
Always perform server-side formatting for currencies, numbers, and dates.
Files:
app/views/pages/dashboard.html.erb
**/*.{html,erb,slim,js,jsx,ts,tsx,css,scss}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Always use functional design tokens (e.g., text-primary, bg-container) from the design system; do not use raw colors or ad-hoc classes.
Files:
app/views/pages/dashboard.html.erb
**/app/**/*.erb
📄 CodeRabbit inference engine (CLAUDE.md)
Always use the
iconhelper for icons in templates; never uselucide_icondirectly.
Files:
app/views/pages/dashboard.html.erb
**/app/views/**/*.erb
📄 CodeRabbit inference engine (CLAUDE.md)
**/app/views/**/*.erb: Keep domain logic OUT of view templates; logic belongs in component files, not templates.
Use partials only for primarily static, simple HTML with minimal logic, and only when not likely to be reused.
Prefer native HTML over JavaScript components for UI elements (e.g., use<dialog>,<details><summary>).
Leverage Turbo frames for page sections over client-side JavaScript solutions.
Use query params (not localStorage/sessions) for client state management.
Perform server-side formatting for currencies, numbers, and dates in templates.
Files:
app/views/pages/dashboard.html.erb
**/app/**/*.{rb,erb,js,css}
📄 CodeRabbit inference engine (CLAUDE.md)
Push Rails to its limits before adding new dependencies; a strong technical/business reason is required for new dependencies.
Files:
app/views/pages/dashboard.html.erb
**/*.{rb,erb,haml,slim}
📄 CodeRabbit inference engine (.cursor/rules/general-rules.mdc)
**/*.{rb,erb,haml,slim}: UseCurrent.userfor the current user; do not usecurrent_user
UseCurrent.familyfor the current family; do not usecurrent_family
Ignore i18n methods; hardcode strings in English for now (do not useI18n.t,t, or similar)
Files:
app/views/pages/dashboard.html.erb
app/views/**/*.html.erb
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
app/views/**/*.html.erb: Prefer native HTML elements (e.g., ,) over JS-based components
Leverage Turbo frames to break up pages instead of JS-driven client-side solutions
Prefer native client-side form validation when possible
Files:
app/views/pages/dashboard.html.erb
app/{models,controllers,views}/**/*.{rb,erb}
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Avoid N+1 queries
Files:
app/views/pages/dashboard.html.erb
{app/javascript/controllers/**/*.{js,ts},app/views/**/*.erb,app/components/**/*.erb}
📄 CodeRabbit inference engine (.cursor/rules/stimulus_conventions.mdc)
Use declarative Stimulus actions in ERB (data-action) instead of imperative event listeners in controller lifecycle (e.g., avoid addEventListener in connect); controllers should just respond to actions
Files:
app/views/pages/dashboard.html.erb
{app/components/**/*.{js,ts,erb},app/views/**/*.erb}
📄 CodeRabbit inference engine (.cursor/rules/stimulus_conventions.mdc)
Component-scoped Stimulus controllers in app/components must be used only within their component views, not in app/views
Files:
app/views/pages/dashboard.html.erb
{app/views/**,app/helpers/**,app/javascript/controllers/**}
📄 CodeRabbit inference engine (.cursor/rules/ui-ux-design-guidelines.mdc)
{app/views/**,app/helpers/**,app/javascript/controllers/**}: Style UI using TailwindCSS v4.x with the custom design system defined in app/assets/tailwind/maybe-design-system.css
Always start by referencing app/assets/tailwind/maybe-design-system.css to understand available primitives, functional tokens, and component tokens before styling
Prefer functional tokens from the design system over raw Tailwind values (e.g., use text-primary, bg-container, border border-primary instead of text-white, bg-white, border-gray-200)
Files:
app/views/pages/dashboard.html.erb
{app/views/**,app/helpers/**}
📄 CodeRabbit inference engine (.cursor/rules/ui-ux-design-guidelines.mdc)
Always generate semantic HTML
Files:
app/views/pages/dashboard.html.erb
{app/views,app/components}/**/*.html.erb
📄 CodeRabbit inference engine (.cursor/rules/view_conventions.mdc)
{app/views,app/components}/**/*.html.erb: Keep domain logic out of ERB templates; compute values in component/controller code and reference them in the template
Integrate Stimulus declaratively in ERB: templates declare data-controller/data-action; controllers respond to those declarations
Pass data from Rails to Stimulus via data-*-value attributes, not inline JavaScript
Files:
app/views/pages/dashboard.html.erb
🧠 Learnings (2)
📚 Learning: 2025-09-23T22:21:06.637Z
Learnt from: CR
PR: we-promise/sure#0
File: .cursor/rules/project-conventions.mdc:0-0
Timestamp: 2025-09-23T22:21:06.637Z
Learning: Applies to app/views/**/*.html.erb : Leverage Turbo frames to break up pages instead of JS-driven client-side solutions
Applied to files:
app/views/pages/dashboard.html.erb
📚 Learning: 2025-09-23T22:21:06.637Z
Learnt from: CR
PR: we-promise/sure#0
File: .cursor/rules/project-conventions.mdc:0-0
Timestamp: 2025-09-23T22:21:06.637Z
Learning: Applies to app/views/layouts/application.html.erb : Use Turbo frames in the application layout to load controller actions as demonstrated
Applied to files:
app/views/pages/dashboard.html.erb
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci / test
| <% end %> | ||
|
|
||
| <div class="w-full space-y-6 pb-24"> | ||
| <div class="w-full space-y-6 pb-20"> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify bottom padding accounts for both fixed mobile nav and safe area insets.
The padding reduction from pb-24 to pb-20 (80px) may not properly account for iOS safe area insets on devices with home indicators. According to the AI summary, the mobile bottom nav is now fixed-positioned, but this static padding doesn't include env(safe-area-inset-bottom).
Consider whether the padding should dynamically account for safe area insets:
-<div class="w-full space-y-6 pb-20">
+<div class="w-full space-y-6 pb-[calc(5rem+env(safe-area-inset-bottom))]">Run this script to check how the mobile bottom nav is implemented and whether it accounts for safe areas:
🏁 Script executed:
#!/bin/bash
# Description: Check mobile bottom nav implementation for safe area handling
# Find the mobile bottom nav in layouts
rg -n -A 5 -B 5 "bottom.*nav|mobile.*nav" app/views/layouts/
# Check for safe-area-inset usage in the codebase
rg -n "safe-area-inset" app/views/ app/assets/Length of output: 3627
Include safe-area inset in bottom padding.
Replace the static pb-20 with a calc utility that adds your 5 rem content offset to the iOS safe-area inset:
-<div class="w-full space-y-6 pb-20">
+<div class="w-full space-y-6 pb-[calc(5rem+env(safe-area-inset-bottom))]">📝 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.
| <div class="w-full space-y-6 pb-20"> | |
| <div class="w-full space-y-6 pb-[calc(5rem+env(safe-area-inset-bottom))]"> |
🤖 Prompt for AI Agents
In app/views/pages/dashboard.html.erb around line 26, the container uses a
static bottom padding class `pb-20`; replace it with a padding that adds your
5rem content offset to the iOS safe-area inset so the bottom spacing uses
calc(5rem + env(safe-area-inset-bottom)) — if using Tailwind, swap the class for
a custom utility (e.g. pb-[calc(5rem+env(safe-area-inset-bottom))]) or add an
inline style/pattern that sets padding-bottom: calc(5rem +
env(safe-area-inset-bottom))) to ensure the safe-area inset is included.
1 participant
Attempting to fix PWA issues as per #69.
Fixes #149.
Summary by CodeRabbit