Skip to content

Conversation

@tassoevan
Copy link
Contributor

@tassoevan tassoevan commented Nov 6, 2025

Proposed changes (including videos or screenshots)

It allows to have stores that follow a common interface for both Minimongo collections and cached stores, making it easier to switch between them.

Issue(s)

ARCH-1867

Steps to test or reproduce

Further comments

Summary by CodeRabbit

Release Notes

  • Refactor
    • Improved internal store architecture for enhanced data consistency and reactivity.
    • Reorganized permission and user data synchronization mechanisms for better code maintainability.
    • Streamlined cache store initialization patterns for more reliable client-side state management.

It allows to have stores that follow a common interface for both
Minimongo collections and cached stores, making it easier to switch
between them.
@dionisio-bot
Copy link
Contributor

dionisio-bot bot commented Nov 6, 2025

Looks like this PR is ready to merge! 🎉
If you have any trouble, please check the PR guidelines

@changeset-bot
Copy link

changeset-bot bot commented Nov 6, 2025

⚠️ No Changeset found

Latest commit: a0891f5

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@tassoevan tassoevan added this to the 7.13.0 milestone Nov 6, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 6, 2025

Walkthrough

The changes refactor the store and collection injection pattern across the caching system. CachedStore's watchReady method is removed in favor of reactive watch-based readiness checking. MinimongoCollection is converted to accept injected stores via constructor parameters. DocumentMapStore is introduced with a typed interface for invalidation callbacks, and Users store is updated to use DocumentMapStore instead of Minimongo.

Changes

Cohort / File(s) Summary
Readiness checking refactor
apps/meteor/app/authorization/client/hasPermission.ts, apps/meteor/client/lib/cachedStores/CachedStore.ts
Removed watchReady() method from CachedStore and replaced direct readiness checks with reactive watch(PermissionsCachedStore.useReady, ...) pattern in hasPermission.ts
Store hooks typing
apps/meteor/client/lib/cachedStores/DocumentMapStore.ts
Introduced IDocumentMapStoreHooks<T> interface to type invalidation callbacks (onInvalidate, onInvalidateAll), replacing inline type in createDocumentMapStore signature
Collection injection pattern
apps/meteor/client/meteor/minimongo/MinimongoCollection.ts
Changed to accept injected zustand store via constructor instead of internal creation; exposed public use field; made recomputeAll() and scheduleRecomputationsFor() public; added internal scheduleRecomputation() method
Store integration updates
apps/meteor/client/stores/Users.ts, apps/meteor/client/meteor/overrides/userAndUsers.ts
Updated Users store to use createDocumentMapStore with hooks instead of Minimongo; wired invalidation hooks to MinimongoCollection in userAndUsers.ts; changed Meteor.users to use the new Minimongo-backed collection

Sequence Diagram

sequenceDiagram
    participant App as Application
    participant Users as Users Store
    participant DMS as DocumentMapStore
    participant MC as MinimongoCollection
    participant MM as Meteor.users

    App->>Users: Initialize Users.create()
    Users->>DMS: createDocumentMapStore(hooks)
    DMS->>Users: Return store with use + hooks
    Users->>MC: new MinimongoCollection(Users.use)
    MC-->>Users: Initialized with injected store
    
    rect rgb(200, 220, 255)
    Note over DMS,MC: Invalidation Flow
    DMS->>Users: onInvalidate(docs)
    Users->>MC: scheduleRecomputationsFor(docs)
    MC-->>MM: Updated collection
    end
    
    rect rgb(220, 200, 255)
    Note over DMS,MC: Full Invalidation Flow
    DMS->>Users: onInvalidateAll()
    Users->>MC: recomputeAll()
    MC-->>MM: Full refresh
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Specific areas for extra attention:
    • Public API changes to MinimongoCollection visibility and constructor parameters (potential breaking changes for consumers)
    • Integration between DocumentMapStore hooks and MinimongoCollection's invalidation handling in userAndUsers.ts
    • Verify reactive watch-based readiness checking in hasPermission.ts maintains functional equivalence to removed watchReady() method
    • Confirm store injection pattern works correctly with zustand's StoreApi and UseBoundStore types

Possibly related PRs

Suggested labels

stat: QA assured

Suggested reviewers

  • aleksandernsilva

Poem

🐰 Stores nested deep, now backwards they flow,
Injected with grace where collections used to grow,
Hooks dance with zeal, invalidating the dance,
Watch-ready eyes now take their keen glance,
Reactive delight in this pattern's new stance!

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title 'chore: Invert store/collection injection' accurately describes the main refactoring effort across all modified files, which centralizes on inverting how stores and collections are injected into components.
Linked Issues check ✅ Passed The PR addresses ARCH-1867 by inverting store/collection injection pattern to enable common interface usage between Minimongo collections and cached stores, allowing easier switching between them.
Out of Scope Changes check ✅ Passed All changes are directly related to inverting store/collection injection: refactoring readiness checks, removing watchReady method, introducing IDocumentMapStoreHooks interface, and updating MinimongoCollection to accept injected stores.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch chore/store-collection-injection

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 3310983 and db4cdbf.

📒 Files selected for processing (6)
  • apps/meteor/app/authorization/client/hasPermission.ts (1 hunks)
  • apps/meteor/client/lib/cachedStores/CachedStore.ts (0 hunks)
  • apps/meteor/client/lib/cachedStores/DocumentMapStore.ts (2 hunks)
  • apps/meteor/client/meteor/minimongo/MinimongoCollection.ts (3 hunks)
  • apps/meteor/client/meteor/overrides/userAndUsers.ts (1 hunks)
  • apps/meteor/client/stores/Users.ts (1 hunks)
💤 Files with no reviewable changes (1)
  • apps/meteor/client/lib/cachedStores/CachedStore.ts
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: apps/meteor/ee/server/startup/federation.ts:39-74
Timestamp: 2025-11-05T20:53:57.761Z
Learning: In Rocket.Chat (apps/meteor/app/settings/server/CachedSettings.ts), the settings.watchMultiple() method immediately invokes its callback with current values if all requested settings exist in the store, then continues watching for subsequent changes. It does not wait for a setting to change before the first invocation.
📚 Learning: 2025-11-05T20:53:57.761Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: apps/meteor/ee/server/startup/federation.ts:39-74
Timestamp: 2025-11-05T20:53:57.761Z
Learning: In Rocket.Chat (apps/meteor/app/settings/server/CachedSettings.ts), the settings.watchMultiple() method immediately invokes its callback with current values if all requested settings exist in the store, then continues watching for subsequent changes. It does not wait for a setting to change before the first invocation.

Applied to files:

  • apps/meteor/app/authorization/client/hasPermission.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings by mapping subscription documents to room IDs, never undefined, even when user has no room subscriptions.

Applied to files:

  • apps/meteor/client/stores/Users.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings (mapping subscription documents to room IDs), never undefined, even when user has no room subscriptions.

Applied to files:

  • apps/meteor/client/stores/Users.ts
🧬 Code graph analysis (4)
apps/meteor/client/meteor/overrides/userAndUsers.ts (4)
apps/meteor/client/lib/user.ts (1)
  • userIdStore (9-9)
apps/meteor/client/meteor/minimongo/MinimongoCollection.ts (1)
  • MinimongoCollection (13-79)
apps/meteor/client/stores/Users.ts (1)
  • Users (10-12)
apps/meteor/client/meteor/user.ts (2)
  • watchUserId (7-7)
  • watchUser (9-13)
apps/meteor/client/meteor/minimongo/MinimongoCollection.ts (1)
apps/meteor/client/lib/cachedStores/DocumentMapStore.ts (1)
  • IDocumentMapStore (3-144)
apps/meteor/app/authorization/client/hasPermission.ts (2)
apps/meteor/client/meteor/watch.ts (1)
  • watch (5-24)
apps/meteor/client/cachedStores/PermissionsCachedStore.ts (1)
  • PermissionsCachedStore (6-10)
apps/meteor/client/stores/Users.ts (2)
apps/meteor/client/lib/cachedStores/DocumentMapStore.ts (2)
  • IDocumentMapStoreHooks (146-161)
  • createDocumentMapStore (169-308)
apps/meteor/client/lib/cachedStores/createGlobalStore.ts (1)
  • createGlobalStore (5-14)
🔇 Additional comments (1)
apps/meteor/app/authorization/client/hasPermission.ts (1)

59-61: Reactive readiness guard looks good

Subscribing through watch keeps this check reactive while still short‑circuiting when the store isn’t ready. 👍


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@tassoevan tassoevan marked this pull request as ready for review November 6, 2025 16:45
@tassoevan tassoevan requested a review from a team as a code owner November 6, 2025 16:45
@codecov
Copy link

codecov bot commented Nov 6, 2025

Codecov Report

❌ Patch coverage is 91.30435% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.09%. Comparing base (22c6e0b) to head (a0891f5).
⚠️ Report is 1 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #37412      +/-   ##
===========================================
- Coverage    68.13%   68.09%   -0.04%     
===========================================
  Files         3364     3364              
  Lines       115814   115749      -65     
  Branches     20909    20904       -5     
===========================================
- Hits         78909    78824      -85     
- Misses       34213    34232      +19     
- Partials      2692     2693       +1     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ggazzo ggazzo added the stat: QA assured Means it has been tested and approved by a company insider label Nov 7, 2025
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Nov 7, 2025
@ggazzo ggazzo merged commit 95eefa8 into develop Nov 7, 2025
16 of 17 checks passed
@ggazzo ggazzo deleted the chore/store-collection-injection branch November 7, 2025 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stat: QA assured Means it has been tested and approved by a company insider stat: ready to merge PR tested and approved waiting for merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants