EPMRPP-89496 || Migrate client-javascript to TypeScript#271
Open
maria-hambardzumian wants to merge 3 commits into
Open
EPMRPP-89496 || Migrate client-javascript to TypeScript#271maria-hambardzumian wants to merge 3 commits into
maria-hambardzumian wants to merge 3 commits into
Conversation
Contributor
Author
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
WalkthroughThe pull request migrates the client from CommonJS JavaScript to typed TypeScript, adds public models and constants, updates REST/OAuth/proxy/reporting modules, and changes Jest, compiler, npm packaging, and publishing configuration to build from ChangesTypeScript migration and public contracts
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/oauth.ts (1)
52-60: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHonor the supplied OAuth debug option.
RestClientpassesdebuginto this constructor, but Line 60 ignores it and reads onlyrestClientConfig.debug. Client-level OAuth diagnostics are therefore disabled unless the nested REST option is also set.Proposed fix
- this.debug = this.restClientConfig.debug || false; + this.debug = config.debug ?? this.restClientConfig.debug ?? false;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/oauth.ts` around lines 52 - 60, Update the OAuthInterceptor constructor’s debug assignment to honor the supplied OAuth-level config.debug value passed by RestClient, while preserving the existing restClientConfig.debug fallback when the OAuth option is absent.
🧹 Nitpick comments (2)
jest.config.js (1)
3-3: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid disabling TypeScript diagnostics without a required CI typecheck.
diagnostics: falselets Jest execute code with broken TypeScript contracts, weakening the migration’s validation. Keep diagnostics enabled or ensure CI runstsc --noEmitas a required check.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jest.config.js` at line 3, Update the ts-jest configuration in the TypeScript transform entry to remove diagnostics: false so Jest performs TypeScript diagnostics, or alternatively add a required CI tsc --noEmit check. Preserve the existing tsconfig.json configuration..eslintrc (1)
24-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winScope the disabled import rules instead of disabling them globally.
Turning off
import/no-unresolvedandimport/extensionsfor the entire repository can hide broken imports outside the TypeScript source. Prefer configuring the TypeScript resolver or applying overrides only to the affected files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.eslintrc around lines 24 - 27, Update the ESLint configuration entries for import/no-unresolved and import/extensions to apply only through a TypeScript-specific resolver or targeted file overrides, rather than disabling them repository-wide. Preserve the existing camelcase and valid-jsdoc settings, and ensure unaffected JavaScript files continue receiving import validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@__tests__/oauth.spec.js`:
- Around line 5-7: The axios mock’s isAxiosError implementation in the
jest.mock('axios', ...) block incorrectly identifies errors by response
presence; match real Axios by requiring error.isAxiosError === true, and update
the test’s rejected error value to include isAxiosError: true when it represents
an AxiosError.
In `@src/lib/rest.ts`:
- Around line 53-59: Export the RestClientOptions interface so the
ConstructorParameters<typeof RestClient>[0] type used by helpers.getServerResult
remains publicly nameable during declaration emit. Change only the interface’s
visibility and preserve its existing fields and types.
---
Outside diff comments:
In `@src/lib/oauth.ts`:
- Around line 52-60: Update the OAuthInterceptor constructor’s debug assignment
to honor the supplied OAuth-level config.debug value passed by RestClient, while
preserving the existing restClientConfig.debug fallback when the OAuth option is
absent.
---
Nitpick comments:
In @.eslintrc:
- Around line 24-27: Update the ESLint configuration entries for
import/no-unresolved and import/extensions to apply only through a
TypeScript-specific resolver or targeted file overrides, rather than disabling
them repository-wide. Preserve the existing camelcase and valid-jsdoc settings,
and ensure unaffected JavaScript files continue receiving import validation.
In `@jest.config.js`:
- Line 3: Update the ts-jest configuration in the TypeScript transform entry to
remove diagnostics: false so Jest performs TypeScript diagnostics, or
alternatively add a required CI tsc --noEmit check. Preserve the existing
tsconfig.json configuration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fffa4a88-0e2d-4af0-a88e-987e39f808e7
📒 Files selected for processing (49)
.eslintrc.github/workflows/publish.yml.gitignore__tests__/client-id.spec.js__tests__/config.spec.js__tests__/helpers.spec.js__tests__/oauth.spec.js__tests__/proxyHelper.spec.js__tests__/publicReportingAPI.spec.js__tests__/report-portal-client.spec.js__tests__/rest.spec.js__tests__/statistics.spec.jsindex.d.tsjest.config.jslib/constants/events.jslib/constants/statuses.jslib/publicReportingAPI.jspackage.jsonsrc/constants.tssrc/helpers.tssrc/lib/commons/config.tssrc/lib/commons/errors.tssrc/lib/constants/events.tssrc/lib/constants/index.tssrc/lib/constants/launchModes.tssrc/lib/constants/logLevels.tssrc/lib/constants/outputs.tssrc/lib/constants/statuses.tssrc/lib/constants/testItemTypes.tssrc/lib/helpers.tssrc/lib/logger.tssrc/lib/models/common.tssrc/lib/models/config.tssrc/lib/models/index.tssrc/lib/models/requests.tssrc/lib/models/responses.tssrc/lib/oauth.tssrc/lib/pjson.tssrc/lib/proxyHelper.tssrc/lib/publicReportingAPI.tssrc/lib/report-portal-client.tssrc/lib/rest.tssrc/models.tssrc/statistics/client-id.tssrc/statistics/constants.tssrc/statistics/statistics.tssrc/types/vendor.d.tsstatistics/constants.jstsconfig.json
💤 Files with no reviewable changes (5)
- lib/publicReportingAPI.js
- lib/constants/statuses.js
- statistics/constants.js
- lib/constants/events.js
- index.d.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
__tests__/oauth.spec.js (1)
403-433: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the proxy wiring, not only the log.
This test proves that the proxy branch is entered, but not that
axios.postreceives the appropriate agent andproxy: false. Capture the request config and assert those fields to protect the actual proxy contract.Suggested assertion
expect(consoleSpy).toHaveBeenCalledWith( `[OAuth] Making token request to ${baseConfig.tokenEndpoint} with proxy agent`, '', ); + const requestConfig = axios.post.mock.calls[0][2]; + expect(requestConfig.proxy).toBe(false); + expect(requestConfig.httpsAgent ?? requestConfig.httpAgent).toBeDefined();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@__tests__/oauth.spec.js` around lines 403 - 433, Update the test “logs the proxied token request when debug and proxy are both enabled” to capture the config passed to axios.post and assert that the request uses the configured proxy agent and sets proxy to false, while preserving the existing token and log assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@__tests__/oauth.spec.js`:
- Around line 403-433: Update the test “logs the proxied token request when
debug and proxy are both enabled” to capture the config passed to axios.post and
assert that the request uses the configured proxy agent and sets proxy to false,
while preserving the existing token and log assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fcf07188-9241-4267-a4c8-e47ac9002ea1
📒 Files selected for processing (7)
.eslintrc__tests__/oauth.spec.js__tests__/rest.spec.jssrc/helpers.tssrc/lib/commons/config.tssrc/lib/models/config.tssrc/lib/rest.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/helpers.ts
- tests/rest.spec.js
- .eslintrc
- src/lib/models/config.ts
- src/lib/commons/config.ts
- src/lib/rest.ts
…96-migrate-to-typescript
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/report-portal-client.ts (1)
375-385: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPropagate REST failures instead of converting them to success.
mergeLaunchescontinues withlaunch/mergeafter a search failure, using an empty launch list, and both merge and file-upload catches resolve normally. This can issue invalid merge requests and make failed log uploads appear successful.Proposed fix
- (error): Array<string | number> => { + (error) => { this.logDebug(`Error during launches search with UUIDs: ${launchUUIds}`, error); console.dir(error); - return []; + throw error; }, ... .catch((error) => { this.logDebug(`Error merging launches with UUIDs: ${launchUUIds}`, error); console.dir(error); + throw error; }); ... .catch((error) => { this.logDebug(`Error save log with file: ${fileObj.name}`, error); console.dir(error); + throw error; });Also applies to: 393-396, 721-734
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/report-portal-client.ts` around lines 375 - 385, Update mergeLaunches and the related file-upload error handlers to rethrow or reject REST failures after logging instead of returning successful fallback values. In the launch UUID search catch, do not continue to getMergeLaunchesRequest or call launch/merge with an empty list; ensure the promise remains rejected. Apply the same failure propagation to the catches around the upload flow at the referenced symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/lib/report-portal-client.ts`:
- Around line 375-385: Update mergeLaunches and the related file-upload error
handlers to rethrow or reject REST failures after logging instead of returning
successful fallback values. In the launch UUID search catch, do not continue to
getMergeLaunchesRequest or call launch/merge with an empty list; ensure the
promise remains rejected. Apply the same failure propagation to the catches
around the upload flow at the referenced symbols.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a827787d-3d6f-46f4-8b06-1698f26f436c
📒 Files selected for processing (3)
__tests__/report-portal-client.spec.jssrc/lib/models/requests.tssrc/lib/report-portal-client.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/report-portal-client.spec.js
- src/lib/models/requests.ts
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary by CodeRabbit