Skip to content

[WIP] [Experimental] new architecture #6155

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 14 commits into
base: uj/arch-distill
Choose a base branch
from
Open

Conversation

kanwarujjaval
Copy link
Member

No description provided.

@kanwarujjaval kanwarujjaval requested a review from Copilot April 19, 2025 09:06
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements experimental architectural changes across multiple modules and improves error handling and code clarity. Key changes include updating the database connection call in pluginManager.js, adding a fallback for performance.now() in log.js, fixing a typographical error in changeStreamReader.js, and enhancing optional chaining in ingestor.js.

Reviewed Changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.

File Description
plugins/pluginManager.js Updates the dbConnection invocation method
api/utils/log.js Adds a fallback function for performance.now()
api/parts/data/changeStreamReader.js Fixes a spelling typo in the onData parameter comment
api/ingestor.js Enhances optional chaining with default values
Files not reviewed (1)
  • .eslintrc.json: Language not supported
Comments suppressed due to low confidence (1)

plugins/pluginManager.js:1841

  • The new arrow function reverses the parameter order for dbConnection compared to the original bind call. Please verify that the intended argument order is preserved to avoid unintended behavior.
databases = await Promise.all(dbs.map((db) => this.dbConnection(db, return_original)));

@kanwarujjaval kanwarujjaval requested a review from Copilot April 23, 2025 05:45
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR updates the architecture by removing the extra “dont-enclose” parameter from all require calls and adapting code to changes in API responses (e.g. findOneAndUpdate now returning a .value field).

  • Removes the extra parameter across multiple modules to simplify module loading
  • Adjusts handling of findOneAndUpdate responses in app user and similar modules
  • Employs optional chaining for configuration object access in the ingestor module

Reviewed Changes

Copilot reviewed 22 out of 24 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
plugins/locale/frontend/app.js Removed "dont-enclose" argument from require call for countlyConfig
plugins/enterpriseinfo/frontend/app.js Updated require of countlyConfig by removing the extra parameter
plugins/dashboards/frontend/app.js Removed "dont-enclose" argument for consistency
plugins/crashes/frontend/app.js Removed the extra parameter from countlyConfig import
frontend/express/public/javascripts/countly/countly.config.sample.js Added new property INGESTOR_URL configuration
frontend/express/libs/members.js Removed "dont-enclose" from require call for configs
frontend/express/app.js Updated countlyConfig import by removing extra parameter
api/utils/utils.js Removed extra parameter on countlyConfig require call
api/parts/mgmt/app_users.js Updated result extraction from findOneAndUpdate to use .value.seq
api/parts/jobs/index.js Removed unnecessary parameter from countlyConfig require call
api/parts/data/changeStreamReader.js Fixed a typo in a comment and removed extra require parameter
api/ingestor.js Removed extra parameter; added optional chaining for configuration access
api/api.js Removed extra parameter from countlyConfig require call
api/aggregator.js Removed extra parameter from countlyConfig require call
Files not reviewed (2)
  • .eslintrc.json: Language not supported
  • package.json: Language not supported

@kanwarujjaval kanwarujjaval marked this pull request as ready for review May 7, 2025 10:45
@kanwarujjaval kanwarujjaval changed the title [Experimental] new architecture [WIP] [Experimental] new architecture May 13, 2025
@kanwarujjaval kanwarujjaval self-assigned this May 13, 2025
@kanwarujjaval kanwarujjaval changed the base branch from newarchitecture to uj/arch-distill June 4, 2025 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant