fix: infinite preference sync feedback loop and default item page size - #1632
fix: infinite preference sync feedback loop and default item page size#1632IceeAn wants to merge 2 commits into
Conversation
Summary by CodeRabbit
WalkthroughThe preference composable changes the default page size, synchronizes watcher updates, reconciles server snapshots with concurrent local edits, and coordinates retries when saving settings fails. ChangesPreference synchronization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PreferenceWatcher
participant PreferenceSync
participant SettingsAPI
PreferenceWatcher->>PreferenceSync: detect synchronous preference change
PreferenceSync->>SettingsAPI: save settings
SettingsAPI-->>PreferenceSync: return settings or throw error
PreferenceSync->>PreferenceSync: schedule retry or reconcile pending changes
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ Simplify code
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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/composables/use-preferences.ts`:
- Line 234: In the server-refresh flow around refreshViewPreferencesFromServer,
limit applyingServerSnapshot to only the synchronous preferences.value =
mergeSyncedSettings(...) assignment, clearing it before the awaited refresh
completes so edits during the pending request invoke markDirty() and increment
localRevision. Add a regression test covering a preference edit made before the
refresh resolves and verify the edit is not overwritten.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ac85487e-3126-4724-9f77-33d95681cddb
📒 Files selected for processing (1)
frontend/composables/use-preferences.ts
| markDirty(); | ||
| }, | ||
| { deep: true } | ||
| { deep: true, flush: "sync" } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the relevant composables file without running repository code.
if git ls-files | grep -Fx 'frontend/composables/use-preferences.ts' > /dev/null; then
echo "== file exists =="
wc -l frontend/composables/use-preferences.ts
echo
echo "== outline =="
ast-grep outline frontend/composables/use-preferences.ts --view compact || true
echo
echo "== relevant sections =="
sed -n '160,270p' frontend/composables/use-preferences.ts
else
echo "frontend/composables/use-preferences.ts not found"
echo "matching files:"
git ls-files | grep -i 'use-preferences' || true
fi
echo
echo "== watcher configuration usage =="
rg -n 'watch\s*\(|flush:\s*"sync"|applyingServerSnapshot|localRevision|refreshViewPreferencesFromServer' frontend/composables/use-preferences.tsRepository: sysadminsmedia/homebox
Length of output: 3448
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== import / helpers / preference mutations =="
sed -n '1,175p' frontend/composables/use-preferences.ts
echo
echo "== Vue package/version references for local environment constraints =="
git ls-files | grep -E '(^|/)(package.json|pnpm-lock.yaml|yarn.lock|package-lock.json)$' | while read -r f; do
echo "--- $f"
rg -n '"vue"|github:vuejs/vue|vuejs/vue|flush' "$f" | head -50 || true
doneRepository: sysadminsmedia/homebox
Length of output: 4908
🌐 Web query:
Vue 3 watch flush sync synchronous before DOM updates unbatched documentation
💡 Result:
In Vue 3, the flush: 'sync' option for watchers (watch and watchEffect) forces the watcher callback to execute synchronously as soon as a reactive dependency is mutated [1][2]. Unlike the default flush: 'pre' or the flush: 'post' options, which queue effects in a scheduler to batch updates, flush: 'sync' bypasses this batching entirely [1][3]. Key characteristics of flush: 'sync' include: - Immediate Execution: The callback is invoked immediately upon the change of a reactive dependency, before any Vue-managed component updates or subsequent state mutations are processed [1][2]. - No Batching: Because it is unbatched, if multiple reactive dependencies are mutated in a single synchronous block, the watcher will trigger repeatedly for each individual mutation [1]. - Use with Caution: Vue documentation strongly advises caution when using flush: 'sync'. It can lead to performance issues and data consistency problems—such as observing partial state updates—especially if watched sources are mutated frequently (e.g., arrays or complex objects) [1][2]. For most use cases, the default flush: 'pre' (before DOM updates) or flush: 'post' (after DOM updates) is preferred [1]. watchSyncEffect() is available as a convenient alias for watchEffect(() => {}, { flush: 'sync' }) [1][2].
Citations:
- 1: https://vuejs.org/guide/essentials/watchers
- 2: https://vuejs.org/api/reactivity-core
- 3: https://gist.github.com/AlexVipond/521c2452a2b7c94d950389638f22be58
Scope server-snapshot suppression to snapshot application only.
applyingServerSnapshot stays true during the full await of refreshViewPreferencesFromServer(preferences). An edit made while that refresh is pending will invoke the synchronous watcher after assignment, and markDirty() will not increment localRevision, so the later snapshot can overwrite the user input unless the server snapshot assigns exactly the current local value. Keep the suppression limited to preferences.value = mergeSyncedSettings(...) and add a regression test for editing preferences before refresh completes.
🤖 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 `@frontend/composables/use-preferences.ts` at line 234, In the server-refresh
flow around refreshViewPreferencesFromServer, limit applyingServerSnapshot to
only the synchronous preferences.value = mergeSyncedSettings(...) assignment,
clearing it before the awaited refresh completes so edits during the pending
request invoke markDirty() and increment localRevision. Add a regression test
covering a preference edit made before the refresh resolves and verify the edit
is not overwritten.
Source: MCP tools
3e36a51 to
cb90a49
Compare
|
Preference merging now preserves local edits made while a server refresh is in progress. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@frontend/composables/use-preferences.ts`:
- Around line 89-118: Guard both merge/diff loops against the keys "__proto__",
"constructor", and "prototype" before reading or assigning them. Update
mergeSyncedValue and the analogous getPreferenceChange builder to skip these
keys, preserving normal handling for all other preference fields and preventing
prototype mutation on the result objects.
- Around line 246-253: Update the setSettings handling in the preferences sync
flow to remove the error variable and the subsequent error check, since the
Result response only exposes item. Retain the existing catch block and
scheduleRetry behavior for rejected requests.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 73c6d814-d4ad-445a-b503-0639c63dc797
📒 Files selected for processing (1)
frontend/composables/use-preferences.ts
| function isPlainObject(value: unknown): value is Record<string, unknown> { | ||
| return value !== null && typeof value === "object" && !Array.isArray(value); | ||
| } | ||
|
|
||
| function mergeSyncedValue(serverValue: unknown, localValue: unknown, localChange?: PreferenceChange): unknown { | ||
| if (localChange === undefined) { | ||
| return serverValue; | ||
| } | ||
|
|
||
| if (localChange === true || !isPlainObject(serverValue) || !isPlainObject(localValue)) { | ||
| return localValue; | ||
| } | ||
|
|
||
| const mergedValue: Record<string, unknown> = { ...serverValue }; | ||
| const keys = new Set([...Object.keys(serverValue), ...Object.keys(localValue)]); | ||
|
|
||
| for (const key of keys) { | ||
| const nestedChange = localChange[key]; | ||
| if (nestedChange !== undefined) { | ||
| mergedValue[key] = mergeSyncedValue(serverValue[key], localValue[key], nestedChange); | ||
| continue; | ||
| } | ||
|
|
||
| if (!(key in mergedValue)) { | ||
| mergedValue[key] = localValue[key]; | ||
| } | ||
| } | ||
|
|
||
| return mergedValue; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Guard against __proto__/constructor/prototype keys in the merge/diff loops.
mergeSyncedValue iterates Object.keys(serverValue) ∪ Object.keys(localValue) and writes via mergedValue[key] = ...; the analogous diff builder (getPreferenceChange) does the same into a fresh changedFields = {}. Since serverValue originates from JSON.parse'd server data, a payload containing a literal "__proto__" key is a normal own property there, but once it's re-assigned via computed bracket notation onto an object that doesn't already own that key, the assignment invokes the real __proto__ setter and mutates that object's prototype link instead of a plain data property. Skip these dangerous keys defensively.
🛡️ Proposed fix
+const UNSAFE_KEYS = new Set(["__proto__", "constructor", "prototype"]);
+
function mergeSyncedValue(serverValue: unknown, localValue: unknown, localChange?: PreferenceChange): unknown {
...
for (const key of keys) {
+ if (UNSAFE_KEYS.has(key)) {
+ continue;
+ }
+
const nestedChange = localChange[key]; for (const key of keys) {
+ if (UNSAFE_KEYS.has(key)) {
+ continue;
+ }
+
const nestedChange = getPreferenceChange(previousValue[key], nextValue[key]);Also applies to: 140-178
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 104-114: Recursive/iterative merge copies attacker-controllable keys from a source object into a target via a computed property assignment without rejecting dangerous keys, allowing prototype pollution. Skip or block "proto", "constructor", and "prototype" keys (e.g. if (key === "__proto__" || key === "constructor" || key === "prototype") continue;), use a null-prototype object (Object.create(null)), or use a safe merge utility instead.
Context: for (const key of keys) {
const nestedChange = localChange[key];
if (nestedChange !== undefined) {
mergedValue[key] = mergeSyncedValue(serverValue[key], localValue[key], nestedChange);
continue;
}
if (!(key in mergedValue)) {
mergedValue[key] = localValue[key];
}
}
Note: [CWE-1321] Improperly Controlled Modification of Object Prototype Attributes ('Prototype Pollution').
(prototype-pollution-recursive-merge-typescript)
🤖 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 `@frontend/composables/use-preferences.ts` around lines 89 - 118, Guard both
merge/diff loops against the keys "__proto__", "constructor", and "prototype"
before reading or assigning them. Update mergeSyncedValue and the analogous
getPreferenceChange builder to skip these keys, preserving normal handling for
all other preference fields and preventing prototype mutation on the result
objects.
Source: Linters/SAST tools
| let error = false; | ||
| try { | ||
| ({ error } = await api.user.setSettings(buildSyncedSettings(preferences.value))); | ||
| } catch { | ||
| scheduleRetry(); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "type Result" frontend/lib -A 10
rg -n "public setSettings" -A 6 frontend/lib/api/classes/users.ts
ast-grep run --pattern 'interface Result<$T> { $$$ }' --lang typescript frontend/libRepository: sysadminsmedia/homebox
Length of output: 979
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== non-generated API result type =="
cat -n frontend/lib/api/types/non-generated.ts
echo
echo "== users.ts outline and relevant methods =="
wc -l frontend/lib/api/classes/users.ts
ast-grep outline frontend/lib/api/classes/users.ts --view expanded || true
sed -n '1,80p' frontend/lib/api/classes/users.ts
echo
echo "== use-preferences around setSettings usage =="
wc -l frontend/composables/use-preferences.ts
sed -n '220,270p' frontend/composables/use-preferences.ts
echo
echo "== search Result definitions/usages =="
rg -n "export type Result|interface Result|class Result|type Result" frontend/lib frontend/composables -A 8 -B 2
echo
echo "== http implementation type helpers if present =="
rg -n "class Api|interface Http|put<|\.put<" frontend/lib/api -A 8 -B 2Repository: sysadminsmedia/homebox
Length of output: 12723
Drop the error check; Result<T> has no error field.
UserApi.setSettings(...) returns Result<Record<string, unknown>>, and frontend/lib/api/types/non-generated.ts defines Result<T> as { item: T }, so ({ error } = await ...) and the subsequent if (error) have no matching response shape.
🤖 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 `@frontend/composables/use-preferences.ts` around lines 246 - 253, Update the
setSettings handling in the preferences sync flow to remove the error variable
and the subsequent error check, since the Result response only exposes item.
Retain the existing catch block and scheduleRetry behavior for rejected
requests.
1 participant
What type of PR is this?
What this PR does / why we need it:
This PR fixes the infinite loop issue involving requests to
v1/users/self/settings, which occurs when settings are modified under poor network conditions (when the request takes longer than 1s). It can be reproduced by setting the network throttling to 3G in Chrome DevTools.It also sets default item page size to 12. The default page size is 10 but the available options are 12/24/48/96. Also, setting the page size to 10 leaves empty space at the end when each row displays 3 or 4 cards.
frontend/composables/use-preferences.ts:Which issue(s) this PR fixes:
Fixes a bug related to #1617
Special notes for your reviewer:
This PR fixes another bug mentioned in #1617 , but not the main one.