Unify experiment codebase: fix PID generation, timing accuracy, Firebase sync, and XSS vulnerabilities#20
Conversation
Co-authored-by: hashexplaindata <221828969+hashexplaindata@users.noreply.github.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Pull request overview
Unifies multiple pending updates into a single experiment implementation focused on research integrity and reliability (participant ID persistence, timing accuracy, safer DOM updates, and improved Firestore sync behavior).
Changes:
- Adds persistent participant ID generation using
crypto.randomUUID()with a fallback, stored inlocalStorage. - Refactors screen transitions and trial interaction handling (debounce/locking, response latency capture, AI badge DOM construction).
- Updates Firestore sync to include local backup, offline persistence attempt, server-ack waiting, and safer UI error messaging.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
firebase.json |
Expands Firebase Hosting ignore rules to exclude benchmarks/tests/verification assets from deploy. |
code/style.css |
Adds transition/polish and mobile interaction constraints, plus selectable PID styling. |
code/index.html |
Adjusts viewport meta, UI copy/layout structure, and makes PID text selectable. |
code/experiment.js |
Implements PID persistence, improved showScreen, timing/locking changes, XSS-safe badge/error UI, and revised Firestore sync pipeline. |
💡 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.
| const outgoing = STATE.activeScreen; | ||
| STATE.activeScreen = target; | ||
|
|
||
| if (outgoing) { |
There was a problem hiding this comment.
On the first transition, the initial active screen (screen-1) has no inline display style. Removing the active class immediately reverts it to .screen { display: none; }, so the fade-out won't run even though display: none is also delayed by 400ms. To make the initial fade-out consistent, ensure the outgoing screen stays display:flex during the opacity transition (e.g., set outgoing.style.display = 'flex' before removing active, or adjust the CSS so non-active screens can remain displayed while fading).
| if (outgoing) { | |
| if (outgoing) { | |
| // Ensure outgoing screen stays visible during fade-out | |
| outgoing.style.display = 'flex'; |
| await batch.commit(); | ||
| await db.waitForPendingWrites(); | ||
|
|
||
| try { localStorage.removeItem('telemetry_backup_' + STATE.pid); } catch(e) {} | ||
| try { localStorage.removeItem('experiment_pid'); } catch(e) {} |
There was a problem hiding this comment.
await db.waitForPendingWrites() can wait indefinitely when the client is offline (the preceding batch.commit() can still resolve when persistence is enabled). This can leave participants stuck on the “Synchronizing Payload…” screen with no PID shown and no local-fallback message. Consider adding an explicit timeout around waitForPendingWrites() and, on timeout, keep the local backup and surface the same local-save message + final-actions so the UI can always complete.
| color: var(--text-primary); | ||
| overflow: hidden; /* Prevent scrolling during experiment */ | ||
| height: 100vh; | ||
| overflow: hidden; |
There was a problem hiding this comment.
body { height: 100dvh; } is a good fix for mobile browser UI chrome, but without a fallback some older browsers that don’t support dvh will ignore it and may end up with an incorrect layout/scroll behavior. Consider keeping height: 100vh; as a fallback before height: 100dvh; so both old and new engines behave correctly.
| overflow: hidden; | |
| overflow: hidden; | |
| height: 100vh; |
| const STATE = { | ||
| pid: Math.random().toString(36).substring(2, 15) + Math.random().toString(36).substring(2, 15), | ||
| pid: localStorage.getItem('experiment_pid') || generatePID(), | ||
| condition: CFG.CONDITION, | ||
| covariate: 0, |
There was a problem hiding this comment.
STATE.pid is initialized with localStorage.getItem('experiment_pid') at module load. In some environments (e.g., storage blocked/disabled), localStorage access can throw a SecurityError and prevent the entire script from running. Wrap the read in a try/catch (or use a small helper) and fall back to generatePID() when storage is unavailable.
|
|
||
| if (outgoing) { | ||
| outgoing.classList.remove('active'); | ||
| setTimeout(() => { outgoing.style.display = 'none'; }, 400); |
There was a problem hiding this comment.
showScreen() schedules setTimeout(() => outgoing.style.display = 'none', 400) but doesn't cancel/guard previously scheduled hide timers. If the user navigates A→B and quickly back to A within 400ms, the earlier timer can still fire and hide the now-active screen. Track and clear a separate outgoing-hide timer (or check outgoing !== STATE.activeScreen inside the callback) to prevent stale timers from hiding the active view.
| setTimeout(() => { outgoing.style.display = 'none'; }, 400); | |
| setTimeout(() => { | |
| // Only hide if this screen is not the current active screen | |
| if (outgoing !== STATE.activeScreen) { | |
| outgoing.style.display = 'none'; | |
| } | |
| }, 400); |
Unified 16 unmerged PRs into a single coherent implementation. Prioritized research integrity (participant blinding, data fidelity) over all other concerns. Eliminated console logging, fixed double-tap race conditions, corrected Firebase sync pipeline, and removed XSS attack vectors.
Participant ID & State Persistence
crypto.randomUUID()withMath.random()fallback for non-HTTPS contextsactiveScreenandpendingTransitionTimerfor transition correctnessTiming & Interaction Accuracy
trialStartTimein nestedrequestAnimationFrame + setTimeout(0)to ensure measurement starts only after pixels paintisTrialActivelock prevents double-tap contaminationresponse_latency_msfield to trial data (was missing)Screen Transitions
display: noneby 400ms so CSS opacity transition completesDOM.screenscacheclearTimeoutprevents overlapping fade-in timersFirebase Sync Pipeline
Pre-backup → Network → Acknowledgment → Cleanup
db.waitForPendingWrites()before cleanup prevents premature success statelocalBackupSucceededflag gates error message text; always displayDOM.finalActions(no dead screens)Security & Experimental Integrity
console.*calls (exposes condition assignment to participants)innerHTMLwithtextContentandcreateElementfor dynamic contenthistory.go(1)(previouspushStateapproach grew history stack unboundedly)UI & Deployment
user-select: noneon#display-pidso participants can copy their IDuser-scalable=no(standard) instead ofuser-scalable=0benchmark.html, test files, andtelemetry_verification/from deploymentValidation
The experiment completes end-to-end in both
?condition=controland?condition=aiwith:pointer-events: none)Original prompt