Skip to content

Remove Onyx.disconnect() - #857

Merged
roryabraham merged 2 commits into
Expensify:mainfrom
callstack-internal:feature/onyx-disconnect-removal
Oct 21, 2026
Merged

Remove Onyx.disconnect()#857
roryabraham merged 2 commits into
Expensify:mainfrom
callstack-internal:feature/onyx-disconnect-removal

Conversation

@fabioh8010

@fabioh8010 fabioh8010 commented Oct 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Details

Follow-up from #834 (comment) and Expensify/App#100271.

Onyx.connect() and Onyx.connectWithoutView() already return a Connection handle, and calling unsubscribe() on it ends the subscription. Onyx.disconnect() only wraps that call, so this PR removes it. Call connection.unsubscribe() directly instead.

Related Issues

Expensify/App#102650

Linked E/App PR

Expensify/App#103351

Automated Tests

No new tests, since this doesn't change behavior. Existing tests now call connection.unsubscribe() instead of Onyx.disconnect().

Manual Tests

This is refactor work, so we will do a sanity test using the linked E/App PR:

  1. Log in to the app.
  2. Verify the app loads and works normally.
  3. Go to a chat, send a message, react to it, scroll the chat list a bit, assert all works.
  4. Log out.

Author Checklist

  • I linked the correct issue in the ### Related Issues section above
  • I linked the corresponding Expensify/App PR in the ### Linked E/App PR section above, and verified this change against it (E/App CI passed and manual testing completed)
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android / native
    • Android / Chrome
    • iOS / native
    • iOS / Safari
    • MacOS / Chrome / Safari
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
    • I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
  • I verified that if a function's arguments changed that all usages have also been updated correctly
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • The file is named correctly
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • If we are not using the full Onyx data that we loaded, I've added the proper selector in order to ensure the component only re-renders when the data it is using changes
    • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
    • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
    • All JSX used for rendering exists in the render method
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
  • I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: Native
Android: mWeb Chrome
iOS: Native
Screen.Recording.iPhone.17.19-10-2026.at.12.19.21.mp4
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2026-10-19.at.12.12.47.mov

@fabioh8010
Remove Onyx.disconnect()
513b3ef 
`connect()`/`connectWithoutView()` already return a `Connection` handle whose
`unsubscribe()` tears the subscription down, so `disconnect()` is a redundant
wrapper around it. Call `connection.unsubscribe()` directly instead.
@fabioh8010
fabioh8010 requested a review from a team as a code owner October 7, 2026 15:21
@fabioh8010
fabioh8010 marked this pull request as draft October 7, 2026 15:21
@melvin-bot
melvin-bot Bot requested review from iwiznia and removed request for a team October 7, 2026 15:22
@fabioh8010 fabioh8010 mentioned this pull request Oct 7, 2026
Open
@fabioh8010

fabioh8010 commented Oct 19, 2026

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 19, 2026 •
edited
Loading

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-19T11:23:48.867233Z 64aaf01 Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 19, 2026

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 👍

Reviewed commit: 64aaf01b43

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@fabioh8010 fabioh8010 changed the title [WIP][HOLD on https://github.com/Expensify/App/pull/98956] Remove Onyx.disconnect() Remove Onyx.disconnect() Oct 19, 2026
@fabioh8010
fabioh8010 marked this pull request as ready for review October 19, 2026 11:21
@roryabraham
roryabraham merged commit 43a16b4 into Expensify:main Oct 21, 2026
19 of 20 checks passed
@os-botify

os-botify Bot commented Oct 21, 2026

Copy link
Copy Markdown
Contributor

🚀 Published to npm in 3.0.122 🎉

jmusial pushed a commit to jmusial/react-native-onyx that referenced this pull request Oct 27, 2026
@roryabraham @jmusial
Merge pull request Expensify#857 from callstack-internal/feature/onyx…
51491c4 
…-disconnect-removal

Remove Onyx.disconnect()
This was referenced Oct 30, 2026
Open
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@roryabraham roryabraham roryabraham approved these changes
@iwiznia iwiznia Awaiting requested review from iwiznia

Assignees

@fabioh8010 fabioh8010

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@fabioh8010 @roryabraham