-
Notifications
You must be signed in to change notification settings - Fork 0
340 local checklist notes #341
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
1be3407
cfd7be4
d75cef4
1ab14ce
1d0a439
7eb44c5
c214af7
33eb6d1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,213 @@ | ||
| /** | ||
| * LocalTextAdapter - Y.Text-compatible adapter for local checklist comments | ||
| * | ||
| * Mimics Y.Text interface to enable NoteEditor to work with plain strings | ||
| * in local checklists without requiring full Yjs infrastructure. | ||
| */ | ||
|
|
||
| /** | ||
| * Creates adapter factory functions for local checklist text fields. | ||
| * Encapsulates all the path resolution and caching logic. | ||
| * | ||
| * @param {Function} getChecklist - Getter for current checklist state | ||
| * @param {Function} updateState - Function to update checklist state: (updater: (prev) => next) => void | ||
| * @param {Function} save - Function to trigger debounced persistence (reads current state when it fires) | ||
| * @returns {Object} Factory functions and cache control | ||
| */ | ||
| export function createLocalAdapterFactories(getChecklist, updateState, save) { | ||
| const cache = new Map(); | ||
|
|
||
| function getOrCreateAdapter(path, getValue, getUpdatedState) { | ||
| if (cache.has(path)) { | ||
| return cache.get(path); | ||
| } | ||
|
|
||
| const currentValue = getValue(getChecklist()); | ||
|
|
||
| const adapter = new LocalTextAdapter(currentValue, newValue => { | ||
| updateState(prev => { | ||
| if (!prev) return prev; | ||
| const updated = getUpdatedState(prev, newValue); | ||
| // Trigger save after state is updated - save() reads current state when it fires | ||
| save(); | ||
| return updated; | ||
| }); | ||
| }); | ||
|
|
||
| cache.set(path, adapter); | ||
| return adapter; | ||
| } | ||
|
|
||
| // ROB2: domain comments and preliminary text fields | ||
| function getRob2Text(sectionKey, fieldKey, questionKey) { | ||
| let path, getValue, getUpdatedState; | ||
|
|
||
| if (sectionKey.startsWith('domain') && questionKey) { | ||
| path = `${sectionKey}.${questionKey}.comment`; | ||
| getValue = cl => cl?.[sectionKey]?.answers?.[questionKey]?.comment || ''; | ||
| getUpdatedState = (prev, newValue) => ({ | ||
| ...prev, | ||
| [sectionKey]: { | ||
| ...prev[sectionKey], | ||
| answers: { | ||
| ...prev[sectionKey]?.answers, | ||
| [questionKey]: { | ||
| ...prev[sectionKey]?.answers?.[questionKey], | ||
| comment: newValue, | ||
| }, | ||
| }, | ||
| }, | ||
| }); | ||
| } else if (sectionKey === 'preliminary') { | ||
| path = `preliminary.${fieldKey}`; | ||
| getValue = cl => cl?.preliminary?.[fieldKey] || ''; | ||
| getUpdatedState = (prev, newValue) => ({ | ||
| ...prev, | ||
| preliminary: { | ||
| ...prev.preliminary, | ||
| [fieldKey]: newValue, | ||
| }, | ||
| }); | ||
| } else { | ||
| return null; | ||
| } | ||
|
|
||
| return getOrCreateAdapter(path, getValue, getUpdatedState); | ||
| } | ||
|
|
||
| // AMSTAR2: question notes | ||
| function getQuestionNote(questionKey) { | ||
| const path = `notes.${questionKey}`; | ||
| const getValue = cl => cl?.notes?.[questionKey] || ''; | ||
| const getUpdatedState = (prev, newValue) => ({ | ||
| ...prev, | ||
| notes: { | ||
| ...prev.notes, | ||
| [questionKey]: newValue, | ||
| }, | ||
| }); | ||
|
|
||
| return getOrCreateAdapter(path, getValue, getUpdatedState); | ||
| } | ||
|
|
||
| // ROBINS-I: domain comments, sectionB comments, and section text fields | ||
| function getRobinsText(sectionKey, fieldKey, questionKey) { | ||
| let path, getValue, getUpdatedState; | ||
|
|
||
| if (sectionKey.startsWith('domain') && questionKey) { | ||
| path = `${sectionKey}.${questionKey}.comment`; | ||
| getValue = cl => cl?.[sectionKey]?.answers?.[questionKey]?.comment || ''; | ||
| getUpdatedState = (prev, newValue) => ({ | ||
| ...prev, | ||
| [sectionKey]: { | ||
| ...prev[sectionKey], | ||
| answers: { | ||
| ...prev[sectionKey]?.answers, | ||
| [questionKey]: { | ||
| ...prev[sectionKey]?.answers?.[questionKey], | ||
| comment: newValue, | ||
| }, | ||
| }, | ||
| }, | ||
| }); | ||
| } else if (sectionKey === 'sectionB' && questionKey) { | ||
| path = `sectionB.${questionKey}.comment`; | ||
| getValue = cl => cl?.sectionB?.[questionKey]?.comment || ''; | ||
| getUpdatedState = (prev, newValue) => ({ | ||
| ...prev, | ||
| sectionB: { | ||
| ...prev.sectionB, | ||
| [questionKey]: { | ||
| ...prev.sectionB?.[questionKey], | ||
| comment: newValue, | ||
| }, | ||
| }, | ||
| }); | ||
| } else if (['sectionA', 'sectionC', 'sectionD', 'planning'].includes(sectionKey)) { | ||
| path = `${sectionKey}.${fieldKey}`; | ||
| getValue = cl => cl?.[sectionKey]?.[fieldKey] || ''; | ||
| getUpdatedState = (prev, newValue) => ({ | ||
| ...prev, | ||
| [sectionKey]: { | ||
| ...prev[sectionKey], | ||
| [fieldKey]: newValue, | ||
| }, | ||
| }); | ||
| } else { | ||
| return null; | ||
| } | ||
|
|
||
| return getOrCreateAdapter(path, getValue, getUpdatedState); | ||
| } | ||
|
|
||
| function clearCache() { | ||
| cache.clear(); | ||
| } | ||
|
|
||
| return { | ||
| getRob2Text, | ||
| getQuestionNote, | ||
| getRobinsText, | ||
| clearCache, | ||
| }; | ||
| } | ||
|
|
||
| export class LocalTextAdapter { | ||
| constructor(initialValue = '', onUpdate) { | ||
| this._value = initialValue; | ||
| this._onUpdate = onUpdate; | ||
| this._observers = new Set(); | ||
|
|
||
| // Mock doc object with transact method (executes immediately) | ||
| this.doc = { | ||
| transact: fn => { | ||
| fn(); | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| toString() { | ||
| return this._value; | ||
| } | ||
|
|
||
| get length() { | ||
| return this._value.length; | ||
| } | ||
|
|
||
| observe(callback) { | ||
| this._observers.add(callback); | ||
| } | ||
|
|
||
| unobserve(callback) { | ||
| this._observers.delete(callback); | ||
| } | ||
|
|
||
| delete(index, length) { | ||
| const before = this._value.substring(0, index); | ||
| const after = this._value.substring(index + length); | ||
| this._value = before + after; | ||
| this._notifyUpdate(); | ||
| } | ||
|
|
||
| insert(index, text) { | ||
| const before = this._value.substring(0, index); | ||
| const after = this._value.substring(index); | ||
| this._value = before + text + after; | ||
| this._notifyUpdate(); | ||
| } | ||
|
|
||
| _notifyUpdate() { | ||
| if (this._onUpdate) { | ||
| this._onUpdate(this._value); | ||
| } | ||
| this._observers.forEach(callback => callback()); | ||
| } | ||
|
|
||
| // Allow external sync of value (e.g., when checklist reloads) | ||
| _syncValue(newValue) { | ||
| if (this._value !== newValue) { | ||
| this._value = newValue; | ||
| this._observers.forEach(callback => callback()); | ||
| } | ||
|
Comment on lines
+199
to
+211
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Biome lint error: avoid implicit returns in forEach callbacks. Arrow expressions implicitly return, which triggers lint errors. Wrap callbacks in a block to avoid returning a value. Proposed fix- this._observers.forEach(callback => callback());
+ this._observers.forEach(callback => {
+ callback();
+ });
@@
- this._observers.forEach(callback => callback());
+ this._observers.forEach(callback => {
+ callback();
+ });🧰 Tools🪛 Biome (2.3.13)[error] 202-202: This callback passed to forEach() iterable method should not return a value. Either remove this return or remove the returned value. (lint/suspicious/useIterableCallbackReturn) [error] 209-209: This callback passed to forEach() iterable method should not return a value. Either remove this return or remove the returned value. (lint/suspicious/useIterableCallbackReturn) 🤖 Prompt for AI Agents |
||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Race condition when navigating between checklists loses or corrupts data
High Severity
The refactored
debouncedSavereadsparams.checklistIdwhen it fires, not when scheduled. When navigating from checklist A to B, the pending save isn't cancelled in thecreateEffect. If the save fires during loading, it may save A's data to B's ID (data corruption). If it fires after loading completes, A's unsaved changes are lost. The old code capturedchecklistIdat schedule time, avoiding this issue.Additional Locations (1)
packages/web/src/components/checklist/LocalChecklistView.jsx#L54-L102