Skip to content

Commit 899e42c

Browse files
Tim020claude
Tim020
and
claude
committed
Address PR #905 review comments: third batch
Safety and robustness fixes from consolidated review: Backend: - C-1: Guard on_message against JSON/key errors (malformed WS messages) - C-2: Guard ScriptRoom.apply_update and send COLLAB_ERROR on failure - C-3: Add finally to on_close _broadcast to guarantee close_active_room runs - C-4: Wrap on_close DB cleanup in try/except to prevent silent failures - C-5: Null-guard session entry in REQUEST_SCRIPT_EDIT/CUTS (race with on_close) - C-8: Fix no_active_script_draft null guard for missing script/revision - H-1: Add RBAC guards (editor check + Role.WRITE) to SAVE/DISCARD_SCRIPT_DRAFT - H-2: Broadcast COLLAB_ERROR warning when show changes while draft is active - H-3: Move blocking file I/O to run_in_executor in checkpoint/load/delete - H-4/H-5: Distinguish WebSocketClosedError (debug) from unexpected errors (warning) - H-6: Log notification failures in save_room instead of silently swallowing - H-11: Notify clients via COLLAB_ERROR before discarding a corrupt draft file - M-7: Remove manual self.on_close() call from write_message (Tornado handles it) - M-8: Null-guard digi_settings.get("current_show") in on_close - SQ-4: Add explanatory comments to intentionally empty Alembic migration Frontend: - C-6: Remove orphaned setupAutoSave() call in saveScript() - C-7: Apply _unwrapYjsValue() to keyed initial population in useYMap() - H-7: Surface sync timeout failure to user via SET_SYNC_ERROR + toast watcher - H-8: Surface COLLAB_ERROR to user via SET_COLLAB_ERROR + toast watcher - H-9: Fix discardAndStartFresh to only hide modal on success; add error toasts - H-10: Show warning toast when getMaxScriptPage() fails - M-3: Return false from applySync/applyUpdate/applyAwareness catch blocks - M-9: Add null guards in _ydocLineToPlain for missing parts; wrap _syncLocalPageScript in try/catch - M-13: Fix applyAwareness to return false for empty/failed payloads (was true) All 709 backend tests and 136 frontend tests pass. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent e94d2d3 commit 899e42c

File tree

10 files changed

+338
-81
lines changed

10 files changed

+338
-81
lines changed

‎client/src/store/modules/scriptDraft.js‎

Lines changed: 26 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,8 @@ export default {
3737
saveError: null,
3838
savePage: 0,
3939
saveTotalPages: 0,
40+
syncError: null,
41+
collabError: null,
4042
},
4143
mutations: {
4244
SET_DRAFT_ROOM(state) {
@@ -76,6 +78,12 @@ export default {
7678
state.savePage = page;
7779
state.saveTotalPages = total;
7880
},
81+
SET_SYNC_ERROR(state, error) {
82+
state.syncError = error;
83+
},
84+
SET_COLLAB_ERROR(state, error) {
85+
state.collabError = error;
86+
},
7987
CLEAR_DRAFT_STATE(state) {
8088
state.isRoomActive = false;
8189
state.isConnected = false;
@@ -89,6 +97,8 @@ export default {
8997
state.saveError = null;
9098
state.savePage = 0;
9199
state.saveTotalPages = 0;
100+
state.syncError = null;
101+
state.collabError = null;
92102
_ydoc = null;
93103
_provider = null;
94104
},
@@ -130,7 +140,7 @@ export default {
130140
}
131141
}, 100);
132142

133-
// Watchdog: log an error only if this is still the active provider
143+
// Watchdog: surface a sync failure to the user if this is still the active provider
134144
_syncTimeoutId = setTimeout(() => {
135145
clearInterval(_syncIntervalId);
136146
_syncIntervalId = null;
@@ -140,6 +150,11 @@ export default {
140150
);
141151
if (provider === _provider && !provider.synced) {
142152
log.error('ScriptDraft: Sync timeout after 10 seconds');
153+
context.commit(
154+
'SET_SYNC_ERROR',
155+
'Failed to sync with the collaboration server after 10 seconds. ' +
156+
'Please try refreshing the page.'
157+
);
143158
}
144159
}, 10000);
145160

@@ -218,8 +233,10 @@ export default {
218233
context.commit('SET_DRAFT_SAVE_PHASE', null);
219234
context.commit('SET_DRAFT_SAVE_ERROR', message.DATA?.error);
220235
},
221-
COLLAB_ERROR(_context, message) {
222-
log.error('Collab error received:', message.DATA?.error);
236+
COLLAB_ERROR(context, message) {
237+
const error = message.DATA?.error || 'A collaboration error occurred';
238+
log.error('Collab error received:', error);
239+
context.commit('SET_COLLAB_ERROR', error);
223240
},
224241
},
225242
getters: {
@@ -275,6 +292,12 @@ export default {
275292
IS_DRAFT_SYNCED(state) {
276293
return state.isSynced;
277294
},
295+
DRAFT_SYNC_ERROR(state) {
296+
return state.syncError;
297+
},
298+
DRAFT_COLLAB_ERROR(state) {
299+
return state.collabError;
300+
},
278301
DRAFT_COLLABORATORS(state) {
279302
return state.collaborators;
280303
},

‎client/src/utils/yjs/ScriptDocProvider.js‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,7 @@ export default class ScriptDocProvider {
108108
}
109109
} catch (e) {
110110
log.error('ScriptDocProvider: Failed to handle sync message', e);
111+
return false;
111112
}
112113

113114
return true;
@@ -123,14 +124,15 @@ export default class ScriptDocProvider {
123124
Y.applyUpdate(this.doc, decoded, 'server');
124125
} catch (e) {
125126
log.error('ScriptDocProvider: Failed to apply update', e);
127+
return false;
126128
}
127129

128130
return true;
129131
}
130132
applyAwareness(data) {
131133
if (!this._connected) return false;
132134
const payload = data.payload;
133-
if (!payload) return true;
135+
if (!payload) return false;
134136

135137
try {
136138
const decoded = decodeBase64(payload);
@@ -139,9 +141,8 @@ export default class ScriptDocProvider {
139141
return { type: 'AWARENESS', state: awarenessState };
140142
} catch (e) {
141143
log.error('ScriptDocProvider: Failed to handle awareness message', e);
144+
return false;
142145
}
143-
144-
return true;
145146
}
146147
setLocalAwareness(state) {
147148
if (!this._connected) return;

‎client/src/utils/yjs/ScriptDocProvider.test.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -152,10 +152,10 @@ describe('ScriptDocProvider', () => {
152152
expect(provider.applyAwareness({ payload: 'dA==' })).toBe(false);
153153
});
154154

155-
it('returns true when payload is missing', () => {
155+
it('returns false when payload is missing', () => {
156156
const { provider } = makeProvider();
157157
provider._connected = true;
158-
expect(provider.applyAwareness({})).toBe(true);
158+
expect(provider.applyAwareness({})).toBe(false);
159159
});
160160

161161
it('decodes payload and returns AWARENESS result', () => {

‎client/src/utils/yjs/useYjsBinding.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ export function useYMap(ymap, keys = null) {
1818

1919
if (keys) {
2020
keys.forEach((key) => {
21-
Vue.set(data, key, ymap.get(key));
21+
Vue.set(data, key, _unwrapYjsValue(ymap.get(key)));
2222
});
2323
} else {
2424
ymap.forEach((value, key) => {

‎client/src/vue_components/show/config/script/ScriptEditor.vue‎

Lines changed: 38 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -409,6 +409,8 @@ export default {
409409
'DRAFT_SAVE_ERROR',
410410
'DRAFT_SAVE_PHASE',
411411
'DRAFT_SAVE_PROGRESS',
412+
'DRAFT_SYNC_ERROR',
413+
'DRAFT_COLLAB_ERROR',
412414
]),
413415
},
414416
watch: {
@@ -452,6 +454,16 @@ export default {
452454
this._collabSaveToast.message =
453455
page === 0 ? 'Saving script...' : `Saving page ${page} of ${total} (${percent}%)`;
454456
},
457+
DRAFT_SYNC_ERROR(error) {
458+
if (error) {
459+
this.$toast.error(error);
460+
}
461+
},
462+
DRAFT_COLLAB_ERROR(error) {
463+
if (error) {
464+
this.$toast.error(error);
465+
}
466+
},
455467
IS_DRAFT_SAVING: function onSavingChanged(saving) {
456468
if (!this.IS_CURRENT_EDITOR) return;
457469
if (saving && !this._collabSaveToast) {
@@ -543,6 +555,9 @@ export default {
543555
this.currentMaxPage = respJson.max_page;
544556
} else {
545557
log.error('Unable to get current max page');
558+
this.$toast.warning(
559+
'Failed to load script page count — page navigation may not work correctly.'
560+
);
546561
}
547562
},
548563
onEditClick() {
@@ -563,13 +578,14 @@ export default {
563578
this.requestEdit();
564579
},
565580
async discardAndStartFresh() {
566-
this.$bvModal.hide('draft-resume-modal');
567581
const response = await fetch(makeURL('/api/v1/show/script/draft'), { method: 'DELETE' });
568582
if (response.ok) {
583+
this.$bvModal.hide('draft-resume-modal');
569584
await this.GET_SCRIPT_CONFIG_STATUS();
570585
this.requestEdit();
571586
} else {
572587
log.error('Failed to discard draft');
588+
this.$toast.error('Failed to discard draft, please try again.');
573589
}
574590
},
575591
async confirmDiscardDraft() {
@@ -581,6 +597,8 @@ export default {
581597
const response = await fetch(makeURL('/api/v1/show/script/draft'), { method: 'DELETE' });
582598
if (response.ok) {
583599
await this.GET_SCRIPT_CONFIG_STATUS();
600+
} else {
601+
this.$toast.error('Failed to discard draft, please try again.');
584602
}
585603
}
586604
},
@@ -799,7 +817,6 @@ export default {
799817
this.savingInProgress = true;
800818
await this.SAVE_SCRIPT_CUTS(this.linePartCuts);
801819
this.resetCutsToSaved();
802-
this.setupAutoSave();
803820
this.savingInProgress = false;
804821
}
805822
},
@@ -840,22 +857,23 @@ export default {
840857
const lineParts = partsArray
841858
? Array.from({ length: partsArray.length }, (_, i) => {
842859
const p = partsArray.get(i);
860+
if (!p) return null;
843861
return {
844-
id: zeroToNull(p.get('_id')),
862+
id: zeroToNull(p.get('_id') ?? 0),
845863
line_id: lineId,
846864
part_index: p.get('part_index'),
847-
character_id: zeroToNull(p.get('character_id')),
848-
character_group_id: zeroToNull(p.get('character_group_id')),
865+
character_id: zeroToNull(p.get('character_id') ?? 0),
866+
character_group_id: zeroToNull(p.get('character_group_id') ?? 0),
849867
line_text: p.get('line_text') ? p.get('line_text').toString() : '',
850868
};
851-
})
869+
}).filter(Boolean)
852870
: [];
853871
return {
854872
id: lineId,
855873
line_type: yMap.get('line_type'),
856-
act_id: zeroToNull(yMap.get('act_id')),
857-
scene_id: zeroToNull(yMap.get('scene_id')),
858-
stage_direction_style_id: zeroToNull(yMap.get('stage_direction_style_id')),
874+
act_id: zeroToNull(yMap.get('act_id') ?? 0),
875+
scene_id: zeroToNull(yMap.get('scene_id') ?? 0),
876+
stage_direction_style_id: zeroToNull(yMap.get('stage_direction_style_id') ?? 0),
859877
line_parts: lineParts,
860878
};
861879
},
@@ -869,13 +887,17 @@ export default {
869887
this.localPageScript = [];
870888
return;
871889
}
872-
const pages = ydoc.getMap('pages');
873-
const pageArray = pages.get(this.currentEditPageKey);
874-
this.localPageScript = pageArray
875-
? Array.from({ length: pageArray.length }, (_, i) =>
876-
this._ydocLineToPlain(pageArray.get(i))
877-
)
878-
: [];
890+
try {
891+
const pages = ydoc.getMap('pages');
892+
const pageArray = pages.get(this.currentEditPageKey);
893+
this.localPageScript = pageArray
894+
? Array.from({ length: pageArray.length }, (_, i) =>
895+
this._ydocLineToPlain(pageArray.get(i))
896+
).filter(Boolean)
897+
: [];
898+
} catch (e) {
899+
log.error('ScriptEditor: Error syncing local page script from Y.Doc', e);
900+
}
879901
},
880902
/**
881903
* Set up the Y.Doc → localPageScript bridge after initial sync completes.

‎server/alembic_config/versions/14f82e306537_.py‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,8 @@
1717

1818

1919
def upgrade() -> None:
20-
pass
20+
pass # intentionally empty — schema merge migration only
2121

2222

2323
def downgrade() -> None:
24-
pass
24+
pass # intentionally empty — schema merge migration only

0 commit comments

Comments
 (0)