You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
🦸 Review Hero (could not post inline comments — showing here instead)
packages/aggregator/jest.config.js:5
[Design & Architecture]suggestion
The identical transform block ('^.+\\.jsx?$': ['ts-jest', { isolatedModules: true, tsconfig: '<rootDir>/tsconfig.json' }]) is copy-pasted into 5 separate jest.config.js files (aggregator, auth, dhis-api, kobo-api, web-config-server). Since these all extend the shared jest.config-js.json, the DRY approach is to replace the old Babel transformer in that base config (or create a new jest.config-node-js.json base for migrated packages) rather than duplicating the override in every consumer. That way, future packages migrated off Babel just extend the base and get it for free.
package.json:43
[Design & Architecture]suggestion
The root script package:build:types is now the sole build step for migrated packages, but its name implies it only emits type declarations (which was true under the old tsconfig-js.json with emitDeclarationOnly). With tsconfig-node-js.json it now emits JS too. Consider renaming it (e.g. package:build:tsc) or introducing a separate script for these packages, so the name doesn't mislead contributors into thinking it's declaration-only. The existing non-migrated packages (database, utils, access-policy) still use the old tsconfig-js.json where it truly is types-only, so the same script name now means different things depending on which tsconfig a package uses.
The variable is still named taskAgr (typo for taskArg). Pre-existing, but since you're already editing these lines, it's a good opportunity to fix the typo.
packages/web-config-server/tsconfig.json:1
Tupaia Conventions
nitpick
The old Babel module-resolver plugin provided clean absolute-style imports ('/apiV1/...'). Now that tsconfig.json exists, you could define paths aliases (e.g., "@src/*": ["src/*"]) to avo...
packages/web-config-server/tsconfig.json:1
Tupaia Conventions
suggestion
This is a new file — good that it excludes src/__tests__. Note that the old Babel build also excluded __mocks__ directories (--ignore "**/__tests__","**/__mocks__"). There are currently no `_...
Importing from './' (bare directory index) works but is fragile — it imports the package's own barrel index.js, which likely re-exports getDataBuilder itself, creating a circular dependency path. The old absolute alias had the same issue, so this isn't a regression, but worth noting as a cl...
Local fix prompt (copy to your coding agent)
Fix these issues identified on the pull request. One commit per issue fixed.
packages/aggregator/jest.config.js:5: The identical transform block ('^.+\\.jsx?$': ['ts-jest', { isolatedModules: true, tsconfig: '<rootDir>/tsconfig.json' }]) is copy-pasted into 5 separate jest.config.js files (aggregator, auth, dhis-api, kobo-api, web-config-server). Since these all extend the shared jest.config-js.json, the DRY approach is to replace the old Babel transformer in that base config (or create a new jest.config-node-js.json base for migrated packages) rather than duplicating the override in every consumer. That way, future packages migrated off Babel just extend the base and get it for free.
package.json:43: The root script package:build:types is now the sole build step for migrated packages, but its name implies it only emits type declarations (which was true under the old tsconfig-js.json with emitDeclarationOnly). With tsconfig-node-js.json it now emits JS too. Consider renaming it (e.g. package:build:tsc) or introducing a separate script for these packages, so the name doesn't mislead contributors into thinking it's declaration-only. The existing non-migrated packages (database, utils, access-policy) still use the old tsconfig-js.json where it truly is types-only, so the same script name now means different things depending on which tsconfig a package uses.
packages/web-config-server/src/apiV1/dataBuilders/getDataBuilder.js:1: Importing from './' (bare directory index) works but is fragile — it imports the package's own barrel index.js, which likely re-exports getDataBuilder itself, creating a circular dependency path. The old absolute alias had the same issue, so this isn't a regression, but worth noting as a cleanup candidate.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue #:
Changes:
Screenshots:
🦸 Review Hero