Skip to content

fix(live-meeting): bind page to shared store cache for live reactivity#166

Open
rubenvdlinde wants to merge 1 commit into
developmentfrom
feat/live-meeting-reactive-store-binding
Open

fix(live-meeting): bind page to shared store cache for live reactivity#166
rubenvdlinde wants to merge 1 commit into
developmentfrom
feat/live-meeting-reactive-store-binding

Conversation

@rubenvdlinde
Copy link
Copy Markdown
Contributor

Summary

  • Promote meeting / allItems / participants from local data to computed getters reading the shared store cache, so liveUpdatesPlugin re-fetches on or-object-* / or-collection-* events flow into the rendered UI.
  • Add setActivePinia(pinia) before initializeStores() in src/main.js; the bootstrap calls useObjectStore() outside any Vue component, where PiniaVuePlugin's auto-activation hasn't run yet.

Why

End-to-end live updates only worked through the WS push chain (OR backend → notify_push → plugin → store). The page itself didn't reactively re-render because LiveMeeting copied store results into local state once at mount. Two-browser test (PUT meeting via API → other browser shows new title in seconds without reload) is now passing.

Test plan

  • Browser-to-browser e2e: PUT meeting title via curl → browser-3 store + DOM both updated within ~6s, no reload (verified 2026-05-10).
  • Page boots without "Cannot read properties of undefined (reading '_s')" from initializeStores.
  • Confirm CI green.

…activity

Pre-migration LiveMeeting.fetchData() copied results into local data
fields, which made the page non-reactive to live updates: the
liveUpdatesPlugin updated the store cache on `or-object-*` events, but
the local copy never re-read. Promote `meeting`, `allItems`, and
`participants` to computed getters reading straight from objectStore;
fetchData() now just triggers initial fetches to seed the cache. The
canonical e2e (PUT meeting via API → page rerenders without reload)
now works against the OR + notify_push backend chain.

Also call setActivePinia(pinia) before initializeStores() in main.js;
without it, useObjectStore() invoked from non-Vue context throws
"Cannot read properties of undefined (reading '_s')" because the
PiniaVuePlugin only auto-activates when Vue itself is constructed.
@github-actions
Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/decidesk @ b1022fa

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
composer ✅ 100/100
npm
PHPUnit ⏭️
Newman ⏭️
Playwright ⏭️

Quality workflow — 2026-05-10 11:56 UTC

Download the full PDF report from the workflow artifacts.

@MWest2020
Copy link
Copy Markdown
Member

Review — fix(live-meeting): bind page to shared store cache for live reactivity

Verdict: 🔴 REQUEST_CHANGES — Core reactivity fix is correct (computed getters + setActivePinia), maar branch is 11 commits achter op development en mist daardoor de ERESOLVE-fix voor de lockfile — 5 CI-checks (lint, eslint, stylelint, npm-security, npm-license) zijn rood door npm ci faal.

Wat goed gaat

De Pinia-store binding via computed getters is exact de juiste manier om Vue 2 Options API aan een gedeelde store te koppelen — geen per-component watcher-boilerplate, liveUpdatesPlugin refetches komen vanzelf in de UI. Memory management is netjes: liveSubs array + objectStore.unsubscribe(handle) loop in beforeDestroy ruimt alle drie subscriptions op. De setActivePinia(pinia) toevoeging in main.js is correct en de inline comment legt uit waarom (PiniaVuePlugin auto-activeert pas na new Vue({ pinia }), terwijl initializeStores() daarvoor draait). refreshInterval netjes verwijderd.

Findings

1. 🔴 Blocker — Branch 11 commits achter op development — npm ci faalt — package-lock.json

Sinds deze branch divergeerde heeft development een ERESOLVE peer-dep conflict gefixt: @nextcloud/eslint-config@8.4.2 vereist eslint-import-resolver-typescript@^3.8.0, wiens transitieve peer @typescript-eslint/utils@8.59.2 botst met de pinned @typescript-eslint/utils@7.18.0 van @typescript-eslint/eslint-plugin@^7.18.0. Alle 5 frontend CI-jobs (lint-check, eslint, stylelint, npm-security, npm-license) falen op npm ci met exit code 1, vóór er ook maar één regel lint-werk gedraaid wordt.

Impact: Branch-protection required checks staan rood. PR kan niet mergen zoals-is. Niet veroorzaakt door deze PR — alleen pijnlijk zichtbaar omdat de branch achterloopt.

Suggested fix: gh pr update-branch 166 --repo ConductionNL/decidesk om de 11 upstream commits in te pullen. Daarna CI opnieuw checken — should go green.

2. 🟡 Concern — allItems/participants computed leunt op undocumented dual-path @self.relationssrc/views/LiveMeeting.vue:222

allItems() {
  const collection = this.objectStore.collections?.['agenda-item'] ?? []
  return collection.filter(i =>
    i?.['@self']?.relations?.meeting === this.id
    || i?.relations?.meeting === this.id
  )
}

Geen ander component in de codebase leest store.collections direct — alle tab-componenten gebruiken server-side fetchCollection met een @self.relations.meeting filter-param. Als de liveUpdatesPlugin items zonder de @self envelope cached, of als een toekomstige schema-wijziging de relation anders nest, geeft deze filter silent een lege array zonder error. participants heeft hetzelfde patroon.

Impact: Bij envelope/schema-drift toont de live-meeting view lege lijsten zonder zichtbare fout — moeilijk te debuggen omdat de store de data wel heeft.

Suggested fix: Of (a) voeg een unit test toe die de filter test met beide path-shapes, of (b) lijn met de tab-pattern door de client-side filter te elimineren en de server-side @self.relations.meeting param door te geven aan fetchCollection (die toch al draait op mount en bij subscribe-refetches).

3. 🟢 Minor — Promise.all fetch failure rendert silent lege pagina — src/views/LiveMeeting.vue:345

Wanneer één van de drie parallelle fetches throwt, rejected Promise.all direct, finally zet loading = false, en de pagina rendert met meeting = {}, allItems = [], participants = []. De gebruiker ziet een lege functioneel-uitziende pagina zonder error message of retry. Niet nieuw door deze PR (oude sequential code had dezelfde gap), maar Promise.all maakt het marginaal waarschijnlijker (één transient 503 faalt alle drie i.p.v. maar één).

Impact: Een raadslid die tijdens een netwerkglitch een live-meeting-sessie binnenkomt, ziet een lege functioneel-uitziende pagina zonder feedback over wat er misging.

Suggested fix: Na finally, check if (!this.meeting?.id) en toon een NcEmptyContent met retry button, of minimaal showError uit @nextcloud/dialogs. Niet-blokkerend.


Geconsolideerde review via /review-pr — Thorough mode.

Copy link
Copy Markdown
Member

@MWest2020 MWest2020 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Zie consolidated review comment voor complimenten, findings en suggested fixes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants