fix live preview for display height and depth sliders#2263
Open
nehaaprasad wants to merge 1 commit intoMentra-Community:devfrom
Open
fix live preview for display height and depth sliders#2263nehaaprasad wants to merge 1 commit intoMentra-Community:devfrom
nehaaprasad wants to merge 1 commit intoMentra-Community:devfrom
Conversation
📝 WalkthroughWalkthroughA bug fix modifies the position settings screen to pass setter functions directly to the slider's Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mobile/src/app/miniapps/settings/position.tsx`:
- Around line 43-44: The handlers currently call the persisted/synced setter
(setDashboardDepth) on every onValueChange and again on onValueSet, causing
excessive storage/network writes; change the component to update a transient
local state during drag (use a local state variable and call setDashboardDepth
only from onValueSet) so onValueChange updates only local UI state and
onValueSet performs the single persisted/synced write; apply the same change for
the other pair at lines 53-54 (the second persisted setter).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7a83af77-fd24-4a93-8069-87234e500d0e
📒 Files selected for processing (1)
mobile/src/app/miniapps/settings/position.tsx
Comment on lines
+43
to
44
| onValueChange={setDashboardDepth} | ||
| onValueSet={setDashboardDepth} |
There was a problem hiding this comment.
Avoid persisting/syncing settings on every drag tick.
Line 43 and Line 53 now call persisted/server-synced setters on every onValueChange, and Line 44/Line 54 call them again on release. This creates high-frequency storage + network writes and duplicate final writes.
💡 Proposed fix (live preview + single persisted write on release)
-import {useCallback, useEffect} from "react"
+import {useCallback, useEffect, useState} from "react"
@@
export default function ScreenSettingsScreen() {
@@
const [dashboardDepth, setDashboardDepth] = useSetting(SETTINGS.dashboard_depth.key)
const [dashboardHeight, setDashboardHeight] = useSetting(SETTINGS.dashboard_height.key)
+ const [previewDepth, setPreviewDepth] = useState(dashboardDepth ?? 5)
+ const [previewHeight, setPreviewHeight] = useState(dashboardHeight ?? 4)
@@
useEffect(() => {
setEnabled(false)
return () => setEnabled(true)
}, [setEnabled])
+
+ useEffect(() => {
+ setPreviewDepth(dashboardDepth ?? 5)
+ }, [dashboardDepth])
+
+ useEffect(() => {
+ setPreviewHeight(dashboardHeight ?? 4)
+ }, [dashboardHeight])
@@
<SliderSetting
@@
- value={dashboardDepth ?? 5}
+ value={previewDepth}
@@
- onValueChange={setDashboardDepth}
- onValueSet={setDashboardDepth}
+ onValueChange={setPreviewDepth}
+ onValueSet={(value) => {
+ setPreviewDepth(value)
+ setDashboardDepth(value)
+ }}
/>
@@
<SliderSetting
@@
- value={dashboardHeight ?? 4}
+ value={previewHeight}
@@
- onValueChange={setDashboardHeight}
- onValueSet={setDashboardHeight}
+ onValueChange={setPreviewHeight}
+ onValueSet={(value) => {
+ setPreviewHeight(value)
+ setDashboardHeight(value)
+ }}
/>Also applies to: 53-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mobile/src/app/miniapps/settings/position.tsx` around lines 43 - 44, The
handlers currently call the persisted/synced setter (setDashboardDepth) on every
onValueChange and again on onValueSet, causing excessive storage/network writes;
change the component to update a transient local state during drag (use a local
state variable and call setDashboardDepth only from onValueSet) so onValueChange
updates only local UI state and onValueSet performs the single persisted/synced
write; apply the same change for the other pair at lines 53-54 (the second
persisted setter).
Member
|
Hey @naaa760 thanks for the PR. have you tested this? |
Author
|
@aisraelov yess |
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
Test plan
Closes #2250
Summary by CodeRabbit