Uh oh!
There was an error while loading. Please reload this page.
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 (1)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughReplaced "rewards" terminology with "earnings" across UI strings, added discussion "new comments" keys and AI Assist hint, implemented polling and a new "load new comments" flow, added weekly-earnings notification types and USD fields (types + UI + WS/HTTP handling), and small wallet translation arg change. Changes
Sequence DiagramsequenceDiagram
participant UI as Discussion UI
participant QC as QueryClient
participant API as Post Header API
participant State as Component State
rect rgba(100,200,255,0.5)
Note over QC,API: Polling (every 30s)
QC->>API: fetch post header (children count)
API-->>QC: return header (children)
end
rect rgba(200,255,100,0.5)
Note over QC,State: Detect new comments
QC->>State: provide latest children count
State->>State: compare vs loadedCountRef -> set newCommentCount
end
rect rgba(255,200,100,0.5)
Note over UI,State: Render prompt
alt newCommentCount > 0
State->>UI: show "new comments" button
end
end
rect rgba(255,150,150,0.5)
Note over UI,QC: User loads new comments
UI->>QC: invalidate discussions query (on click)
QC->>API: refetch discussions
API-->>QC: updated comments
QC->>State: update allComments, reset loadedCountRef/newCommentCount
QC->>UI: refreshed comment list
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/web/src/features/shared/discussion/index.tsx (1)
175-185: Consider extracting the duplicated "load new comments" button into a reusable component.The button markup is duplicated at Lines 175-185 and 221-231 with identical styling and logic.
♻️ Proposed refactor: Extract shared button component
+ const NewCommentsButton = useMemo(() => (+ <button+ onClick={handleLoadNewComments}+ className="w-full my-3 py-2.5 px-4 rounded-lg bg-blue-dark-sky/10 dark:bg-blue-dark-sky/20 text-blue-dark-sky hover:bg-blue-dark-sky/20 dark:hover:bg-blue-dark-sky/30 transition-colors text-sm font-medium cursor-pointer text-center"+ >+ {i18next.t(+ newCommentCount === 1 ? "discussion.new-comment" : "discussion.new-comments",+ { n: newCommentCount }+ )}+ </button>+ ), [handleLoadNewComments, newCommentCount]);+ // Then use in both places: - {topLevelComments.length === 0 && newCommentCount > 0 && (- <button ...>...</button>- )}+ {topLevelComments.length === 0 && newCommentCount > 0 && NewCommentsButton} // and - {newCommentCount > 0 && (- <button ...>...</button>- )}+ {newCommentCount > 0 && NewCommentsButton}Also applies to: 221-231
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/features/shared/discussion/index.tsx` around lines 175 - 185, The duplicated "load new comments" button markup should be extracted into a reusable component (e.g., LoadNewCommentsButton) and used in both places instead of repeating JSX; create a component that accepts props: newCommentCount: number, onClick: () => void (and optional className or i18n key if needed), render the same i18next translation logic and styling, then replace the inline button instances that call handleLoadNewComments and reference newCommentCount with <LoadNewCommentsButton newCommentCount={newCommentCount} onClick={handleLoadNewComments} /> to remove duplication.
🤖 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/shared/discussion/index.tsx`:
- Around line 85-88: handleLoadNewComments currently calls
queryClient.invalidateQueries(...) and immediately calls setNewCommentCount(0),
causing a race; change it to await the invalidateQueries call (or use .then) and
only call setNewCommentCount(0) after invalidateQueries resolves for
discussionsQueryOptions.queryKey so the reset happens once invalidation
completes; update the handleLoadNewComments callback to be async (or return the
promise) and reference queryClient.invalidateQueries,
discussionsQueryOptions.queryKey, and setNewCommentCount when making this
change.
- Around line 62-83: The new-comment detection is using mismatched counts
(postHeader.children vs allComments.length); compute topLevelComments before the
effects and use topLevelComments.length consistently: initialize loadedCountRef
with parent.children or topLevelComments.length, change the postHeader effect to
compare postHeader.children (direct replies) against loadedCountRef.current
derived from topLevelComments.length, and update loadedCountRef.current =
topLevelComments.length (and setNewCommentCount(0)) in the refetch/reset effect;
alternatively, if you prefer keeping parent.children, update loadedCountRef only
after a successful refetch inside handleLoadNewComments so both comparisons use
the same metric (top-level count) and reference the symbols loadedCountRef,
postHeader, topLevelComments, allComments, and handleLoadNewComments.
---
Nitpick comments:
In `@apps/web/src/features/shared/discussion/index.tsx`:
- Around line 175-185: The duplicated "load new comments" button markup should
be extracted into a reusable component (e.g., LoadNewCommentsButton) and used in
both places instead of repeating JSX; create a component that accepts props:
newCommentCount: number, onClick: () => void (and optional className or i18n key
if needed), render the same i18next translation logic and styling, then replace
the inline button instances that call handleLoadNewComments and reference
newCommentCount with <LoadNewCommentsButton newCommentCount={newCommentCount}
onClick={handleLoadNewComments} /> to remove duplication.
🪄 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: 1a8c0851-382c-4b11-843c-d97f9afc62fc
📒 Files selected for processing (3)
apps/web/src/features/i18n/locales/en-US.jsonapps/web/src/features/shared/ai-assist/ai-assist-dialog.tsxapps/web/src/features/shared/discussion/index.tsx
Uh oh!
There was an error while loading. Please reload this page.
Comment on lines
+85
to
+88
| const handleLoadNewComments = useCallback(() => { | ||
| queryClient.invalidateQueries({ queryKey: discussionsQueryOptions.queryKey }); | ||
| setNewCommentCount(0); | ||
| }, [queryClient, discussionsQueryOptions.queryKey]); |
Contributor
There was a problem hiding this comment.
Potential race condition: setNewCommentCount(0) called before query invalidation completes.
The handleLoadNewComments callback resets newCommentCount to 0 immediately, but the query invalidation is asynchronous. If the user clicks the button multiple times rapidly, or if the reset effect at Line 78-83 fires before invalidation completes, the count could be inconsistent.
🛡️ Proposed fix: Reset count only after invalidation completes
const handleLoadNewComments = useCallback(() => {
- queryClient.invalidateQueries({ queryKey: discussionsQueryOptions.queryKey });- setNewCommentCount(0);+ queryClient.invalidateQueries({ queryKey: discussionsQueryOptions.queryKey })+ .then(() => {+ setNewCommentCount(0);+ });
}, [queryClient, discussionsQueryOptions.queryKey]);📝 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
| consthandleLoadNewComments=useCallback(()=>{ | |
| queryClient.invalidateQueries({queryKey: discussionsQueryOptions.queryKey}); | |
| setNewCommentCount(0); | |
| },[queryClient,discussionsQueryOptions.queryKey]); | |
| consthandleLoadNewComments=useCallback(()=>{ | |
| queryClient.invalidateQueries({queryKey: discussionsQueryOptions.queryKey}) | |
| .then(()=>{ | |
| setNewCommentCount(0); | |
| }); | |
| },[queryClient,discussionsQueryOptions.queryKey]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/features/shared/discussion/index.tsx` around lines 85 - 88,
handleLoadNewComments currently calls queryClient.invalidateQueries(...) and
immediately calls setNewCommentCount(0), causing a race; change it to await the
invalidateQueries call (or use .then) and only call setNewCommentCount(0) after
invalidateQueries resolves for discussionsQueryOptions.queryKey so the reset
happens once invalidation completes; update the handleLoadNewComments callback
to be async (or return the promise) and reference queryClient.invalidateQueries,
discussionsQueryOptions.queryKey, and setNewCommentCount when making this
change.
feruzm
marked this pull request as draft
March 26, 2026 12:26
feruzm
marked this pull request as ready for review
March 26, 2026 12:26
feruzm
marked this pull request as draft
March 26, 2026 12:34
feruzm
marked this pull request as ready for review
March 26, 2026 12:34
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/web/src/features/shared/discussion/index.tsx (2)
82-94:⚠️ Potential issue | 🟠 MajorThe new-comment baseline is still using the wrong count.
Line 91 resets
loadedCountRefwithallComments.length, butpostHeader.childrenis the direct-reply count. On any thread with nested replies, the badge will stop surfacing new top-level comments after the next refetch.Suggested fix
+ const topLevelComments = useMemo(+ () =>+ allComments.filter(+ (x) => x.parent_author === parent.author && x.parent_permlink === parent.permlink+ ),+ [allComments, parent.author, parent.permlink]+ );+ useEffect(() => { if (postHeader && postHeader.children > loadedCountRef.current) { setNewCommentCount(postHeader.children - loadedCountRef.current); } }, [postHeader]); // Reset baseline when discussions are refetched useEffect(() => { - if (allComments.length > 0) {- loadedCountRef.current = allComments.length;- setNewCommentCount(0);- }- }, [allComments.length]);+ loadedCountRef.current = topLevelComments.length;+ setNewCommentCount(0);+ }, [topLevelComments.length]);Once this is fixed, please add a nested-reply regression test for the indicator path. As per coding guidelines "All new features in
@ecency/webrequire tests".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/features/shared/discussion/index.tsx` around lines 82 - 94, The baseline for new-comment detection is being reset using allComments.length which counts nested replies; instead reset loadedCountRef.current to the top-level reply count (postHeader.children) so the badge tracks new direct replies correctly—update the effect that runs on allComments change to set loadedCountRef.current = postHeader?.children ?? loadedCountRef.current and keep setNewCommentCount(0); also add a regression test exercising the indicator with nested replies to ensure new top-level comments still trigger the badge (referencing loadedCountRef, setNewCommentCount, postHeader, and allComments).
96-102:⚠️ Potential issue | 🟡 MinorDon’t clear the badge before the refetch finishes.
setNewCommentCount(0)and the baseline advance happen beforeinvalidateQueriesresolves, so a slow or failed refetch can hide unseen comments permanently.Suggested fix
- const handleLoadNewComments = useCallback(() => {- if (postHeader) {- loadedCountRef.current = postHeader.children;- }- setNewCommentCount(0);- queryClient.invalidateQueries({ queryKey: discussionsQueryOptions.queryKey });- }, [queryClient, discussionsQueryOptions.queryKey, postHeader]);+ const handleLoadNewComments = useCallback(async () => {+ await queryClient.invalidateQueries({ queryKey: discussionsQueryOptions.queryKey });+ }, [queryClient, discussionsQueryOptions.queryKey]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/features/shared/discussion/index.tsx` around lines 96 - 102, The handler handleLoadNewComments currently clears the badge and advances loadedCountRef before the refetch completes; change it to first trigger the refresh and only clear/update state after the refetch succeeds (or at least after invalidate completes). Call queryClient.invalidateQueries({ queryKey: discussionsQueryOptions.queryKey }) and in its promise continuation (then/catch) update loadedCountRef.current = postHeader.children and call setNewCommentCount(0) on success (and avoid clearing on failure or handle errors), leaving setNewCommentCount and loadedCountRef updates tied to the resolution of invalidateQueries rather than before it.
🧹 Nitpick comments (2)
apps/web/src/features/shared/discussion/index.tsx (1)
37-45: ExtractNewCommentsButtonprops into an interface.This is the only new inline object shape in the file; a named interface keeps it aligned with the repo’s TypeScript convention.
Suggested cleanup
-function NewCommentsButton({ count, onClick }: { count: number; onClick: () => void }) {+interface NewCommentsButtonProps {+ count: number;+ onClick: () => void;+}++function NewCommentsButton({ count, onClick }: NewCommentsButtonProps) {As per coding guidelines "Prefer
interfacefor defining object shapes in TypeScript".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/features/shared/discussion/index.tsx` around lines 37 - 45, Extract the inline prop type for NewCommentsButton into a named interface: create an interface NewCommentsButtonProps { count: number; onClick: () => void } and then update the component signature from function NewCommentsButton({ count, onClick }: { count: number; onClick: () => void }) to function NewCommentsButton(props: NewCommentsButtonProps) or function NewCommentsButton({ count, onClick }: NewCommentsButtonProps); keep the same prop names (count, onClick) and usage inside the component.apps/web/src/entities/ws-notifications.ts (1)
145-152: Optional cleanup: extract shared USD breakdown shape to avoid drift.Both weekly-earnings interfaces repeat the same USD fields. A small shared interface keeps them synchronized over time.
♻️ Proposed refactor
+interface WeeklyEarningsUsdBreakdown {+ total_usd?: string;+ author_usd?: string;+ curation_usd?: string;+}+ export interface WsWeeklyEarningsNotification extends BaseWsNotification { type: "weekly_earnings"; extra?: { - total_usd?: string;- author_usd?: string;- curation_usd?: string;- };+ } & WeeklyEarningsUsdBreakdown; } export interface ApiWeeklyEarningsNotification extends BaseAPiNotification { type: "weekly_earnings"; - total_usd?: string;- author_usd?: string;- curation_usd?: string;-}+} & WeeklyEarningsUsdBreakdown;Also applies to: 311-316
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/entities/ws-notifications.ts` around lines 145 - 152, Extract the repeated USD fields into a single shared interface (e.g., WeeklyEarningsUsdBreakdown or UsdBreakdown) and replace the inline extra? structures in WsWeeklyEarningsNotification and the other weekly-earnings interface (the one with the same total_usd/author_usd/curation_usd fields) to reference that shared type; update any import/exports or usages to use the new interface name so both notifications stay synchronized.
🤖 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/shared/notifications/notification-types/notification-weekly-earnings-type.tsx`:
- Around line 10-22: Add component tests for NotificationWeeklyEarningsType to
cover both message branches: render the component with a notification object
that includes total_usd, author_usd, and curation_usd to assert the full
"notification.weekly-earnings" output, and render it with only total_usd
(author_usd or curation_usd missing/undefined) to assert the fallback
"notification.weekly-earnings-short" output. Use the same i18next mocking used
across the codebase (or mock i18next.t) and React Testing Library to render
NotificationWeeklyEarningsType and assert the expected translated text appears
when hasBreakdown (the hasBreakdown logic checking notification.author_usd &&
notification.curation_usd) is true and when it is false. Ensure tests live
alongside the component tests and name them to indicate both branches are
covered.
---
Duplicate comments:
In `@apps/web/src/features/shared/discussion/index.tsx`:
- Around line 82-94: The baseline for new-comment detection is being reset using
allComments.length which counts nested replies; instead reset
loadedCountRef.current to the top-level reply count (postHeader.children) so the
badge tracks new direct replies correctly—update the effect that runs on
allComments change to set loadedCountRef.current = postHeader?.children ??
loadedCountRef.current and keep setNewCommentCount(0); also add a regression
test exercising the indicator with nested replies to ensure new top-level
comments still trigger the badge (referencing loadedCountRef,
setNewCommentCount, postHeader, and allComments).
- Around line 96-102: The handler handleLoadNewComments currently clears the
badge and advances loadedCountRef before the refetch completes; change it to
first trigger the refresh and only clear/update state after the refetch succeeds
(or at least after invalidate completes). Call queryClient.invalidateQueries({
queryKey: discussionsQueryOptions.queryKey }) and in its promise continuation
(then/catch) update loadedCountRef.current = postHeader.children and call
setNewCommentCount(0) on success (and avoid clearing on failure or handle
errors), leaving setNewCommentCount and loadedCountRef updates tied to the
resolution of invalidateQueries rather than before it.
---
Nitpick comments:
In `@apps/web/src/entities/ws-notifications.ts`:
- Around line 145-152: Extract the repeated USD fields into a single shared
interface (e.g., WeeklyEarningsUsdBreakdown or UsdBreakdown) and replace the
inline extra? structures in WsWeeklyEarningsNotification and the other
weekly-earnings interface (the one with the same
total_usd/author_usd/curation_usd fields) to reference that shared type; update
any import/exports or usages to use the new interface name so both notifications
stay synchronized.
In `@apps/web/src/features/shared/discussion/index.tsx`:
- Around line 37-45: Extract the inline prop type for NewCommentsButton into a
named interface: create an interface NewCommentsButtonProps { count: number;
onClick: () => void } and then update the component signature from function
NewCommentsButton({ count, onClick }: { count: number; onClick: () => void }) to
function NewCommentsButton(props: NewCommentsButtonProps) or function
NewCommentsButton({ count, onClick }: NewCommentsButtonProps); keep the same
prop names (count, onClick) and usage inside the component.
🪄 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: 1b5e6b82-9441-48ab-a170-90fec7b12f24
📒 Files selected for processing (9)
apps/web/src/api/notifications-ws-api.tsapps/web/src/app/(dynamicPages)/profile/[username]/wallet/(token)/hbd/_components/hive-transaction-row.tsxapps/web/src/entities/ws-notifications.tsapps/web/src/features/i18n/locales/en-US.jsonapps/web/src/features/shared/discussion/index.tsxapps/web/src/features/shared/notifications/notification-list-item.tsxapps/web/src/features/shared/notifications/notification-types/notification-payouts-type.tsxapps/web/src/features/shared/notifications/notification-types/notification-weekly-earnings-type.tsxpackages/sdk/src/modules/notifications/types/notification.ts
Comment on lines
+10
to
+22
| export function NotificationWeeklyEarningsType({ sourceLink, notification }: Props) { | ||
| const totalUsd = notification.total_usd ?? "0"; | ||
| const authorUsd = notification.author_usd ?? "0"; | ||
| const curationUsd = notification.curation_usd ?? "0"; | ||
| const hasBreakdown = notification.author_usd && notification.curation_usd; | ||
| const message = hasBreakdown | ||
| ? i18next.t("notification.weekly-earnings", { | ||
| total: totalUsd, | ||
| author: authorUsd, | ||
| curation: curationUsd | ||
| }) | ||
| : i18next.t("notification.weekly-earnings-short", { total: totalUsd }); |
Contributor
There was a problem hiding this comment.
Add coverage for both weekly-earnings message branches.
This renderer now has two user-visible paths (notification.weekly-earnings vs notification.weekly-earnings-short), but the change set doesn’t include a spec for either branch. A small component test here would lock down the fallback when the breakdown is missing.
As per coding guidelines "All new features in @ecency/web require tests".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/web/src/features/shared/notifications/notification-types/notification-weekly-earnings-type.tsx`
around lines 10 - 22, Add component tests for NotificationWeeklyEarningsType to
cover both message branches: render the component with a notification object
that includes total_usd, author_usd, and curation_usd to assert the full
"notification.weekly-earnings" output, and render it with only total_usd
(author_usd or curation_usd missing/undefined) to assert the fallback
"notification.weekly-earnings-short" output. Use the same i18next mocking used
across the codebase (or mock i18next.t) and React Testing Library to render
NotificationWeeklyEarningsType and assert the expected translated text appears
when hasBreakdown (the hasBreakdown logic checking notification.author_usd &&
notification.curation_usd) is true and when it is false. Ensure tests live
alongside the component tests and name them to indicate both branches are
covered.
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 freeto 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
UI Updates