atom.io clean up bonded joins

jeremybanka commented 2 months ago

User description

✅ finish find coverage
♻️ simplify getFromStore
♻️ simplify set and dispose
✨ remove molecule key from any bonded joins

PR Type

enhancement, bug_fix

Description

Enhanced test coverage for molecule initialization and disposal scenarios.
Simplified content molecule creation by removing unnecessary bonding logic.
Refactored disposal logic to remove redundant checks and improve error logging.
Improved handling of counterfeit tokens across various store operations.
Enhanced molecule disposal process to ensure proper cleanup of joins.

Changes walkthrough 📝

Relevant files

Tests

immortal.test.ts

Enhance test coverage for molecule initialization and disposal

packages/atom.io/__tests__/experimental/immortal/immortal.test.ts

Updated test descriptions for clarity on molecule existence.

Added a new test case for implicit initialization with existing
molecules.

Added assertions for disposal state verification.

+32/-1

Enhancement

join.ts `Simplify content molecule creation in joins` packages/atom.io/data/src/join.ts Removed unnecessary bonding of join tokens in content molecules. Simplified constructor logic for content molecules.	+2/-4
find-in-store.ts `Refactor molecule growth logic in store finding` packages/atom.io/internal/src/families/find-in-store.ts Moved molecule growth logic outside of immortal lifespan check. Ensured counterfeit tokens are handled consistently.	+4/-4
get-from-store.ts `Simplify state retrieval and improve error handling` packages/atom.io/internal/src/get-state/get-from-store.ts Removed redundant molecule growth logic. Improved error logging for counterfeit tokens.	+5/-18
dispose-molecule.ts `Enhance molecule disposal to clean up joins` packages/atom.io/internal/src/molecule/dispose-molecule.ts Added logic to remove molecule from joins during disposal. Ensured joins and molecules are cleaned up properly.	+6/-3
set-into-store.ts `Simplify state setting and enhance error handling` packages/atom.io/internal/src/set-state/set-into-store.ts Simplified token finding logic by removing lifespan checks. Improved error logging for counterfeit tokens.	+6/-9

Bug fix

dispose-from-store.ts

Refactor disposal logic and improve error logging

packages/atom.io/internal/src/families/dispose-from-store.ts

Removed redundant immortal lifespan check.

Improved error logging for disposal traces.

+5/-7

💡 PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

vercel[bot] commented 2 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
atom-io-fyi	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Aug 21, 2024 7:21am
wayfarer-quest	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Aug 21, 2024 7:21am

changeset-bot[bot] commented 2 months ago

⚠️ No Changeset found

Latest commit: 25d565ad2653e46b68e714d1772c532928b1648a

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

github-actions[bot] commented 2 months ago

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Key issues to review

Error Handling
The error message for disposal issues might be misleading if `maybeToken` is not counterfeit but simply null or undefined. This could lead to confusion when debugging. Consider refining the condition to differentiate these scenarios more clearly. Error Handling
Similar to `dispose-from-store.ts`, the error handling here could be improved by distinguishing between different reasons why `maybeToken` might not be valid, such as being counterfeit or not existing at all.

github-actions[bot] commented 2 months ago

PR Code Suggestions ✨

Category	Suggestion	Score
Possible bug	Restore molecule bonding logic to ensure correct functionality ___ Restore the bonding logic in the `ContentMolecule` constructor to ensure that molecules are correctly bonded to the join token. [packages/atom.io/data/src/join.ts [417-421]](https://github.com/jeremybanka/wayforge/pull/2434/files#diff-8ede6a54b258362ab00273b6cca0dabe03db72089527006b1cca991abe187da8R417-R421) ```diff new: class ContentMolecule { public constructor( - _: CtorToolkit, + toolkit: CtorToolkit, public key: string, - ) {} + ) { + toolkit.bond(joinToken, { as: null } as any) + } }, ``` Suggestion importance[1-10]: 9 Why: The suggestion addresses a potential bug by restoring the bonding logic in the `ContentMolecule` constructor, which is crucial for ensuring that molecules are correctly bonded to the join token, maintaining the intended functionality.	9
Possible bug	Correct the type parameter in the atom family definition to match expected types ___ Replace the incorrect type parameter in the `atomFamily` function from `string` to `number` to match the expected type for the `countStates` atom family. [packages/atom.io/__tests__/experimental/immortal/immortal.test.ts [54-57]](https://github.com/jeremybanka/wayforge/pull/2434/files#diff-07f4bcf7cde881144c4d396d2b1b2c345ae79bf0dc9e2ebfbb924625f7af8d99R54-R57) ```diff -const countStates = atomFamily({ +const countStates = atomFamily({ key: `count`, default: 0, }) ``` Suggestion importance[1-10]: 8 Why: The suggestion correctly identifies a type mismatch in the `atomFamily` function, which could lead to runtime errors or unexpected behavior. Correcting the type parameter from `string` to `number` aligns with the intended use of the `countStates` atom family.	8
Enhancement	Adjust the check for counterfeit tokens to use direct property access ___ Ensure that the disposal logic checks for the `counterfeit` property on `maybeToken` to handle cases where the token is counterfeit. [packages/atom.io/internal/src/families/dispose-from-store.ts [52-53]](https://github.com/jeremybanka/wayforge/pull/2434/files#diff-f86854992abef68242dbc7fd4725a72e20fd9992f4097f931b36e633b5abf792R52-R53) ```diff -if (!maybeToken \|\| `counterfeit` in maybeToken) { +if (!maybeToken \|\| maybeToken.counterfeit) { const disposal = store.disposalTraces.buffer.find( (item) => item?.key === key, ) ``` Suggestion importance[1-10]: 7 Why: The suggestion improves code readability and correctness by using direct property access to check for counterfeit tokens, which is a more straightforward and less error-prone approach.	7
Enhancement	Enhance molecule disposal logic to thoroughly clean up all related data ___ Ensure that the disposal of molecule joins also removes the molecule from the `relations` map to prevent memory leaks and ensure correct cleanup. [packages/atom.io/internal/src/molecule/dispose-molecule.ts [35-38]](https://github.com/jeremybanka/wayforge/pull/2434/files#diff-70817b012eb1c8499334a404aa38ac052c80401d5cf9f665cec8d88f3f03189bR35-R38) ```diff for (const join of molecule.joins.values()) { join.relations.delete(molecule.key) join.molecules.delete(molecule.stringKey) + join.relations.forEach((relation) => relation.delete(molecule.key)) } ``` Suggestion importance[1-10]: 6 Why: The suggestion enhances the disposal logic by ensuring that all related data is cleaned up, which helps prevent memory leaks. However, it may not be critical unless memory issues are observed.	6

coveralls commented 2 months ago

coverage: 92.05% (+0.06%) from 91.992% when pulling 25d565ad2653e46b68e714d1772c532928b1648a on atom.io-clean-up-bonded-joins into a57ddc6e74f94003e4c885e1a9970c8177353cf7 on main.

jeremybanka / wayforge