Fix race conditon

[erseco]

This pull request improves the reliability and correctness of collaborative editing and form field synchronization in the Decker task card UI. The main changes ensure that initial document and form values are captured from the server-rendered HTML before any collaboration starts, and that event-based synchronization is used instead of fixed timeouts. This prevents data loss and race conditions when users join a collaborative session.

Collaboration sync improvements:

• Added event-based sync tracking in decker-collaboration.js, including a promise-based mechanism and safety timeout to reliably detect when initial sync completes, supporting both multi-user and single-user cases.
• Introduced new API methods: isSynced(), onSynced(callback), and initializeContentWithFallback(originalHtml) to allow other code to react to sync completion and initialize Quill content only after sync is confirmed.

Form field initialization improvements:

• Added captureOriginalFormValues(context) to snapshot all original form and select field values, as well as the initial Quill HTML, before any collaborative binding occurs. This ensures the correct initial state for the first user. [1] [2]
• Updated initializeFormFieldValues() to use event-based sync and populate Yjs form fields from the original snapshot for the first user, rather than relying on current DOM values or timeouts. Also handles edge cases where no snapshot or remote data is available. [1] [2]

Quill content initialization:

• Changed Quill collaborative content initialization to use the new event-based API and original HTML snapshot, ensuring content is only set after sync and not overwritten by late sync events.

API deprecation:

• Marked setInitialContent(html) as deprecated in favor of the new event-based initialization methods.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ 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".

[chatgpt-codex-connector]

Handle y-webrtc synced event boolean

WebrtcProvider’s synced event passes a boolean (provider.on('synced', synced => …)), but the new handler destructures an object parameter, so synced is always undefined and the sync promise never resolves on real sync completion. As a result, onSynced callbacks (used to initialize form fields and Quill content) only fire after the single-user heuristic or the 5s safety timeout, leaving collaborators waiting ~5s before initial remote values populate in multi-user sessions.

Useful? React with 👍 / 👎.

[erseco]

@copilot can you review and fix this?

[Copilot]

@erseco I've opened a new pull request, #209, to work on those changes. Once the pull request is ready, I'll request review from you.

[Copilot]

Pull request overview

This pull request addresses race conditions in collaborative editing by implementing event-based synchronization instead of fixed timeouts. The changes capture initial form and editor values before collaboration starts, preventing data loss when the first user joins a session.

Changes:

• Added event-based sync tracking with isSynced(), onSynced(), and initializeContentWithFallback() API methods in the collaboration module
• Introduced captureOriginalFormValues() to snapshot server-rendered values before any collaborative binding
• Refactored form field initialization to use event-based sync and populate Yjs from the original snapshot for first users

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 14 comments.

File	Description
public/assets/js/task-card.js	Adds snapshot capture of original form values and updates form field initialization to use event-based sync instead of timeouts
public/assets/js/decker-collaboration.js	Implements sync state tracking with promise-based callbacks, adds new API methods for sync detection, and updates content initialization

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Potential XSS vulnerability: Setting innerHTML with user-controlled content (tempDiv.innerHTML = originalHtml on line 708) without sanitization could allow malicious HTML injection. While originalHtml is supposed to come from server-rendered HTML, it's stored in the snapshot and could potentially be tampered with. Consider sanitizing the HTML or using a safer approach like textContent if only plain text extraction is needed.

[Copilot]

Resource leak: The checkSingleUser function schedules itself recursively with setTimeout (line 461), but there's no mechanism to cancel these pending timers when the session is destroyed. If destroy() is called while checkSingleUser is scheduled to run, the callback will execute after destruction, potentially accessing destroyed state. Consider storing timeout IDs and clearing them in destroy().

[Copilot]

Performance: The checkSingleUser function calls awareness.getStates() on every iteration (line 443), which could be expensive if getStates() creates a new Map or performs other operations each time. The function runs up to 10 times with 100ms intervals. Consider caching the result or using a more efficient check if available.

[Copilot]

Line 206 filters out empty strings when checking if originalValue should be set to Yjs, but this means fields with empty string values from the server won't be synchronized. If a field is intentionally empty on the server (e.g., a cleared date field), this value should still be synchronized to Yjs so other users see the empty state. Consider removing the check for empty string, or handle it differently for different field types (e.g., dates vs text fields).

[Copilot]

Resource leak: The syncTimeout timer (line 469) is not cleared in the destroy() method (lines 768-796). While line 478 clears the timeout when sync completes normally, if destroy() is called before sync completes (e.g., when quickly switching between tasks), the timeout will fire after destruction, potentially calling syncPromiseResolve on a destroyed session. Store syncTimeout in a way that allows it to be cleared during cleanup.

[Copilot]

Race condition: The isSynced flag can be set to true by three different code paths (synced event on line 428, single-user check on line 448, and timeout on line 472), but there's no locking mechanism. If multiple paths execute nearly simultaneously, syncPromiseResolve could potentially be called multiple times. While Promise.resolve() is idempotent, it's better practice to check isSynced before setting it and calling resolve. Consider adding a check like 'if (!isSynced)' before line 448 and 472 (already present on line 427).

[Copilot]

The checkSingleUser function doesn't handle the case where maxSingleUserChecks is reached without resolving sync. If we reach 10 checks (1 second) without detecting single-user mode or multiple peers, and synced event never fires, the only fallback is the 2-second safety timeout. Consider explicitly handling this case after max checks are exhausted to avoid waiting unnecessarily for the timeout.

Suggested change

	setTimeout(checkSingleUser, 100);
	setTimeout(checkSingleUser, 100);
	} else {
	// Max checks exhausted without clear single-user or multi-peer detection.
	// Fall back to proceeding now instead of waiting for the 2s safety timeout.
	console.warn('Decker Collaboration: Max single-user checks reached without sync, proceeding');
	isSynced = true;
	syncPromiseResolve();

[Copilot]

Line 206 checks if originalValue is not undefined and not an empty string, but on line 213 it checks el.checked = originalValue without verifying the value is defined. For checkboxes, if originalValue is undefined, this will set checked to undefined which coerces to false. This could incorrectly uncheck a checkbox that should remain checked. Consider adding a check: if (originalValue !== undefined) before setting el.checked.

Suggested change

	el.checked = originalValue;
	if (originalValue !== undefined) {
	el.checked = originalValue;
	}