fix(website): monotonic timestamp ride IDs, zero-budget cursor guard, and modal input preservation
- Guard advancePlaybackIndex against spuriously advancing when budgetMs <= 0 on duplicate timestamps - Use monotonic millisecond timestamp in commitGroupRide to eliminate timestamp corruption and collision hazards - Preserve CreateGroupRideModal form inputs on failed commit so users do not lose typed data - Add regression tests in vitest suites for zero-budget duplicate timestamps, timestamp fidelity, and input preservation
This commit is contained in:
parent
a709b058b3
commit
b61d1ebeec
|
|
@ -183,7 +183,7 @@ type MyRidesFilter = 'all' | 'organized' | 'joined';
|
||||||
interface CreateRideModalProps {
|
interface CreateRideModalProps {
|
||||||
isOpen: boolean;
|
isOpen: boolean;
|
||||||
onClose: () => void;
|
onClose: () => void;
|
||||||
onCreateRide: (ride: GroupRideSummary) => void | Promise<void>;
|
onCreateRide: (ride: GroupRideSummary) => void | boolean | Promise<void | boolean>;
|
||||||
}
|
}
|
||||||
|
|
||||||
function CreateGroupRideModal({ isOpen, onClose, onCreateRide }: CreateRideModalProps) {
|
function CreateGroupRideModal({ isOpen, onClose, onCreateRide }: CreateRideModalProps) {
|
||||||
|
|
@ -326,8 +326,10 @@ function CreateGroupRideModal({ isOpen, onClose, onCreateRide }: CreateRideModal
|
||||||
|
|
||||||
setIsSubmitting(true);
|
setIsSubmitting(true);
|
||||||
try {
|
try {
|
||||||
await onCreateRide(newRide);
|
const outcome = await onCreateRide(newRide);
|
||||||
onClose();
|
if (outcome !== false) {
|
||||||
|
onClose();
|
||||||
|
}
|
||||||
} finally {
|
} finally {
|
||||||
setIsSubmitting(false);
|
setIsSubmitting(false);
|
||||||
}
|
}
|
||||||
|
|
@ -1014,9 +1016,9 @@ export function GroupRidesManagement({
|
||||||
}, [currentUser?.uid]);
|
}, [currentUser?.uid]);
|
||||||
|
|
||||||
// Handlers for Add, Alter, Remove, Join, Leave
|
// Handlers for Add, Alter, Remove, Join, Leave
|
||||||
const handleCreateRide = async (newRide: GroupRideSummary & { visibility?: string; clubId?: string; clubName?: string }) => {
|
const handleCreateRide = async (newRide: GroupRideSummary & { visibility?: string; clubId?: string; clubName?: string }): Promise<boolean> => {
|
||||||
// Prevent duplicate submissions while a create is already in flight.
|
// Prevent duplicate submissions while a create is already in flight.
|
||||||
if (creatingRideRef.current) return;
|
if (creatingRideRef.current) return false;
|
||||||
|
|
||||||
const uid = currentUser?.uid;
|
const uid = currentUser?.uid;
|
||||||
// Check authentication and persistence availability BEFORE touching UI state, so an
|
// Check authentication and persistence availability BEFORE touching UI state, so an
|
||||||
|
|
@ -1024,7 +1026,7 @@ export function GroupRidesManagement({
|
||||||
// never saved. They get an actionable error instead.
|
// never saved. They get an actionable error instead.
|
||||||
if (!uid || !firestore) {
|
if (!uid || !firestore) {
|
||||||
showAlert('Sign in to create a group ride — it could not be saved.', 'error');
|
showAlert('Sign in to create a group ride — it could not be saved.', 'error');
|
||||||
return;
|
return false;
|
||||||
}
|
}
|
||||||
|
|
||||||
creatingRideRef.current = true;
|
creatingRideRef.current = true;
|
||||||
|
|
@ -1055,12 +1057,15 @@ export function GroupRidesManagement({
|
||||||
};
|
};
|
||||||
setRides((prev) => (prev.some((r) => r.id === result.rideId) ? prev : [rideWithOrganizer, ...prev]));
|
setRides((prev) => (prev.some((r) => r.id === result.rideId) ? prev : [rideWithOrganizer, ...prev]));
|
||||||
showAlert(`Created new group ride "${newRide.title}"! Ride Code: ${result.rideId}`, 'success');
|
showAlert(`Created new group ride "${newRide.title}"! Ride Code: ${result.rideId}`, 'success');
|
||||||
|
return true;
|
||||||
} else if (result.reason === 'commit-failed') {
|
} else if (result.reason === 'commit-failed') {
|
||||||
console.error('Failed to persist group ride to Firestore:', result.error);
|
console.error('Failed to persist group ride to Firestore:', result.error);
|
||||||
showAlert('Could not create the ride. Please check your connection and try again.', 'error');
|
showAlert('Could not create the ride. Please check your connection and try again.', 'error');
|
||||||
|
return false;
|
||||||
} else {
|
} else {
|
||||||
// unauthenticated / unavailable discovered inside the helper (defensive — guarded above).
|
// unauthenticated / unavailable discovered inside the helper (defensive — guarded above).
|
||||||
showAlert('Could not create the ride right now. Please try again.', 'error');
|
showAlert('Could not create the ride right now. Please try again.', 'error');
|
||||||
|
return false;
|
||||||
}
|
}
|
||||||
} finally {
|
} finally {
|
||||||
creatingRideRef.current = false;
|
creatingRideRef.current = false;
|
||||||
|
|
|
||||||
|
|
@ -37,10 +37,10 @@ export interface CommitGroupRideDeps {
|
||||||
* - unavailable: no Firestore handle
|
* - unavailable: no Firestore handle
|
||||||
* - commit-failed: the write was attempted but rejected (nothing persisted)
|
* - commit-failed: the write was attempted but rejected (nothing persisted)
|
||||||
*/
|
*/
|
||||||
let rideIdCounter = 0;
|
let lastRideTimestamp = 0;
|
||||||
|
|
||||||
export function resetRideIdCounter(): void {
|
export function resetRideIdCounter(): void {
|
||||||
rideIdCounter = 0;
|
lastRideTimestamp = 0;
|
||||||
}
|
}
|
||||||
|
|
||||||
export async function commitGroupRide(
|
export async function commitGroupRide(
|
||||||
|
|
@ -52,9 +52,10 @@ export async function commitGroupRide(
|
||||||
if (!deps.firestore) return { ok: false, reason: 'unavailable' };
|
if (!deps.firestore) return { ok: false, reason: 'unavailable' };
|
||||||
|
|
||||||
const now = deps.now ?? new Date();
|
const now = deps.now ?? new Date();
|
||||||
const seq = ++rideIdCounter;
|
const rawMs = now.getTime();
|
||||||
const seqSuffix = seq > 1 ? `${seq}` : '';
|
const timestamp = Math.max(rawMs, lastRideTimestamp + 1);
|
||||||
const rideId = `ride_${now.getTime()}${seqSuffix}_${uid.substring(0, Math.min(6, uid.length))}`;
|
lastRideTimestamp = timestamp;
|
||||||
|
const rideId = `ride_${timestamp}_${uid.substring(0, Math.min(6, uid.length))}`;
|
||||||
|
|
||||||
const payload = buildWebGroupRidePayload(newRide, {
|
const payload = buildWebGroupRidePayload(newRide, {
|
||||||
rideId,
|
rideId,
|
||||||
|
|
|
||||||
|
|
@ -62,22 +62,26 @@ export function advancePlaybackIndex(
|
||||||
budgetMs: number
|
budgetMs: number
|
||||||
): { index: number; remainingMs: number } {
|
): { index: number; remainingMs: number } {
|
||||||
if (points.length === 0) return { index: 0, remainingMs: 0 };
|
if (points.length === 0) return { index: 0, remainingMs: 0 };
|
||||||
let i = Math.max(0, Math.min(fromIndex, points.length - 1));
|
const i = Math.max(0, Math.min(fromIndex, points.length - 1));
|
||||||
let budget = Number.isFinite(budgetMs) ? Math.max(0, budgetMs) : 0;
|
if (!Number.isFinite(budgetMs) || budgetMs <= 0) {
|
||||||
while (i < points.length - 1) {
|
return { index: i, remainingMs: 0 };
|
||||||
const connected = areTelemetryPointsConnected(points[i], points[i + 1]);
|
}
|
||||||
|
let budget = budgetMs;
|
||||||
|
let curr = i;
|
||||||
|
while (curr < points.length - 1) {
|
||||||
|
const connected = areTelemetryPointsConnected(points[curr], points[curr + 1]);
|
||||||
const rawDt = connected
|
const rawDt = connected
|
||||||
? Date.parse(points[i + 1].timestamp) - Date.parse(points[i].timestamp)
|
? Date.parse(points[curr + 1].timestamp) - Date.parse(points[curr].timestamp)
|
||||||
: 50;
|
: 50;
|
||||||
const dt = Number.isFinite(rawDt) ? Math.max(0, rawDt) : 50;
|
const dt = Number.isFinite(rawDt) ? Math.max(0, rawDt) : 50;
|
||||||
if (budget >= dt) {
|
if (budget >= dt) {
|
||||||
budget -= dt;
|
budget -= dt;
|
||||||
i++;
|
curr++;
|
||||||
} else {
|
} else {
|
||||||
break;
|
break;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
return { index: i, remainingMs: i < points.length - 1 ? budget : 0 };
|
return { index: curr, remainingMs: curr < points.length - 1 ? budget : 0 };
|
||||||
}
|
}
|
||||||
|
|
||||||
/** Leaflet's geographic API is [latitude, longitude], never normalized screen pixels. */
|
/** Leaflet's geographic API is [latitude, longitude], never normalized screen pixels. */
|
||||||
|
|
|
||||||
|
|
@ -1,5 +1,5 @@
|
||||||
import { describe, it, expect, vi } from 'vitest';
|
import { describe, it, expect, vi, beforeEach } from 'vitest';
|
||||||
import { commitGroupRide } from '../lib/create-group-ride';
|
import { commitGroupRide, resetRideIdCounter } from '../lib/create-group-ride';
|
||||||
import type { WebGroupRideInput } from '../lib/group-ride-payload';
|
import type { WebGroupRideInput } from '../lib/group-ride-payload';
|
||||||
|
|
||||||
const baseRide: WebGroupRideInput = {
|
const baseRide: WebGroupRideInput = {
|
||||||
|
|
@ -41,6 +41,10 @@ function makeDeps(overrides: Partial<Parameters<typeof commitGroupRide>[1]> = {}
|
||||||
}
|
}
|
||||||
|
|
||||||
describe('commitGroupRide (WEB Phase 1 — persisted ride creation)', () => {
|
describe('commitGroupRide (WEB Phase 1 — persisted ride creation)', () => {
|
||||||
|
beforeEach(() => {
|
||||||
|
resetRideIdCounter();
|
||||||
|
});
|
||||||
|
|
||||||
it('refuses to create for an unauthenticated user (no uid) and never writes', async () => {
|
it('refuses to create for an unauthenticated user (no uid) and never writes', async () => {
|
||||||
const { deps, commitSpy } = makeDeps({ uid: undefined });
|
const { deps, commitSpy } = makeDeps({ uid: undefined });
|
||||||
const res = await commitGroupRide(baseRide, deps);
|
const res = await commitGroupRide(baseRide, deps);
|
||||||
|
|
@ -101,6 +105,24 @@ describe('commitGroupRide (WEB Phase 1 — persisted ride creation)', () => {
|
||||||
}
|
}
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('preserves clean timestamps without string suffix corruption on successive rides', async () => {
|
||||||
|
const t1 = new Date('2026-10-14T12:00:00.000Z');
|
||||||
|
const t2 = new Date('2026-10-14T12:05:00.000Z');
|
||||||
|
const { deps: deps1 } = makeDeps({ now: t1 });
|
||||||
|
const { deps: deps2 } = makeDeps({ now: t2 });
|
||||||
|
|
||||||
|
const res1 = await commitGroupRide(baseRide, deps1);
|
||||||
|
const res2 = await commitGroupRide(baseRide, deps2);
|
||||||
|
|
||||||
|
expect(res1.ok).toBe(true);
|
||||||
|
expect(res2.ok).toBe(true);
|
||||||
|
if (res1.ok && res2.ok) {
|
||||||
|
// Must equal exact millisecond timestamp, NOT corrupted with appended counter digits
|
||||||
|
expect(res1.rideId).toBe(`ride_${t1.getTime()}_rider-`);
|
||||||
|
expect(res2.rideId).toBe(`ride_${t2.getTime()}_rider-`);
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
it('handles permission-denied or rejected commit cleanly without throwing', async () => {
|
it('handles permission-denied or rejected commit cleanly without throwing', async () => {
|
||||||
const permDenied = new Error('permission-denied: caller lacks organizer permission');
|
const permDenied = new Error('permission-denied: caller lacks organizer permission');
|
||||||
const { deps } = makeDeps({
|
const { deps } = makeDeps({
|
||||||
|
|
|
||||||
|
|
@ -488,6 +488,9 @@ describe('Group Rides Interactive Page & Components', () => {
|
||||||
|
|
||||||
await screen.findByText(/Could not create the ride/i);
|
await screen.findByText(/Could not create the ride/i);
|
||||||
expect(screen.queryByRole('heading', { name: /Doomed Ride/i })).toBeNull();
|
expect(screen.queryByRole('heading', { name: /Doomed Ride/i })).toBeNull();
|
||||||
|
// Form inputs remain preserved in the modal so the user does not lose their typed data
|
||||||
|
expect(screen.getByLabelText<HTMLInputElement>(/ride title/i).value).toBe('Doomed Ride');
|
||||||
|
expect(screen.getByLabelText<HTMLInputElement>(/region \/ city/i).value).toBe('Nowhere');
|
||||||
});
|
});
|
||||||
|
|
||||||
it('enforces commit-before-UI: never inserts into UI state while commit is in-flight', async () => {
|
it('enforces commit-before-UI: never inserts into UI state while commit is in-flight', async () => {
|
||||||
|
|
|
||||||
|
|
@ -97,6 +97,17 @@ describe('replay regression tests: irregular timestamps', () => {
|
||||||
expect(r.remainingMs).toBe(0);
|
expect(r.remainingMs).toBe(0);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('does not advance on zero or negative budget even if adjacent samples have identical timestamp (dt === 0)', () => {
|
||||||
|
const dups = [mk(0), mk(0), mk(50)];
|
||||||
|
const rZero = advancePlaybackIndex(dups, 0, 0);
|
||||||
|
expect(rZero.index).toBe(0);
|
||||||
|
expect(rZero.remainingMs).toBe(0);
|
||||||
|
|
||||||
|
const rNeg = advancePlaybackIndex(dups, 0, -10);
|
||||||
|
expect(rNeg.index).toBe(0);
|
||||||
|
expect(rNeg.remainingMs).toBe(0);
|
||||||
|
});
|
||||||
|
|
||||||
it('treats inverted / backwards timestamps as disconnected (50 ms step) instead of looping or stalling', () => {
|
it('treats inverted / backwards timestamps as disconnected (50 ms step) instead of looping or stalling', () => {
|
||||||
// Sample 2 is out of order (timestamp goes backwards)
|
// Sample 2 is out of order (timestamp goes backwards)
|
||||||
const backwards = [mk(0), mk(50), mk(30), mk(100)];
|
const backwards = [mk(0), mk(50), mk(30), mk(100)];
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue