Secure Telemetry Engine Against Device/Network Variance - #4
hashexplaindata wants to merge 1 commit into
Conversation
- Implement nested requestAnimationFrame + setTimeout(0) for millisecond-accurate RT measurement - Enforce strict absolute positioning, hardware acceleration, and viewport constraints to prevent DOM geometry shifts - Debounce pointerdown events and implement navigation locks to prevent state machine duplication - Add two-tier network resilience using localStorage caching and Firebase offline persistence 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 enhances the robustness and data integrity of the telemetry engine, particularly in challenging testing environments. It addresses critical vulnerabilities related to timing accuracy, UI rendering consistency across devices, state management during rapid user interactions, and data persistence during network outages. The changes ensure that experimental data collection remains reliable and free from external variances, providing a more stable and accurate platform for behavioral conformity studies. Highlights
Changelog
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 several patches to improve the robustness and data integrity of the behavioral telemetry engine. The changes address potential issues with timing accuracy, UI rendering consistency on mobile devices, state management race conditions, and network resilience. The patches are well-implemented and address the described vulnerabilities effectively. My review includes one high-severity suggestion for experiment.js to improve the localStorage backup mechanism, ensuring data can be more reliably recovered across user sessions.
| try { | ||
| localStorage.setItem(`telemetry_backup_${STATE.pid}`, JSON.stringify(STATE.results)); | ||
| } catch (e) { | ||
| console.warn("Local storage backup failed (Quota/Privacy mode).", e); | ||
| } |
There was a problem hiding this comment.
The localStorage backup mechanism is a great addition for network resilience. However, its effectiveness is limited because the participant_id (STATE.pid) is regenerated on every page load. If the user experiences a sync failure and then reloads the page, the new session will have a new pid, and there's no way for the application to find the backup data stored under the old pid. The comment mentions manual recovery via devtools, but even that is difficult without knowing the old pid.
To make this backup truly effective for recovery across sessions, consider persisting the pid in localStorage as well.
Example:
// At the start of the script
function getOrSetPID() {
let pid = localStorage.getItem('telemetry_pid');
if (!pid) {
pid = Math.random().toString(36).substring(2, 15) + Math.random().toString(36).substring(2, 15);
localStorage.setItem('telemetry_pid', pid);
}
return pid;
}
const STATE = {
pid: getOrSetPID(),
// ... other state properties
};
// In init(), you could then check for a backup for this persistent PID.This would make the data recovery process much more robust, allowing for automatic or at least simplified manual recovery.
![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: add455e3c2
ℹ️ 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".
| DOM.syncStatus.innerHTML = ` | ||
| <div style="text-align:center;"> | ||
| <span style="color:#ff453a; display:block; margin-bottom:8px;">⚠️ Sync Failed (Network Error)</span> | ||
| <span style="font-size:0.75rem; color:var(--text-secondary);">Your data is saved locally. Please do not close this tab until reconnected.</span> |
There was a problem hiding this comment.
When localStorage.setItem fails (for example, quota exhaustion or private browsing restrictions), this error path still tells participants "Your data is saved locally." That message is inaccurate in exactly the failure mode where no local backup exists, so users may close the tab and lose unsynced results. Please track whether the backup write succeeded and only show this reassurance when it actually did.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR hardens the experiment UI/telemetry flow against device rendering variance, rapid multi-input, navigation backtracking, and intermittent network conditions to reduce data corruption/loss during mass testing.
Changes:
- Adds stricter mobile viewport + CSS constraints to reduce layout/interaction variance (overscroll, selection, badge rendering/click interception).
- Strengthens experiment state machine against double-taps and repeated submissions (trial lock + debounced
pointerdown, finalize button lock, navigation interception). - Adds network-resilience measures for writes (localStorage backup + Firestore offline persistence enablement) and updates sync-failure messaging.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
code/style.css |
Adds mobile overscroll + selection constraints; strengthens AI badge rendering stability and prevents badge click interception. |
code/index.html |
Tightens viewport meta settings to reduce mobile zoom/viewport variance. |
code/experiment.js |
Updates navigation lock behavior, debounces/locks trial selection + final submission, and adds local/offline persistence for batch writes with revised failure UI. |
💡 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.
| onSyncSuccess(); | ||
|
|
||
| // Clean up localStorage if sync (or cache write) was successful | ||
| localStorage.removeItem(`telemetry_backup_${STATE.pid}`); |
There was a problem hiding this comment.
With Firestore offline persistence enabled, batch.commit() can resolve while the write is only queued locally (not yet acknowledged by the backend). Calling onSyncSuccess() immediately (and removing the localStorage backup) can therefore present a false “synced” state and allow the user to close the tab before the data ever reaches the server. Consider waiting for backend acknowledgment (e.g., db.waitForPendingWrites()), or showing a distinct “saved locally, pending sync” state and only clearing the backup once the server confirms receipt.
| onSyncSuccess(); | |
| // Clean up localStorage if sync (or cache write) was successful | |
| localStorage.removeItem(`telemetry_backup_${STATE.pid}`); | |
| // Wait until all pending writes have been acknowledged by the backend | |
| // before treating the payload as fully synced and clearing the backup. | |
| await db.waitForPendingWrites(); | |
| // Clean up localStorage only after confirmed backend sync | |
| localStorage.removeItem(`telemetry_backup_${STATE.pid}`); | |
| onSyncSuccess(); |
| const onSelect = (e) => { | ||
| // Prevent default touch behaviors (like zooming or scrolling) that might leak through | ||
| e.preventDefault(); |
There was a problem hiding this comment.
e.preventDefault() on a pointerdown handler generally won’t prevent scrolling/zooming on touch devices; those default actions are primarily controlled via the CSS touch-action property. Since the page sets touch-action: pan-y globally (index.html), consider using an appropriate touch-action value on the choice cards (e.g., manipulation/none) if the goal is to block gesture side-effects, or adjust the comment to avoid implying preventDefault() provides that guarantee.
| -webkit-user-select: none; | ||
| user-select: none; | ||
| } | ||
|
|
There was a problem hiding this comment.
user-select: none is applied to the entire body, which makes all text (including the Participant ID shown on screen 10) non-selectable/copyable. If participants need to copy/paste the PID, consider scoping user-select: none to only the interactive trial UI, or explicitly re-enabling selection for the PID element(s) (similar to how textarea is handled in index.html).
| /* Allow text selection for specific elements (e.g., Participant ID, textareas) */ | |
| textarea, | |
| #participant-id, | |
| .pid-display, | |
| .allow-text-select { | |
| -webkit-user-select: text; | |
| user-select: text; | |
| } |
| <!-- Added strict viewport properties to prevent Safari double-tap-to-zoom and viewport scaling --> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0, maximum-scale=1.0, user-scalable=0, viewport-fit=cover"> |
There was a problem hiding this comment.
The viewport meta disables zoom (maximum-scale=1.0 + user-scalable=0), which can create accessibility issues for low-vision users. Also, user-scalable=0 is less standard than user-scalable=no and may not be consistently honored; consider allowing zoom (recommended) or at least using the conventional yes/no values if zoom must be restricted.
| <!-- Added strict viewport properties to prevent Safari double-tap-to-zoom and viewport scaling --> | |
| <meta name="viewport" content="width=device-width, initial-scale=1.0, maximum-scale=1.0, user-scalable=0, viewport-fit=cover"> | |
| <!-- Adjusted viewport properties to maintain layout while allowing user zoom for accessibility --> | |
| <meta name="viewport" content="width=device-width, initial-scale=1.0, viewport-fit=cover"> |
| window.addEventListener('popstate', (e) => { | ||
| // Force them forward again | ||
| window.history.pushState(null, document.title, window.location.href); | ||
| // Prevent any default back-navigation behavior | ||
| e.preventDefault(); |
There was a problem hiding this comment.
The popstate handler calls e.preventDefault(), but popstate is not cancelable in browsers, so this is a no-op and can be misleading. Also, pushing a new history entry on every popstate can bloat the history stack if the user repeatedly triggers back navigation; consider using history.forward()/history.go(1) or replaceState in the handler to avoid unbounded history growth.
| window.addEventListener('popstate', (e) => { | |
| // Force them forward again | |
| window.history.pushState(null, document.title, window.location.href); | |
| // Prevent any default back-navigation behavior | |
| e.preventDefault(); | |
| window.addEventListener('popstate', () => { | |
| // Force them forward again without growing the history stack | |
| window.history.go(1); |
| try { | ||
| localStorage.setItem(`telemetry_backup_${STATE.pid}`, JSON.stringify(STATE.results)); | ||
| } catch (e) { | ||
| console.warn("Local storage backup failed (Quota/Privacy mode).", e); | ||
| } | ||
|
|
||
| try { | ||
| // Check for Firebase (initialized in index.html via firebase-config.js) | ||
| if (typeof firebase !== 'undefined' && firebase.apps.length > 0) { | ||
| const db = firebase.firestore(); | ||
|
|
||
| // Enable offline persistence immediately if not already active. | ||
| // This ensures Firestore will cache the write and synchronize later | ||
| // if the device momentarily drops connection. | ||
| try { | ||
| await db.enablePersistence({ synchronizeTabs: true }); | ||
| } catch (err) { | ||
| if (err.code === 'failed-precondition') { | ||
| console.warn('Persistence: Multiple tabs open.'); | ||
| } else if (err.code === 'unimplemented') { | ||
| console.warn('Persistence: Browser unsupported.'); | ||
| } | ||
| } | ||
|
|
||
| const batch = db.batch(); | ||
|
|
||
| STATE.results.forEach(data => { | ||
| const docRef = db.collection(CFG.COLLECTION).doc(); | ||
| batch.set(docRef, data); | ||
| }); | ||
|
|
||
| // If offline, this resolves immediately because of persistence, writing to cache. | ||
| // When connection returns, Firestore syncs it automatically. | ||
| await batch.commit(); | ||
| onSyncSuccess(); | ||
|
|
||
| // Clean up localStorage if sync (or cache write) was successful | ||
| localStorage.removeItem(`telemetry_backup_${STATE.pid}`); | ||
|
|
||
| } else { | ||
| console.warn("Firebase not detected. Payload logged to console:", STATE.results); | ||
| setTimeout(onSyncSuccess, 1500); // Simulate sync delay | ||
| } | ||
| } catch (error) { | ||
| 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 | ||
| DOM.syncStatus.innerHTML = ` | ||
| <div style="text-align:center;"> | ||
| <span style="color:#ff453a; display:block; margin-bottom:8px;">⚠️ Sync Failed (Network Error)</span> | ||
| <span style="font-size:0.75rem; color:var(--text-secondary);">Your data is saved locally. Please do not close this tab until reconnected.</span> | ||
| </div>`; |
There was a problem hiding this comment.
The failure UI always says “Your data is saved locally”, but localStorage.setItem(...) can throw (and you explicitly catch/log that case). If the backup fails (quota/private mode), this message becomes inaccurate; track whether the local backup succeeded and tailor the displayed message accordingly (or attempt an alternative persistence mechanism).
|
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. |
Patched multiple vulnerabilities in the RM-2 Behavioral Conformity Telemetry Engine to ensure the integrity of experimental data collection in mass-testing environments.
performance.now()timer inloadNextTrial()previously initiated before the browser's painting engine fully rendered the pixels on slower devices. This was patched using a nestedrequestAnimationFramestructure paired with asetTimeout(0), guaranteeing physical visibility before measurement begins.-webkit-font-smoothing: antialiasedandbackface-visibility: hidden. Mobile viewports were locked down (user-scalable=0,viewport-fit=cover,overscroll-behavior-y: none) and touch callouts disabled (user-select: none) to prevent mechanical distortion of reaction times. Addedpointer-events: noneto the badge to prevent misclicks.pointerdownlistener, synchronously locking state (STATE.isTrialActive = false), and completely removing the event listener upon execution. Navigation leaks were sealed viareplaceState/pushStateandpopstateinterception. Prevented multiple batch write payloads by instantly disabling the finalize button.batch.commit(). Patched with a two-tier resilience system: instantly cachingSTATE.resultstolocalStoragebefore attempting the network request, and enabling Firestore's native offline persistence (db.enablePersistence({ synchronizeTabs: true })) to cache the batch write locally if offline.PR created automatically by Jules for task 14949949574972649731 started by @hashexplaindata