chore: code review improvements for operation-logs branch

Phase 1 - Code Quality:
- Add clarifying comment in lww-update.meta-reducer.ts explaining
  Date.now() choice for LWW conflict resolution
- Document multi-instance deployment limitations in README.md
  (passkey challenges, snapshot locks)

Phase 2 - Testing:
- Add TODAY_TAG selector edge case tests (deleted/archived tasks)

Phase 3 - Minor Polish:
- Add createValidationErrorResponse() helper to hide Zod validation
  details in production (sync.routes.ts)
- Add database index @@index([userId, opType]) for faster restore
  point queries
This commit is contained in:
Johannes Millan 2026-01-03 13:17:42 +01:00
parent 374ba9873a
commit 0f8d2cbc42
6 changed files with 120 additions and 13 deletions

View file

@ -259,6 +259,39 @@ Used for full-state operations (BackupImport, SyncImport, Repair):
| **Entity Type Allowlist** | Prevents injection of invalid entity types |
| **Request Deduplication** | Prevents duplicate operations on retry |
## Multi-Instance Deployment Considerations
When deploying multiple server instances behind a load balancer, be aware of these limitations:
### Passkey Challenge Storage
**Issue**: WebAuthn challenges are stored in an in-memory Map, which doesn't work across instances.
**Symptom**: Passkey registration/login fails if the challenge generation request hits instance A but verification hits instance B.
**Solution for multi-instance**:
- Implement Redis-backed challenge storage
- Or use sticky sessions (less ideal)
**Current status**: A warning is logged at startup in production if in-memory storage is used.
### Snapshot Generation Locks
**Issue**: Concurrent snapshot generation prevention uses an in-memory Map.
**Symptom**: Same user may trigger duplicate snapshot computations across different instances.
**Impact**: Performance only (no data corruption) - snapshots are deterministic.
**Solution for multi-instance**:
- Implement Redis distributed lock (optional, only for performance)
### Single-Instance Deployment
For single-instance deployments, these limitations do not apply. The current implementation is fully functional and well-tested for single-instance use.
## Security Notes
- **Set JWT_SECRET** to a secure random value in production (min 32 characters).

View file

@ -0,0 +1,2 @@
-- CreateIndex
CREATE INDEX "operations_user_id_op_type_idx" ON "operations"("user_id", "op_type");

View file

@ -83,6 +83,7 @@ model Operation {
@@index([userId, entityType, entityId])
@@index([userId, clientId])
@@index([userId, receivedAt])
@@index([userId, opType]) // Speeds up restore point listing (queries for SYNC_IMPORT/BACKUP_IMPORT ops)
@@map("operations")
}

View file

@ -20,6 +20,19 @@ import {
const gunzipAsync = promisify(zlib.gunzip);
/**
* Helper to create validation error response.
* In production, hides detailed Zod error info to prevent schema leakage.
*/
const createValidationErrorResponse = (
zodIssues: z.ZodIssue[],
): { error: string; details?: z.ZodIssue[] } => {
if (process.env.NODE_ENV === 'production') {
return { error: 'Validation failed' };
}
return { error: 'Validation failed', details: zodIssues };
};
// Validation constants
const CLIENT_ID_REGEX = /^[a-zA-Z0-9_-]+$/;
const MAX_CLIENT_ID_LENGTH = 255;
@ -210,10 +223,9 @@ export const syncRoutes = async (fastify: FastifyInstance): Promise<void> => {
`[user:${userId}] Upload validation failed`,
parseResult.error.issues,
);
return reply.status(400).send({
error: 'Validation failed',
details: parseResult.error.issues,
});
return reply
.status(400)
.send(createValidationErrorResponse(parseResult.error.issues));
}
const { ops, clientId, lastKnownServerSeq, requestId } = parseResult.data;
@ -460,10 +472,9 @@ export const syncRoutes = async (fastify: FastifyInstance): Promise<void> => {
`[user:${userId}] Download validation failed`,
parseResult.error.issues,
);
return reply.status(400).send({
error: 'Validation failed',
details: parseResult.error.issues,
});
return reply
.status(400)
.send(createValidationErrorResponse(parseResult.error.issues));
}
const { sinceSeq, limit = 500, excludeClient } = parseResult.data;
@ -604,10 +615,13 @@ export const syncRoutes = async (fastify: FastifyInstance): Promise<void> => {
// Validate request body
const parseResult = UploadSnapshotSchema.safeParse(body);
if (!parseResult.success) {
return reply.status(400).send({
error: 'Validation failed',
details: parseResult.error.issues,
});
Logger.warn(
`[user:${userId}] Snapshot upload validation failed`,
parseResult.error.issues,
);
return reply
.status(400)
.send(createValidationErrorResponse(parseResult.error.issues));
}
const {

View file

@ -325,6 +325,53 @@ describe('workContext selectors', () => {
expect(result).toEqual(['parent']); // subtask excluded
});
it('should filter out deleted tasks (taskIds referencing non-existent tasks)', () => {
const task1 = {
id: 'task1',
tagIds: [],
dueDay: todayStr,
subTaskIds: [],
} as Partial<TaskCopy> as TaskCopy;
// task2 exists in TODAY_TAG.taskIds but not in taskState (deleted)
const todayTagWithDeletedTask = {
...TODAY_TAG,
taskIds: ['task1', 'deleted-task', 'also-deleted'],
};
const tagState = fakeEntityStateFromArray([todayTagWithDeletedTask]);
const taskState = fakeEntityStateFromArray([task1]) as any;
const result = selectTodayTaskIds.projector(tagState, taskState);
expect(result).toEqual(['task1']); // deleted tasks filtered out
});
it('should filter out archived tasks with dueDay === today', () => {
const activeTask = {
id: 'active-task',
tagIds: [],
dueDay: todayStr,
subTaskIds: [],
// No 'archived' property - this is the active task
} as Partial<TaskCopy> as TaskCopy;
// In the app, archived tasks are stored in TimeTrackingState, not TaskState.
// An archived task would not exist in the regular task entities.
// This test verifies that if somehow an archived task ID is in TODAY_TAG.taskIds,
// it gets filtered out because it doesn't exist in the active task state.
const todayTagWithArchivedRef = {
...TODAY_TAG,
taskIds: ['active-task', 'archived-task-id'],
};
const tagState = fakeEntityStateFromArray([todayTagWithArchivedRef]);
// Only active task in state - archived-task-id doesn't exist
const taskState = fakeEntityStateFromArray([activeTask]) as any;
const result = selectTodayTaskIds.projector(tagState, taskState);
expect(result).toEqual(['active-task']); // archived task ID filtered out
});
// Tests for dueWithTime fallback (issue #5841)
it('should include task with dueWithTime for today but no dueDay (fallback)', () => {
// Create a timestamp for today (e.g., 8:00 AM today)

View file

@ -398,6 +398,13 @@ export const lwwUpdateMetaReducer: MetaReducer = (
updatedFeatureState = (adapter as EntityAdapter<any>).addOne(
{
...entityData,
// INTENTIONAL: We set modified to Date.now() (local time), not the original timestamp.
// Rationale:
// - Vector clocks are the authoritative conflict resolution mechanism, not `modified`
// - The `modified` field is used for UI display ("last edited X minutes ago")
// - Setting it to local time reflects when THIS client applied the winning state
// - The original timestamp from the winning client is preserved in entityData but
// gets overwritten here because local display should show local application time
modified: Date.now(),
} as any,
featureState as any,
@ -410,7 +417,10 @@ export const lwwUpdateMetaReducer: MetaReducer = (
id: entityId,
changes: {
...entityData,
modified: Date.now(), // Update modified timestamp
// INTENTIONAL: We set modified to Date.now() (local time), not the original timestamp.
// See comment above for rationale - vector clocks drive conflict resolution,
// `modified` is for UI display of "when this client last saw this change"
modified: Date.now(),
},
},
featureState as any,