Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (6)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds PWA install support: a new usePwaInstall hook and helpers capture beforeinstallprompt and appinstalled, expose install controls, integrate an install CTA in landing page and a route-scoped banner, update manifest/Next PWA config and metadata, and add tests and i18n strings. Changes
Sequence DiagramsequenceDiagram
participant User
participant Browser
participant App as App (client-init)
participant Hook as usePwaInstall
participant UI as UI Components
participant Storage as localStorage
Note over Browser,App: initial page load
Browser->>App: beforeinstallprompt (browser event)
App->>Hook: global listener captures event
Hook->>Hook: store deferredPrompt, set canInstall=true
Hook->>UI: expose canInstall/installed
User->>UI: clicks install CTA
UI->>Hook: call install()
Hook->>Browser: deferredPrompt.prompt()
Browser->>User: show install dialog
User-->>Browser: accept/dismiss
Browser->>Hook: userChoice result
Hook->>Hook: clear deferredPrompt, update installed/canInstall
alt dismissed
Hook->>Storage: write dismissal timestamp
end
Hook->>UI: update rendering (hide CTA / show installed state)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
apps/web/src/app/_components/landing-page/index.tsx (1)
361-363: Use route constants instead of a hardcoded/mobilepath.Please route this through the shared
src/routes.tsconstant to keep navigation paths centralized and safer for future route refactors.As per coding guidelines, "Use Next.js App Router with the routing architecture defined in src/routes.ts for route constants."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/_components/landing-page/index.tsx` around lines 361 - 363, Replace the hardcoded href="/mobile" on the Link in the landing-page component with the centralized route constant from src/routes.ts: import the exported routes object (or the named constant) and use routes.mobile (or the appropriate exported name) instead of the string; update the Link invocation (Link href={routes.mobile}) and add the import for the routes constant at the top of the module so navigation uses the shared route constant rather than a literal path.apps/web/src/specs/features/landing-page.spec.tsx (1)
181-189: Add one interaction test for the install-click path.Consider adding a case where
usePwaInstallis mocked withcanInstall: trueand asserting click behavior triggers install flow (instead of plain navigation fallback). This would cover the new user-visible branch more completely.As per coding guidelines, "Use React Testing Library with Vitest for component tests, testing user-visible behavior rather than implementation details."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/specs/features/landing-page.spec.tsx` around lines 181 - 189, Add an interaction test for LandingDownloadLinks that mocks usePwaInstall to return canInstall: true and a jest/vitest spy for onInstall, then render the component, simulate a user click on the PWA link (use userEvent) and assert the onInstall spy was called (and that no navigation fallback occurred). Specifically, mock the module that exports usePwaInstall to return { canInstall: true, install: mockInstall } (or named fields used in the component), import userEvent from `@testing-library/user-event`, render LandingDownloadLinks, find the link via screen.getByRole(..., { name: /landing-page\.install-web-app/ }), userEvent.click it, and expect the mockInstall toHaveBeenCalled (and optionally assert href still equals "/mobile" but click triggers install). Use Vitest mocks (vi.fn/vi.mock) and async assertions (await/await waitFor) as appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/features/banners/pwa-install-banner.tsx`:
- Line 11: Replace the unprefixed localStorage key by updating the DISMISSED_KEY
constant to use the app storage prefix (e.g. change "pwa-install-dismissed-at"
to "ecency_pwa-install-dismissed-at") so it matches the ls helper's
auto-prefixed convention; ensure all reads/writes in pwa-install-banner.tsx that
reference DISMISSED_KEY continue to use that constant so existing storage access
remains consistent.
- Around line 93-97: The icon-only dismiss Button in PwaInstallBanner lacks an
accessible name; update the Button (the one using onClick={handleDismiss} and
icon={<UilMultiply .../>}) to include an explicit accessible label (e.g.,
aria-label="Dismiss" or aria-label="Close install banner") or provide an
associated visually-hidden text element so screen readers can announce the
control; ensure the label text is clear and localized if applicable.
In `@apps/web/src/features/i18n/locales/en-US.json`:
- Line 2827: The string value for the "no-ls-description" locale key has awkward
grammar; update the value for "no-ls-description" to clearer, natural English
(e.g., "It seems you or your browser has disabled local data for our website.
Unfortunately, you won't be able to use many Ecency features such as drafts,
local decks, the advanced trading dashboard, and more. Please use the latest
version of a modern browser or enable local data in your browser settings.") so
punctuation/capitalization and phrasing are corrected and consistent with UI
tone.
---
Nitpick comments:
In `@apps/web/src/app/_components/landing-page/index.tsx`:
- Around line 361-363: Replace the hardcoded href="/mobile" on the Link in the
landing-page component with the centralized route constant from src/routes.ts:
import the exported routes object (or the named constant) and use routes.mobile
(or the appropriate exported name) instead of the string; update the Link
invocation (Link href={routes.mobile}) and add the import for the routes
constant at the top of the module so navigation uses the shared route constant
rather than a literal path.
In `@apps/web/src/specs/features/landing-page.spec.tsx`:
- Around line 181-189: Add an interaction test for LandingDownloadLinks that
mocks usePwaInstall to return canInstall: true and a jest/vitest spy for
onInstall, then render the component, simulate a user click on the PWA link (use
userEvent) and assert the onInstall spy was called (and that no navigation
fallback occurred). Specifically, mock the module that exports usePwaInstall to
return { canInstall: true, install: mockInstall } (or named fields used in the
component), import userEvent from `@testing-library/user-event`, render
LandingDownloadLinks, find the link via screen.getByRole(..., { name:
/landing-page\.install-web-app/ }), userEvent.click it, and expect the
mockInstall toHaveBeenCalled (and optionally assert href still equals "/mobile"
but click triggers install). Use Vitest mocks (vi.fn/vi.mock) and async
assertions (await/await waitFor) as appropriate.
🪄 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: CHILL
Plan: Pro
Run ID: 5839e1bb-71ce-4170-be53-ac184d063817
📒 Files selected for processing (15)
apps/web/next.config.jsapps/web/src/app/_components/landing-page/index.tsxapps/web/src/app/_components/landing-page/landing-download-links.tsxapps/web/src/app/client-init.tsxapps/web/src/app/layout.tsxapps/web/src/app/manifest.jsonapps/web/src/features/banners/index.tsxapps/web/src/features/banners/pwa-install-banner.tsxapps/web/src/features/i18n/locales/en-US.jsonapps/web/src/features/pwa-install/index.tsapps/web/src/features/pwa-install/is-ios-safari.tsapps/web/src/features/pwa-install/use-pwa-install.tsapps/web/src/specs/features/landing-page.spec.tsxapps/web/src/specs/features/pwa-install/is-ios-safari.spec.tsapps/web/src/specs/features/pwa-install/use-pwa-install.spec.ts
| import { useCallback, useEffect, useState } from "react"; | ||
| import { isIosSafari, usePwaInstall } from "@/features/pwa-install"; | ||
|
|
||
| const DISMISSED_KEY = "pwa-install-dismissed-at"; |
Contributor
There was a problem hiding this comment.
Use prefixed localStorage key for consistency with app storage conventions.
Line 11 uses an unprefixed key, which can drift from existing storage naming conventions.
💡 Suggested fix
-const DISMISSED_KEY = "pwa-install-dismissed-at";
+const DISMISSED_KEY = "ecency_pwa-install-dismissed-at";📝 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
| const DISMISSED_KEY = "pwa-install-dismissed-at"; | |
| const DISMISSED_KEY = "ecency_pwa-install-dismissed-at"; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/features/banners/pwa-install-banner.tsx` at line 11, Replace the
unprefixed localStorage key by updating the DISMISSED_KEY constant to use the
app storage prefix (e.g. change "pwa-install-dismissed-at" to
"ecency_pwa-install-dismissed-at") so it matches the ls helper's auto-prefixed
convention; ensure all reads/writes in pwa-install-banner.tsx that reference
DISMISSED_KEY continue to use that constant so existing storage access remains
consistent.
Comment on lines
+93
to
+97
| <Button | ||
| onClick={handleDismiss} | ||
| appearance="white-link" | ||
| icon={<UilMultiply className="w-4 h-4" />} | ||
| /> |
Contributor
There was a problem hiding this comment.
Add an accessible name to the dismiss icon button.
The dismiss button is icon-only and has no explicit text alternative, so screen readers won’t get a reliable control label.
💡 Suggested fix
<Button
onClick={handleDismiss}
appearance="white-link"
+ aria-label={i18next.t("banners.install-dismiss")}
+ title={i18next.t("banners.install-dismiss")}
icon={<UilMultiply className="w-4 h-4" />}
/>📝 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
| <Button | |
| onClick={handleDismiss} | |
| appearance="white-link" | |
| icon={<UilMultiply className="w-4 h-4" />} | |
| /> | |
| <Button | |
| onClick={handleDismiss} | |
| appearance="white-link" | |
| aria-label={i18next.t("banners.install-dismiss")} | |
| title={i18next.t("banners.install-dismiss")} | |
| icon={<UilMultiply className="w-4 h-4" />} | |
| /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/features/banners/pwa-install-banner.tsx` around lines 93 - 97,
The icon-only dismiss Button in PwaInstallBanner lacks an accessible name;
update the Button (the one using onClick={handleDismiss} and icon={<UilMultiply
.../>}) to include an explicit accessible label (e.g., aria-label="Dismiss" or
aria-label="Close install banner") or provide an associated visually-hidden text
element so screen readers can announce the control; ensure the label text is
clear and localized if applicable.
| "banners": { | ||
| "no-ls-title": "Unable to use local data", | ||
| "no-ls-description": "It seems You or your browser has disabled local data for our website. Unfortunately, You can't use the most of Ecency features such as drafts, local decks, advanced trading dashboard and more. Please, use latest versions of any modern browser or turn on local data manually." | ||
| "no-ls-description": "It seems You or your browser has disabled local data for our website. Unfortunately, You can't use the most of Ecency features such as drafts, local decks, advanced trading dashboard and more. Please, use latest versions of any modern browser or turn on local data manually.", |
Contributor
There was a problem hiding this comment.
Fix awkward grammar in local-data warning copy.
Line 2827 reads unnaturally (“can't use the most of Ecency features”), which reduces UI text quality.
💡 Suggested copy edit
- "no-ls-description": "It seems You or your browser has disabled local data for our website. Unfortunately, You can't use the most of Ecency features such as drafts, local decks, advanced trading dashboard and more. Please, use latest versions of any modern browser or turn on local data manually.",
+ "no-ls-description": "It seems you or your browser has disabled local data for our website. Unfortunately, you can't use most Ecency features such as drafts, local decks, advanced trading dashboard, and more. Please use the latest version of a modern browser or turn on local data manually.",📝 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
| "no-ls-description": "It seems You or your browser has disabled local data for our website. Unfortunately, You can't use the most of Ecency features such as drafts, local decks, advanced trading dashboard and more. Please, use latest versions of any modern browser or turn on local data manually.", | |
| "no-ls-description": "It seems you or your browser has disabled local data for our website. Unfortunately, you can't use most Ecency features such as drafts, local decks, advanced trading dashboard, and more. Please use the latest version of a modern browser or turn on local data manually.", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/features/i18n/locales/en-US.json` at line 2827, The string value
for the "no-ls-description" locale key has awkward grammar; update the value for
"no-ls-description" to clearer, natural English (e.g., "It seems you or your
browser has disabled local data for our website. Unfortunately, you won't be
able to use many Ecency features such as drafts, local decks, the advanced
trading dashboard, and more. Please use the latest version of a modern browser
or enable local data in your browser settings.") so punctuation/capitalization
and phrasing are corrected and consistent with UI tone.
feruzm
added
the
patch
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.
Summary by CodeRabbit
New Features
Configuration
Bug Fixes
Tests