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 selected for processing (1)
📝 WalkthroughWalkthroughRefactors many barrel imports into feature-specific modules, removes numerous Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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 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}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/web/src/specs/features/landing-page.spec.tsx (1)
39-47: Keep the real@/utilshandlers in this suite.Replacing
handleInvalidandhandleOnInputwith no-ops means the form tests can still pass if the component wiring to those handlers breaks. Only the external boundaries need mocking here.As per coding guidelines, "Mock external dependencies with `vi.fn()`, not internal functions."♻️ Proposed fix
-// Extend the global `@/utils` mock with landing-page-specific exports -vi.mock("@/utils", async (importOriginal) => { - const actual = await importOriginal<any>(); - return { - ...actual, - handleInvalid: vi.fn(), - handleOnInput: vi.fn() - }; -});🤖 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 39 - 47, The test suite currently overrides internal utility handlers by returning handleInvalid and handleOnInput as vi.fn() inside the vi.mock("@/utils", async (importOriginal) => { ... }) block; remove those two overrides so the mock spreads actual and does not replace handleInvalid or handleOnInput (keep only external-only mocks if needed) so the real handlers are used in landing-page.spec.tsx while still using importOriginal/actual for other mocked exports.
🤖 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/app/_components/landing-page/index.tsx`:
- Around line 109-117: Replace hardcoded English alt strings in AssetPicture
usages (e.g., the instance with basePath="illustration-earn-money") by sourcing
meaningful labels through the i18n translator (t('...')) and add those new keys
to en-US.json; for purely decorative assets (fish, bubble, phone, etc.) set
alt="" and aria-hidden="true" on the AssetPicture so screen readers ignore them.
Update every affected AssetPicture occurrence called out in the review
(including the blocks around lines with basePath values and other listed ranges)
to follow this pattern and ensure only meaningful labels are localized via
t(...) with entries added to en-US.json.
- Around line 83-85: The LandingPage server component currently calls the shared
initI18next() which reads from ls and memoizes process-wide state, causing
locale leaks across requests; replace that call and the process-wide i18next.t
usage with a request-scoped translator (e.g., create or get a server translator
instance for this request) or move translation to the client: remove
initI18next() and i18next.t.bind(i18next) from LandingPage and instead call the
request-scoped function you provide (e.g.,
getServerTranslator/getTranslatorForRequest or createI18nInstance) and use its t
method so each request gets its own locale-aware translator.
In `@apps/web/src/specs/features/landing-page.spec.tsx`:
- Around line 69-77: The test "scrolls to earn-money section on click" spies on
document.getElementById via vi.spyOn but never restores the original
implementation; update the test to capture the spy (e.g. const spy =
vi.spyOn(document, "getElementById").mockReturnValue(mockElement as any)) and
call spy.mockRestore() at the end of the test (or add a global afterEach(() =>
vi.restoreAllMocks()) for the file) so document.getElementById is restored and
does not leak into other tests involving LandingHeroActions or
mockScrollIntoView.
---
Nitpick comments:
In `@apps/web/src/specs/features/landing-page.spec.tsx`:
- Around line 39-47: The test suite currently overrides internal utility
handlers by returning handleInvalid and handleOnInput as vi.fn() inside the
vi.mock("@/utils", async (importOriginal) => { ... }) block; remove those two
overrides so the mock spreads actual and does not replace handleInvalid or
handleOnInput (keep only external-only mocks if needed) so the real handlers are
used in landing-page.spec.tsx while still using importOriginal/actual for other
mocked exports.
🪄 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: 64a1d093-15c1-46d5-8209-85ab9836b0c1
📒 Files selected for processing (3)
apps/web/src/app/(dynamicPages)/entry/[category]/[author]/[permlink]/page.tsxapps/web/src/app/_components/landing-page/index.tsxapps/web/src/specs/features/landing-page.spec.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/app/(dynamicPages)/entry/[category]/[author]/[permlink]/page.tsx
Comment on lines
+83
to
+85
| export async function LandingPage() { | ||
| await initI18next(); | ||
| const t = i18next.t.bind(i18next); |
Contributor
There was a problem hiding this comment.
Don't bootstrap translations from the shared client initializer here.
LandingPage is now an async server component, but initI18next() still resolves the language from ls.get(...) and memoizes a process-wide promise in apps/web/src/features/i18n/index.ts. That means this render can't reliably honor the current visitor's locale and can reuse the first initialized language for later requests. Please switch this to a request-scoped/server translator or keep the translation work on the client.
🤖 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 83 - 85,
The LandingPage server component currently calls the shared initI18next() which
reads from ls and memoizes process-wide state, causing locale leaks across
requests; replace that call and the process-wide i18next.t usage with a
request-scoped translator (e.g., create or get a server translator instance for
this request) or move translation to the client: remove initI18next() and
i18next.t.bind(i18next) from LandingPage and instead call the request-scoped
function you provide (e.g., getServerTranslator/getTranslatorForRequest or
createI18nInstance) and use its t method so each request gets its own
locale-aware translator.
Comment on lines
+109
to
117
| <AssetPicture | ||
| basePath="illustration-earn-money" | ||
| alt="earn-money" | ||
| width={373} | ||
| height={442} | ||
| priority | ||
| fetchPriority="high" | ||
| sizes="(max-width: 640px) 280px, (max-width: 768px) 320px, (max-width: 1024px) 360px, 373px" | ||
| className="mx-auto sm:m-0" | ||
| /> |
Contributor
There was a problem hiding this comment.
Localize the new image labels, and hide decorative art from assistive tech.
These new alt values are hardcoded English strings, and many of them are purely decorative (fish, bubble, phone). That adds noisy screen-reader output and bypasses the page's i18n path. Use alt=""/aria-hidden="true" for decorative assets, and source the meaningful labels from t(...).
As per coding guidelines, "All new strings must be added to en-US.json only for internationalization."
Also applies to: 141-148, 159-166, 191-198, 209-257, 287-292, 320-328
🤖 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 109 - 117,
Replace hardcoded English alt strings in AssetPicture usages (e.g., the instance
with basePath="illustration-earn-money") by sourcing meaningful labels through
the i18n translator (t('...')) and add those new keys to en-US.json; for purely
decorative assets (fish, bubble, phone, etc.) set alt="" and aria-hidden="true"
on the AssetPicture so screen readers ignore them. Update every affected
AssetPicture occurrence called out in the review (including the blocks around
lines with basePath values and other listed ranges) to follow this pattern and
ensure only meaningful labels are localized via t(...) with entries added to
en-US.json.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/web/src/app/_components/landing-page/index.tsx (1)
83-85:⚠️ Potential issue | 🟠 MajorUse a request-scoped translator in this Server Component.
initI18next()+ singletoni18next.t.bind(i18next)in a server render path can leak locale state across requests. Please switch to a request-scoped server translator (or keep translation on client-only boundaries).#!/bin/bash # Verify global i18n init + language mutation pattern and server usage set -e echo "== LandingPage server usage ==" sed -n '70,110p' apps/web/src/app/_components/landing-page/index.tsx echo echo "== i18n initializer behavior ==" sed -n '100,180p' apps/web/src/features/i18n/index.ts🤖 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 83 - 85, LandingPage currently calls the global initI18next() and then uses singleton i18next.t.bind(i18next), which can leak locale state across requests; instead obtain a request-scoped translator instance and use its t method (e.g. have initI18next return a per-request i18n instance or call i18next.createInstance()/cloneInstance() inside LandingPage) and replace i18next.t.bind(i18next) with the returnedInstance.t (bound to that instance), ensuring you do not mutate the global i18next.language or other shared state.
🧹 Nitpick comments (2)
apps/web/src/specs/features/landing-page.spec.tsx (1)
9-23: Avoidanyin new test mocks under strict mode.Please replace explicit
anyusages with concrete mock/state types to keep type-safety intact.As per coding guidelines, "TypeScript strict mode is enabled; all new code should include proper types".♻️ Suggested typing cleanup
+interface MockGlobalStoreState { + theme: string; + toggleUiProp: (value: string) => void; +} + vi.mock("@/core/global-store", () => ({ - useGlobalStore: vi.fn((selector: any) => { - const state = { + useGlobalStore: vi.fn((selector: (state: MockGlobalStoreState) => unknown) => { + const state: MockGlobalStoreState = { theme: mockTheme, toggleUiProp: mockToggleUiProp }; return selector(state); }) })); vi.mock("@ecency/sdk", async (importOriginal) => { - const actual = await importOriginal<any>(); - return { ...actual, subscribeEmail: (...args: any[]) => mockSubscribeEmail(...args) }; + const actual = await importOriginal<typeof import("@ecency/sdk")>(); + return { ...actual, subscribeEmail: (...args: unknown[]) => mockSubscribeEmail(...args) }; });🤖 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 9 - 23, Replace the explicit any types in the test mocks by using the real state and function types: for the useGlobalStore mock (the vi.fn call), type the selector parameter as the actual global state selector signature (e.g., (s: GlobalState) => R) and import or declare the concrete GlobalState shape used by the app so the returned state object matches that type; for mockSubscribeEmail, replace the any args and return type with the real subscribeEmail function signature (import the type or use ReturnType/Parameters<typeof subscribeEmail> when mocking in the vi.mock callback) and type mockSubscribeEmail accordingly; ensure the vi.mock(importOriginal) wrapper preserves the module types (use the module’s exported subscribeEmail type) so no any types remain.apps/web/src/app/_components/landing-page/index.tsx (1)
79-80: Prefer aninterfaceforSvgAssetprops.Inline object type here conflicts with the TS guideline used in this repo.
As per coding guidelines, "Prefer `interface` for defining object shapes in TypeScript".♻️ Suggested refactor
+interface SvgAssetProps { + path: string; + alt: string; + className?: string; +} + -function SvgAsset({ path, alt, className }: { path: string; alt: string; className?: string }) { +function SvgAsset({ path, alt, className }: SvgAssetProps) { return <img src={`${defaults.base}/assets/${path}`} alt={alt} className={className} loading="lazy" />; }🤖 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 79 - 80, Replace the inline prop type for the SvgAsset component with a named interface: create an interface (e.g., SvgAssetProps) describing path: string, alt: string, className?: string and update the function signature to use SvgAssetProps; ensure the component name SvgAsset and its usage remain unchanged and that the new interface is exported if the props type is reused elsewhere.
🤖 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/app/_components/landing-page/index.tsx`:
- Around line 172-173: All anchor/Link elements that use target="_blank" (e.g.,
the Link element rendering t("landing-page.hive-blockchain")) must include
rel="noopener noreferrer" to prevent reverse-tabnabbing; update the Link
components in this module (also the other occurrences around the block at lines
~373-390) to add rel="noopener noreferrer" alongside target="_blank" so every
external new-tab link uses both attributes.
In `@apps/web/src/specs/features/landing-page.spec.tsx`:
- Around line 40-46: The test currently mocks internal validation utilities by
returning mocked handleInvalid and handleOnInput in the vi.mock for "@/utils";
remove those mocked properties so the mock returns only ...actual (or remove the
vi.mock entirely if not stubbing externals) and let the real handleInvalid and
handleOnInput run; update or delete any assertions that referenced these mocks
and keep mocking reserved for true external dependencies only (referencing
vi.mock, handleInvalid, handleOnInput, and "@/utils" to locate the change).
---
Duplicate comments:
In `@apps/web/src/app/_components/landing-page/index.tsx`:
- Around line 83-85: LandingPage currently calls the global initI18next() and
then uses singleton i18next.t.bind(i18next), which can leak locale state across
requests; instead obtain a request-scoped translator instance and use its t
method (e.g. have initI18next return a per-request i18n instance or call
i18next.createInstance()/cloneInstance() inside LandingPage) and replace
i18next.t.bind(i18next) with the returnedInstance.t (bound to that instance),
ensuring you do not mutate the global i18next.language or other shared state.
---
Nitpick comments:
In `@apps/web/src/app/_components/landing-page/index.tsx`:
- Around line 79-80: Replace the inline prop type for the SvgAsset component
with a named interface: create an interface (e.g., SvgAssetProps) describing
path: string, alt: string, className?: string and update the function signature
to use SvgAssetProps; ensure the component name SvgAsset and its usage remain
unchanged and that the new interface is exported if the props type is reused
elsewhere.
In `@apps/web/src/specs/features/landing-page.spec.tsx`:
- Around line 9-23: Replace the explicit any types in the test mocks by using
the real state and function types: for the useGlobalStore mock (the vi.fn call),
type the selector parameter as the actual global state selector signature (e.g.,
(s: GlobalState) => R) and import or declare the concrete GlobalState shape used
by the app so the returned state object matches that type; for
mockSubscribeEmail, replace the any args and return type with the real
subscribeEmail function signature (import the type or use
ReturnType/Parameters<typeof subscribeEmail> when mocking in the vi.mock
callback) and type mockSubscribeEmail accordingly; ensure the
vi.mock(importOriginal) wrapper preserves the module types (use the module’s
exported subscribeEmail type) so no any types remain.
🪄 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: b023a801-e87e-4a12-8e33-918fb4568961
📒 Files selected for processing (2)
apps/web/src/app/_components/landing-page/index.tsxapps/web/src/specs/features/landing-page.spec.tsx
Comment on lines
+40
to
+46
| vi.mock("@/utils", async (importOriginal) => { | ||
| const actual = await importOriginal<any>(); | ||
| return { | ||
| ...actual, | ||
| handleInvalid: vi.fn(), | ||
| handleOnInput: vi.fn() | ||
| }; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/web/src/specs/features/landing-page.spec.tsx | head -60Repository: ecency/vision-next
Length of output: 2603
🏁 Script executed:
# Search for usage of handleInvalid and handleOnInput in the test file
rg "handleInvalid|handleOnInput" apps/web/src/specs/features/landing-page.spec.tsx -A 2 -B 2Repository: ecency/vision-next
Length of output: 151
🏁 Script executed:
# Also check what these utilities do - find their definitions
rg "export.*handleInvalid|export.*handleOnInput" apps/web/src/utils/ -A 3Repository: ecency/vision-next
Length of output: 446
🏁 Script executed:
# Get the full test file to see actual test cases and assertions
cat -n apps/web/src/specs/features/landing-page.spec.tsx | tail -n +55Repository: ecency/vision-next
Length of output: 6074
🏁 Script executed:
# Also check if the component under test uses these utilities
rg "handleInvalid|handleOnInput" apps/web/src/app/_components/landing-page/ -lRepository: ecency/vision-next
Length of output: 130
🏁 Script executed:
cat -n apps/web/src/app/_components/landing-page/landing-subscribe-form.tsxRepository: ecency/vision-next
Length of output: 1957
Remove mocking of internal form validation utilities.
The handleInvalid and handleOnInput utilities are internal application code for form validation—not external dependencies. Mocking them couples tests to implementation details without adding coverage (the test assertions never reference these mocks).
Per the learnings: mock external dependencies only. Test the actual validation behavior if needed, or remove these unused mocks.
Suggested change
-vi.mock("@/utils", async (importOriginal) => {
- const actual = await importOriginal<any>();
- return {
- ...actual,
- handleInvalid: vi.fn(),
- handleOnInput: vi.fn()
- };
-});📝 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
| vi.mock("@/utils", async (importOriginal) => { | |
| const actual = await importOriginal<any>(); | |
| return { | |
| ...actual, | |
| handleInvalid: vi.fn(), | |
| handleOnInput: vi.fn() | |
| }; | |
| // Mock removed - use actual implementations or remove unused imports |
🤖 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 40 - 46, The
test currently mocks internal validation utilities by returning mocked
handleInvalid and handleOnInput in the vi.mock for "@/utils"; remove those
mocked properties so the mock returns only ...actual (or remove the vi.mock
entirely if not stubbing externals) and let the real handleInvalid and
handleOnInput run; update or delete any assertions that referenced these mocks
and keep mocking reserved for true external dependencies only (referencing
vi.mock, handleInvalid, handleOnInput, and "@/utils" to locate the change).
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
Performance Improvements
Refactor
Tests