Open
Conversation
ncarazon
force-pushed
the
feat/key-factors-3d-iteration
branch
from
9450f3f to
f7cf7ec
Compare
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThreaded Changes
Sequence DiagramsequenceDiagram
participant User
participant Chart as NumericChart / NumericTimeline
participant TimelineOverlay as TimelineNewsAnnotations
participant Marker as NewsAnnotationMarker
participant Floating as FloatingUI / FloatingPortal
participant Popup as NewsAnnotationPopup
User->>Chart: open timeline (with keyFactors)
Chart->>TimelineOverlay: provide positioned clusters, chartHeight, axis offset
User->>Marker: hover / click marker at xPixel
Marker->>TimelineOverlay: onHoverStart / onClick (cluster id)
TimelineOverlay->>Floating: compute placement (useFloating)
Floating-->>TimelineOverlay: position & merged styles
TimelineOverlay->>Popup: open popup for cluster (render up to 3 cards)
User->>Popup: interact (click key-factor / external link / outside)
Popup->>TimelineOverlay: close request (hover out / escape / outside click)
TimelineOverlay->>Marker: clear active cluster id
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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 |
Contributor
🚀 Preview EnvironmentYour preview environment is ready!
Details
ℹ️ Preview Environment InfoIsolation:
Limitations:
Cleanup:
|
cemreinanc
approved these changes
Mar 27, 2026
front_end/src/components/charts/primitives/news_annotations/news_annotation_toggle.tsx
Outdated
Show resolved
Hide resolved
front_end/src/components/charts/primitives/news_annotations/news_annotation_popup_card.tsx
Show resolved
Hide resolved
front_end/src/components/charts/primitives/news_annotations/news_annotation_popup_card.tsx
Outdated
Show resolved
Hide resolved
ncarazon
force-pushed
the
feat/key-factors-2nd-iteration
branch
2 times, most recently
from
d7a8288 to
d35a854
Compare
ncarazon
force-pushed
the
feat/key-factors-3d-iteration
branch
from
f7cf7ec to
daa579c
Compare
![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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
front_end/src/components/charts/primitives/chart_container.tsx (1)
50-51:⚠️ Potential issue | 🟠 Major
headerExtrais unintentionally hidden whenchartTitleandzoomare absent.On Line 50, the header is gated by
!!chartTitle || !!zoom, soheaderExtra-only usage never renders. IncludeheaderExtrain the outer condition to make the new slot reliable.Suggested fix
- {(!!chartTitle || !!zoom) && !isEmbed && ( + {(!!chartTitle || !!zoom || !!headerExtra) && !isEmbed && (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@front_end/src/components/charts/primitives/chart_container.tsx` around lines 50 - 51, The header block is currently only rendered when !!chartTitle || !!zoom and thus hides headerExtra-only usage; update the outer conditional that renders the header div (the JSX guarded by (!!chartTitle || !!zoom) && !isEmbed) to also check for headerExtra (e.g., (!!chartTitle || !!zoom || !!headerExtra) && !isEmbed) so that headerExtra is shown even if chartTitle and zoom are absent; adjust references to chartTitle, zoom, headerExtra, and isEmbed accordingly.
♻️ Duplicate comments (1)
front_end/src/components/charts/primitives/news_annotations/news_annotation_toggle.tsx (1)
24-24:⚠️ Potential issue | 🟡 MinorHardcoded
aria-labelshould use i18n.The
aria-label="Toggle news annotations"is a hardcoded English string. Per project conventions, useuseTranslations()to support localization.Based on learnings: "Do not hardcode English strings in TSX components... Prefer using useTranslations() and i18n strings for UI text across components."
🌐 Proposed fix for i18n
+"use client"; + +import { faNewspaper } from "@fortawesome/free-solid-svg-icons"; +import { FontAwesomeIcon } from "@fortawesome/react-fontawesome"; +import { useTranslations } from "next-intl"; +import { FC } from "react"; + +import cn from "@/utils/core/cn"; + const NewsAnnotationToggle: FC<Props> = ({ enabled, onToggle }) => { + const t = useTranslations(); return ( <button className={cn(...)} onClick={onToggle} - aria-label="Toggle news annotations" + aria-label={t("toggleNewsAnnotations")} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@front_end/src/components/charts/primitives/news_annotations/news_annotation_toggle.tsx` at line 24, The aria-label on the NewsAnnotationToggle component is hardcoded ("Toggle news annotations"); change it to use the i18n hook useTranslations and a translation key instead: import and call useTranslations within the NewsAnnotationToggle (or the component that renders the toggle), replace the static aria-label with the returned localized string (e.g., t('news.toggleAnnotations') or your project's key) so the rendered button/checkbox uses the translated label; update or add the corresponding i18n entry for that key in your locale files.
🧹 Nitpick comments (2)
front_end/src/components/charts/primitives/news_annotations/news_annotation_popup.tsx (1)
38-51: Consider showing a count indicator when annotations are truncated.When a cluster has more than 3 annotations, users see only the first 3 with no indication that more exist. Consider adding a "+N more" indicator or similar UI feedback.
💡 Optional: Add truncation indicator
const visibleAnnotations = cluster.annotations.slice( 0, MAX_VISIBLE_ANNOTATIONS ); + const hiddenCount = cluster.annotations.length - visibleAnnotations.length; return ( <div ref={ref} className="flex w-[280px] flex-col gap-[5px] rounded-xl p-0"> {visibleAnnotations.map((annotation) => ( <NewsAnnotationPopupCard key={annotation.keyFactor.id} annotation={annotation} questionType={questionType} /> ))} + {hiddenCount > 0 && ( + <div className="text-center text-xs text-gray-500"> + +{hiddenCount} more + </div> + )} </div> );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@front_end/src/components/charts/primitives/news_annotations/news_annotation_popup.tsx` around lines 38 - 51, The list currently slices cluster.annotations into visibleAnnotations (using MAX_VISIBLE_ANNOTATIONS) and renders NewsAnnotationPopupCard for each, but provides no indication when more exist; compute remainingCount = cluster.annotations.length - visibleAnnotations.length and, if remainingCount > 0, render a small truncation indicator (e.g., a "+{remainingCount} more" element) after the mapped NewsAnnotationPopupCard items inside the same container; ensure the indicator uses the same styling/spacing as the cards and include an accessible label (aria-label) so screen readers know there are additional annotations.front_end/src/components/charts/primitives/news_annotations/news_annotation_popup_card.tsx (1)
92-98: Consider addingloading="lazy"for thumbnail images.For better performance, especially when multiple annotation cards are rendered, lazy loading the thumbnail images can reduce initial load time.
⚡ Optional performance improvement
{imgUrl && ( <img className="size-10 rounded object-cover" src={imgUrl} alt={title} + loading="lazy" /> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@front_end/src/components/charts/primitives/news_annotations/news_annotation_popup_card.tsx` around lines 92 - 98, Add native lazy-loading to the thumbnail <img> rendered when imgUrl is present in the NewsAnnotationPopupCard component: update the JSX block that checks imgUrl (the element using src={imgUrl} alt={title} and className="size-10 rounded object-cover") to include loading="lazy" so the browser defers offscreen image decoding; keep existing props (src, alt, className) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@front_end/src/components/charts/primitives/news_annotations/news_annotation_marker.tsx`:
- Around line 21-35: The marker button in news_annotation_marker.tsx (the button
rendering inside the NewsAnnotationMarker component that uses props isActive,
count, and onClick) is missing an explicit type and accessible name/state;
update the <button> to include type="button", add an accessible name
(aria-label) that reflects its content (e.g., include count when count>1 and a
description when showing the newspaper icon), and expose its pressed state via
aria-pressed={isActive} so assistive tech can convey active/inactive; ensure the
aria-label text is derived from the existing count/isActive values and keep
FontAwesomeIcon usage unchanged.
In
`@front_end/src/components/charts/primitives/news_annotations/news_annotation_popup_card.tsx`:
- Around line 57-64: The interactive card uses a non-semantic div with only
onClick (see url and handleClick in NewsAnnotationPopupCard), which is not
keyboard-accessible; update the component so when url is present either wrap the
card in an anchor element or replace the div with a semantic interactive element
(e.g., <a> or <button>) and ensure keyboard handlers and accessibility
attributes are present: make it focusable (tabIndex if needed), handle
Enter/Space in onKeyDown to call handleClick, and add an appropriate
aria-label/role to describe the action.
In `@front_end/src/components/charts/primitives/news_annotations/utils.ts`:
- Around line 82-93: The current loop in the clusters spacing pass (using
clusters[], MIN_MARKER_GAP and xPixel) adjusts both prev.xPixel and curr.xPixel
by a midpoint which can break spacing with earlier neighbors; change it to a
monotonic forward push: iterate i from 1..clusters.length-1, compute prev =
clusters[i-1] and curr = clusters[i], and if curr.xPixel < prev.xPixel +
MIN_MARKER_GAP set curr.xPixel = prev.xPixel + MIN_MARKER_GAP (only move the
current cluster forward) so each update preserves the minimum gap with all
previous clusters.
- Around line 19-21: The timestamp computation uses new Date(news.published_at)
without validating the parsed date, which yields NaN for malformed strings and
breaks downstream math; modify the logic in the timestamp assignment to parse
news.published_at into a Date object, check validity with
isNaN(parsed.getTime()) (or Number.isNaN), and only use its Unix seconds when
valid; otherwise fall back to parsing kf.created_at (also validate it) and
finally fallback to Math.floor(Date.now()/1000) if both dates are invalid—update
the code around the timestamp constant and references to news.published_at and
kf.created_at accordingly.
---
Outside diff comments:
In `@front_end/src/components/charts/primitives/chart_container.tsx`:
- Around line 50-51: The header block is currently only rendered when
!!chartTitle || !!zoom and thus hides headerExtra-only usage; update the outer
conditional that renders the header div (the JSX guarded by (!!chartTitle ||
!!zoom) && !isEmbed) to also check for headerExtra (e.g., (!!chartTitle ||
!!zoom || !!headerExtra) && !isEmbed) so that headerExtra is shown even if
chartTitle and zoom are absent; adjust references to chartTitle, zoom,
headerExtra, and isEmbed accordingly.
---
Duplicate comments:
In
`@front_end/src/components/charts/primitives/news_annotations/news_annotation_toggle.tsx`:
- Line 24: The aria-label on the NewsAnnotationToggle component is hardcoded
("Toggle news annotations"); change it to use the i18n hook useTranslations and
a translation key instead: import and call useTranslations within the
NewsAnnotationToggle (or the component that renders the toggle), replace the
static aria-label with the returned localized string (e.g.,
t('news.toggleAnnotations') or your project's key) so the rendered
button/checkbox uses the translated label; update or add the corresponding i18n
entry for that key in your locale files.
---
Nitpick comments:
In
`@front_end/src/components/charts/primitives/news_annotations/news_annotation_popup_card.tsx`:
- Around line 92-98: Add native lazy-loading to the thumbnail <img> rendered
when imgUrl is present in the NewsAnnotationPopupCard component: update the JSX
block that checks imgUrl (the element using src={imgUrl} alt={title} and
className="size-10 rounded object-cover") to include loading="lazy" so the
browser defers offscreen image decoding; keep existing props (src, alt,
className) unchanged.
In
`@front_end/src/components/charts/primitives/news_annotations/news_annotation_popup.tsx`:
- Around line 38-51: The list currently slices cluster.annotations into
visibleAnnotations (using MAX_VISIBLE_ANNOTATIONS) and renders
NewsAnnotationPopupCard for each, but provides no indication when more exist;
compute remainingCount = cluster.annotations.length - visibleAnnotations.length
and, if remainingCount > 0, render a small truncation indicator (e.g., a
"+{remainingCount} more" element) after the mapped NewsAnnotationPopupCard items
inside the same container; ensure the indicator uses the same styling/spacing as
the cards and include an accessible label (aria-label) so screen readers know
there are additional annotations.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2bec821b-4d1e-4a1e-a227-e9f880486ec3
📒 Files selected for processing (16)
front_end/src/app/(main)/questions/[id]/components/question_layout/consumer_question_layout/index.tsxfront_end/src/app/(main)/questions/[id]/components/question_layout/question_info.tsxfront_end/src/app/(main)/questions/[id]/components/question_view/consumer_question_view/timeline/index.tsxfront_end/src/app/(main)/questions/[id]/components/question_view/forecaster_question_view/index.tsxfront_end/src/components/charts/numeric_chart.tsxfront_end/src/components/charts/numeric_timeline.tsxfront_end/src/components/charts/primitives/chart_container.tsxfront_end/src/components/charts/primitives/news_annotations/news_annotation_marker.tsxfront_end/src/components/charts/primitives/news_annotations/news_annotation_popup.tsxfront_end/src/components/charts/primitives/news_annotations/news_annotation_popup_card.tsxfront_end/src/components/charts/primitives/news_annotations/news_annotation_toggle.tsxfront_end/src/components/charts/primitives/news_annotations/timeline_news_annotations.tsxfront_end/src/components/charts/primitives/news_annotations/types.tsfront_end/src/components/charts/primitives/news_annotations/utils.tsfront_end/src/components/detailed_question_card/detailed_question_card/continuous_chart_card.tsxfront_end/src/components/detailed_question_card/detailed_question_card/index.tsx
front_end/src/components/charts/primitives/news_annotations/news_annotation_marker.tsx
Outdated
Show resolved
Hide resolved
Comment on lines
+57
to
+64
| return ( | ||
| <div | ||
| className={cn( | ||
| "dark flex gap-3 rounded-xl bg-blue-800 p-4 dark:bg-blue-300-dark", | ||
| url && "cursor-pointer" | ||
| )} | ||
| onClick={handleClick} | ||
| > |
Contributor
There was a problem hiding this comment.
Clickable div lacks keyboard accessibility.
The card is interactive when url is present but uses a div with only onClick. This is not accessible to keyboard users. Consider using a proper interactive element or adding ARIA attributes.
♿ Proposed fix for accessibility
return (
<div
className={cn(
"dark flex gap-3 rounded-xl bg-blue-800 p-4 dark:bg-blue-300-dark",
url && "cursor-pointer"
)}
onClick={handleClick}
+ role={url ? "button" : undefined}
+ tabIndex={url ? 0 : undefined}
+ onKeyDown={url ? (e) => {
+ if (e.key === "Enter" || e.key === " ") {
+ e.preventDefault();
+ handleClick();
+ }
+ } : undefined}
>Alternatively, wrap the entire card in an <a> tag when url is present.
📝 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
| return ( | |
| <div | |
| className={cn( | |
| "dark flex gap-3 rounded-xl bg-blue-800 p-4 dark:bg-blue-300-dark", | |
| url && "cursor-pointer" | |
| )} | |
| onClick={handleClick} | |
| > | |
| return ( | |
| <div | |
| className={cn( | |
| "dark flex gap-3 rounded-xl bg-blue-800 p-4 dark:bg-blue-300-dark", | |
| url && "cursor-pointer" | |
| )} | |
| onClick={handleClick} | |
| role={url ? "button" : undefined} | |
| tabIndex={url ? 0 : undefined} | |
| onKeyDown={url ? (e) => { | |
| if (e.key === "Enter" || e.key === " ") { | |
| e.preventDefault(); | |
| handleClick(); | |
| } | |
| } : undefined} | |
| > |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@front_end/src/components/charts/primitives/news_annotations/news_annotation_popup_card.tsx`
around lines 57 - 64, The interactive card uses a non-semantic div with only
onClick (see url and handleClick in NewsAnnotationPopupCard), which is not
keyboard-accessible; update the component so when url is present either wrap the
card in an anchor element or replace the div with a semantic interactive element
(e.g., <a> or <button>) and ensure keyboard handlers and accessibility
attributes are present: make it focusable (tabIndex if needed), handle
Enter/Space in onKeyDown to call handleClick, and add an appropriate
aria-label/role to describe the action.
ncarazon
force-pushed
the
feat/key-factors-3d-iteration
branch
from
daa579c to
cdfdc61
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
front_end/src/components/detailed_question_card/detailed_question_card/continuous_chart_card.tsx (1)
251-257: Consider memoizing the toggle callback.The inline arrow function
() => setShowNewsAnnotations((prev) => !prev)creates a new reference on each render. While not critical since it's only used when the toggle is visible, wrapping it inuseCallbackwould be more consistent with the other callbacks in this component.💡 Optional optimization
Add a memoized callback alongside
handleCursorChangeandhandleChartReady:+ const handleToggleNewsAnnotations = useCallback(() => { + setShowNewsAnnotations((prev) => !prev); + }, []); // ... in renderTimeline: onToggleNewsAnnotations={ hasNewsKeyFactors - ? () => setShowNewsAnnotations((prev) => !prev) + ? handleToggleNewsAnnotations : undefined }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@front_end/src/components/detailed_question_card/detailed_question_card/continuous_chart_card.tsx` around lines 251 - 257, The inline toggle handler for news annotations should be memoized: create a useCallback named (for example) toggleNewsAnnotations that calls setShowNewsAnnotations(prev => !prev) and include setShowNewsAnnotations in its dependency array, then pass hasNewsKeyFactors ? toggleNewsAnnotations : undefined to onToggleNewsAnnotations; this matches the memoization pattern used for handleCursorChange and handleChartReady and prevents creating a new function each render.front_end/src/components/charts/primitives/news_annotations/news_annotation_popup.tsx (1)
38-51: Consider indicating when annotations are truncated.When a cluster has more than 3 annotations, users see only the first 3 with no indication that more exist. Consider adding a "+N more" indicator.
💡 Proposed enhancement
const visibleAnnotations = cluster.annotations.slice( 0, MAX_VISIBLE_ANNOTATIONS ); + const hiddenCount = cluster.annotations.length - visibleAnnotations.length; return ( <div ref={ref} className="flex w-[280px] flex-col gap-[5px] rounded-xl p-0"> {visibleAnnotations.map((annotation) => ( <NewsAnnotationPopupCard key={annotation.keyFactor.id} annotation={annotation} questionType={questionType} /> ))} + {hiddenCount > 0 && ( + <div className="text-center text-sm text-gray-600"> + +{hiddenCount} more + </div> + )} </div> );Note: The "+N more" text should use
useTranslations()for i18n compliance. Based on learnings, avoid hardcoding English strings in TSX components.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@front_end/src/components/charts/primitives/news_annotations/news_annotation_popup.tsx` around lines 38 - 51, The popup currently slices cluster.annotations into visibleAnnotations (using MAX_VISIBLE_ANNOTATIONS) and renders NewsAnnotationPopupCard for each, but provides no UI that more items exist; update the component to compute remaining = cluster.annotations.length - MAX_VISIBLE_ANNOTATIONS and, when remaining > 0, render a localized "+N more" indicator (use useTranslations() and a plural/count-safe key, e.g. t('news.moreAnnotations', { count: remaining })) placed after the mapped NewsAnnotationPopupCard list so users know there are hidden annotations; ensure you reference visibleAnnotations, MAX_VISIBLE_ANNOTATIONS, cluster, and NewsAnnotationPopupCard when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@front_end/src/components/charts/primitives/news_annotations/news_annotation_popup.tsx`:
- Around line 38-51: The popup currently slices cluster.annotations into
visibleAnnotations (using MAX_VISIBLE_ANNOTATIONS) and renders
NewsAnnotationPopupCard for each, but provides no UI that more items exist;
update the component to compute remaining = cluster.annotations.length -
MAX_VISIBLE_ANNOTATIONS and, when remaining > 0, render a localized "+N more"
indicator (use useTranslations() and a plural/count-safe key, e.g.
t('news.moreAnnotations', { count: remaining })) placed after the mapped
NewsAnnotationPopupCard list so users know there are hidden annotations; ensure
you reference visibleAnnotations, MAX_VISIBLE_ANNOTATIONS, cluster, and
NewsAnnotationPopupCard when making the change.
In
`@front_end/src/components/detailed_question_card/detailed_question_card/continuous_chart_card.tsx`:
- Around line 251-257: The inline toggle handler for news annotations should be
memoized: create a useCallback named (for example) toggleNewsAnnotations that
calls setShowNewsAnnotations(prev => !prev) and include setShowNewsAnnotations
in its dependency array, then pass hasNewsKeyFactors ? toggleNewsAnnotations :
undefined to onToggleNewsAnnotations; this matches the memoization pattern used
for handleCursorChange and handleChartReady and prevents creating a new function
each render.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 88b20820-f7b5-49a7-8d39-7f99a123ae5e
📒 Files selected for processing (16)
front_end/src/app/(main)/questions/[id]/components/question_layout/consumer_question_layout/index.tsxfront_end/src/app/(main)/questions/[id]/components/question_layout/question_info.tsxfront_end/src/app/(main)/questions/[id]/components/question_view/consumer_question_view/timeline/index.tsxfront_end/src/app/(main)/questions/[id]/components/question_view/forecaster_question_view/index.tsxfront_end/src/components/charts/numeric_chart.tsxfront_end/src/components/charts/numeric_timeline.tsxfront_end/src/components/charts/primitives/chart_container.tsxfront_end/src/components/charts/primitives/news_annotations/news_annotation_marker.tsxfront_end/src/components/charts/primitives/news_annotations/news_annotation_popup.tsxfront_end/src/components/charts/primitives/news_annotations/news_annotation_popup_card.tsxfront_end/src/components/charts/primitives/news_annotations/news_annotation_toggle.tsxfront_end/src/components/charts/primitives/news_annotations/timeline_news_annotations.tsxfront_end/src/components/charts/primitives/news_annotations/types.tsfront_end/src/components/charts/primitives/news_annotations/utils.tsfront_end/src/components/detailed_question_card/detailed_question_card/continuous_chart_card.tsxfront_end/src/components/detailed_question_card/detailed_question_card/index.tsx
✅ Files skipped from review due to trivial changes (4)
- front_end/src/app/(main)/questions/[id]/components/question_layout/consumer_question_layout/index.tsx
- front_end/src/components/charts/primitives/news_annotations/types.ts
- front_end/src/components/charts/primitives/news_annotations/news_annotation_toggle.tsx
- front_end/src/components/charts/primitives/news_annotations/news_annotation_marker.tsx
🚧 Files skipped from review as they are similar to previous changes (8)
- front_end/src/app/(main)/questions/[id]/components/question_view/forecaster_question_view/index.tsx
- front_end/src/app/(main)/questions/[id]/components/question_view/consumer_question_view/timeline/index.tsx
- front_end/src/app/(main)/questions/[id]/components/question_layout/question_info.tsx
- front_end/src/components/detailed_question_card/detailed_question_card/index.tsx
- front_end/src/components/charts/primitives/chart_container.tsx
- front_end/src/components/charts/primitives/news_annotations/timeline_news_annotations.tsx
- front_end/src/components/charts/primitives/news_annotations/utils.ts
- front_end/src/components/charts/primitives/news_annotations/news_annotation_popup_card.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
front_end/src/app/(main)/questions/[id]/components/key_factors/item_view/key_factor_strength_item.tsx (1)
191-197: Analytics sent before submission may track failed votes.In
submitImpactVote, the analytics event is dispatched before callingsubmit. If the submission fails, the analytics event will have already been sent. This is inconsistent with thesubmitfunction which sends analytics only after success.Consider moving the analytics call after
submitcompletes, or usetry/awaitto track only successful impact votes:♻️ Proposed fix for consistent analytics timing
const submitImpactVote = useCallback( - (option: ImpactOption) => { + async (option: ImpactOption) => { if (!user || user.is_bot) return; const currentVote = aggregate?.user_vote; if (!currentVote || currentVote <= 0) return; const newScore = IMPACT_SCORE_MAP[option]; if (newScore === currentVote) return; - sendAnalyticsEvent("KeyFactorVote", { - event_label: newScore.toString(), - variant: "impact", - impact_level: option, - }); - - submit(newScore, { skipAnalytics: true }); + try { + await submit(newScore, { skipAnalytics: true }); + sendAnalyticsEvent("KeyFactorVote", { + event_label: newScore.toString(), + variant: "impact", + impact_level: option, + }); + } catch { + // Analytics not sent on failure + } }, [user, aggregate?.user_vote, submit] );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@front_end/src/app/`(main)/questions/[id]/components/key_factors/item_view/key_factor_strength_item.tsx around lines 191 - 197, The analytics event for impact votes is sent before the network submission, which can record failed votes; in the submitImpactVote flow call submit(...) first and only after it resolves successfully invoke sendAnalyticsEvent("KeyFactorVote", { event_label: newScore.toString(), variant: "impact", impact_level: option }), or use try/await around submit to send analytics in the success path and avoid sending on errors; update the submitImpactVote function (referencing submit and sendAnalyticsEvent) so analytics are emitted after submit completes without errors.front_end/src/app/(main)/questions/[id]/components/key_factors/item_view/index.tsx (1)
128-133: Analytics sent before API call may track failed downvote attempts.Similar to
submitImpactVotein the sibling file, the analytics event is dispatched before thevoteKeyFactorcall. If the API call fails (caught at lines 147-149), the downvote analytics will already have been recorded.For consistency with the
submitfunction inkey_factor_strength_item.tsx(which sends analytics only after success), consider moving this after the API response:♻️ Proposed fix for consistent analytics timing
const handleDownvoteReasonSelect = useCallback( async (reason: DownvoteReason) => { if (!user || user.is_bot) return; setShowDownvoteThanks(true); const apiReason = DOWNVOTE_REASON_TO_API[reason]; - sendAnalyticsEvent("KeyFactorVote", { - event_label: downScore.toString(), - variant: "downvote_reason", - reason: apiReason, - }); - try { const resp = await voteKeyFactor({ id: liveKeyFactor.id, vote: downScore, user: user.id, vote_type: downVoteType, vote_reason: apiReason, }); + + sendAnalyticsEvent("KeyFactorVote", { + event_label: downScore.toString(), + variant: "downvote_reason", + reason: apiReason, + }); + if (resp) { const updated = resp as unknown as KeyFactorVoteAggregate; commentsFeed?.setKeyFactorVote(liveKeyFactor.id, updated); } } catch (e) { console.error("Failed to submit vote reason", e); } }, [user, liveKeyFactor.id, downScore, downVoteType, commentsFeed] );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@front_end/src/app/`(main)/questions/[id]/components/key_factors/item_view/index.tsx around lines 128 - 133, The analytics event for downvotes is sent before the API call; move the sendAnalyticsEvent call so it runs only after a successful voteKeyFactor() response to avoid tracking failed attempts—locate the block using sendAnalyticsEvent("KeyFactorVote", { event_label: downScore.toString(), variant: "downvote_reason", reason: apiReason }) and the voteKeyFactor(...) call in the same handler (the downvote path in this component), call voteKeyFactor first, await its success, and then invoke sendAnalyticsEvent with the same payload (retain downScore and apiReason) inside the try/then-success branch; keep the existing error handling around voteKeyFactor unchanged.front_end/src/components/charts/primitives/chart_container.tsx (1)
65-101: Redundantuppercaseclass on line 89.The
uppercaseclass is already applied unconditionally on line 79, making the conditional application on line 89 (isEmbed && "uppercase ...") redundant for that specific class.♻️ Suggested cleanup
className={cn( - "ChartZoomButton rounded px-1 py-0.5 text-xs font-normal uppercase leading-4 text-gray-600 hover:text-blue-800 focus:outline-none dark:text-gray-600-dark hover:dark:text-blue-800-dark md:text-sm", + "ChartZoomButton rounded px-1 py-0.5 text-xs font-normal uppercase leading-4 text-gray-600 hover:text-blue-800 focus:outline-none dark:text-gray-600-dark hover:dark:text-blue-800-dark md:text-sm", { "text-gray-900 dark:text-gray-900-dark": selected, }, { "bg-gray-300 dark:bg-gray-300-dark": hover || selected, }, - isEmbed && - "uppercase text-gray-600 dark:text-gray-600-dark md:text-xs" + isEmbed && "md:text-xs" )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@front_end/src/components/charts/primitives/chart_container.tsx` around lines 65 - 101, The className on the zoom buttons applies "uppercase" unconditionally and then again inside the conditional for isEmbed; update the Tab rendering in the TabGroup/TabList (where tabOptions is mapped) to remove the redundant "uppercase" from the isEmbed branch so only the unconditional "uppercase" remains—adjust the conditional expression inside the className computation for the button in that Tab (used in the mapping over tabOptions) to omit "uppercase" while keeping the other isEmbed styles intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@front_end/src/components/charts/primitives/news_annotations/timeline_news_annotations.tsx`:
- Around line 32-48: The pending closeTimeout created in handleHoverEnd can fire
after the component unmounts and call setActiveClusterId, so add a useEffect
cleanup that clears closeTimeoutRef.current (if present) and sets it to null on
unmount; reference the existing closeTimeoutRef, handleHoverEnd,
handleHoverStart and setActiveClusterId when locating where to add the cleanup
to prevent state updates on an unmounted component.
---
Nitpick comments:
In
`@front_end/src/app/`(main)/questions/[id]/components/key_factors/item_view/index.tsx:
- Around line 128-133: The analytics event for downvotes is sent before the API
call; move the sendAnalyticsEvent call so it runs only after a successful
voteKeyFactor() response to avoid tracking failed attempts—locate the block
using sendAnalyticsEvent("KeyFactorVote", { event_label: downScore.toString(),
variant: "downvote_reason", reason: apiReason }) and the voteKeyFactor(...) call
in the same handler (the downvote path in this component), call voteKeyFactor
first, await its success, and then invoke sendAnalyticsEvent with the same
payload (retain downScore and apiReason) inside the try/then-success branch;
keep the existing error handling around voteKeyFactor unchanged.
In
`@front_end/src/app/`(main)/questions/[id]/components/key_factors/item_view/key_factor_strength_item.tsx:
- Around line 191-197: The analytics event for impact votes is sent before the
network submission, which can record failed votes; in the submitImpactVote flow
call submit(...) first and only after it resolves successfully invoke
sendAnalyticsEvent("KeyFactorVote", { event_label: newScore.toString(), variant:
"impact", impact_level: option }), or use try/await around submit to send
analytics in the success path and avoid sending on errors; update the
submitImpactVote function (referencing submit and sendAnalyticsEvent) so
analytics are emitted after submit completes without errors.
In `@front_end/src/components/charts/primitives/chart_container.tsx`:
- Around line 65-101: The className on the zoom buttons applies "uppercase"
unconditionally and then again inside the conditional for isEmbed; update the
Tab rendering in the TabGroup/TabList (where tabOptions is mapped) to remove the
redundant "uppercase" from the isEmbed branch so only the unconditional
"uppercase" remains—adjust the conditional expression inside the className
computation for the button in that Tab (used in the mapping over tabOptions) to
omit "uppercase" while keeping the other isEmbed styles intact.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 28f5f9e9-9142-48f8-9a79-0ce9bc0e78b4
⛔ Files ignored due to path filters (1)
front_end/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (17)
front_end/messages/cs.jsonfront_end/messages/en.jsonfront_end/messages/es.jsonfront_end/messages/pt.jsonfront_end/messages/zh-TW.jsonfront_end/messages/zh.jsonfront_end/package.jsonfront_end/src/app/(main)/questions/[id]/components/key_factors/comment_detail_panel.tsxfront_end/src/app/(main)/questions/[id]/components/key_factors/item_view/index.tsxfront_end/src/app/(main)/questions/[id]/components/key_factors/item_view/key_factor_strength_item.tsxfront_end/src/components/charts/numeric_chart.tsxfront_end/src/components/charts/primitives/chart_container.tsxfront_end/src/components/charts/primitives/news_annotations/news_annotation_marker.tsxfront_end/src/components/charts/primitives/news_annotations/news_annotation_popup.tsxfront_end/src/components/charts/primitives/news_annotations/news_annotation_popup_card.tsxfront_end/src/components/charts/primitives/news_annotations/news_annotation_toggle.tsxfront_end/src/components/charts/primitives/news_annotations/timeline_news_annotations.tsx
✅ Files skipped from review due to trivial changes (9)
- front_end/package.json
- front_end/messages/en.json
- front_end/messages/pt.json
- front_end/messages/zh.json
- front_end/messages/zh-TW.json
- front_end/messages/cs.json
- front_end/src/components/charts/primitives/news_annotations/news_annotation_popup.tsx
- front_end/messages/es.json
- front_end/src/components/charts/primitives/news_annotations/news_annotation_toggle.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- front_end/src/components/charts/primitives/news_annotations/news_annotation_marker.tsx
- front_end/src/components/charts/primitives/news_annotations/news_annotation_popup_card.tsx
Comment on lines
+32
to
+48
| const [activeClusterId, setActiveClusterId] = useState<number | null>(null); | ||
| const closeTimeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null); | ||
|
|
||
| const handleHoverStart = useCallback((index: number) => { | ||
| if (closeTimeoutRef.current) { | ||
| clearTimeout(closeTimeoutRef.current); | ||
| closeTimeoutRef.current = null; | ||
| } | ||
| setActiveClusterId(index); | ||
| }, []); | ||
|
|
||
| const handleHoverEnd = useCallback(() => { | ||
| closeTimeoutRef.current = setTimeout(() => { | ||
| setActiveClusterId(null); | ||
| closeTimeoutRef.current = null; | ||
| }, 150); | ||
| }, []); |
Contributor
There was a problem hiding this comment.
Clear timeout on unmount to prevent state updates on unmounted component.
If the component unmounts while the close timeout is pending, setActiveClusterId(null) will be called on an unmounted component, potentially causing a memory leak or React warning.
🛡️ Proposed fix
+import { FC, useCallback, useEffect, useMemo, useRef, useState } from "react";
const TimelineNewsAnnotations: FC<Props> = ({
clusters,
chartHeight,
axisBottomOffset,
questionType,
}) => {
const [activeClusterId, setActiveClusterId] = useState<number | null>(null);
const closeTimeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null);
+ useEffect(() => {
+ return () => {
+ if (closeTimeoutRef.current) {
+ clearTimeout(closeTimeoutRef.current);
+ }
+ };
+ }, []);
+
const handleHoverStart = useCallback((index: number) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@front_end/src/components/charts/primitives/news_annotations/timeline_news_annotations.tsx`
around lines 32 - 48, The pending closeTimeout created in handleHoverEnd can
fire after the component unmounts and call setActiveClusterId, so add a
useEffect cleanup that clears closeTimeoutRef.current (if present) and sets it
to null on unmount; reference the existing closeTimeoutRef, handleHoverEnd,
handleHoverStart and setActiveClusterId when locating where to add the cleanup
to prevent state updates on an unmounted component.
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.
This PR implements the 3d iteration of the Key Factors Redesign
Iteration 3: Timeline Annotations
Summary by CodeRabbit
New Features
Refactor
Style