Save client-side settings on the client; server-config.json for server only#59
Save client-side settings on the client; server-config.json for server only#59alishair7071 wants to merge 5 commits intoAOSSIE-Org:mainfrom
Conversation
📝 WalkthroughWalkthroughMoves UI/preferences (theme, mouse sensitivity, invert, saved IP) to client-only localStorage, initializes theme on load, updates gesture hook to read client settings at runtime, and filters WebSocket-config updates so only whitelisted server keys are persisted to server-config.json. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WS_Server as "WebSocket\nServer"
participant ServerFS as "server-config.json\n(File System)"
Client->>WS_Server: send update-config (payload: { frontendPort })
WS_Server->>WS_Server: filter msg.config using SERVER_CONFIG_KEYS
alt server keys present
WS_Server->>ServerFS: write filtered server-config.json
ServerFS-->>WS_Server: write OK
WS_Server->>Client: notify config-updated
else no server keys
WS_Server->>Client: acknowledge (no server changes)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routes/settings.tsx (1)
171-198:⚠️ Potential issue | 🟠 Major
parseInton a non-numeric port string sendsNaN(serialized asnull) to the server, corrupting config.If the user types a non-numeric value in the Port field,
parseInt("abc", 10)returnsNaN.JSON.stringify({ frontendPort: NaN })serializes it asnull, which would corruptserver-config.json. The subsequent redirect to:NaN/settingswould also fail.Add client-side validation before sending:
Proposed fix
socket.onopen = () => { // Only server settings; client settings (like ip) stay in localStorage + const parsedPort = parseInt(frontendPort, 10); + if (isNaN(parsedPort) || parsedPort < 1 || parsedPort > 65535) { + alert('Please enter a valid port number (1–65535).'); + socket.close(); + return; + } socket.send(JSON.stringify({ type: 'update-config', config: { - frontendPort: parseInt(frontendPort, 10), + frontendPort: parsedPort, mouseInvert: invertScroll, mouseSensitivity: sensitivity, } }));
🧹 Nitpick comments (1)
src/server/websocket.ts (1)
57-70: Consider hoistingSERVER_CONFIG_KEYSto module scope and adding basic value validation.Two observations:
SERVER_CONFIG_KEYSis re-declared on everyupdate-configmessage. Moving it to module scope (next to the imports) avoids re-creation and makes the whitelist easier to find and maintain.The key filtering is good, but there's no validation of the values. A client could send
{ frontendPort: "not-a-number", mouseSensitivity: null }, and these would be written directly intoserver-config.json. Basic type checks (e.g.,typeof filtered.frontendPort === 'number') would guard against corrupt config files.Proposed refactor
Move the constant to module scope:
+// Only these keys may be written to server-config.json +const SERVER_CONFIG_KEYS = ['host', 'frontendPort', 'address', 'mouseInvert', 'mouseSensitivity'] as const; + export function createWsServer(server: Server) {Add lightweight value validation inside the handler:
const filtered: Record<string, unknown> = {}; for (const key of SERVER_CONFIG_KEYS) { if (Object.prototype.hasOwnProperty.call(incoming, key)) { filtered[key] = incoming[key]; } } + // Basic type guards + if ('frontendPort' in filtered && typeof filtered.frontendPort !== 'number') { + delete filtered.frontendPort; + } + if ('mouseSensitivity' in filtered && typeof filtered.mouseSensitivity !== 'number') { + delete filtered.mouseSensitivity; + } + if ('mouseInvert' in filtered && typeof filtered.mouseInvert !== 'boolean') { + delete filtered.mouseInvert; + } const newConfig = { ...current, ...filtered };
|
I wanted someone else to do it |
|
@imxade is my PR not right? if someone other is ready to do it and you also wanna to make by some one else, no worry i am agreed |
I wanted you to guide someone else tackling these two issues that someone else can be anyone without any merged pr but interested |
|
Ok @imxade, i think one of contributer commented on this issue to solve |
no need for assignment just tell him he can go ahead with it, you have the ownership of these two issues Edit: both of you are contributors, so I marked the issue as resolved |
|
@alishair7071 only host, port and address are part of server config make changes accordingly |
|
Ok @imxade, I wait for @aniket866 to solve this as you suggest, if he will not come in then i will change it by myself |
do it yourself, I've closed that issue and reviewed the other pr |
…ntendPort', 'address') only are for server
|
Sir @imxade i have changed it accordingly by myself as you suggested that only host, port and address should be stored on server file, so it is ready you can review i, first we will solve this issue then we can move to second issue if no one will come even after 2 days waiting |
…rage and reactive,
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/routes/settings.tsx`:
- Around line 195-207: The label "Theme" is not programmatically associated with
the select, so update the Theme field to associate the label and control: add a
unique id (e.g., "theme-select") to the <select> and set the <label>'s htmlFor
to that id (or alternatively nest the <select> inside the <label>) so screen
readers can link the label to the control; ensure this change touches the select
element that uses value={theme} and onChange={(e) => setTheme(...)} and
references THEMES.DARK/THEMES.LIGHT.
- Around line 231-268: Add a saving guard and cleanup: introduce a component
state (e.g., isSaving) and check it at the start of the Save button's onClick
(referencing frontendPort and the button's onClick handler) to ignore additional
clicks and disable the button and show a loading indicator while saving; when
initiating the WebSocket (new WebSocket(wsUrl) / socket) set isSaving=true, and
on success/failure (socket.onopen, socket.onmessage error branch,
socket.onerror) set isSaving=false on failure and proceed with redirect on
success. Also store the socket and any setTimeout id in refs so you can close
socket and clearTimeout in a useEffect cleanup (to handle component unmount) and
ensure socket.close() is called on all terminal paths to avoid leaks and racing
redirects.
🧹 Nitpick comments (5)
src/hooks/useTrackpadGesture.ts (2)
19-27: DuplicateCLIENT_KEYSconstants — extract to a shared module.The same localStorage key strings (
rein_mouse_sensitivity,rein_mouse_invert) are defined here and insrc/routes/settings.tsx(lines 11–15). If either copy drifts, the settings page and gesture handler will silently disagree. Consider extracting these into a shared constants file (e.g.,src/constants/clientKeys.ts).Example shared module
Create
src/constants/clientKeys.ts:export const CLIENT_KEYS = { SENSITIVITY: 'rein_mouse_sensitivity', INVERT: 'rein_mouse_invert', THEME: 'rein-theme', } as const; export const CLIENT_DEFAULTS = { SENSITIVITY: 1.0, INVERT: false, } as const;Then import from both
useTrackpadGesture.tsandsettings.tsx.
29-33: Remove unused_defaultSensitivityparameterThe parameter accepts a default of
1.5but is never read—getClientSettings()supplies its own default of1.0. This creates a confusing API with conflicting default values.Option A: Remove the parameter
export const useTrackpadGesture = ( send: (msg: any) => void, scrollMode: boolean, - _defaultSensitivity: number = 1.5 ) => {Option B: Use it as the fallback
-function getClientSettings(): { sensitivity: number; invert: boolean } { - if (typeof localStorage === 'undefined') return { sensitivity: 1.0, invert: false }; +function getClientSettings(defaultSensitivity: number = 1.0): { sensitivity: number; invert: boolean } { + if (typeof localStorage === 'undefined') return { sensitivity: defaultSensitivity, invert: false }; const s = localStorage.getItem(CLIENT_KEYS.SENSITIVITY); - const sensitivity = s !== null && !Number.isNaN(Number(s)) ? Number(s) : 1.0; + const sensitivity = s !== null && !Number.isNaN(Number(s)) ? Number(s) : defaultSensitivity; const invert = localStorage.getItem(CLIENT_KEYS.INVERT) === 'true'; return { sensitivity, invert }; }src/routes/settings.tsx (3)
26-29: The first-change guard pattern works but is verbose.Three separate
isFirst*refs to skip the initial effect run is a lot of boilerplate. A simpler alternative: check whether the new value actually differs from what's already in localStorage before writing, which removes the need for allisFirst*refs entirely.Example: deduplicate with a write-if-changed helper
function persistIfChanged(key: string, value: string) { if (typeof window === 'undefined') return; if (localStorage.getItem(key) !== value) { localStorage.setItem(key, value); } }Then the persistence effects simplify to:
useEffect(() => { - if (typeof window === 'undefined' || !hasLoadedFromStorage.current) return - if (isFirstSensitivity.current) { isFirstSensitivity.current = false; return } - localStorage.setItem(CLIENT_KEYS.SENSITIVITY, String(sensitivity)) + if (!hasLoadedFromStorage.current) return + persistIfChanged(CLIENT_KEYS.SENSITIVITY, String(sensitivity)) }, [sensitivity])This eliminates
isFirstSensitivity,isFirstInvert, andisFirstThemerefs.
76-88: QR code and display URL always reflect the current server port, not the user's input.Both the QR URL (Line 81) and the display link (Line 289) use
CONFIG.FRONTEND_PORTrather than thefrontendPortstate variable. This means if a user changes the port in the input but hasn't saved yet, the QR/link still show the old port. This is arguably correct (the new port isn't active until saved), but could confuse users. A small note or visual indicator that the QR reflects the current port might help.
118-120: Cleanup may miss a socket still inCONNECTINGstate.The cleanup function only closes the socket when
readyState === WebSocket.OPEN, but if the component unmounts while the socket is still connecting (CONNECTINGstate), it won't be closed.WebSocket.close()is valid to call inCONNECTINGstate.Suggested fix
return () => { - if (socket.readyState === WebSocket.OPEN) socket.close(); + if (socket.readyState === WebSocket.OPEN || socket.readyState === WebSocket.CONNECTING) socket.close(); }
| <div className="form-control w-full"> | ||
| <label className="label"> | ||
| <span className="label-text">Theme</span> | ||
| </label> | ||
| <select | ||
| className="select select-bordered w-full" | ||
| value={theme} | ||
| onChange={(e) => setTheme(e.target.value as typeof THEMES.LIGHT | typeof THEMES.DARK)} | ||
| > | ||
| <option value={THEMES.DARK}>Dark (dracula)</option> | ||
| <option value={THEMES.LIGHT}>Light (cupcake)</option> | ||
| </select> | ||
| </div> |
There was a problem hiding this comment.
Accessibility: <label> is not associated with the <select> control.
Static analysis flagged this — the <label> on Line 196 has no htmlFor attribute and the <select> is not nested inside it. Screen readers won't associate the label text "Theme" with the dropdown.
Proposed fix
<div className="form-control w-full">
- <label className="label">
+ <label className="label" htmlFor="theme-select">
<span className="label-text">Theme</span>
</label>
<select
+ id="theme-select"
className="select select-bordered w-full"
value={theme}📝 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.
| <div className="form-control w-full"> | |
| <label className="label"> | |
| <span className="label-text">Theme</span> | |
| </label> | |
| <select | |
| className="select select-bordered w-full" | |
| value={theme} | |
| onChange={(e) => setTheme(e.target.value as typeof THEMES.LIGHT | typeof THEMES.DARK)} | |
| > | |
| <option value={THEMES.DARK}>Dark (dracula)</option> | |
| <option value={THEMES.LIGHT}>Light (cupcake)</option> | |
| </select> | |
| </div> | |
| <div className="form-control w-full"> | |
| <label className="label" htmlFor="theme-select"> | |
| <span className="label-text">Theme</span> | |
| </label> | |
| <select | |
| id="theme-select" | |
| className="select select-bordered w-full" | |
| value={theme} | |
| onChange={(e) => setTheme(e.target.value as typeof THEMES.LIGHT | typeof THEMES.DARK)} | |
| > | |
| <option value={THEMES.DARK}>Dark (dracula)</option> | |
| <option value={THEMES.LIGHT}>Light (cupcake)</option> | |
| </select> | |
| </div> |
🧰 Tools
🪛 Biome (2.3.14)
[error] 196-198: A form label must be associated with an input.
Consider adding a for or htmlFor attribute to the label element or moving the input element to inside the label element.
(lint/a11y/noLabelWithoutControl)
🤖 Prompt for AI Agents
In `@src/routes/settings.tsx` around lines 195 - 207, The label "Theme" is not
programmatically associated with the select, so update the Theme field to
associate the label and control: add a unique id (e.g., "theme-select") to the
<select> and set the <label>'s htmlFor to that id (or alternatively nest the
<select> inside the <label>) so screen readers can link the label to the
control; ensure this change touches the select element that uses value={theme}
and onChange={(e) => setTheme(...)} and references THEMES.DARK/THEMES.LIGHT.
| <button | ||
| className="btn btn-neutral w-full" | ||
| onClick={() => { | ||
| const protocol = window.location.protocol === 'https:' ? 'wss:' : 'ws:'; | ||
| const host = window.location.host; | ||
| const wsUrl = `${protocol}//${host}/ws`; | ||
| const socket = new WebSocket(wsUrl); | ||
| const portNum = parseInt(frontendPort, 10) | ||
| if (Number.isNaN(portNum) || portNum < 1 || portNum > 65535) { | ||
| alert('Please enter a valid port (1–65535).') | ||
| return | ||
| } | ||
| const protocol = window.location.protocol === 'https:' ? 'wss:' : 'ws:' | ||
| const host = window.location.host | ||
| const wsUrl = `${protocol}//${host}/ws` | ||
| const socket = new WebSocket(wsUrl) | ||
|
|
||
| socket.onerror = () => { | ||
| alert('Could not connect to server. Is the app running?') | ||
| } | ||
| socket.onopen = () => { | ||
| socket.send(JSON.stringify({ | ||
| type: 'update-config', | ||
| config: { | ||
| frontendPort: parseInt(frontendPort), | ||
| mouseInvert: invertScroll, | ||
| mouseSensitivity: sensitivity, | ||
| config: { frontendPort: portNum }, | ||
| })) | ||
| } | ||
| socket.onmessage = (event) => { | ||
| try { | ||
| const data = JSON.parse(event.data) | ||
| if (data.type === 'config-updated' && data.success) { | ||
| socket.close() | ||
| const newProtocol = window.location.protocol | ||
| const newHostname = window.location.hostname | ||
| const newUrl = `${newProtocol}//${newHostname}:${portNum}/settings` | ||
| setTimeout(() => { window.location.href = newUrl }, 800) | ||
| } else if (data.type === 'config-updated' && !data.success) { | ||
| alert('Failed to save config: ' + (data.error || 'Unknown error')) | ||
| } | ||
| })); | ||
|
|
||
| // Give server time to write config and restart | ||
| setTimeout(() => { | ||
| socket.close(); | ||
| const newProtocol = window.location.protocol; | ||
| const newHostname = window.location.hostname; | ||
| const newUrl = `${newProtocol}//${newHostname}:${frontendPort}/settings`; | ||
| window.location.href = newUrl; | ||
| }, 1000); | ||
| }; | ||
| } catch (_e) { | ||
| // ignore non-JSON messages | ||
| } | ||
| } |
There was a problem hiding this comment.
Save button has no guard against double-clicks or loading feedback.
If the user clicks "Save Config" multiple times, each click opens a new WebSocket and sends a config update. This could cause multiple redirects racing or confusing error messages. Consider disabling the button while the save is in progress and showing a loading indicator.
Additionally, the WebSocket created in the onClick handler is not cleaned up if the component unmounts mid-save (e.g., user navigates away). This is a minor leak since the page will redirect anyway, but worth noting.
Sketch: add a saving guard
+const [saving, setSaving] = useState(false)
...
<button
- className="btn btn-neutral w-full"
+ className="btn btn-neutral w-full"
+ disabled={saving}
onClick={() => {
+ if (saving) return
+ setSaving(true)
const portNum = parseInt(frontendPort, 10)
if (Number.isNaN(portNum) || portNum < 1 || portNum > 65535) {
alert('Please enter a valid port (1–65535).')
+ setSaving(false)
return
}
...
+ socket.onerror = () => {
+ alert('Could not connect to server. Is the app running?')
+ setSaving(false)
+ }
}}
>
- Save Config
+ {saving ? 'Saving…' : 'Save Config'}
</button>🧰 Tools
🪛 Biome (2.3.14)
[error] 231-271: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a form element. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset
(lint/a11y/useButtonType)
🤖 Prompt for AI Agents
In `@src/routes/settings.tsx` around lines 231 - 268, Add a saving guard and
cleanup: introduce a component state (e.g., isSaving) and check it at the start
of the Save button's onClick (referencing frontendPort and the button's onClick
handler) to ignore additional clicks and disable the button and show a loading
indicator while saving; when initiating the WebSocket (new WebSocket(wsUrl) /
socket) set isSaving=true, and on success/failure (socket.onopen,
socket.onmessage error branch, socket.onerror) set isSaving=false on failure and
proceed with redirect on success. Also store the socket and any setTimeout id in
refs so you can close socket and clearTimeout in a useEffect cleanup (to handle
component unmount) and ensure socket.close() is called on all terminal paths to
avoid leaks and racing redirects.
|
@imxade by the way i noticed that there are UI issues on light theme that is not the part of this issue/PR, we can solve that later |
create a separate issue with image/video |
…reshold/scroll logic
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/hooks/useTrackpadGesture.ts`:
- Around line 19-27: getClientSettings currently treats an empty string in
localStorage as a valid sensitivity (Number('') === 0), which can
unintentionally disable movement; update the validation in getClientSettings
(and the CLIENT_KEYS.SENSITIVITY usage) to reject empty or non-positive values —
e.g., require s !== '' && Number(s) > 0 (or similar) before using Number(s),
otherwise fall back to the default 1.0 so sensitivity is never zero or negative.
- Around line 21-27: getClientSettings currently ignores the hook's
_defaultSensitivity parameter and always uses 1.0; either remove the unused
_defaultSensitivity from the hook signature or make getClientSettings accept a
defaultSensitivity argument and use it when localStorage has no valid
sensitivity. Concretely: modify getClientSettings to accept defaultSensitivity
(number) and replace the hardcoded 1.0 fallback with that parameter, then update
the call site that invokes getClientSettings to pass _defaultSensitivity (the
hook parameter); alternatively, remove _defaultSensitivity from the
useTrackpadGesture signature and all callers to avoid a misleading unused
parameter.
- Around line 158-172: In useTrackpadGesture's scrollMode branch the code
negates sumX/sumY into scrollDx/scrollDy and then multiplies by scrollSign,
causing double negation and inverted behavior; to fix, stop negating at
assignment (set scrollDx = sumX and scrollDy = sumY or otherwise remove the
leading '-'), keep the later multiplication by scrollSign/sensitivity when
building the 'scroll' message, and ensure variables scrollMode, sumX, sumY,
scrollDx, scrollDy and scrollSign are used as described so single- and
two-finger scrolls share the same direction convention.
🧹 Nitpick comments (1)
src/hooks/useTrackpadGesture.ts (1)
143-146: Reading localStorage on everytouchmoveevent is unnecessary overhead.
getClientSettings()is called for every touch-move event, which fires at high frequency during gestures. While individuallocalStorage.getItemcalls are fast, they are synchronous and this adds up. Consider reading the settings once athandleTouchStart(or caching in a ref that refreshes on touch-start) and reusing the values throughout the gesture.♻️ Suggested approach (cache in refs, read on touch start)
Add refs alongside the existing ones:
+ const clientSensitivity = useRef(1.0); + const clientInvert = useRef(false);Read at the start of each gesture in
handleTouchStart:const handleTouchStart = (e: React.TouchEvent) => { + const cs = getClientSettings(); + clientSensitivity.current = cs.sensitivity; + clientInvert.current = cs.invert; if (ongoingTouches.current.length === 0) {Then in
handleTouchMove, replace the per-event read:- const { sensitivity, invert } = getClientSettings(); - const scrollSign = invert ? 1 : -1; + const sensitivity = clientSensitivity.current; + const scrollSign = clientInvert.current ? 1 : -1;
| const CLIENT_KEYS = { SENSITIVITY: 'rein_mouse_sensitivity', INVERT: 'rein_mouse_invert' } as const; | ||
|
|
||
| function getClientSettings(): { sensitivity: number; invert: boolean } { | ||
| if (typeof localStorage === 'undefined') return { sensitivity: 1.0, invert: false }; | ||
| const s = localStorage.getItem(CLIENT_KEYS.SENSITIVITY); | ||
| const sensitivity = s !== null && !Number.isNaN(Number(s)) ? Number(s) : 1.0; | ||
| const invert = localStorage.getItem(CLIENT_KEYS.INVERT) === 'true'; | ||
| return { sensitivity, invert }; | ||
| } |
There was a problem hiding this comment.
getClientSettings helper is clean and SSR-safe — nice addition.
The typeof localStorage guard and the Number.isNaN validation are well done. One minor note: Number('') is 0 and Number.isNaN(0) is false, so an empty string in localStorage would parse as sensitivity 0 (effectively disabling movement). If that's undesirable, consider adding Number(s) > 0 or s !== '' to the guard.
🤖 Prompt for AI Agents
In `@src/hooks/useTrackpadGesture.ts` around lines 19 - 27, getClientSettings
currently treats an empty string in localStorage as a valid sensitivity
(Number('') === 0), which can unintentionally disable movement; update the
validation in getClientSettings (and the CLIENT_KEYS.SENSITIVITY usage) to
reject empty or non-positive values — e.g., require s !== '' && Number(s) > 0
(or similar) before using Number(s), otherwise fall back to the default 1.0 so
sensitivity is never zero or negative.
| function getClientSettings(): { sensitivity: number; invert: boolean } { | ||
| if (typeof localStorage === 'undefined') return { sensitivity: 1.0, invert: false }; | ||
| const s = localStorage.getItem(CLIENT_KEYS.SENSITIVITY); | ||
| const sensitivity = s !== null && !Number.isNaN(Number(s)) ? Number(s) : 1.0; | ||
| const invert = localStorage.getItem(CLIENT_KEYS.INVERT) === 'true'; | ||
| return { sensitivity, invert }; | ||
| } |
There was a problem hiding this comment.
_defaultSensitivity parameter is accepted but never used; getClientSettings hardcodes its own default.
_defaultSensitivity (Line 32) is part of the public signature and callers may pass a custom value, but it is silently ignored — getClientSettings() always falls back to 1.0. Either plumb the default through:
-function getClientSettings(): { sensitivity: number; invert: boolean } {
- if (typeof localStorage === 'undefined') return { sensitivity: 1.0, invert: false };
+function getClientSettings(defaultSensitivity: number = 1.0): { sensitivity: number; invert: boolean } {
+ if (typeof localStorage === 'undefined') return { sensitivity: defaultSensitivity, invert: false };
const s = localStorage.getItem(CLIENT_KEYS.SENSITIVITY);
- const sensitivity = s !== null && !Number.isNaN(Number(s)) ? Number(s) : 1.0;
+ const sensitivity = s !== null && !Number.isNaN(Number(s)) ? Number(s) : defaultSensitivity;…and call it as getClientSettings(_defaultSensitivity) on Line 145, or remove the parameter from the hook signature to avoid misleading callers.
Also applies to: 32-33
🤖 Prompt for AI Agents
In `@src/hooks/useTrackpadGesture.ts` around lines 21 - 27, getClientSettings
currently ignores the hook's _defaultSensitivity parameter and always uses 1.0;
either remove the unused _defaultSensitivity from the hook signature or make
getClientSettings accept a defaultSensitivity argument and use it when
localStorage has no valid sensitivity. Concretely: modify getClientSettings to
accept defaultSensitivity (number) and replace the hardcoded 1.0 fallback with
that parameter, then update the call site that invokes getClientSettings to pass
_defaultSensitivity (the hook parameter); alternatively, remove
_defaultSensitivity from the useTrackpadGesture signature and all callers to
avoid a misleading unused parameter.
| } else if (scrollMode) { | ||
| // Scroll mode: single finger scrolls, or two-finger scroll in cursor mode | ||
| send({ type: 'scroll', dx: -sumX * sensitivity, dy: -sumY * sensitivity }); | ||
| // Scroll mode: single finger scrolls; dominant axis (from main) for cleaner scroll | ||
| const scrollDx = -sumX; | ||
| const scrollDy = -sumY; | ||
| const absDx = Math.abs(scrollDx); | ||
| const absDy = Math.abs(scrollDy); | ||
| const useHorizontal = absDx > axisThreshold * absDy; | ||
| const useVertical = absDy > axisThreshold * absDx; | ||
| const dx = useHorizontal && !useVertical ? scrollDx : 0; | ||
| const dy = useVertical && !useHorizontal ? scrollDy : 0; | ||
| send({ | ||
| type: 'scroll', | ||
| dx: Math.round(dx * sensitivity * scrollSign * 10) / 10, | ||
| dy: Math.round(dy * sensitivity * scrollSign * 10) / 10, | ||
| }); |
There was a problem hiding this comment.
invert setting works backwards in scrollMode due to the extra negation.
Lines 160–161 negate sumX/sumY (scrollDx = -sumX), but then Lines 170–171 also apply scrollSign (which already encodes inversion). The double negation means that when invert is false (scrollSign = −1), scrollMode scrolls in the opposite direction compared to 2-finger scroll — and toggling invert flips them out of sync.
Remove the upfront negation so both scroll paths share the same direction convention:
🐛 Proposed fix
} else if (scrollMode) {
- // Scroll mode: single finger scrolls; dominant axis (from main) for cleaner scroll
- const scrollDx = -sumX;
- const scrollDy = -sumY;
+ // Scroll mode: single finger scrolls; dominant axis for cleaner scroll
+ const scrollDx = sumX;
+ const scrollDy = sumY;
const absDx = Math.abs(scrollDx);
const absDy = Math.abs(scrollDy);📝 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.
| } else if (scrollMode) { | |
| // Scroll mode: single finger scrolls, or two-finger scroll in cursor mode | |
| send({ type: 'scroll', dx: -sumX * sensitivity, dy: -sumY * sensitivity }); | |
| // Scroll mode: single finger scrolls; dominant axis (from main) for cleaner scroll | |
| const scrollDx = -sumX; | |
| const scrollDy = -sumY; | |
| const absDx = Math.abs(scrollDx); | |
| const absDy = Math.abs(scrollDy); | |
| const useHorizontal = absDx > axisThreshold * absDy; | |
| const useVertical = absDy > axisThreshold * absDx; | |
| const dx = useHorizontal && !useVertical ? scrollDx : 0; | |
| const dy = useVertical && !useHorizontal ? scrollDy : 0; | |
| send({ | |
| type: 'scroll', | |
| dx: Math.round(dx * sensitivity * scrollSign * 10) / 10, | |
| dy: Math.round(dy * sensitivity * scrollSign * 10) / 10, | |
| }); | |
| } else if (scrollMode) { | |
| // Scroll mode: single finger scrolls; dominant axis for cleaner scroll | |
| const scrollDx = sumX; | |
| const scrollDy = sumY; | |
| const absDx = Math.abs(scrollDx); | |
| const absDy = Math.abs(scrollDy); | |
| const useHorizontal = absDx > axisThreshold * absDy; | |
| const useVertical = absDy > axisThreshold * absDx; | |
| const dx = useHorizontal && !useVertical ? scrollDx : 0; | |
| const dy = useVertical && !useHorizontal ? scrollDy : 0; | |
| send({ | |
| type: 'scroll', | |
| dx: Math.round(dx * sensitivity * scrollSign * 10) / 10, | |
| dy: Math.round(dy * sensitivity * scrollSign * 10) / 10, | |
| }); |
🤖 Prompt for AI Agents
In `@src/hooks/useTrackpadGesture.ts` around lines 158 - 172, In
useTrackpadGesture's scrollMode branch the code negates sumX/sumY into
scrollDx/scrollDy and then multiplies by scrollSign, causing double negation and
inverted behavior; to fix, stop negating at assignment (set scrollDx = sumX and
scrollDy = sumY or otherwise remove the leading '-'), keep the later
multiplication by scrollSign/sensitivity when building the 'scroll' message, and
ensure variables scrollMode, sumX, sumY, scrollDx, scrollDy and scrollSign are
used as described so single- and two-finger scrolls share the same direction
convention.
|
@imxade resolved conflicts |
2 participants
Summary
Ensures only server config is written to server-config.json. The server accepts and writes only host, frontendPort, and address; any other key from the client is ignored. Client-side settings (e.g. Server IP, theme, mouse sensitivity, invert scroll) stay on the client only and never enter server-config.json. Documents the split in code.
Fixes #55
Changes
src/server/websocket.tsupdate-config, only merge whitelisted server keys:host,frontendPort,address. Any other key from the client (e.g.mouseInvert,mouseSensitivity, or client-only data) is ignored so it never enters server-config.json.src/config.tsxsrc/routes/settings.tsxTesting
mouseInvert,mouseSensitivity, or any other key inupdate-configdoes not write them to the file. Server IP and other client settings remain client-side only.Summary by CodeRabbit
New Features
Bug Fixes