Feat: extend hover-mirror cursor sync to all consumer view question types#4834
Open
ncarazon wants to merge 2 commits into
Open
Feat: extend hover-mirror cursor sync to all consumer view question types#4834ncarazon wants to merge 2 commits into
ncarazon wants to merge 2 commits into
Conversation
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR extends chart cursor synchronization across question types, enabling hovering on forecast timelines to update corresponding sidebar displays in consumer views while controlling tooltip visibility. Changes span context state management, component prop wiring, and aggregation-value timestamp alignment. ChangesChart Cursor Synchronization and Consumer-View Mirror Updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
front_end/src/components/ui/dropdown_menu.tsx (1)
67-82: ⚡ Quick winRef-based prevItems refactor correctly prevents render loop.
The change from
useStatetouseReffor tracking the base menu items is correct and successfully addresses the render loop issue described in the PR objectives. By eliminatingprevItemsas state, updates to the ref no longer trigger re-renders, breaking the cascade of re-renders caused by increased cursor synchronization activity elsewhere in the codebase.The restoration logic remains functionally correct:
prevItemsRef.currentalways holds the latestitemsprop (refs don't suffer from stale closures), so resettingactiveItemson menu close works as intended.Further optimize the effect by stabilizing
onCloseaccess:The second effect currently depends on
[open, onClose]. If the parent doesn't memoizeonClose, the effect will run every time the parent re-renders while the menu is closed, invokingonClose()spuriously. Consider capturing the latestonClosevia a ref and depending only on[open]:♻️ Suggested effect pattern to eliminate onClose dependency
const prevItemsRef = useRef<MenuItemProps[]>(items); + const onCloseRef = useRef(onClose); const [activeItems, setActiveItems] = useState<MenuItemProps[]>(items); + useEffect(() => { + onCloseRef.current = onClose; + }, [onClose]); + // Track prop change useEffect(() => { prevItemsRef.current = items; setActiveItems(items); }, [items]); useEffect(() => { if (!open) { setActiveItems(prevItemsRef.current); - if (onClose) onClose(); + if (onCloseRef.current) onCloseRef.current(); } - }, [open, onClose]); + }, [open]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@front_end/src/components/ui/dropdown_menu.tsx` around lines 67 - 82, The effect that resets activeItems on close currently lists onClose in its deps causing spurious runs when parents re-render; create a ref (e.g., onCloseRef) to store the latest onClose and update it in an effect, then change the close-effect to depend only on [open] and call onCloseRef.current() (guarded) instead of onClose; update references to use onCloseRef so the effect no longer re-triggers unnecessarily while preserving correct latest-callback behavior (refer to prevItemsRef, activeItems, setActiveItems, and the close-useEffect).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@front_end/src/components/consumer_post_card/group_forecast_card/numeric_forecast_card.tsx`:
- Around line 81-87: The fallback refTs is currently taken from sortedChoices[0]
which can point to an older series due to earlier reordering; update the logic
that computes refTimestamps/refTs to use a global latest timestamp instead of
the first sorted row — for example derive refTimestamps from the last element
(sortedChoices[sortedChoices.length - 1]?.aggregationTimestamps) or compute the
max last-entry timestamp across all sortedChoices, then keep the existing
cursorTimestamp override logic (use cursorTimestamp if not null, otherwise use
that global latest timestamp or null). Ensure you update references to
refTimestamps and refTs accordingly.
In
`@front_end/src/components/consumer_post_card/group_forecast_card/percentage_forecast_card.tsx`:
- Around line 94-98: The fallback reference timestamp is currently taken from
allChoices[0] (refTimestamps) which can be shorter than other choices; change
the fallback logic so that when cursorTimestamp is null you compute the latest
available timestamp across all choices by inspecting each choice. Specifically,
replace using allChoices[0]?.aggregationTimestamps and
refTimestamps[refTimestamps.length - 1] with logic that iterates over
allChoices, extracts each choice.aggregationTimestamps' last element (if any),
finds the maximum (most recent) timestamp, and use that as the fallback refTs;
preserve the early returns when there are no timestamps or refTs is null.
---
Nitpick comments:
In `@front_end/src/components/ui/dropdown_menu.tsx`:
- Around line 67-82: The effect that resets activeItems on close currently lists
onClose in its deps causing spurious runs when parents re-render; create a ref
(e.g., onCloseRef) to store the latest onClose and update it in an effect, then
change the close-effect to depend only on [open] and call onCloseRef.current()
(guarded) instead of onClose; update references to use onCloseRef so the effect
no longer re-triggers unnecessarily while preserving correct latest-callback
behavior (refer to prevItemsRef, activeItems, setActiveItems, and the
close-useEffect).
🪄 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: 0dc2f2f2-bf08-4ae8-8535-533daa056aad
📒 Files selected for processing (15)
front_end/src/app/(main)/questions/[id]/components/group_timeline.tsxfront_end/src/app/(main)/questions/[id]/components/multiple_choices_chart_view/index.tsxfront_end/src/app/(main)/questions/[id]/components/question_page_shell/index.tsxfront_end/src/app/(main)/questions/[id]/components/question_view/consumer_question_view/consumer_group_chart.tsxfront_end/src/app/(main)/questions/[id]/components/question_view/consumer_question_view/consumer_list_chart_shell.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/question_header/question_header_cp_status.tsxfront_end/src/components/consumer_post_card/binary_cp_bar.tsxfront_end/src/components/consumer_post_card/group_forecast_card/numeric_forecast_card.tsxfront_end/src/components/consumer_post_card/group_forecast_card/percentage_forecast_card.tsxfront_end/src/components/detailed_question_card/detailed_group_card/index.tsxfront_end/src/components/detailed_question_card/detailed_question_card/continuous_chart_card.tsxfront_end/src/components/detailed_question_card/detailed_question_card/multiple_choice_chart_card.tsxfront_end/src/components/ui/dropdown_menu.tsxfront_end/src/contexts/continuous_chart_cursor_context.tsx
Contributor
🚀 Preview EnvironmentYour preview environment is ready!
Details
ℹ️ Preview Environment InfoIsolation:
Limitations:
Cleanup:
|
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.
Resolves #4764
Summary
In consumer view, hovering the forecast timeline only synced the mini-distribution on the left for continuous questions. All other question types showed stale values regardless of cursor position. Tooltip also duplicated information already visible in the sidebar.
Implemented changes:
ContinuousChartCursorContext: addedactiveBinaryValue/setActiveBinaryValueto carry the binary CP value at cursor positionDetailedContinuousChartCard: computes CP probability at cursor timestamp and pushes it to context viauseLayoutEffectfor same-frame gauge updates; suppresses floating tooltip in consumer binary viewConsumerListChartShell: addedcursorTimestamp/setCursorTimestamptoListChartExpandedContextso sidebar components can read the hovered timestamp; resets tonullon mouse leaveDetailedMultipleChoiceChartCard: pushes cursor timestamp to context in consumer view; hides floating tooltipGroupTimeline: addedonCursorChangeandhideTooltipprops and wires them through toMultiChoicesChartViewConsumerGroupChart: forwards cursor timestamp to context and hides tooltip for continuous groupsPercentageForecastCard: resolves each choice's display value at the cursor timestamp using per-choice history lookup (fixes wrong initial values when sub-questions have different history lengths)NumericForecastCard: same cursor-aware value resolution for numeric group sub-questionsQuestionHeaderCPStatus/BinaryCPBar: acceptcursorBinaryValue/overrideValueto display cursor-position CP instead of latestQuestionPageShell: extractedBinaryCursorGaugeas an isolated context consumer soConsumerShellno longer re-renders on every cursor moveDropdownMenu: convertedprevItemsfrom state to ref to prevent a render loop triggered by the increased re-render frequencyDemo video
feat-consumer-view-hover-sync-all-question-types.mp4
Summary by CodeRabbit
New Features
Improvements
Bug Fixes