mirror of
https://github.com/wekan/wekan.git
synced 2026-11-05 21:44:59 -05:00
Fix #6507: fresh-install reactive loop / Admin panel broken / "Cannot read properties of undefined (reading 'remove')".
On a fresh install (no Settings document yet, and Meteor.user() briefly null right after the first admin logs in) the Admin panel would not open, the title bar sometimes failed to load or showed "You are not authorized to view this page", and the console looped `Uncaught (in promise) TypeError: Cannot read properties of undefined (reading 'remove')` through a Tracker computation. Root cause: ReactiveCacheClient.getCurrentSetting() and getCurrentUser() guarded their DataCache with `!this.__x || !this.__x.get()`. Every other getter creates its DataCache ONCE (`if (!this.__x)`), but these two ALSO rebuilt it whenever the cached value was FALSY — so while the setting/user was absent (a fresh install, or any transient null) a brand-new DataCache, and a brand-new Tracker.autorun, was constructed on EVERY reactive read. That continuous create/teardown churned computations across every app-wide template (header, admin gates) and raced Blaze's view teardown, producing the infinite reactive loop and the removed-DomRange crash (the same WebKit Blaze class the getBoard staleWhileRevalidate comment guards). Fixed by creating the DataCache once, keyed on `!this.__currentSetting` / `!this.__currentUser` only, like every other getter; the autorun already updates reactively when the Settings document / user appears, so no rebuild is ever needed. Returning a stable null until they exist is correct and loop-free. Test: tests/reactiveCacheCurrentGetters.test.cjs. Thanks to AmigaAbattoir and xet7 ! Fixes #6507,
This commit is contained in:
1 parent
0c5208cf73
commit
72768e05fe
2 files changed
+79
-2
No files matched your search
@@ -1041,7 +1041,17 @@ const ReactiveCacheClient = {
|
||||
return ret;
|
||||
},
|
||||
getCurrentSetting() {
|
||||
if (!this.__currentSetting || !this.__currentSetting.get()) {
|
||||
// #6507: create the DataCache ONCE (like every other getter above), keyed on
|
||||
// `!this.__currentSetting` only. The old guard ALSO rebuilt it whenever the cached
|
||||
// value was FALSY (it OR-ed a check on the getter result), so on a fresh install
|
||||
// with no Settings document (or any moment the setting is briefly absent) a new
|
||||
// DataCache — and a new Tracker.autorun — was built on EVERY reactive read. That
|
||||
// continuous create/teardown churned computations app-wide and raced Blaze's
|
||||
// view teardown, producing an infinite reactive loop and the "Cannot read
|
||||
// properties of undefined (reading 'remove')" crash (the removed-DomRange class
|
||||
// the getBoard staleWhileRevalidate comment above also guards against). Returning
|
||||
// a stable null until the document exists is correct and loop-free.
|
||||
if (!this.__currentSetting) {
|
||||
this.__currentSetting = new DataCache(() => {
|
||||
const _ret = Settings.findOne();
|
||||
return _ret;
|
||||
@@ -1051,7 +1061,10 @@ const ReactiveCacheClient = {
|
||||
return ret;
|
||||
},
|
||||
getCurrentUser() {
|
||||
if (!this.__currentUser || !this.__currentUser.get()) {
|
||||
// #6507: same fix as getCurrentSetting — create the DataCache once. The old
|
||||
// guard rebuilt it on every read while Meteor.user() was null (before login
|
||||
// lands / a fresh install), thrashing computations and crashing Blaze teardown.
|
||||
if (!this.__currentUser) {
|
||||
this.__currentUser = new DataCache(() => {
|
||||
const _ret = Meteor.user();
|
||||
return _ret;
|
||||
|
||||
@@ -0,0 +1,64 @@
|
||||
'use strict';
|
||||
|
||||
// Regression for #6507: fresh install (no Settings document, Meteor.user() briefly
|
||||
// null) — the Admin panel would not open, the title bar sometimes failed to load /
|
||||
// showed "You are not authorized", and the console looped
|
||||
// Uncaught (in promise) TypeError: Cannot read properties of undefined (reading 'remove')
|
||||
// through a Tracker computation (a Blaze removed-DomRange teardown crash).
|
||||
//
|
||||
// Root cause: ReactiveCacheClient.getCurrentSetting() / getCurrentUser() guarded the
|
||||
// DataCache with `!this.__x || !this.__x.get()`, so whenever the value was FALSY they
|
||||
// rebuilt the DataCache — and its Tracker.autorun — on EVERY reactive read, churning
|
||||
// computations app-wide and racing Blaze teardown. Every other getter creates its
|
||||
// DataCache once (`if (!this.__x)`). This guards that the two current-* getters do the
|
||||
// same, and models the churn behaviourally.
|
||||
// Run: node tests/reactiveCacheCurrentGetters.test.cjs
|
||||
|
||||
const assert = require('assert');
|
||||
const fs = require('fs');
|
||||
const path = require('path');
|
||||
|
||||
let passed = 0;
|
||||
function check(name, fn) { fn(); passed += 1; console.log(' ok -', name); }
|
||||
|
||||
const src = fs.readFileSync(path.join(__dirname, '..', 'imports', 'reactiveCache.js'), 'utf8');
|
||||
|
||||
check('getCurrentSetting/getCurrentUser create the DataCache ONCE (no recreate-on-falsy)', () => {
|
||||
assert.ok(/getCurrentSetting\(\) \{[\s\S]*?if \(!this\.__currentSetting\) \{/.test(src),
|
||||
'getCurrentSetting must guard on !this.__currentSetting only');
|
||||
assert.ok(/getCurrentUser\(\) \{[\s\S]*?if \(!this\.__currentUser\) \{/.test(src),
|
||||
'getCurrentUser must guard on !this.__currentUser only');
|
||||
});
|
||||
|
||||
check('the recreate-on-falsy anti-pattern is gone from the whole file', () => {
|
||||
assert.ok(!/\|\| !this\.__[a-zA-Z]+\.get\(\)/.test(src),
|
||||
'no getter may rebuild its DataCache when the cached value is falsy');
|
||||
});
|
||||
|
||||
// Behavioural model: the guard must build the cache once even when the value stays
|
||||
// falsy across many reactive reads (the fresh-install case).
|
||||
check('the guard builds the cache once across many falsy reads (models the fix)', () => {
|
||||
function makeGetter() {
|
||||
const self = { __currentSetting: undefined, builds: 0 };
|
||||
const build = () => { self.builds += 1; return { get: () => undefined /* no Settings doc */ }; };
|
||||
return () => {
|
||||
// The FIXED guard:
|
||||
if (!self.__currentSetting) self.__currentSetting = build();
|
||||
return { self, value: self.__currentSetting.get() };
|
||||
};
|
||||
}
|
||||
const get = makeGetter();
|
||||
let last;
|
||||
for (let i = 0; i < 25; i++) last = get();
|
||||
assert.strictEqual(last.self.builds, 1, 'the DataCache is built exactly once, not once per read');
|
||||
|
||||
// And the OLD guard would have rebuilt every time — proving the bug shape.
|
||||
let oldBuilds = 0, cache;
|
||||
for (let i = 0; i < 25; i++) {
|
||||
if (!cache || !cache.get()) { oldBuilds += 1; cache = { get: () => undefined }; }
|
||||
cache.get();
|
||||
}
|
||||
assert.strictEqual(oldBuilds, 25, 'sanity: the old `|| !get()` guard rebuilt on every read');
|
||||
});
|
||||
|
||||
console.log(`\nreactiveCacheCurrentGetters: ${passed} checks passed`);
|
||||
Reference in new issue
Block a user