實作雲端同步功能：書籤同偏好設定跨裝置共步 #193
Conversation
正式引入 Supabase 整合，分使用者做得透過 Google 帳號同步 `hakkaBookmarks` 同 `romanizerJoiningMode`。
主要實作內容包含：
- 新建立 `js/supabase-config.js` 同 `js/cloud-sync.js` 模組
- 修改 `index.html` 整合雲端同步 UI 還有調節 JS 載入順序，確保 SDK 優先
- 修改 `main.js` 同 `updateProgressDropdown` 揭露分全域存取，還有在儲存書籤後自動觸發同步
- 修改 `style.css` 加入雲端同步按鈕樣式同狀態指示動畫
- 改善書籤合併（Merge）邏輯，比照主程式「共隻表格淨保留最新類別」个規則
- 整理程式碼，拿核開發階段个 debug log，還過加入 timeout 安全機制
AI 貢獻分工：
- Claude 4.5 Opus (#Opus):
- 負責最初个技術規劃、架構設計（Supabase）還有核心同步邏輯（拉取/上傳/合併）个功能實作。
- 關鍵突破：發覺 `updateProgressDropdown` 區域作用域問題，提供揭露分全域（window）个修正方案。
- Claude 4.5 Sonnet (#Sonnet):
- 負責功能中期个深度除錯（CORS、Key 錯誤、SDK 載入順序、同步旗標 Deadlock）。
- 修正書籤合併邏輯，精確對齊主程式「共隻表格單一類別」个汰換規則。
- 負責最後个 Code Quality Review，補正無核个 `syncToCloud` 呼叫、整理 JSDoc 還有改善 UI 顯示細節。
- Gemini 3 Flash (#G3Flash): 負責細部參數調整同最後个 Git 分支管理、Commit 稿件彙整任務。
Co-authored-by: Opus 4.5 <Opus4.5@Antigravity>
Co-authored-by: Sonnet 4.5 <Sonnet4.5@Antigravity>
Signed-off-by: GHSRobert <robertus0617@gmail.com> (Commit Helper: Gemini 3 Flash <Gemini-3-Flash@Antigravity>)
Summary of ChangesHello @GJRobert, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 此拉取請求的核心目標是為應用程式導入跨裝置的雲端同步功能。透過整合 Supabase 服務與 Google 帳號認證，使用者現在可以將他們的書籤和個人偏好設定安全地儲存於雲端，並在不同裝置間無縫存取。這項功能顯著提升了使用者體驗，確保資料的一致性與可用性，同時也改進了資料合併的處理方式，使其更加穩健。 Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
這是一個很棒的更新，成功地整合了 Supabase 實現了書籤和偏好設定的雲端同步功能。程式碼結構清晰，包含了像是請求超時、操作防抖等可靠性設計，相當不錯。
我主要提出了幾點建議：
- 在
index.html中發現一個多餘的</div>標籤，可能導致 HTML 結構錯誤。 - 在
js/supabase-config.js中，建議將 Supabase 的金鑰改為使用環境變數管理，以提升安全性。 - 在
js/cloud-sync.js中有一個未被使用的常數，建議移除以保持程式碼整潔。
整體來說，這是一個高品質的提交，修正這些小問題後會更加完善。
| </label> | ||
| </div> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
這個 </div> 標籤是多餘的，並且破壞了 HTML 結構。<div id="learning-mode-controls"> 應該包住 <details> 元素，但這個 </div> 卻被放在 <details> 元素內部，導致標籤未正確巢狀。請移除此行以修正 HTML 結構。
| const SUPABASE_ANON_KEY = | ||
| 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJzdXBhYmFzZSIsInJlZiI6InBsZ3F6cWx6a2J3b3prYXBqY3N1Iiwicm9sZSI6ImFub24iLCJpYXQiOjE3NjkyNDkzODMsImV4cCI6MjA4NDgyNTM4M30.uRtdi-y0ar6h_S1rJKlNd-r7Wvrt3NMHF1aoTgo2efg'; |
There was a problem hiding this comment.
將 SUPABASE_ANON_KEY 這類金鑰直接硬編碼在原始碼中存在安全風險。雖然這是公開的匿名金鑰，但最佳實踐是透過環境變數來管理，以避免金鑰被意外提交到版本控制系統中。這樣做也能讓金鑰的輪替與管理更加方便與安全。
建議將金鑰移至環境變數中，並在建置過程中注入，例如：
const SUPABASE_ANON_KEY = process.env.SUPABASE_ANON_KEY;
js/cloud-sync.js
Outdated
| */ | ||
|
|
||
| const BOOKMARK_LIMIT = 10; | ||
| const SYNC_KEYS = ['hakkaBookmarks', 'romanizerJoiningMode']; |
There was a problem hiding this comment.
這個 SYNC_KEYS 常數已經被定義，但在程式碼中並未被使用。為了保持程式碼的簡潔與可維護性，建議移除未使用的變數。
Code Review: PR #193 - Supabase Cloud Sync ImplementationThank you for this substantial feature addition! The cloud sync implementation provides a solid foundation, but I've identified several areas that need attention before merging. 🔴 Critical Issues (Must Fix)1. Security: Verify Row Level Security (RLS) PoliciesFile: The Supabase Anonymous Key is publicly exposed in the repository. While this is expected for client-side applications, you must ensure proper Row Level Security policies are configured in Supabase to prevent unauthorized access. Required RLS Policies: -- Users can only read their own data
CREATE POLICY "Users can read own sync data" ON user_sync_data
FOR SELECT USING (auth.uid() = user_id);
-- Users can only insert/update their own data
CREATE POLICY "Users can upsert own sync data" ON user_sync_data
FOR INSERT WITH CHECK (auth.uid() = user_id);
CREATE POLICY "Users can update own sync data" ON user_sync_data
FOR UPDATE USING (auth.uid() = user_id);Action: Please confirm these policies (or equivalent) are configured in your Supabase dashboard. 2. Inadequate Error HandlingFile: The } catch (err) {
console.error('[CloudSync] 上傳失敗:', err);
// No user notification!
}Recommendation: Add user-facing error messages and status updates: } catch (err) {
console.error('[CloudSync] 上傳失敗:', err);
updateSyncStatusUI('error');
// Optionally: implement retry mechanism
}Similar issue in 3. Race Condition with setTimeoutFile: await syncToCloud();
setTimeout(() => {
if (typeof window.updateProgressDropdown === 'function') {
window.updateProgressDropdown();
}
}, 100);Problem: The 100ms timeout is arbitrary and race-prone. If Fix: Remove the timeout and update UI immediately after sync completes: await syncToCloud();
if (typeof window.updateProgressDropdown === 'function') {
window.updateProgressDropdown();
}🟡 High Priority Issues (Should Fix)4. Insufficient Data ValidationFile: The function mergeBookmarks(localBookmarks, cloudBookmarks) {
const allBookmarks = [...cloudBookmarks, ...localBookmarks];
allBookmarks.forEach((bm) => {
const tableKey = bm.tableName; // No check if tableName exists!
});
}Recommendation: Add validation: function validateBookmark(bookmark) {
return bookmark &&
typeof bookmark === 'object' &&
typeof bookmark.tableName === 'string' &&
typeof bookmark.timestamp === 'number' &&
typeof bookmark.cat === 'string';
}
function mergeBookmarks(localBookmarks, cloudBookmarks) {
if (!Array.isArray(localBookmarks)) localBookmarks = [];
if (!Array.isArray(cloudBookmarks)) cloudBookmarks = [];
const validLocal = localBookmarks.filter(validateBookmark);
const validCloud = cloudBookmarks.filter(validateBookmark);
// ... rest of merge logic
}5. Timeout Race ConditionFile: Using const { data, error } = await Promise.race([queryPromise, timeoutPromise]);Problem: If timeout fires, the Supabase query continues in background. Recommendation: Check if Supabase JS client supports AbortController, or handle timeout differently to prevent resource waste. 6. Global Scope PollutionFiles: Multiple functions and variables are exposed to global scope without namespacing:
Recommendation: Use module pattern or IIFE to encapsulate: const CloudSync = (function() {
// Private state
const BOOKMARK_LIMIT = 10;
let state = { isLoggedIn: false, user: null };
// Private functions
async function syncFromCloud() { /* ... */ }
// Public API
return {
init: initCloudSync,
signIn: signInWithGoogle,
trigger: triggerCloudSync,
};
})();7. Script Loading Order IssueFile: <script src="https://cdn.jsdelivr.net/npm/@supabase/supabase-js@2"></script>
<script type="text/javascript" src="js/supabase-config.js" defer></script>Problem: Supabase SDK loads without Recommendation: Either load all with 🔵 Medium Priority Issues (Nice to Have)8. Magic NumbersExtract hardcoded values to constants: const CONFIG = {
QUERY_TIMEOUT_MS: 10000, // line 125
SUCCESS_INDICATOR_DURATION_MS: 2000, // line 196
ERROR_INDICATOR_DURATION_MS: 3000, // line 206
SYNC_DEBOUNCE_MS: 1000, // line 343
};9. Inefficient DOM QueriesDOM elements are queried repeatedly in every function call: function updateSyncUI() {
const cloudSyncBtn = document.getElementById('cloudSyncBtn'); // Queried every call
}
function updateSyncStatusUI() {
const cloudSyncBtn = document.getElementById('cloudSyncBtn'); // Same element, queried again
}Recommendation: Cache DOM references after initialization. 10. Missing Error BoundaryFile: if (typeof initCloudSync === 'function') {
initCloudSync(); // No error handling
}Recommendation: Wrap in try-catch to prevent breaking app initialization if cloud sync fails. 🟢 Positive Aspects
📊 Overall AssessmentStatus: Request Changes The implementation provides a solid foundation, but the critical security and data integrity issues should be addressed before merging. The core functionality is well-designed, but needs attention to:
🎯 Recommended Action PlanBefore Merge:
After Merge (Future Improvements):
Let me know if you need clarification on any of these points or help implementing the fixes! 🤖 Generated with Claude Sonnet 4.5 |
- 將 debounce 時間從 1 秒改為 30 秒 - 新增頁面離開時自動同步機制（visibilitychange + beforeunload） - 確保用戶離開頁面前資料不會遺失
…oauth-redirect-K2p8t）
Deploying hakspring with
|
| Latest commit: |
8b0a391
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://6bb0ca29.hakspring.pages.dev |
| Branch Preview URL: | https://feat-cloud-sync.hakspring.pages.dev |
Pull Request Review: Cloud Sync Feature ImplementationOverviewThis PR introduces Supabase-based cloud synchronization for user bookmarks and romanizer preferences. The implementation is well-structured with good separation of concerns, but there are several security, reliability, and code quality issues that need to be addressed. 🔴 Critical Security Issues1. Exposed Supabase Anon Key in Client Code (js/supabase-config.js:7-8)Severity: HIGH The
Recommendation:
2. Potential XSS via innerHTML (js/cloud-sync.js:283, 297)Severity: MEDIUM The code uses Recommendation:
|
- 修復 HTML 結構：調整 </details> 和 </div> 的順序 - 移除未使用的 SYNC_KEYS 常數
- 移除 setTimeout(100ms) 競合：同步完成後立即更新 UI - 新增 showSyncError：同步失敗時顯示用戶可見的錯誤訊息 - 移除不可靠的 beforeunload，只使用 visibilitychange
- OAuth redirect 保留 query parameters（腔調、級別、類別等狀態） - syncToCloud() 失敗時顯示錯誤通知 - showSyncError() 使用 dataset 避免 race condition
…th-redirect-K2p8t） Claude/fix oauth redirect k2p8t ## 同步機制優化 - Debounce 時間從 1 秒改為 30 秒，減少 Supabase API 呼叫次數 - 使用 visibilitychange 在頁面離開時同步，移除不可靠的 beforeunload - 移除 setTimeout(100ms) 競合，同步完成後立即更新 UI ## 錯誤處理 - 新增 showSyncError() 顯示用戶可見的錯誤訊息 - syncToCloud() 失敗時通知用戶 - 使用 dataset 存儲原始 title，避免快速觸發的 race condition ## OAuth 改善 - redirect URL 保留 query parameters，登入後能回到原本的狀態 ## 程式碼清理 - 修復 HTML 結構（</details> 和 </div> 順序） - 移除未使用的 SYNC_KEYS 常數
Pull Request Review: 雲端同步功能實作OverviewThis PR implements cloud synchronization using Supabase for syncing bookmarks and user preferences across devices. The implementation spans 5 files with significant additions to the codebase. 🔒 Security ConcernsCRITICAL: Exposed API KeysFile: const SUPABASE_URL = 'https://plgqzqlzkbwozkapjcsu.supabase.co';
const SUPABASE_ANON_KEY = 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9...';Issues:
Recommendations:
Authentication Flow SecurityFile: The OAuth implementation looks secure with proper redirect handling. Good practice preserving query parameters for state restoration. 🐛 Potential Bugs & Issues1. Race Condition in Sync StateFile: async function syncFromCloud() {
cloudSyncState.isSyncing = true;
// ... async operations ...
cloudSyncState.isSyncing = false;
}Issue: If Recommendation: async function syncFromCloud() {
if (cloudSyncState.isSyncing) {
console.log('[CloudSync] Sync already in progress, skipping');
return;
}
cloudSyncState.isSyncing = true;
try {
// ... sync logic ...
} finally {
cloudSyncState.isSyncing = false;
}
}2. Missing Error Boundary in mergeBookmarksFile: function mergeBookmarks(localBookmarks, cloudBookmarks) {
const allBookmarks = [...cloudBookmarks, ...localBookmarks];
// ...
}Issue: No validation that Recommendation: function mergeBookmarks(localBookmarks, cloudBookmarks) {
const local = Array.isArray(localBookmarks) ? localBookmarks : [];
const cloud = Array.isArray(cloudBookmarks) ? cloudBookmarks : [];
const allBookmarks = [...cloud, ...local];
// ...
}3. Timeout Promise Memory LeakFile: const timeoutPromise = new Promise((_, reject) =>
setTimeout(() => reject(new Error('Supabase 查詢超時')), 10000),
);
const { data, error } = await Promise.race([queryPromise, timeoutPromise]);Issue: If Recommendation: let timeoutId;
const timeoutPromise = new Promise((_, reject) => {
timeoutId = setTimeout(() => reject(new Error('Supabase 查詢超時')), 10000);
});
try {
const { data, error } = await Promise.race([queryPromise, timeoutPromise]);
clearTimeout(timeoutId);
// ... rest of logic
} catch (err) {
clearTimeout(timeoutId);
throw err;
}4. Page Unload Sync ReliabilityFile: document.addEventListener('visibilitychange', () => {
if (document.visibilityState === 'hidden' && hasPendingSync) {
syncToCloud();
}
});Issue: Recommendation:
5. Function Availability CheckFile: if (typeof triggerCloudSync === 'function') {
triggerCloudSync();
}Issue: This check assumes Recommendation:
⚡ Performance Considerations1. Debounce TimingFile: syncDebounceTimer = setTimeout(() => {
syncToCloud();
hasPendingSync = false;
}, 30000); // 30 秒後同步Analysis: 30-second debounce is reasonable to reduce API calls. However, combined with the page unload sync, this should work well in most scenarios. Recommendation: Consider making this configurable or documenting the trade-off between sync frequency and API usage. 2. Multiple Authentication State ChecksFile: The code checks for existing sessions and sets up auth state listeners. This is fine, but there's a duplicate sync call:
Recommendation: Document why both are necessary or consolidate if possible. 3. Unnecessary Global ExposureFile: window.updateProgressDropdown = updateProgressDropdown;Issue: While necessary for the current architecture, exposing functions globally increases the risk of conflicts and makes the codebase harder to maintain. Recommendation: Consider refactoring to use events or a module system in the future. 🧪 Test CoverageCRITICAL ISSUE: This PR adds 5,383 lines of new/modified code with ZERO test coverage. Missing Tests:
Recommendation:
📝 Code Quality & Best PracticesPositives ✅
Areas for Improvement 📋
🎯 Specific File Reviews
|
| Category | Status |
|---|---|
| Security | |
| Functionality | ✅ Core features work as intended |
| Error Handling | |
| Performance | ✅ Good debouncing and optimization |
| Test Coverage | ❌ No tests added |
| Code Quality |
🎬 Recommendations
Must Fix Before Merge 🔴
- Verify Supabase RLS policies are properly configured
- Add race condition guard in
syncFromCloud() - Add input validation in
mergeBookmarks()
Should Fix Soon 🟡
- Add basic unit tests for merge logic
- Fix timeout promise memory leak
- Add SRI hashes for CDN scripts
- Extract magic numbers to constants
Nice to Have 🟢
- Implement proper module system
- Add integration tests
- Document sync behavior limitations
- Refactor global function exposure
💬 Final Thoughts
This is a solid implementation of cloud sync functionality with thoughtful UI/UX considerations. The code is generally well-structured and includes good error handling. The main concerns are around security verification (RLS policies), race conditions, and lack of test coverage. With the critical issues addressed, this will be a valuable feature for users.
Great work on the AI collaboration mentioned in the PR description - the division of labor between Claude Opus, Sonnet, and Gemini Flash appears to have been effective! 🤖
Co-Reviewed-By: Claude Sonnet 4.5 noreply@anthropic.com
- syncFromCloud() 開頭檢查 isSyncing，避免並行執行 - mergeBookmarks() 驗證參數為陣列，防止損壞資料導致崩潰
Pull Request Review: Cloud Sync ImplementationOverall AssessmentThis PR implements cloud synchronization using Supabase for bookmarks and user preferences. The implementation is solid with good architectural separation, but there are several areas that need attention regarding security, error handling, and best practices. 🔴 Critical Issues1. Exposed Supabase Anonymous Key in Public RepositoryLocation: The Supabase anonymous key is hardcoded and committed to the public repository. While this is the "anon" key designed for client-side use, it still poses risks: Risks:
Recommendations:
2. Race Condition in Error HandlingLocation: The if (cloudSyncBtn.dataset.errorTimeoutId) {
clearTimeout(Number(cloudSyncBtn.dataset.errorTimeoutId));
}Issue: If Recommendation: // Only set originalTitle if it hasn't been set yet
if (!cloudSyncBtn.dataset.originalTitle) {
cloudSyncBtn.dataset.originalTitle = cloudSyncBtn.title;
}This is already implemented correctly in the code, so this is good!
|
Code Review: PR #193 - 雲端同步功能Great work on implementing the Supabase cloud sync feature! The implementation demonstrates solid understanding of async patterns and provides useful functionality. Here's my detailed feedback: 🎯 Overall Assessment: 6.5/10 - Functional with Areas for ImprovementThe implementation works but needs hardening for production use, particularly around race conditions and error handling. 🔴 Critical Issues (Must Fix)1. Race Condition in Sync OperationsLocation:
Scenario: // User saves bookmark → triggerCloudSync() debounces for 30s
// User clicks sync button → syncFromCloud() calls syncToCloud()
// Debounce timer fires → calls syncToCloud() AGAIN
// Result: Two concurrent upsert requests to SupabaseFix: let cloudSyncLock = false;
async function syncToCloud() {
if (cloudSyncLock) {
console.log('[CloudSync] Upload already in progress');
return;
}
cloudSyncLock = true;
try {
// ... existing code
} finally {
cloudSyncLock = false;
}
}2. Unsafe JSON ParsingLocation: If localStorage is corrupted (rare but possible), Fix: function safeParseBookmarks(key, defaultValue = []) {
try {
const data = localStorage.getItem(key);
return data ? JSON.parse(data) : defaultValue;
} catch (err) {
console.error(`[CloudSync] Corrupted ${key}, resetting:`, err);
localStorage.setItem(key, JSON.stringify(defaultValue));
return defaultValue;
}
}
// Usage:
const localBookmarks = safeParseBookmarks('hakkaBookmarks', []);3. Missing Bookmark Schema ValidationLocation:
Fix: function isValidBookmark(bm) {
return (
bm &&
typeof bm === 'object' &&
typeof bm.tableName === 'string' &&
typeof bm.timestamp === 'number' &&
typeof bm.rowId === 'number' &&
typeof bm.cat === 'string'
);
}
function mergeBookmarks(localBookmarks, cloudBookmarks) {
const local = Array.isArray(localBookmarks)
? localBookmarks.filter(isValidBookmark)
: [];
const cloud = Array.isArray(cloudBookmarks)
? cloudBookmarks.filter(isValidBookmark)
: [];
// ... rest of merge logic
}🟡 Important Issues (Should Fix)4. Double Sync on Page LoadLocation: When the page loads with an existing session,
Fix: let initialSessionChecked = false;
client.auth.onAuthStateChange(async (event, session) => {
if (event === 'INITIAL_SESSION' && !initialSessionChecked) {
initialSessionChecked = true;
return; // Skip, will be handled by getSession() below
}
if (session?.user) {
cloudSyncState.isLoggedIn = true;
cloudSyncState.user = session.user;
updateSyncUI(true, session.user);
if (event === 'SIGNED_IN') {
await syncFromCloud();
}
} else {
// ... logout logic
}
});5. Event Listener Memory LeakLocation:
Fix: let unloadListenerAttached = false;
function setupPageUnloadSync() {
if (unloadListenerAttached) return;
unloadListenerAttached = true;
document.addEventListener('visibilitychange', handleVisibilityChange);
}
function handleVisibilityChange() {
if (document.visibilityState === 'hidden' && hasPendingSync) {
if (syncDebounceTimer) {
clearTimeout(syncDebounceTimer);
syncDebounceTimer = null;
}
syncToCloud();
hasPendingSync = false;
}
}6. Missing Retry LogicLocation: When sync fails (network issues, timeouts), there's no automatic retry. Users must manually click to retry. Suggested Enhancement: async function syncFromCloudWithRetry(retries = 3) {
for (let attempt = 1; attempt <= retries; attempt++) {
try {
return await syncFromCloud();
} catch (err) {
if (attempt === retries) {
throw err; // Final attempt failed
}
const delay = Math.min(1000 * Math.pow(2, attempt), 10000);
console.log(`[CloudSync] Retry ${attempt}/${retries} after ${delay}ms`);
await new Promise(r => setTimeout(r, delay));
}
}
}🟢 Minor Issues (Nice to Have)7. Generic Error MessagesLocation: Error messages like "同步失敗，請稍後再試" don't help users understand the issue (network? authentication? timeout?). Suggestion: function getErrorMessage(error) {
if (error.message?.includes('timeout')) {
return '網路連線逾時，請檢查網路連線';
}
if (error.code === 'PGRST301') {
return '登入已過期，請重新登入';
}
return `同步失敗：${error.message || '未知錯誤'}`;
}
// Usage:
showSyncError(getErrorMessage(err));8. State Management Could Be More RobustLocation: Direct mutation of global Future Refactor Suggestion: const CloudSyncManager = {
_state: { /* ... */ },
_listeners: [],
setState(updates) {
this._state = { ...this._state, ...updates };
this._notifyListeners();
},
getState() {
return { ...this._state }; // Return copy
},
subscribe(listener) {
this._listeners.push(listener);
}
};🔒 Security Notes9. Supabase CredentialsLocation: The hardcoded anonymous key is visible in client-side code. This is standard practice for Supabase, but you MUST ensure: ✅ Row Level Security (RLS) policies are enabled on -- Verify this is configured in Supabase dashboard:
CREATE POLICY "Users can only access their own data"
ON user_sync_data
USING (auth.uid() = user_id);Without RLS, anyone with the anon key can read/modify all user data. ✅ What's Done Well
🧪 Test CoverageConsider adding tests for:
📝 Recommendations SummaryBefore Merging:
Post-Merge Improvements:
🎉 ConclusionThis is a solid first implementation of cloud sync! The core functionality works well, and the code is generally well-structured. The main concerns are around edge cases (race conditions, corrupted data) and error recovery. Once the critical issues are addressed, this will be production-ready. Nice collaboration between Claude Opus, Sonnet, and Gemini Flash! 👏 Recommended Action: Request changes for critical issues #1-3 and #9, then approve once fixed. |
Code Review: PR #193 - 雲端同步功能感謝提交這個 PR！這是一個相當完整的 Supabase 整合實作。我進行了全面的代碼審查，發現了一些需要注意的問題。 🔴 Critical Issues (建議修復後再合併)1. Supabase RLS 政策文件缺失位置: 雖然暴露 anon key 是 Supabase 的標準做法，但安全性完全依賴於 Row Level Security (RLS) 政策。建議：
2. Race Condition: hasPendingSync 未重置位置: function triggerCloudSync() {
hasPendingSync = true;
syncDebounceTimer = setTimeout(() => {
syncToCloud(); // 如果失敗，hasPendingSync 永遠不會重置
hasPendingSync = false;
}, 30000);
}建議修復: syncDebounceTimer = setTimeout(async () => {
try {
await syncToCloud();
} finally {
hasPendingSync = false; // 確保總是重置
}
}, 30000);3. 缺少輸入驗證位置:
allBookmarks.forEach((bm) => {
const tableKey = bm.tableName; // 如果 tableName 是 undefined 會怎樣？
});建議修復: function mergeBookmarks(localBookmarks, cloudBookmarks) {
const local = Array.isArray(localBookmarks) ? localBookmarks : [];
const cloud = Array.isArray(cloudBookmarks) ? cloudBookmarks : [];
// 過濾無效書籤
const allBookmarks = [...cloud, ...local].filter(bm =>
bm &&
typeof bm === 'object' &&
bm.tableName &&
typeof bm.timestamp === 'number'
);
// ... 其餘邏輯
}
|
正式引入 Supabase 整合，分使用者做得透過 Google 帳號同步
hakkaBookmarks同romanizerJoiningMode。 主要實作內容包含：js/supabase-config.js同js/cloud-sync.js模組index.html整合雲端同步 UI 還有調節 JS 載入順序，確保 SDK 優先main.js同updateProgressDropdown揭露分全域存取，還有在儲存書籤後自動觸發同步style.css加入雲端同步按鈕樣式同狀態指示動畫AI 貢獻分工：
updateProgressDropdown區域作用域問題，提供揭露分全域（window）个修正方案。syncToCloud呼叫、整理 JSDoc 還有改善 UI 顯示細節。)