🧹 Implement LocalStorage Fallback for Sync Failure - #6
hashexplaindata wants to merge 1 commit into
Conversation
Modified code/experiment.js to save experimental results to localStorage
when Firebase synchronization fails. This ensures data preservation in
case of network errors.
Implemented logic:
localStorage.setItem('failed_sync', JSON.stringify(STATE.results));
Co-authored-by: hashexplaindata <221828969+hashexplaindata@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Summary of ChangesHello, 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! This pull request significantly improves the experiment tool's data safety by introducing a fallback mechanism. In the event of a network synchronization failure with Firebase, experimental data is now automatically saved to Highlights
Changelog
Activity
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
This pull request introduces a valuable data safety feature by saving experiment results to localStorage when the network sync fails. However, the current implementation has a high-risk flaw where it overwrites previously saved data, potentially leading to data loss if multiple sessions fail to sync. I've added a comment with a suggestion to append data instead. Additionally, the localStorage operation itself is not error-handled, which could be an issue if storage is full or disabled. My suggestion also includes a try...catch block for this. Finally, while this PR successfully saves the data, there is no corresponding mechanism to recover and re-sync this data later. This should be considered for a follow-up implementation to make the feature fully effective.
| console.error("Critical Sync Failure:", error); | ||
| DOM.syncStatus.innerHTML = `<span style="color:#ff453a">⚠️ Sync Failed. Error: ${error.code || 'Network'}</span>`; | ||
| // Potential fallback: Save to localStorage for later recovery | ||
| localStorage.setItem('failed_sync', JSON.stringify(STATE.results)); |
There was a problem hiding this comment.
This implementation risks data loss by overwriting previously saved failed syncs. If a user has a failed sync, and then runs another session that also fails to sync, the data from the first session will be lost. The data should be appended to any existing failed sync data.
Additionally, localStorage.setItem can fail if storage is full or disabled. It's crucial to wrap this in a try...catch block to handle such errors gracefully, especially since this is a data safety feature. My suggestion includes this, along with a user-facing message for this failure case.
try {
const FAILED_SYNC_KEY = 'failed_sync_payloads';
const existingPayloads = JSON.parse(localStorage.getItem(FAILED_SYNC_KEY)) || [];
existingPayloads.push(STATE.results);
localStorage.setItem(FAILED_SYNC_KEY, JSON.stringify(existingPayloads));
} catch (storageError) {
console.error('Fallback to localStorage failed:', storageError);
DOM.syncStatus.innerHTML += '<br><span style="color:#ff453a; font-size: 0.8em;">Critical: Could not save a local backup. Data may be lost.</span>';
}![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a55f68229b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| console.error("Critical Sync Failure:", error); | ||
| DOM.syncStatus.innerHTML = `<span style="color:#ff453a">⚠️ Sync Failed. Error: ${error.code || 'Network'}</span>`; | ||
| // Potential fallback: Save to localStorage for later recovery | ||
| localStorage.setItem('failed_sync', JSON.stringify(STATE.results)); |
There was a problem hiding this comment.
Using a fixed key (failed_sync) means every new sync failure overwrites the previous unsent participant data, so running multiple offline/failed sessions on the same device will silently discard earlier telemetry. If this fallback is meant for recovery, the key should be namespaced (for example by participant ID and timestamp) or appended to a stored queue.
Useful? React with 👍 / 👎.
| console.error("Critical Sync Failure:", error); | ||
| DOM.syncStatus.innerHTML = `<span style="color:#ff453a">⚠️ Sync Failed. Error: ${error.code || 'Network'}</span>`; | ||
| // Potential fallback: Save to localStorage for later recovery | ||
| localStorage.setItem('failed_sync', JSON.stringify(STATE.results)); |
There was a problem hiding this comment.
localStorage.setItem can throw (for example QuotaExceededError or SecurityError in private/restricted browsing), and this call is inside the outer catch without its own guard. In those environments the fallback path itself throws, which can surface as an unhandled rejection and still lose the payload during the exact failure mode this code is trying to protect.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Adds a client-side persistence fallback so experiment telemetry isn’t lost when the Firebase batch write fails, aligning with the experiment tool’s reliability/data-safety goals.
Changes:
- Persist
STATE.resultstolocalStorageon sync failure inexecuteBatchPayload.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| console.error("Critical Sync Failure:", error); | ||
| DOM.syncStatus.innerHTML = `<span style="color:#ff453a">⚠️ Sync Failed. Error: ${error.code || 'Network'}</span>`; | ||
| // Potential fallback: Save to localStorage for later recovery | ||
| localStorage.setItem('failed_sync', JSON.stringify(STATE.results)); |
There was a problem hiding this comment.
localStorage.setItem(...) can throw (e.g., SecurityError when storage is blocked in iframes/private mode, or QuotaExceededError). Since this is inside the catch for the sync failure, an exception here would mask the original failure and still lose the data. Wrap the fallback write in its own try/catch (and ideally guard on typeof localStorage !== 'undefined') and surface/log a secondary error if persisting locally fails.
| localStorage.setItem('failed_sync', JSON.stringify(STATE.results)); | |
| // Best-effort local persistence of failed sync; avoid masking original error | |
| if (typeof localStorage !== 'undefined') { | |
| try { | |
| localStorage.setItem('failed_sync', JSON.stringify(STATE.results)); | |
| } catch (storageError) { | |
| console.error("Secondary failure: unable to persist failed sync to localStorage:", storageError); | |
| } | |
| } else { | |
| console.warn("localStorage is not available; failed sync data was not persisted locally."); | |
| } |
| console.error("Critical Sync Failure:", error); | ||
| DOM.syncStatus.innerHTML = `<span style="color:#ff453a">⚠️ Sync Failed. Error: ${error.code || 'Network'}</span>`; | ||
| // Potential fallback: Save to localStorage for later recovery | ||
| localStorage.setItem('failed_sync', JSON.stringify(STATE.results)); |
There was a problem hiding this comment.
Using a single fixed key (failed_sync) will overwrite any prior failed payloads (including retries or multiple participants on the same device). Consider namespacing the key with STATE.pid and/or a timestamp, and consider clearing the stored payload after a successful sync so the browser doesn’t retain stale telemetry indefinitely.
| console.error("Critical Sync Failure:", error); | ||
| DOM.syncStatus.innerHTML = `<span style="color:#ff453a">⚠️ Sync Failed. Error: ${error.code || 'Network'}</span>`; | ||
| // Potential fallback: Save to localStorage for later recovery | ||
| localStorage.setItem('failed_sync', JSON.stringify(STATE.results)); |
There was a problem hiding this comment.
This persists STATE.results (including free-form semantic_justification) to localStorage, which is long-lived and readable by any JS running on this origin (increasing impact of any future XSS). If persistence is required, consider storing the minimum necessary fields and/or using sessionStorage with an explicit user download/export flow, and document/communicate the retention/cleanup behavior.
| localStorage.setItem('failed_sync', JSON.stringify(STATE.results)); | |
| // Persist a reduced, session-scoped payload without free-form justification | |
| const failedSyncPayload = STATE.results.map(({ semantic_justification, ...rest }) => rest); | |
| sessionStorage.setItem('failed_sync', JSON.stringify(failedSyncPayload)); |
|
Closing: Fixes have already been incorporated directly into the master branch. |
Understood. Acknowledging that this work is now obsolete and stopping work on this task. |
The experiment tool now preserves experimental data in
localStorageif the network sync to Firebase fails. This is a critical data safety improvement that prevents the loss of participant telemetry due to connectivity issues.Changes:
executeBatchPayloadcatch block with alocalStorage.setItemcall.PR created automatically by Jules for task 11155458263089221268 started by @hashexplaindata