Conversation
Signed-off-by: ragnep <ragneinfo@gmail.com>
📝 WalkthroughWalkthroughAdds a brain-activity feature: new React component and heatmap UI, view-model and heatmap-layout helpers, a viewport hook, a React Query hook for identity activity, integration into the user page, and comprehensive unit tests covering loading, error, success, and helper logic. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UserPageBrainActivity
participant HK as useIdentityActivity Hook
participant API as Backend API
participant VM as buildViewModel
participant HM as Heatmap UI
UI->>UI: derive identity from profile (handle or primary_wallet)
UI->>HK: call hook with identity & enabled=true
HK->>HK: normalize & validate identity
HK->>API: GET /identities/{identity}/activity
API-->>HK: return activity response
HK-->>UI: deliver query state (loading | error | success)
alt loading
UI->>HM: render loading skeleton
else error
UI->>HM: render error message
else success
UI->>VM: build view model from response
VM-->>UI: return view model (cells, totals, labels)
UI->>HM: render heatmap with cells and tooltips
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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: 1
🧹 Nitpick comments (2)
components/user/brain/UserPageBrainActivity.tsx (1)
157-165: Redundant identity check after hook call.The
identityvariable is derived fromgetProfileWaveIdentity(profile)and is used to gate theuseIdentityActivityhook viaenabled: identity.length > 0. However, Line 163 checksif (!identity)which can never be true sincegetProfileWaveIdentityalways returns a non-null string (fromprofile.handle ?? profile.query ?? profile.primary_wallet).If the intention is to handle edge cases where all three properties are empty strings, the check should be
if (identity.length === 0)for consistency with theenabledcondition.♻️ Suggested fix for consistency
- if (!identity) { + if (identity.length === 0) { return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/user/brain/UserPageBrainActivity.tsx` around lines 157 - 165, The `identity` value comes from getProfileWaveIdentity(profile) and is already used to enable useIdentityActivity via enabled: identity.length > 0, so the subsequent truthy check if (!identity) is incorrect; replace that check with if (identity.length === 0) (or remove it and rely on the hook’s enabled flag) so the condition matches the same empty-string semantics used for useIdentityActivity; update the check near where identity is defined and where useIdentityActivity is called to reference identity consistently.__tests__/components/user/brain/UserPageDrops.test.tsx (1)
27-31: Test assertion may not match implementation behavior.The test asserts that
brain-activity-cardis null whenprofileisnull. This is correct since the component returnsnullwhenprofileis falsy. However, the test name "hides Drops when no profile handle" is slightly misleading as it now also tests the BrainActivity component. Consider updating the test name to reflect the broader scope.♻️ Suggested name update
- it("hides Drops when no profile handle", () => { + it("renders nothing when profile is null", () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/user/brain/UserPageDrops.test.tsx` around lines 27 - 31, The test name is misleading because it asserts that multiple elements (brain-activity-card, drops, brain-sidebar) are absent when rendering UserPageDrops with profile={null}; rename the test to reflect the broader behavior (e.g., change the it(...) string to "hides Drops and BrainActivity components when no profile handle" or similar) so it matches the assertions against UserPageDrops, brain-activity-card, drops, and brain-sidebar.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hooks/useIdentityActivity.ts`:
- Around line 19-29: The query key lowercases normalizedIdentity but the API
call in commonApiFetch uses the original normalizedIdentity, causing a case
mismatch; update the endpoint construction in the useQuery queryFn so that
commonApiFetch's endpoint uses the lowercased normalizedIdentity (i.e., call
toLowerCase() on normalizedIdentity before encodeURIComponent) to match the
queryKey behavior used in useQuery/UserPageBrainActivityResponse and align with
the pattern in useIdentity.ts.
---
Nitpick comments:
In `@__tests__/components/user/brain/UserPageDrops.test.tsx`:
- Around line 27-31: The test name is misleading because it asserts that
multiple elements (brain-activity-card, drops, brain-sidebar) are absent when
rendering UserPageDrops with profile={null}; rename the test to reflect the
broader behavior (e.g., change the it(...) string to "hides Drops and
BrainActivity components when no profile handle" or similar) so it matches the
assertions against UserPageDrops, brain-activity-card, drops, and brain-sidebar.
In `@components/user/brain/UserPageBrainActivity.tsx`:
- Around line 157-165: The `identity` value comes from
getProfileWaveIdentity(profile) and is already used to enable
useIdentityActivity via enabled: identity.length > 0, so the subsequent truthy
check if (!identity) is incorrect; replace that check with if (identity.length
=== 0) (or remove it and rely on the hook’s enabled flag) so the condition
matches the same empty-string semantics used for useIdentityActivity; update the
check near where identity is defined and where useIdentityActivity is called to
reference identity consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8337c020-08aa-408b-b186-2fa887aaa9b6
📒 Files selected for processing (8)
__tests__/components/user/brain/UserPageBrainActivity.test.tsx__tests__/components/user/brain/UserPageDrops.test.tsx__tests__/components/user/brain/userPageBrainActivity.helpers.test.tscomponents/react-query-wrapper/ReactQueryWrapper.tsxcomponents/user/brain/UserPageBrainActivity.tsxcomponents/user/brain/UserPageDrops.tsxcomponents/user/brain/userPageBrainActivity.helpers.tshooks/useIdentityActivity.ts
There was a problem hiding this comment.
🧹 Nitpick comments (3)
components/user/brain/UserPageBrainActivityHeatmap.tsx (2)
145-152: Simplify conditional expression.The ternary can be simplified using the logical OR operator.
♻️ Proposed fix
{DAY_LABELS.map((label, index) => ( <div key={`${label || "empty"}-${index}`} className="tw-flex tw-items-center tw-justify-end" > - {label ? label : <span className="tw-opacity-0">Sun</span>} + {label || <span className="tw-opacity-0">Sun</span>} </div> ))}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/user/brain/UserPageBrainActivityHeatmap.tsx` around lines 145 - 152, The map rendering of DAY_LABELS uses a ternary to show label or a placeholder span; simplify by replacing the ternary with a logical OR so the element renders label || <span className="tw-opacity-0">Sun</span>. Update the map callback in UserPageBrainActivityHeatmap (the DAY_LABELS.map(...) block and its key generation) to use the logical OR expression for the content while keeping the same wrapper div and key.
430-454: Use.datasetinstead ofgetAttribute()and add intensity upper bound check.Per static analysis, prefer
.datasetfor data attribute access. Additionally, the intensity value is cast without verifying it's within the valid range (0-4).♻️ Proposed fix
render={({ activeAnchor }) => { - const countValue = Number( - activeAnchor?.getAttribute("data-tooltip-count") - ); - const isoDate = activeAnchor?.getAttribute("data-tooltip-date"); + const countValue = Number(activeAnchor?.dataset.tooltipCount); + const isoDate = activeAnchor?.dataset.tooltipDate; if (!isoDate || Number.isNaN(countValue)) { return null; } - const intensityValue = Number( - activeAnchor?.getAttribute("data-tooltip-intensity") - ); + const intensityValue = Number(activeAnchor?.dataset.tooltipIntensity); const indicatorCell: UserPageBrainActivityCell = { key: "tooltip", isoDate: isoDate, count: countValue, ariaLabel: null, state: countValue > 0 ? "active" : "empty", intensity: - Number.isNaN(intensityValue) || intensityValue < 0 + Number.isNaN(intensityValue) || intensityValue < 0 || intensityValue > 4 ? 0 : (intensityValue as UserPageBrainActivityCell["intensity"]), };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/user/brain/UserPageBrainActivityHeatmap.tsx` around lines 430 - 454, In the render function where activeAnchor is read to build indicatorCell in UserPageBrainActivityHeatmap.tsx, replace getAttribute(...) calls with the DOM dataset access (e.g., activeAnchor?.dataset.tooltipCount, .tooltipDate, .tooltipIntensity), parse the numeric values safely (Number or parseInt) and guard against NaN, and enforce the intensity upper and lower bounds so intensity becomes 0 if NaN or <0, and is clamped to 4 if >4 before casting to UserPageBrainActivityCell["intensity"]; keep the same key/aria/state logic but use the dataset-derived isoDate and countValue.components/user/brain/userPageBrainActivity.helpers.ts (1)
236-242: Hardcoded period label may not reflect actual data range.The
periodLabelis hardcoded to "the last 12 months", but the actual period depends ondate_samples.length. If the API returns a different range (e.g., 6 months or 18 months), this label would be misleading.Consider deriving the label from the actual date range or documenting the API contract that guarantees 12 months of data.
🔧 Proposed fix to derive period label from actual data
+ const dayCount = normalizedSamples.length; + const monthCount = Math.round(dayCount / 30); + const periodLabel = monthCount <= 1 + ? "the last month" + : `the last ${monthCount} months`; + return { - periodLabel: "the last 12 months", + periodLabel, totalDrops, weekCount: cells.length / WEEKDAY_COUNT, monthLabels, cells, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/user/brain/userPageBrainActivity.helpers.ts` around lines 236 - 242, The periodLabel is hardcoded to "the last 12 months" but should reflect the actual range of the data; update the return to compute periodLabel dynamically (using the date_samples array if available, or deriving months from cells and WEEKDAY_COUNT) — e.g., build a human-readable label like "the last N months" or a date range using the first/last dates from date_samples; modify the code around periodLabel in the same function (referencing periodLabel, date_samples, cells, WEEKDAY_COUNT, monthLabels) so the label is generated from the real data instead of being hardcoded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@components/user/brain/userPageBrainActivity.helpers.ts`:
- Around line 236-242: The periodLabel is hardcoded to "the last 12 months" but
should reflect the actual range of the data; update the return to compute
periodLabel dynamically (using the date_samples array if available, or deriving
months from cells and WEEKDAY_COUNT) — e.g., build a human-readable label like
"the last N months" or a date range using the first/last dates from
date_samples; modify the code around periodLabel in the same function
(referencing periodLabel, date_samples, cells, WEEKDAY_COUNT, monthLabels) so
the label is generated from the real data instead of being hardcoded.
In `@components/user/brain/UserPageBrainActivityHeatmap.tsx`:
- Around line 145-152: The map rendering of DAY_LABELS uses a ternary to show
label or a placeholder span; simplify by replacing the ternary with a logical OR
so the element renders label || <span className="tw-opacity-0">Sun</span>.
Update the map callback in UserPageBrainActivityHeatmap (the DAY_LABELS.map(...)
block and its key generation) to use the logical OR expression for the content
while keeping the same wrapper div and key.
- Around line 430-454: In the render function where activeAnchor is read to
build indicatorCell in UserPageBrainActivityHeatmap.tsx, replace
getAttribute(...) calls with the DOM dataset access (e.g.,
activeAnchor?.dataset.tooltipCount, .tooltipDate, .tooltipIntensity), parse the
numeric values safely (Number or parseInt) and guard against NaN, and enforce
the intensity upper and lower bounds so intensity becomes 0 if NaN or <0, and is
clamped to 4 if >4 before casting to UserPageBrainActivityCell["intensity"];
keep the same key/aria/state logic but use the dataset-derived isoDate and
countValue.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 663e532a-6405-48cf-bd72-795115db8541
📒 Files selected for processing (7)
__tests__/components/user/brain/UserPageBrainActivity.test.tsx__tests__/components/user/brain/userPageBrainActivity.helpers.test.tscomponents/user/brain/UserPageBrainActivity.tsxcomponents/user/brain/UserPageBrainActivityHeatmap.tsxcomponents/user/brain/UserPageBrainSidebarWaveItem.tsxcomponents/user/brain/userPageBrainActivity.helpers.tscomponents/waves/drops/WaveDrop.tsx
✅ Files skipped from review due to trivial changes (1)
- components/waves/drops/WaveDrop.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/components/user/brain/UserPageBrainActivity.test.tsx
- tests/components/user/brain/userPageBrainActivity.helpers.test.ts
Signed-off-by: ragnep <ragneinfo@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
components/user/brain/userPageBrainActivityHeatmap.viewport.ts (1)
126-134: Consider potential stale closure in viewport subscription.The
subscribeToViewportcallback readsviewportRef.currentat subscription time, but since it has an empty dependency array,useSyncExternalStorewon't resubscribe if the viewport element changes after initial mount (e.g., due to conditional rendering without aresetKeychange).If this component is always mounted when needed, this is likely fine. Otherwise, consider adding the viewport ref to the subscription logic or using a dependency that triggers resubscription when the element changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/user/brain/userPageBrainActivityHeatmap.viewport.ts` around lines 126 - 134, The subscription callback subscribeToViewport captures viewportRef.current once because its dependency array is empty, so useSyncExternalStore won't resubscribe if the actual DOM element changes; fix this by making the subscription recreate when the element changes—either include the current element in the callback deps (e.g., useCallback((onStoreChange)=> subscribeToViewportMetrics(viewportRef.current, onStoreChange), [viewportRef.current])) or introduce a resetKey that you update when the viewport element mounts and include that key in the deps so useSyncExternalStore (and subscribeToViewport) resubscribes; keep getViewportSnapshot and EMPTY_VIEWPORT_METRICS the same.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@components/user/brain/userPageBrainActivityHeatmap.viewport.ts`:
- Around line 126-134: The subscription callback subscribeToViewport captures
viewportRef.current once because its dependency array is empty, so
useSyncExternalStore won't resubscribe if the actual DOM element changes; fix
this by making the subscription recreate when the element changes—either include
the current element in the callback deps (e.g., useCallback((onStoreChange)=>
subscribeToViewportMetrics(viewportRef.current, onStoreChange),
[viewportRef.current])) or introduce a resetKey that you update when the
viewport element mounts and include that key in the deps so useSyncExternalStore
(and subscribeToViewport) resubscribes; keep getViewportSnapshot and
EMPTY_VIEWPORT_METRICS the same.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5524375e-3da7-4c44-bae4-5c4c6fb310ca
📒 Files selected for processing (6)
__tests__/components/user/brain/UserPageBrainActivity.test.tsxcomponents/user/brain/UserPageBrainActivityHeatmap.tsxcomponents/user/brain/userPageBrainActivity.helpers.tscomponents/user/brain/userPageBrainActivityHeatmap.helpers.tscomponents/user/brain/userPageBrainActivityHeatmap.viewport.tshooks/useIdentityActivity.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/components/user/brain/UserPageBrainActivity.test.tsx
Signed-off-by: ragnep <ragneinfo@gmail.com>
|
simo6529
approved these changes
Mar 17, 2026
There was a problem hiding this comment.
🧹 Nitpick comments (2)
__tests__/components/user/brain/userPageBrainActivityHeatmap.viewport.test.tsx (1)
4-34: LGTM! Consider expanding test coverage.The test correctly validates the snap-to-latest behavior. Consider adding tests for:
- Initial render without a viewport ref (returns
EMPTY_VIEWPORT_METRICS)- Viewport metrics updates on scroll events
- Cleanup behavior when component unmounts
These would increase confidence in edge cases but are optional given the current test validates the primary use case.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/user/brain/userPageBrainActivityHeatmap.viewport.test.tsx` around lines 4 - 34, Add additional unit tests around useHeatmapViewport to cover the suggested edge cases: add a test that calls the hook without assigning viewportRef and asserts it returns EMPTY_VIEWPORT_METRICS; add a test that simulates scroll events on the viewport element and asserts the viewport metrics (scrollLeft/scrollTop/clientWidth/scrollHeight) update accordingly via the hook; and add a test that unmounts the hook after assigning viewportRef and verifies event listeners are cleaned up (e.g., no updates after unmount). Use the existing test patterns around useHeatmapViewport and viewportRef to locate where to add these cases.components/user/brain/UserPageBrainActivity.tsx (1)
74-98: Consider logging when view model construction fails for debugging.When
buildUserPageBrainActivityViewModelreturnsnull(due to invaliddate_samplesor unparseablelast_date), the component silently displays "No activity in last 12 months." This is a reasonable fallback, but adding a debug log whenactivityQuery.dataexists butactivityisnullwould aid troubleshooting malformed API responses.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/user/brain/UserPageBrainActivity.tsx` around lines 74 - 98, When activityQuery.data is present but buildUserPageBrainActivityViewModel(activityQuery.data) returns null, add a debug log to surface the malformed API response; check for activityQuery.data && activity == null and log the raw activityQuery.data (or specific problematic fields like date_samples/last_date) along with a clear message. Insert this check after the activity variable is constructed in UserPageBrainActivity.tsx (referencing buildUserPageBrainActivityViewModel, activityQuery.data, and activity) and use the existing logging mechanism (or console.debug/console.warn) so developers can inspect the invalid payload during debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@__tests__/components/user/brain/userPageBrainActivityHeatmap.viewport.test.tsx`:
- Around line 4-34: Add additional unit tests around useHeatmapViewport to cover
the suggested edge cases: add a test that calls the hook without assigning
viewportRef and asserts it returns EMPTY_VIEWPORT_METRICS; add a test that
simulates scroll events on the viewport element and asserts the viewport metrics
(scrollLeft/scrollTop/clientWidth/scrollHeight) update accordingly via the hook;
and add a test that unmounts the hook after assigning viewportRef and verifies
event listeners are cleaned up (e.g., no updates after unmount). Use the
existing test patterns around useHeatmapViewport and viewportRef to locate where
to add these cases.
In `@components/user/brain/UserPageBrainActivity.tsx`:
- Around line 74-98: When activityQuery.data is present but
buildUserPageBrainActivityViewModel(activityQuery.data) returns null, add a
debug log to surface the malformed API response; check for activityQuery.data &&
activity == null and log the raw activityQuery.data (or specific problematic
fields like date_samples/last_date) along with a clear message. Insert this
check after the activity variable is constructed in UserPageBrainActivity.tsx
(referencing buildUserPageBrainActivityViewModel, activityQuery.data, and
activity) and use the existing logging mechanism (or console.debug/console.warn)
so developers can inspect the invalid payload during debugging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f8f2ff2e-7a67-4425-90ce-a8c97ce3acf4
📒 Files selected for processing (9)
__tests__/components/user/brain/UserPageBrainActivity.test.tsx__tests__/components/user/brain/userPageBrainActivity.helpers.test.ts__tests__/components/user/brain/userPageBrainActivityHeatmap.viewport.test.tsxcomponents/user/brain/UserPageBrainActivity.tsxcomponents/user/brain/UserPageBrainActivityHeatmap.tsxcomponents/user/brain/UserPageBrainSidebarWaveItem.tsxcomponents/user/brain/userPageBrainActivity.helpers.tscomponents/user/brain/userPageBrainActivityHeatmap.helpers.tscomponents/user/brain/userPageBrainActivityHeatmap.viewport.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/components/user/brain/userPageBrainActivity.helpers.test.ts
- tests/components/user/brain/UserPageBrainActivity.test.tsx
- components/user/brain/UserPageBrainActivityHeatmap.tsx
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
2 participants
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
Tests
Style