Improved perspective handling in Vue useModel hook#592
Conversation
WalkthroughThe changes update the handling of the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Component
participant W as Watcher Logic
participant P as PerspectiveRef
participant S as Subscription
C->>W: Provide perspective (static or computed)
W->>P: Check & extract perspective value
P-->>W: Return valid perspective value
W->>S: Subscribe/update collection if value is valid
Possibly related PRs
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
ad4m-hooks/react/src/useModel.ts (1)
50-80: Consider adding an effect cleanup for component unmountWhile the code now properly disposes of queries before creating new ones, it would be good to also dispose of the query when the component unmounts.
useEffect(() => { if (subjectEnsured) subscribeToCollection(); + return () => { + if (modelQueryRef.current) { + modelQueryRef.current.dispose(); + modelQueryRef.current = null; + } + }; }, [subjectEnsured, model, JSON.stringify(query), pageNumber]);ad4m-hooks/vue/src/useModel.ts (2)
56-56: Fix typo in function nameThe function name "handleNewEntires" has a typo - it should be "handleNewEntries".
-function handleNewEntires(newEntries: T[]) { +function handleNewEntries(newEntries: T[]) {Don't forget to update all calls to this function as well.
27-27: Consider using a ref for better lifecycle managementUnlike the React version, this implementation uses a regular variable for the modelQuery instead of a Vue ref. Consider using a ref for better reactivity and lifecycle management.
-let modelQuery: ModelQueryBuilder<T|Ad4mModel> | null = null; +const modelQuery = ref<ModelQueryBuilder<T|Ad4mModel> | null>(null);Then update the usage:
-if (modelQuery) modelQuery.dispose(); +if (modelQuery.value) modelQuery.value.dispose(); -modelQuery = +modelQuery.value =
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ad4m-hooks/react/src/useModel.ts(4 hunks)ad4m-hooks/vue/src/useModel.ts(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
ad4m-hooks/react/src/useModel.ts (1)
core/src/model/Ad4mModel.ts (2)
ModelQueryBuilder(876-1231)Ad4mModel(273-854)
ad4m-hooks/vue/src/useModel.ts (2)
core/src/model/Ad4mModel.ts (2)
Ad4mModel(273-854)ModelQueryBuilder(876-1231)core/src/perspectives/PerspectiveProxy.ts (1)
PerspectiveProxy(282-1179)
🔇 Additional comments (10)
ad4m-hooks/react/src/useModel.ts (4)
28-28: Good addition of a ref for query lifecycle managementAdding a ref to track the ModelQueryBuilder instance is a good practice for properly managing its lifecycle across renders.
52-53: Properly cleaning up resources before creating new queriesThis ensures proper disposal of the previous ModelQueryBuilder before creating a new one, which prevents memory leaks by cleaning up subscriptions.
54-58: Clean assignment of the query instance to the refThe assignment of the ModelQueryBuilder to the ref is clear and maintains the same logic for creating either a class-based or string-based query.
62-66: Consistent use of ref for subscription operationsThe code now consistently uses modelQueryRef.current for both paginated and non-paginated subscriptions, ensuring proper management of the query instance.
Also applies to: 71-72
ad4m-hooks/vue/src/useModel.ts (6)
5-5: Improved type signature for perspective propertyThe updated type signature now properly supports computed references, aligning with the PR objective of allowing direct use of computed properties for better reactivity handling.
29-43: Clean handling of perspective as a ref/computed or direct valueThis implementation elegantly handles both direct perspective values and computed references, with a watcher that keeps perspectiveRef in sync with changes to the computed value.
69-73: Added guard clause for missing perspectiveGood defensive programming practice to check for a valid perspective and return early if it's not available, preventing unnecessary operations and potential errors.
75-80: Proper cleanup of previous query resourcesSimilar to the React version, this code properly disposes of existing queries before creating new ones, preventing memory leaks from orphaned subscriptions.
112-134: Streamlined perspective watcher implementationThe revised watcher logic effectively merges the previous perspective and subject watchers, with improved handling of perspective changes and proper ensureSDNASubjectClass calls.
136-141: Refined query/page watcher with perspective checkThe watcher now checks for a valid perspective before attempting to subscribe, preventing potential errors when perspective is null.
2 participants
The hook now accepts ComputedRef<PerspectiveProxy | null> for the perspective property so that we can pass in computed references instead of functions, enabling reactivity without triggering unnecessary new subscriptions.
The new use of the hook in a Vue component looks like this:
Instead of the old pattern:
Additionally, the perspective and subject ensured watchers in the previous implementation have been merged
Summary by CodeRabbit
perspectiveproperty to ensure prompt and reliable reflections of changes.