jeremybanka / wayforge

TypeScript monorepo. Home of Atom.io.
https://atom.io.fyi
2 stars 2 forks source link

realtime occlusion strategies #2223

Closed jeremybanka closed 4 days ago

jeremybanka commented 1 month ago

User description


PR Type

Enhancement, Bug fix, Tests, Configuration changes


Description


Changes walkthrough ๐Ÿ“

Relevant files
Enhancement
7 files
system.server.ts
Expose mutable state for users in rooms.                                 

apps/core.wayfarer.quest/src/system.server.ts - Added `exposeMutable` for `usersInRoomsAtoms` with `username`.
+1/-0     
Room.tsx
Enhance room component with username state and mutable atoms.

apps/wayfarer.quest/src/app/Room.tsx
  • Added findState and myUsernameState imports.
  • Updated myRoomKey logic to use findRelations.
  • Added usePullMutableAtomFamilyMember for usersInRoomsInternal.
  • Updated button disable conditions and rendering logic.
  • +11/-8   
    my-room.ts
    Refactor myRoomKeyState selector logic.                                   

    apps/wayfarer.quest/src/services/store/my-room.ts - Refactored `myRoomKeyState` selector logic for clarity.
    +8/-8     
    sync-continuity.ts
    Enhance sync continuity with molecule and family member
    initialization.

    packages/atom.io/realtime-client/src/sync-continuity.ts
  • Added growMoleculeInStore and initFamilyMemberInStore functions.
  • Improved initializeContinuity and reveal event handling.
  • Added upsertState function for state updates.
  • +28/-6   
    realtime-continuity-synchronizer.ts
    Improve realtime continuity synchronizer initial payload and
    transaction handling.

    packages/atom.io/realtime-server/src/realtime-continuity-synchronizer.ts
  • Improved initial payload handling in realtimeContinuitySynchronizer.
  • Enhanced transaction subscription with visibility checks.
  • +9/-2     
    realtime-continuity.ts
    Simplify SyncGroup class and add method logic.                     

    packages/atom.io/realtime/src/realtime-continuity.ts - Simplified `SyncGroup` class and `add` method logic.
    +27/-33 
    silo.ts
    Add runTransaction method to Silo class.                                 

    packages/atom.io/src/silo.ts - Added `runTransaction` method to `Silo` class.
    +8/-1     
    Tests
    1 files
    realtime-continuity.test.tsx
    Add tests for mutable atoms in continuity.                             

    packages/atom.io/__tests__/experimental/realtime/realtime-continuity.test.tsx
  • Added new test suite for mutable atoms in continuity.
  • Refactored existing test for synchronizing transactions.
  • +133/-25
    Bug fix
    3 files
    StateEditor.tsx
    Improve StateEditor handling of undefined and null data. 

    packages/atom.io/react-devtools/src/StateEditor.tsx - Improved handling of `undefined` and `null` data in `StateEditor`.
    +11/-10 
    default-components.tsx
    Fix className handling in EditorWrapper.                                 

    packages/hamr/react-json-editor/src/default-components.tsx - Fixed className handling in `EditorWrapper`.
    +3/-1     
    primitive-editors.tsx
    Fix rendering of NullEditor component.                                     

    packages/hamr/react-json-editor/src/editors-by-type/primitive-editors.tsx - Fixed rendering of `NullEditor` component.
    +1/-1     
    Documentation
    2 files
    dry-news-dress.md
    Add changeset for mutable atoms in realtime continuity.   

    .changeset/dry-news-dress.md
  • Added changeset for handling mutable atoms better in realtime
    continuity.
  • +5/-0     
    long-dogs-wait.md
    Add changeset for Silo runTransaction method.                       

    .changeset/long-dogs-wait.md - Added changeset for adding `runTransaction` method to `Silo`.
    +5/-0     
    Configuration changes
    1 files
    .env.dev
    Add environment variables for development.                             

    apps/core.wayfarer.quest/.env.dev - Added environment variables for `PORT` and `CLIENT_ORIGINS`.
    +2/-0     

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

    vercel[bot] commented 1 month 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 20, 2024 0:41am
    wayfarer-quest โœ… Ready (Inspect) Visit Preview ๐Ÿ’ฌ Add feedback Aug 20, 2024 0:41am
    changeset-bot[bot] commented 1 month ago

    ๐Ÿฆ‹ Changeset detected

    Latest commit: 649ded1603792ecbb5e92457f34801d313026ae9

    The changes in this PR will be included in the next version bump.

    This PR includes changesets to release 1 package | Name | Type | | ------- | ----- | | atom.io | Patch |

    Not sure what this means? Click here to learn what changesets are.

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

    github-actions[bot] commented 1 month ago

    PR Reviewer Guide ๐Ÿ”

    โฑ๏ธ Estimated effort to review: 4 ๐Ÿ”ต๐Ÿ”ต๐Ÿ”ต๐Ÿ”ตโšช
    ๐Ÿงช PR contains tests
    ๐Ÿ”’ No security concerns identified
    โšก Key issues to review

    **Possible Bug:** The `exposeMutable` function is used to expose mutable state for `usersInRoomsAtoms` with `username`. Ensure that the username is properly sanitized or validated to prevent potential misuse or data leakage. **Code Clarity:** The refactoring of `myRoomKeyState` in `my-room.ts` simplifies the logic but could benefit from additional comments to explain the changes, especially how the new logic handles null and undefined values. **Performance Concern:** The `syncContinuity` function in `sync-continuity.ts` has complex logic for handling state updates. Review the performance implications of these updates, especially in scenarios with high frequency updates.
    github-actions[bot] commented 1 month ago

    PR Code Suggestions โœจ

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Enhance the upsertState function to handle tokens that are part of a family by initializing and growing molecules as needed ___ **The function upsertState uses setIntoStore to update the store, which might not handle
    cases where the token is part of a family. It's recommended to use growMoleculeInStore and
    initFamilyMemberInStore before setting the state to handle these cases properly.** [packages/atom.io/realtime-client/src/sync-continuity.ts [388]](https://github.com/jeremybanka/wayforge/pull/2223/files#diff-fe8f8fba41e3781bfd71d6a890bd07a6b8a1d79fceba2f83212715f92071e9d5R388-R388) ```diff -setIntoStore(token, value, store) +if (token.family) { + const family = store.families.get(token.family.key); + if (family) { + const molecule = store.molecules.get(token.family.subKey); + if (molecule) { + growMoleculeInStore(molecule, family, store); + } else if (store.config.lifespan === `immortal`) { + throw new Error(`No molecule found for key "${token.family.subKey}"`); + } + initFamilyMemberInStore(family, parseJson(token.family.subKey), store); + } +} +setIntoStore(token, value, store); ```
    Suggestion importance[1-10]: 10 Why: This suggestion ensures that the `upsertState` function correctly handles tokens that are part of a family, which is crucial for maintaining the integrity of the state management system.
    10
    Simplify the JSX structure by making NullWrapper self-closing ___ **Ensure that the NullWrapper component is self-closing if it does not need to wrap any
    children, which simplifies the JSX structure.** [packages/hamr/react-json-editor/src/editors-by-type/primitive-editors.tsx [25]](https://github.com/jeremybanka/wayforge/pull/2223/files#diff-d0043f55c27bd0c80b4f339d3621ef6d0c155bdcec894fff0851f054859ef47aR25-R25) ```diff - + ```
    Suggestion importance[1-10]: 10 Why: The suggestion correctly simplifies the JSX structure by making the NullWrapper component self-closing, which is a clear improvement in code clarity and maintainability.
    10
    Modify the add method to explicitly handle mutable_atom types by using getUpdateToken ___ **The add method in SyncGroup class is handling different types of tokens but does not
    currently support the mutable_atom type explicitly. It's important to handle mutable_atom
    separately to ensure that updates to mutable atoms are correctly managed in the
    continuity.** [packages/atom.io/realtime/src/realtime-continuity.ts [91]](https://github.com/jeremybanka/wayforge/pull/2223/files#diff-e70533323503e2d08244f13189b2e9698dd8049f4dfb9a165f20dab8493697feR91-R91) ```diff -this.globals.push(...(args as AtomToken[])) +if (zeroth.type === `mutable_atom`) { + this.globals.push(getUpdateToken(zeroth)); +} else { + this.globals.push(...(args as AtomToken[])); +} ```
    Suggestion importance[1-10]: 8 Why: This suggestion enhances the `add` method to correctly manage `mutable_atom` types, ensuring that updates to mutable atoms are handled properly in the continuity. This is an important improvement for the system's consistency.
    8
    Adjust the button's disabled condition to match the roomId for clarity and correct functionality ___ **The disabled property for the button is set based on myRoomKey !== null. This logic might
    not correctly reflect the intended behavior, especially if myRoomKey should specifically
    match roomId to enable the button. Consider revising the condition to ensure it aligns
    with the functional requirements.** [apps/wayfarer.quest/src/app/Room.tsx [59]](https://github.com/jeremybanka/wayforge/pull/2223/files#diff-3da64db6f6d61a90003ad559d41a216e2820abee387693fb9f470e86982ca302R59-R59) ```diff -disabled={myRoomKey !== null} +disabled={myRoomKey !== roomId} ```
    Suggestion importance[1-10]: 7 Why: This suggestion improves the clarity and functionality of the button's disabled state by ensuring it matches the roomId, which is a minor but useful enhancement.
    7
    Possible bug
    Add a check to ensure the state exists before exposing it as mutable to prevent runtime errors ___ **It seems that the exposeMutable function is being used without checking if the state
    exists or if the username is valid. This could lead to runtime errors if
    findState(usersInRoomsAtoms, username) returns null or an undefined value. It's
    recommended to add a check to ensure the state exists before exposing it as mutable.** [apps/core.wayfarer.quest/src/system.server.ts [65]](https://github.com/jeremybanka/wayforge/pull/2223/files#diff-6ec367858c982ba29a722c8bb86651414505a99e97dfb05d332a27d4ce3ecb24R65-R65) ```diff -exposeMutable(findState(usersInRoomsAtoms, username)) +const userRoomState = findState(usersInRoomsAtoms, username); +if (userRoomState) { + exposeMutable(userRoomState); +} else { + console.error('Invalid username or state not found:', username); +} ```
    Suggestion importance[1-10]: 9 Why: This suggestion addresses a potential runtime error by ensuring that the state exists before exposing it as mutable. This is a critical improvement for robustness.
    9
    Maintainability
    Improve the readability and consistency of class name construction ___ **Use template literals consistently for constructing class names. This improves readability
    and consistency in the code.** [packages/hamr/react-json-editor/src/default-components.tsx [60]](https://github.com/jeremybanka/wayforge/pull/2223/files#diff-3fb5e9ec5ea462befeb2afe2403bf6a8128230600faa4fd6fa0ca4c45b63debbR60-R60) ```diff -
    +
    ```
    Suggestion importance[1-10]: 8 Why: The suggestion improves readability and consistency by using template literals in a more concise manner, which is beneficial for maintainability.
    8
    coveralls commented 1 month ago

    Coverage Status

    coverage: 93.94% (-0.05%) from 93.985% when pulling 56bc8c2a5fc516c78ba04f39b270ef7421dda414 on back-to-card-gaming into 7063562b28c847d991fddc12a9bee235a817fcde on main.

    coveralls commented 1 month ago

    Coverage Status

    coverage: 93.94% (-0.05%) from 93.985% when pulling 85c90a1e35a9528c6dbe73936033b3f62946b1b1 on back-to-card-gaming into 7063562b28c847d991fddc12a9bee235a817fcde on main.

    coveralls commented 1 month ago

    Coverage Status

    coverage: 93.935% (-0.05%) from 93.985% when pulling ec9cd9d433ba8a7c66d320e49d94f2ba2bf58ce9 on back-to-card-gaming into 7063562b28c847d991fddc12a9bee235a817fcde on main.

    coveralls commented 1 month ago

    Coverage Status

    coverage: 91.941% (+0.06%) from 91.877% when pulling 649ded1603792ecbb5e92457f34801d313026ae9 on back-to-card-gaming into 21db723643b7f7d6de1dac108cf15fc00ebd2d5e on main.