feat(frontend): 設定プロファイルの同期 - #17803
Open
syuilo wants to merge 22 commits into
Open
Conversation
📝 WalkthroughWalkthrough設定値に項目単位の変更日時を追加しました。プロファイル統合、クラウドバックアップ、デバイス間同期、自動同期、設定画面、メニュー表示、ローカライズ、設計仕様書を更新しました。 Changes設定クラウド同期
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant 設定ストア
participant cloudSync
participant PreferencesManager
participant StorageProvider
設定ストア->>cloudSync: 自動同期が有効な状態で起動
cloudSync->>StorageProvider: クラウド設定を取得
StorageProvider-->>cloudSync: 値とmodifiedAtを返却
cloudSync->>PreferencesManager: mergeProfiles()を実行
PreferencesManager-->>cloudSync: 統合済みプロファイルを返却
cloudSync->>PreferencesManager: プロファイルを再読み込み
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #17803 +/- ##
===========================================
- Coverage 14.00% 13.93% -0.07%
===========================================
Files 248 248
Lines 12042 12098 +56
Branches 4042 4057 +15
===========================================
Hits 1686 1686
- Misses 8117 8162 +45
- Partials 2239 2250 +11 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
syuilo
marked this pull request as ready for review
August 3, 2026 13:23
dosubot
Bot
added
the
size:L
Contributor
🖥 Frontend Diagnostics Report(No significant changes) Requests by resource type
V8 heap snapshot statistics
📦 Bundle StatsChunk size diff (6 updated, 0 added, 0 removed)
Startup chunk size (1 updated, 0 added, 0 removed)
Startup chunks are the Vite entry for
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/frontend/src/pages/settings/other.vue (1)
240-249: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
forceCloudBackup/forceCloudSyncにエラーハンドリングがありません。
forceCloudSync(および同じパターンを持つforceCloudBackup)はcloudBackup()/cloudSync()をtry/catchなしで呼び出しています。これらの関数がエラーを投げた場合(例えば、manager.tsのmergeProfilesに関するコメントで挙げたスキーマ不一致によるクラッシュや、通信エラー)、os.success()が呼ばれないだけで、ユーザーには失敗したことが一切通知されません。
PreferencesManager.enableSync()では同様のクラウド操作の失敗時にos.alertでエラーを通知するパターンが既にあります。同様のエラーハンドリングをこの2つの関数にも追加することをおすすめします。🛡️ 修正案
async function forceCloudSync() { - await cloudSync(); - os.success(); + try { + await cloudSync(); + os.success(); + } catch (err) { + os.alert({ + type: 'error', + title: i18n.ts.somethingHappened, + }); + console.error(err); + } }🤖 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 `@packages/frontend/src/pages/settings/other.vue` around lines 240 - 249, Update forceCloudBackup and forceCloudSync to wrap their cloudBackup and cloudSync calls in try/catch handling, respectively. Preserve os.success() only for successful operations, and notify the user of failures through the existing os.alert pattern used by PreferencesManager.enableSync().packages/frontend/src/preferences/manager.ts (1)
550-572: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
enableSyncでcommit()の戻り値が実際には使われていません。Line 550 で
commitedRecordを受け取っていますが、以降の Line 555 のcloudSet呼び出しと Line 571 のrecord[2].sync = trueは、いずれも Line 539 で取得した古いrecordをそのまま使い続けています。この実装には2つの問題があります。
1つ目は、対象キーがアカウント依存・サーバー依存の設定で、まだアカウント・サーバー固有のスコープを持っていない場合です。この場合
commit()(Line 296-317)は新しいスコープのレコードを作成して返しますが、recordは古い(より汎用的な)スコープのレコードのままです。そのためcloudSetは誤ったスコープと古いmodifiedAtを送信し、Line 571 のrecord[2].sync = trueも実際に値を保持している新しいレコードではなく、古いレコードに設定されてしまいます。結果として、UI 上は同期が有効に見えても、実際にはその後のcommit()が正しいレコードのsyncフラグを見つけられず、同期が機能しなくなります。2つ目は、
newValueが現在値と同じ場合です。この場合commit()はdeepEqualによりnullを返す(Line 285-288)ためmodifiedAtは更新されず、古い値またはundefinedのままcloudSetに送られます。今後のmergeProfilesによる比較で、このレコードは常に他の値に負けてしまう可能性があります。
commitedRecordを実際に使用するよう修正することをご検討ください。🐛 修正案
- const commitedRecord = this.commit(key, newValue); + const commitedRecord = this.commit(key, newValue) ?? this.getMatchedRecordOf(key); const done = os.waiting(); try { - await this.io.cloudSet({ key, scope: record[0], value: newValue, meta: { modifiedAt: record[2].modifiedAt } }); + await this.io.cloudSet({ key, scope: commitedRecord[0], value: newValue, meta: { modifiedAt: commitedRecord[2].modifiedAt ?? Date.now() } }); } catch (err) { ... } done({ success: true }); - record[2].sync = true; + commitedRecord[2].sync = true; this.save();🤖 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 `@packages/frontend/src/preferences/manager.ts` around lines 550 - 572, Update enableSync to use the record returned by commit() for cloudSet’s scope and modifiedAt metadata and for setting sync=true, rather than the stale record captured before commit. Handle commit() returning null for an unchanged value without sending undefined or stale metadata, while preserving the existing success and error flows.packages/frontend/src/preferences.ts (1)
55-83: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
cloudBackupもcloudRead/cloudSet経由で同期値を消さないよう分岐してください。
cloudBackup()はバックアップキーを取得・マージしてi/registry/setで保存しますが、ここからcloudSet()を呼ぶと同じキーのclient.preferences.sync配列全体が新規の更新対象で上書きされます。同期フラグ付き設定値や既存のsyncスコープ値を消さないよう、cloudSet()の対象を同步設定の更新のみに絞るか、同期値を合成して書き戻す実装にしてください。
cloudSet()自体もi/registry/get→ 配列更新 →i/registry/setの非アトミックな read-modify-write なので、別のタブ・デバイスで同じキーの別スコープが更新されると後発の書き込みで先発の更新分を失う可能性があります。docs/preferences.mdの「設定値が意図せず失われることが絶対にあってはならない」の設計要件に合わせて、サーバー側の更新条件や compare-and-swap を活用してください。🤖 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 `@packages/frontend/src/preferences.ts` around lines 55 - 83, Update cloudBackup and cloudSet so backup writes preserve existing client.preferences.sync entries and do not overwrite synchronization values outside the intended scope. Make cloudSet’s registry update atomic by using the server-side conditional update or compare-and-swap mechanism, retrying on conflicts as needed so concurrent tabs or devices cannot lose each other’s scope updates.
🤖 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 `@packages/frontend/docs/preferences.md`:
- Around line 7-16: preferences.md に mergeProfiles
の衝突解決規則を追加し、設定消失を防ぐ判定契約を明文化してください。modifiedAt
が同値または欠落した場合、端末時計のずれ、削除・初期値への復元、再アップロード時の古い値による上書きをどう扱うかを明示し、常に新しい変更を保持できるルールにしてください。
- Around line 40-42: Update the same-profile sharing section in preferences
documentation to state that sharing an entire profile across devices is not
recommended, distinguish it from item-level synchronization via
syncBetweenDevices, and document that autoBackup requires a named profile
through youNeedToNameYourProfileToEnableAutoBackup. Replace the recommended
procedure with using separate profiles and enabling syncBetweenDevices for
item-level synchronization, while documenting the relevant sync targets and
conflict behavior.
In `@packages/frontend/src/preferences/manager.ts`:
- Around line 398-420: Update the comparison in fetchCloudValues to compare
cloudValue.value with record[1], not the metadata wrapper cloudValue. Preserve
the existing rewriteRawState, modified tracking, and save behavior so they run
only when the actual preference value changes.
- Around line 184-221: Update mergeProfiles to defensively handle missing
preference records for any key in PREF_DEF: treat undefined a.preferences[key]
or b.preferences[key] as an empty record list before copying or iterating.
Preserve the existing per-scope, latest-modifiedAt merge behavior and avoid
mutating either input profile.
In `@packages/frontend/src/preferences/utility.ts`:
- Around line 219-246: Update cloudBackup to handle concurrent executions
without losing either device or tab’s merged changes. Protect the i/registry/get
→ mergeProfiles → i/registry/set sequence with the same concurrency or
conflict-resolution approach used by cloudSet, such as server-side optimistic
locking, atomic per-key updates, or retrying after conflict with a fresh read,
while preserving the existing backup timestamp update after a successful write.
---
Outside diff comments:
In `@packages/frontend/src/pages/settings/other.vue`:
- Around line 240-249: Update forceCloudBackup and forceCloudSync to wrap their
cloudBackup and cloudSync calls in try/catch handling, respectively. Preserve
os.success() only for successful operations, and notify the user of failures
through the existing os.alert pattern used by PreferencesManager.enableSync().
In `@packages/frontend/src/preferences.ts`:
- Around line 55-83: Update cloudBackup and cloudSet so backup writes preserve
existing client.preferences.sync entries and do not overwrite synchronization
values outside the intended scope. Make cloudSet’s registry update atomic by
using the server-side conditional update or compare-and-swap mechanism, retrying
on conflicts as needed so concurrent tabs or devices cannot lose each other’s
scope updates.
In `@packages/frontend/src/preferences/manager.ts`:
- Around line 550-572: Update enableSync to use the record returned by commit()
for cloudSet’s scope and modifiedAt metadata and for setting sync=true, rather
than the stale record captured before commit. Handle commit() returning null for
an unchanged value without sending undefined or stale metadata, while preserving
the existing success and error flows.
🪄 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 Plus
Run ID: d0860406-651f-4605-821a-0a1936c20c4d
⛔ Files ignored due to path filters (1)
CHANGELOG.mdis excluded by!CHANGELOG.md
📒 Files selected for processing (10)
locales/ja-JP.ymlpackages/frontend/docs/preferences.mdpackages/frontend/src/components/MkMenu.vuepackages/frontend/src/pages/settings/other.vuepackages/frontend/src/preferences.tspackages/frontend/src/preferences/manager.tspackages/frontend/src/preferences/utility.tspackages/frontend/src/store.tspackages/frontend/src/types/menu.tspackages/i18n/src/autogen/locale.ts
Comment on lines
+7
to
+16
| 実装上のミスで、**設定値が意図せず失われる(古い値で上書きされる)ことが絶対にあってはならない。** | ||
|
|
||
| 設定値が失われる、考えられるシナリオの例: | ||
|
|
||
| - 複数のタブでMisskeyを開いていて、タブAで設定を編集した後、タブBを開いたところ、タブBの古い状態の内容で設定が保存され、タブAで行った編集が巻き戻ってしまった。 | ||
| - プロファイルの同期機能をオンにしたところ、サーバーに保存されていた古い設定でローカルが上書きされてしまった。 | ||
|
|
||
| 上記のシナリオが絶対に発生しない設計・実装にしなければならない。 | ||
|
|
||
| 上記以外にも考えられるシナリオがあれば、必ず適切な対処を行う設計・実装にしなければならない。 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 \
'modifiedAt|merge|autoBackup|autoSync|cloudBackup|cloudSync|conflict|retry|Date\.now' \
packages/frontend/src/preferences/manager.ts \
packages/frontend/src/preferences.ts \
packages/frontend/src/preferences/utility.ts \
packages/frontend/src/store.tsRepository: misskey-dev/misskey
Length of output: 39387
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== docs =="
sed -n '1,80p' packages/frontend/docs/preferences.md
echo "== manager merged lines =="
sed -n '177,216p' packages/frontend/src/preferences/manager.ts
echo "== utility merge backup/sync sections =="
sed -n '197,245p' packages/frontend/src/preferences/utility.ts
echo "== tests mentioning preferences merge/timestamp =="
rg -n -C 6 'mergeProfiles|modifiedAt|preferences.*backup|preferences.*sync'| packages/frontend/test 2>/dev/null || trueRepository: misskey-dev/misskey
Length of output: 3813
設定値が失われないことを保証する衝突解決規則を仕様に追加してください。
packages/frontend/src/preferences/manager.ts の mergeProfiles は modifiedAt で最新を選択していますが、この文書では判定契約自体が規定されていません。同じ値の modifiedAt、欠落値、端末時計のずれ、削除や初期値への復元、再アップロード時の古い設定で上書きされない再試行などについて、明示的なルールとして追加してください。
🤖 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 `@packages/frontend/docs/preferences.md` around lines 7 - 16, preferences.md に
mergeProfiles の衝突解決規則を追加し、設定消失を防ぐ判定契約を明文化してください。modifiedAt
が同値または欠落した場合、端末時計のずれ、削除・初期値への復元、再アップロード時の古い値による上書きをどう扱うかを明示し、常に新しい変更を保持できるルールにしてください。
Comment on lines
+40
to
+42
| ### 同じプロファイルを複数のデバイスで使いたい(同期したい) | ||
|
|
||
| autoBackupとautoSyncをオンにする。 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 \
'shareSameProfileBetweenDevicesIsNotRecommended|useSyncBetweenDevicesOptionIfYouWantToSyncSetting|autoSyncAreYouSure_description|youNeedToNameYourProfileToEnableAutoBackup|autoBackup|autoSync|syncBetweenDevices' \
packages/frontend/src/preferences/manager.ts \
packages/frontend/src/preferences.ts \
packages/frontend/src/preferences/utility.ts \
packages/frontend/src/pages/settings/other.vue \
locales/ja-JP.ymlRepository: misskey-dev/misskey
Length of output: 12033
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== docs/preferences.md around referenced lines =="
sed -n '20,60p' packages/frontend/docs/preferences.md
echo
echo "== syncBetweenDevices references =="
rg -n -C 8 'syncBetweenDevices|overrideByAccount|modifyPreference|getPreference|setPreference|pref\.enablePreferencesAutoCloudSync|enablePreferencesAutoCloudSync|enablePreferencesAutoCloudBackup|cloudSync|cloudBackup|_preferencesProfile|shareSameProfileBetweenDevicesIsNotRecommended|useSyncBetweenDevicesOptionIfYouWantToSyncSetting|autoSyncAreYouSure_description' packages/frontend/src packages/frontend/docs || true
echo
echo "== store schema preferences keys =="
rg -n "enablePreferencesAutoCloudSync|enablePreferencesAutoCloudBackup|preventAutoBackup|latestPreferencesBackupAt" packages/frontend/src locales/ja-JP.ymlRepository: misskey-dev/misskey
Length of output: 43442
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== packages/frontend/src/preferences/utility.ts cloudBackup/cloudSync/mergeProfiles =="
sed -n '196,270p' packages/frontend/src/preferences/utility.ts
echo
echo "== packages/frontend/src/preferences/manager.ts sync-related registry methods =="
rg -n -C 12 'isSyncEnabled|set.*Sync|Sync|autoBackup|autoSync|cloudSync|cloudBackup|Registry|registry/get|registry/set|registry/delete|prevent' packages/frontend/src/preferences/manager.ts packages/frontend/src/preferences/utility.ts
echo
echo "== packages/frontend/src/preferences.ts cloud backup/sync periodics =="
sed -n '100,140p' packages/frontend/src/preferences.ts
echo
echo "== packages/frontend/src/pages/settings/index.vue auto backup button prompt =="
sed -n '18,23p;18,22p' packages/frontend/src/pages/settings/index.vueRepository: misskey-dev/misskey
Length of output: 28247
同一プロファイル共有の手順と前提条件を追加してください。
autoBackup と autoSync でプロファイル全体を複数デバイス間で共有できますが、この手順の冒頭だけで非推奨の前提条件や、同期対象・競合時の動作が揃っておりません。指定したいのはプロファイル全体の共有であれば、同一プロファイル共有が非推奨であること、syncBetweenDevices を有効にする項目単位同期とは明確に区別すること、そして autoBackup にプロファイル名が必要なので youNeedToNameYourProfileToEnableAutoBackup もこの手順の前提として記述してください。推奨は項目単位の同期であれば、手順を別プロファイルと syncBetweenDevices の利用に変更してください。
🤖 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 `@packages/frontend/docs/preferences.md` around lines 40 - 42, Update the
same-profile sharing section in preferences documentation to state that sharing
an entire profile across devices is not recommended, distinguish it from
item-level synchronization via syncBetweenDevices, and document that autoBackup
requires a named profile through youNeedToNameYourProfileToEnableAutoBackup.
Replace the recommended procedure with using separate profiles and enabling
syncBetweenDevices for item-level synchronization, while documenting the
relevant sync targets and conflict behavior.
Comment on lines
+184
to
+221
| // 各recordについて、modifiedAtが大きい方を採用する | ||
| // 引数の参照をmutateしないように注意すること | ||
| export function mergeProfiles(a: PreferencesProfile, b: PreferencesProfile): PreferencesProfile { | ||
| const merged = { | ||
| ...a, | ||
| modifiedAt: Math.max(a.modifiedAt, b.modifiedAt), | ||
| preferences: {}, | ||
| } as PreferencesProfile; | ||
|
|
||
| for (const _key in PREF_DEF) { | ||
| const key = _key as keyof PREF; | ||
| const aRecords = a.preferences[key]; | ||
| const bRecords = b.preferences[key]; | ||
|
|
||
| const mergedRecords = [...aRecords]; | ||
|
|
||
| for (const bRecord of bRecords) { | ||
| const existingIndex = mergedRecords.findIndex(([scope]) => isSameScope(scope, bRecord[0])); | ||
| if (existingIndex === -1) { | ||
| mergedRecords.push(bRecord); | ||
| } else { | ||
| const aRecord = mergedRecords[existingIndex]; | ||
| if ((bRecord[2].modifiedAt ?? 0) > (aRecord[2].modifiedAt ?? 0)) { | ||
| mergedRecords[existingIndex] = bRecord; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| (merged.preferences[key] as PrefRecord<typeof key>[]) = mergedRecords; | ||
| } | ||
|
|
||
| return merged; | ||
| } | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-empty-object-type | ||
| type PreferencesManagerEvents = { | ||
| }; | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
mergeProfiles はマージ前にプロファイルの正規化を必要とします。
mergeProfiles は、a.preferences[key] と b.preferences[key] が PREF_DEF の全キーに対応するレコードを持つことを前提にしています。しかし呼び出し元の cloudSync(preferences/utility.ts Line 200-203)や cloudBackup(同 Line 229-232)では、misskeyApi('i/registry/get', ...) as PreferencesProfile | null で取得した値を normalizePreferences() に通さずそのまま mergeProfiles の引数に渡しています。
もしサーバー上のプロファイルが別バージョンのアプリで保存されたもので、現在の PREF_DEF にあるキーを持っていない場合、Line 196 の bRecords = b.preferences[key] は undefined になります。この状態で Line 198-200 の for (const bRecord of bRecords) を実行すると、undefined はイテレート不可のため例外が発生します。
この例外は forceCloudSync・forceCloudBackup(pages/settings/other.vue)などの呼び出し元で捕捉されておらず、ユーザーには何も通知されないまま処理が失敗する可能性があります。docs/preferences.md の指針では「設定値が意図せず失われることが絶対にあってはならない」と明記されているため、想定外のスキーマ差異によるクラッシュは重要度が高い懸念だと考えます。
aRecords/bRecords を undefined に対して防御的にしておくことをご検討ください。
🛡️ 修正案
for (const _key in PREF_DEF) {
const key = _key as keyof PREF;
- const aRecords = a.preferences[key];
- const bRecords = b.preferences[key];
+ const aRecords = a.preferences[key] ?? [];
+ const bRecords = b.preferences[key] ?? [];📝 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
| // 各recordについて、modifiedAtが大きい方を採用する | |
| // 引数の参照をmutateしないように注意すること | |
| export function mergeProfiles(a: PreferencesProfile, b: PreferencesProfile): PreferencesProfile { | |
| const merged = { | |
| ...a, | |
| modifiedAt: Math.max(a.modifiedAt, b.modifiedAt), | |
| preferences: {}, | |
| } as PreferencesProfile; | |
| for (const _key in PREF_DEF) { | |
| const key = _key as keyof PREF; | |
| const aRecords = a.preferences[key]; | |
| const bRecords = b.preferences[key]; | |
| const mergedRecords = [...aRecords]; | |
| for (const bRecord of bRecords) { | |
| const existingIndex = mergedRecords.findIndex(([scope]) => isSameScope(scope, bRecord[0])); | |
| if (existingIndex === -1) { | |
| mergedRecords.push(bRecord); | |
| } else { | |
| const aRecord = mergedRecords[existingIndex]; | |
| if ((bRecord[2].modifiedAt ?? 0) > (aRecord[2].modifiedAt ?? 0)) { | |
| mergedRecords[existingIndex] = bRecord; | |
| } | |
| } | |
| } | |
| (merged.preferences[key] as PrefRecord<typeof key>[]) = mergedRecords; | |
| } | |
| return merged; | |
| } | |
| // eslint-disable-next-line @typescript-eslint/no-empty-object-type | |
| type PreferencesManagerEvents = { | |
| }; | |
| // 各recordについて、modifiedAtが大きい方を採用する | |
| // 引数の参照をmutateしないように注意すること | |
| export function mergeProfiles(a: PreferencesProfile, b: PreferencesProfile): PreferencesProfile { | |
| const merged = { | |
| ...a, | |
| modifiedAt: Math.max(a.modifiedAt, b.modifiedAt), | |
| preferences: {}, | |
| } as PreferencesProfile; | |
| for (const _key in PREF_DEF) { | |
| const key = _key as keyof PREF; | |
| const aRecords = a.preferences[key] ?? []; | |
| const bRecords = b.preferences[key] ?? []; | |
| const mergedRecords = [...aRecords]; | |
| for (const bRecord of bRecords) { | |
| const existingIndex = mergedRecords.findIndex(([scope]) => isSameScope(scope, bRecord[0])); | |
| if (existingIndex === -1) { | |
| mergedRecords.push(bRecord); | |
| } else { | |
| const aRecord = mergedRecords[existingIndex]; | |
| if ((bRecord[2].modifiedAt ?? 0) > (aRecord[2].modifiedAt ?? 0)) { | |
| mergedRecords[existingIndex] = bRecord; | |
| } | |
| } | |
| } | |
| (merged.preferences[key] as PrefRecord<typeof key>[]) = mergedRecords; | |
| } | |
| return merged; | |
| } | |
| // eslint-disable-next-line `@typescript-eslint/no-empty-object-type` | |
| type PreferencesManagerEvents = { | |
| }; |
🤖 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 `@packages/frontend/src/preferences/manager.ts` around lines 184 - 221, Update
mergeProfiles to defensively handle missing preference records for any key in
PREF_DEF: treat undefined a.preferences[key] or b.preferences[key] as an empty
record list before copying or iterating. Preserve the existing per-scope,
latest-modifiedAt merge behavior and avoid mutating either input profile.
Comment on lines
+398
to
+420
| let modified = false; | ||
|
|
||
| for (const _key in PREF_DEF) { | ||
| const key = _key as keyof PREF; | ||
| const record = this.getMatchedRecordOf(key); | ||
| if (record[2].sync && Object.hasOwn(cloudValues, key) && cloudValues[key] !== undefined) { | ||
| const cloudValue = cloudValues[key]; | ||
| if (!deepEqual(cloudValue, record[1])) { | ||
| this.rewriteRawState(key, cloudValue); | ||
| record[1] = cloudValue; | ||
| this.rewriteRawState(key, cloudValue.value); | ||
| record[1] = cloudValue.value; | ||
| record[2].modifiedAt = cloudValue.meta.modifiedAt; | ||
| modified = true; | ||
| if (_DEV_) console.log('cloud fetched', key, cloudValue); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| this.save(); | ||
| if (modified) this.save(); | ||
|
|
||
| if (_DEV_) console.log('cloud fetch completed'); | ||
| } | ||
|
|
||
| public save() { | ||
| private save() { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
fetchCloudValues の比較対象の型が一致していません。
Line 405 の deepEqual(cloudValue, record[1]) は、cloudValue({ value: ValueOf<K>; meta: {...} } という形の値)と record[1](値そのもの)を比較しています。cloudGetBulk(preferences.ts Line 90-93)の戻り値の形からも分かる通り、両者は形が異なるため、この比較はほぼ常に不一致と判定されます。
その結果、クラウド同期(sync)が有効な設定項目が1つでもあると、ページを読み込むたびに実際の値が変わっていなくても modified が true になり、rewriteRawState と this.save() が毎回実行されます。これは、このセグメントの変更意図として記載されている「変更があった場合のみプロファイルを保存し」という方針に反しています。
さらに this.save() は他のタブへ storage イベントを発生させるため、開いている他のタブで不要な reloadProfile() が繰り返し呼び出される可能性もあります。比較対象を cloudValue.value に修正することをおすすめします。
🐛 修正案
- if (!deepEqual(cloudValue, record[1])) {
+ if (!deepEqual(cloudValue.value, record[1])) {📝 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
| let modified = false; | |
| for (const _key in PREF_DEF) { | |
| const key = _key as keyof PREF; | |
| const record = this.getMatchedRecordOf(key); | |
| if (record[2].sync && Object.hasOwn(cloudValues, key) && cloudValues[key] !== undefined) { | |
| const cloudValue = cloudValues[key]; | |
| if (!deepEqual(cloudValue, record[1])) { | |
| this.rewriteRawState(key, cloudValue); | |
| record[1] = cloudValue; | |
| this.rewriteRawState(key, cloudValue.value); | |
| record[1] = cloudValue.value; | |
| record[2].modifiedAt = cloudValue.meta.modifiedAt; | |
| modified = true; | |
| if (_DEV_) console.log('cloud fetched', key, cloudValue); | |
| } | |
| } | |
| } | |
| this.save(); | |
| if (modified) this.save(); | |
| if (_DEV_) console.log('cloud fetch completed'); | |
| } | |
| public save() { | |
| private save() { | |
| let modified = false; | |
| for (const _key in PREF_DEF) { | |
| const key = _key as keyof PREF; | |
| const record = this.getMatchedRecordOf(key); | |
| if (record[2].sync && Object.hasOwn(cloudValues, key) && cloudValues[key] !== undefined) { | |
| const cloudValue = cloudValues[key]; | |
| if (!deepEqual(cloudValue.value, record[1])) { | |
| this.rewriteRawState(key, cloudValue.value); | |
| record[1] = cloudValue.value; | |
| record[2].modifiedAt = cloudValue.meta.modifiedAt; | |
| modified = true; | |
| if (_DEV_) console.log('cloud fetched', key, cloudValue); | |
| } | |
| } | |
| } | |
| if (modified) this.save(); | |
| if (_DEV_) console.log('cloud fetch completed'); | |
| } | |
| private save() { |
🤖 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 `@packages/frontend/src/preferences/manager.ts` around lines 398 - 420, Update
the comparison in fetchCloudValues to compare cloudValue.value with record[1],
not the metadata wrapper cloudValue. Preserve the existing rewriteRawState,
modified tracking, and save behavior so they run only when the actual preference
value changes.
Comment on lines
219
to
246
| export async function cloudBackup() { | ||
| if ($i == null) return; | ||
| if (!canAutoBackup()) { | ||
| throw new Error('cannot auto backup for this profile'); | ||
| } | ||
|
|
||
| let currentProfile = prefer.profile; | ||
|
|
||
| if (_DEV_) console.log('cloud backup', currentProfile); | ||
|
|
||
| const backupedProfile = await misskeyApi('i/registry/get', { | ||
| scope: ['client', 'preferences', 'backups'], | ||
| key: prefer.profile.name, | ||
| }) as PreferencesProfile | null; | ||
|
|
||
| // 古い設定で新しいバックアップを上書きしないようにマージ | ||
| if (backupedProfile != null) { | ||
| currentProfile = mergeProfiles(currentProfile, backupedProfile); | ||
| } | ||
|
|
||
| await misskeyApi('i/registry/set', { | ||
| scope: ['client', 'preferences', 'backups'], | ||
| key: prefer.profile.name, | ||
| value: prefer.profile, | ||
| value: currentProfile, | ||
| }); | ||
|
|
||
| store.set('latestPreferencesBackupAt', Date.now()); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
git ls-files | grep -E 'packages/frontend/src/(preferences(-utility)?|.*preferences.*\.ts)$' | sed -n '1,80p'
echo "== utility outline =="
ast-grep outline packages/frontend/src/preferences/utility.ts --view compact 2>/dev/null | sed -n '1,120p' || true
echo "== utility relevant lines =="
sed -n '180,270p' packages/frontend/src/preferences/utility.ts | cat -n
echo "== search cloudSet/cloudBackup references =="
rg -n "function cloudSet|async function cloudSet|cloudSet|mergeProfiles|cloudBackup|i/registry/(get|set)|latestPreferencesBackupAt" packages/frontend/src -S
echo "== preferences outline/relevant =="
ast-grep outline packages/frontend/src/preferences.ts --view compact 2>/dev/null | sed -n '1,160p' || true
rg -n "cloudSet|mergeProfiles|cloudBackup|backups|registry/get|registry/set|latestPreferencesBackupAt" packages/frontend/src/preferences.ts -S -C 3Repository: misskey-dev/misskey
Length of output: 9603
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== preferences.ts full relevant section =="
sed -n '1,145p' packages/frontend/src/preferences.ts | cat -n
echo "== manager mergeProfiles and save/update relevant sections =="
sed -n '150,210p' packages/frontend/src/preferences/manager.ts | cat -n
sed -n '280,350p' packages/frontend/src/preferences/manager.ts | cat -n
echo "== diff summary/stat =="
git diff --stat
echo "== changed files list =="
git diff --name-onlyRepository: misskey-dev/misskey
Length of output: 9778
cloudBackup を並列書き込み対応に変更してください。
cloudBackup も i/registry/get → mergeProfiles → i/registry/set の順序で行われるため、cloudSet と同様に同時実行時に一方のマージ結果が他方を完全に上書きする可能性があります。2つのデバイスまたはタブがほぼ同時に実行すれば、最新の変更が失われるため、サーバー側での楽観的排他制御やキーごとのアトミック更新、あるいは cloudBackup 側の衝突検知・再取得などが必要になります。
🤖 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 `@packages/frontend/src/preferences/utility.ts` around lines 219 - 246, Update
cloudBackup to handle concurrent executions without losing either device or
tab’s merged changes. Protect the i/registry/get → mergeProfiles →
i/registry/set sequence with the same concurrency or conflict-resolution
approach used by cloudSet, such as server-side optimistic locking, atomic
per-key updates, or retrying after conflict with a fresh read, while preserving
the existing backup timestamp update after a successful write.
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.
What
Resolve #17788
Why
Additional info (optional)
Checklist